Measure pallet timeouts in relay chain blocks, not parachain blocks#268
Measure pallet timeouts in relay chain blocks, not parachain blocks#268ilchu wants to merge 27 commits into
Conversation
Add a BlockNumberProvider associated type to pallet-storage-provider's Config and route every block-number read in the pallet (and the drive/s3 registries, which reuse it via their Config supertrait) through it. Production runtimes supply RelaychainDataProvider, so all timeouts, expiries, checkpoint windows and nonce-recency checks are measured in relay chain blocks (6s) and keep their wall-clock meaning when the parachain block time changes (#113 / PR #131). Mocks supply System. The challenge slash sweep can no longer probe the single key n in on_finalize(n): relay numbers advance by a variable amount (including zero) between consecutive parachain blocks. Replace it with a drain-range sweep in on_initialize that tracks progress in a new LastSweptChallengeBlock cursor, drains all deadline keys strictly below the previous block's relay parent (provably unrespondable), and returns the exact weight consumed instead of pre-reserving it. Two independent bounds keep the block budget safe: a 32-key span cap on probing and a MaxChallengesPerDeadline slash budget per block (the old single-key worst case), with mid-key carry-over via the cursor. Runtime timing constants now derive from a new relay_time module based on RELAY_CHAIN_SLOT_DURATION_MILLIS instead of the parachain MILLISECS_PER_BLOCK; values are numerically unchanged today. Benchmarks advance both clocks via a set_block_number helper (the provider implements set_block_number under runtime-benchmarks), and the paseo runtime tests gain a set_clocks helper for the same reason. Closes #233
The pallet now measures every duration against the relay chain clock, so off-chain actors must snapshot ParachainSystem.LastRelayChainBlockNumber instead of the parachain height wherever a value meets an on-chain check. Provider node: chain_state.current_block now tracks the relay block anchored to each finalized parachain block (one extra storage read per block), feeding /negotiate's valid_until and the checkpoint window math; the fetch/decode is shared via a new storage_client::substrate::fetch_last_relay_block_number helper. JS SDK: new currentRelayBlock/waitForRelayBlock helpers in @web3-storage/layer0. Demos and e2e suites switch their CommitmentPayload nonce snapshots off System.Number (which would be rejected on any network where relay numbers dwarf parachain heights) and wait for agreement expiry / checkpoint windows on the relay clock. full-flow's demo agreement duration grows 15 -> 40 relay blocks since the relay clock keeps ticking while the parachain onboards.
Coverage-gated (tests/*_integration.rs) additions: - /negotiate signs valid_until = relay current_block + RequestTimeout, asserted with a Paseo-scale relay number, and returns 503 chain_state_not_ready when either the clock or the constant is still unknown (both refusal branches were untested). Plus checkpoint-coordinator duty tests (coordinators target): windows derive from the relay-scale block number and move only when the relay clock crosses an interval boundary, never on a repeated relay parent (velocity > 1) or an intra-window advance. Provider coverage: 64.79% locally vs 64.59% base (the dip came from the coordinator's relay-fetch lines, which need a live chain; the gate ignores subxt_client.rs).
PR #131 now targets #268 (relay-block denomination) instead of dev. Conflict resolutions: - runtimes/*/storage.rs: took the relay-block-provider side wholesale. This PR's DefaultCheckpointInterval/Grace rework (10*MINUTES/2*MINUTES of parachain time) is obsolete — the constants are relay-denominated now (100/20 relay blocks = the same ~10 min / ~2 min wall-clock) and no longer rescale with parachain block time, which was the whole point of that rework. - runtimes/*/lib.rs spec_version: both sides bumped independently from the merge base, so take the union: local runtime 2 vs 3 -> 4, paseo 4_002 vs 4_003 -> 4_004, with lineage comments. - Everything else (2s consensus constants, velocity 3, RelayParentOffset, zombienet topology, assign-cores) merged clean.
#275 extracted the wire types into provider-negotiation and removed the provider node's storage-client dependency, but two callers still reached into storage_client::substrate::fetch_last_relay_block_number. Inline the one-shot ParachainSystem::LastRelayChainBlockNumber query as a pub(crate) helper in subxt_client so the node reads the relay clock over its own subxt connection without depending on the client SDK.
The relay_time module's MINUTES/HOURS are name-twins of the parachain time module's MINUTES/HOURS, so a call site like 48 * HOURS gave no hint which clock it measured. Rename the relay-chain units to RC_MINUTES / RC_HOURS in both runtimes (constants + storage.rs timeout params) so the relay-chain denomination is explicit at every use. Values unchanged; the parachain time::* units (Session Period/SessionLength) keep their names.
| StorageProvider::<T>::on_finalize(deadline); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Since you dropped on_finalize in pallet/src/lib.rs. This should be updated as well
danielbui12
left a comment
There was a problem hiding this comment.
Make sure you wire client (rust + ts) with new changes. My assistance feedback:
1. client/src/challenger.rs:375-405 - Missed relay-clock wiring in the Rust SDK. analyze_provider computes last_checkpoint_age as parachain head minus snapshot.checkpoint_block, but the pallet now writes checkpoint_block on the relay clock. On Paseo the subtraction saturates to 0, so every provider reads as freshly checkpointed and the Challenge/Monitor/Skip recommendation runs on a fake age; on local zombienet the clocks nearly coincide so nothing fails. This also means the fetch_last_relay_block_number helper the PR added to client/src/substrate.rs:919 has zero callers in the repo (the provider node uses its own inlined copy per #275).
Fix: swap the blocks().at_latest().number() read for fetch_last_relay_block_number, which simultaneously gives the helper a real caller. (flagged by 3 agents, all high confidence)
2. Provider dashboard and s3-ui compare parachain height against relay-denominated on-chain values in five places - this is the direct answer to your "missed client wiring" question. All silently pass on zombienet and all break on Paseo, and two of them misinform providers about penalty-bearing obligations: - user-interfaces/provider/src/state/provider.state.ts:300-310 - checkpoint-overdue detection never fires, so the dashboard never warns about the window that now costs CheckpointMissPenalty.
- user-interfaces/provider/src/pages/Overview.tsx:112,249 - deregister cooldown shows ~29M blocks remaining and the complete button never unlocks.
- user-interfaces/provider/src/lib/chain-client.ts:343 - every agreement reads active forever, which also wrongly gates the deregister UI.
- user-interfaces/provider/src/lib/chain-client.ts:502 - every challenge reads pending forever.
- user-interfaces/s3-ui/src/components/CheckpointPanel.tsx:183-184 - challenge countdowns show tens of millions of blocks left and never expire.
Fix: add a relayBlockNumber$ (reading ParachainSystem.LastRelayChainBlockNumber, mirroring waitForRelayBlock) to each UI's chain state and use it for all pallet-clock comparisons. Same pattern fixes Earnings.tsx:32-34 (progress/earned-so-far clamps to 0 on Paseo).| /// Challenge deadlines are relay-chain block numbers | ||
| /// ([`Config::BlockNumberProvider`]), which advance by a variable | ||
| /// amount (including zero) between consecutive parachain blocks, so | ||
| /// the sweep drains a *range* of deadline keys and tracks its | ||
| /// progress in [`LastSweptChallengeBlock`] instead of probing the | ||
| /// single key `n` the way a parachain-block-keyed sweep could. | ||
| /// | ||
| /// At `on_initialize` time the validation-data inherent has not run | ||
| /// yet, so [`Pallet::current_block`] returns the relay parent `p` of | ||
| /// the *previous* parachain block. A challenge with deadline `d` is | ||
| /// respondable in any block whose relay parent is `<= d`, and every | ||
| /// later block has relay parent `>= p`, so exactly the keys `< p` | ||
| /// are final here: unrespondable, with `NextChallengeIndex` frozen | ||
| /// (any new challenge gets `deadline = now + ChallengeTimeout >= p`). | ||
| /// Draining them cannot race a valid response. The flip side is a | ||
| /// one-parachain-block lag: a slash lands in the first block *after* | ||
| /// the relay parent passes the deadline. Escape hatches don't care — | ||
| /// `complete_deregister`/`end_agreement` are gated by the | ||
| /// [`PendingChallenges`] counters, not by the sweep having run. | ||
| /// | ||
| /// Two independent bounds keep the block budget safe: [`MAX_SWEEP_SPAN`] | ||
| /// caps how many keys are probed, and a challenge budget of | ||
| /// [`Config::MaxChallengesPerDeadline`] caps how many slashes run — | ||
| /// the same worst case a single fully-loaded deadline always had. On | ||
| /// budget exhaustion the cursor parks just below the partially | ||
| /// drained key and the remainder carries over to later blocks. | ||
| /// | ||
| /// Runs in `on_initialize` rather than `on_finalize` so the actual | ||
| /// work done can be returned as weight instead of pre-reserved. |
There was a problem hiding this comment.
I would prefer using bullet points and simple formula to describe this 🤯
| let end = sweepable.min(last.saturating_add(MAX_SWEEP_SPAN.into())); | ||
| // Per-block slash budget. `.max(1)` so a (nonsensical) zero cap | ||
| // cannot park the cursor forever. | ||
| let mut budget = u32::from(T::MaxChallengesPerDeadline::get()).max(1); |
There was a problem hiding this comment.
I wonder if the MaxChallengesPerDeadline = 1000 reuse here could exhaust the PoV size.
@bkontur WDYT about splitting the constant? Keep MaxChallengesPerDeadline as it is, and add a separate MaxSweepSlashBudget (~100) for the sweep. In worst case, 1000 slashes spread over 10 blocks (~1 minute) instead of one block.
Rewrite the on_initialize sweep docs as bullets with the safety formula (keys < previous relay parent are final), tighten MAX_SWEEP_SPAN and LastSweptChallengeBlock, and fix stale on_finalize references left over from the on_initialize move. Addresses review comments on #268.
The benchmark still called the dropped on_finalize hook, which is now the Hooks default no-op, so it measured nothing and under-charged the slash sweep. Anchor the cursor and relay clock so on_initialize drains exactly the loaded deadline key, and add a verify guard asserting the drain actually happened. Addresses review comment on #268.
The Automatic Slashing sequence still showed the old on_finalize(block_number) single-key take. Reflect the relay-block range drain tracked by LastSweptChallengeBlock. Addresses review comment on #268.
A single deadline can hold up to MaxChallengesPerDeadline (1000) challenges; slashing all of them in one block costs ~5 MB PoV — the whole block budget. Introduce MAX_SWEEP_SLASH_BUDGET (100) and cap the per-block budget at min(MaxChallengesPerDeadline, MAX_SWEEP_SLASH_BUDGET), letting a full deadline drain over several blocks via the existing carry-over cursor. Tighten the benchmark component to the effective budget so weight regeneration stays within the sweep's real ceiling. Addresses review comment on #268.
The field holds the relay-chain block anchored to the latest finalized parachain block, not the parachain height. Rename it (and the local it feeds in /negotiate) so the relay-clock semantics are self-evident, resolving the ambiguity flagged in review. The separate get_current_block trait methods are untouched. Addresses review comment on #268.
Assert no unresolved challenge sits at or below LastSweptChallengeBlock: the sweep drains everything up to its cursor and new challenges always land above it, so a violation flags a stranded-unslashed challenge (e.g. an un-migrated upgrade leaving keys below the anchor). Addresses review comment on #268.
Conflict in pallet/src/lib.rs (both branches added a try_state hook): - imports: kept our `One` and dev's cfg-gated `TryRuntimeError`. - Hooks::try_state: took dev's delegating form (-> do_try_state), which supersedes our inline sweep-cursor check. The sweep-cursor invariant is re-homed into dev's impls/try_state.rs in the following commit.
The merge with dev adopted its Pallet::do_try_state() structure, which superseded the inline try_state hook from the earlier commit. Move the sweep-cursor invariant (no unresolved challenge at or below LastSweptChallengeBlock) in as check_challenge_sweep_cursor (P1.5), with a detection test matching the module's convention.
The ChainState.current_block -> current_relay_block rename missed the tests/ integration files (the earlier clippy check omitted --all-targets, so they were never compiled). Rename the field accesses so `cargo clippy --all-targets` and the integration tests build again.
Per review: the provider should not reach into ParachainSystem's LastRelayChainBlockNumber (a relay-specific storage item) to learn the clock on-chain durations are measured against. Introduce a block-notion-agnostic abstraction: - Pallet: rename Pallet::current_block -> current_anchor_block (the sole place the BlockNumberProvider is read) and expose it via a new StorageProviderApi::current_anchor_block runtime API, implemented in both runtimes. - Provider: replace the dynamic LastRelayChainBlockNumber storage read with a dynamic current_anchor_block runtime-API call, and rename ChainState.current_relay_block -> current_anchor_block. The provider no longer knows or cares whether the anchor is a relay, parachain, or future block number. - Registries call sites updated to current_anchor_block. Behavior-preserving: the runtime API resolves to the same value the provider read before. Note: crates/storage-subxt static bindings are regenerated from node metadata and do not yet include the new API; the provider uses a dynamic call, so this is not a blocker.
dev's #288 bumped subxt 0.44 -> 0.50 across chain-facing crates, which overlapped the two provider files this branch rewrote for the current_anchor_block runtime API. Resolution: - provider-node/src/subxt_client.rs (conflict): kept the anchor-block semantics but ported to subxt 0.50 — fetch via runtime_apis().call_raw ("StorageProviderApi_current_anchor_block") and decode the raw u32, which also avoids depending on the API being in the node metadata snapshot. current_anchor_block() now uses at_current_block(). - chain_state_coordinator.rs (silent auto-merge, 0.44 call left behind): fetch the anchor via block.at().runtime_apis(); hoisted the block handle so anchor + events share one at(). Added a state_call mock arm and updated the field-rename in the coordinator test. - client/src/substrate.rs: took dev's 0.50 version. The branch's fetch_last_relay_block_number helper here was dead code (orphaned when the provider inlined its own copy in f636e25) and 0.44-only, so dropping it matches dev. Full workspace compiles; pallet/provider/client/registry/paseo tests green; clippy clean.
| /// `valid_until`, nonce age) is measured against. Off-chain actors read | ||
| /// this instead of a specific storage item so they need not know whether | ||
| /// the anchor is a relay, parachain, or other block number. | ||
| fn current_anchor_block() -> BlockNumber; |
There was a problem hiding this comment.
I think probably correctly we should used different type here for AnchorBlockNumber, because for example fn challenges_at(block: BlockNumber) uses also BlockNumber, which is really parachain block.
in PolkadotSDK we use also types like:
// In our case AnchorBlockNumberFor<T>
type BlockNumberFor<T, I = ()> =
<<T as pallet_treasury::Config<I>>::BlockNumberProvider as BlockNumberProvider>::BlockNumber;
but not sure if we need this also now, maybe?
|
|
||
| let now = Self::current_anchor_block(); | ||
| if now.is_zero() { | ||
| return weight; |
There was a problem hiding this comment.
I think there are too many returns on the way, and this algo is getting complicated, maybe worth to extract to separate function(s)?
| } | ||
|
|
||
| impl<T: Config> Pallet<T> { | ||
| /// The anchor block: the clock every pallet duration (timeouts, expiries, |
There was a problem hiding this comment.
@ilchu and the last thing, please, check if we need to adjust design mds around this current_anchor_block and blockNumber/deadlines/timeouts handling
bkontur
left a comment
There was a problem hiding this comment.
@ilchu nice, nice, I think it looks pretty good module please check the comments:
#268 (comment)
#268 (comment)
#268 (comment)
analyze_provider compared the parachain head against the relay-denominated snapshot.checkpoint_block, so on live networks the age saturated to 0 and every provider read as freshly checkpointed. Re-add the SDK-side anchor helper (dropped with the subxt 0.50 signatures) as a runtime-API read and measure the age on it, reusing the block-pinned handle for a consistent snapshot.
Locals holding Pallet::current_anchor_block() were still named current_block, reading as parachain height. Rename them (and the checkpoint helpers' parameters) to anchor_block, matching the negotiate handler's naming; fix the stale Pallet::current_block doc link and the ChainStateNotReady messages along the way.
current_anchor_block tells clients where the pallet clock is but not how fast it ticks, so the provider dashboard humanized anchor-denominated durations (agreement duration, checkpoint interval/grace, min/max duration) with Aura.SlotDuration — identical today, silently wrong once the parachain block time changes. Add the paired runtime API (both runtimes return RELAY_CHAIN_SLOT_DURATION_MILLIS) and read it in the dashboard via the unsafe API (no descriptor regeneration; older runtimes keep the 6s default), renaming blockTimeMs to anchorBlockTimeMs so parachain block time can't be rewired in by accident.
The anchor_block_time_millis runtime API hardcoded RELAY_CHAIN_SLOT_DURATION_MILLIS in each runtime's impl block, away from the BlockNumberProvider wiring it describes. Make it a #[pallet::constant] instead: the clock and its tick are now declared side by side in each runtime's storage.rs, the runtime API delegates to the pallet like current_anchor_block does, integrity_test rejects a zero value, and the constant lands in metadata for free.
…arisons The provider dashboard and s3-ui compared the parachain height against anchor-denominated on-chain values, which silently passes on zombienet and breaks on any network where relay numbers dwarf parachain heights: agreement expiry and challenge status in chain-client.ts (whose local blockNumber$ was additionally never fed, so both always read 0), checkpoint-overdue detection, the deregister cooldown, earnings progress, and the s3-ui challenge countdown. Add an anchorBlock$ to each UI's chain state, refreshed per finalized block from the current_anchor_block runtime API via the unsafe API (live metadata, no descriptor regeneration; pre-anchor runtimes fall back to the parachain height, which is their pallet clock), and point all six comparisons at it.
Thank you, should be fixed :) |
The per-block anchor refresh is a fire-and-forget promise, so a result resolving after disconnect() (or a network switch) could write a stale anchor from the previous chain into the fresh state. Publish only if the client the call was made on is still the current client.
Summary
Implements #233: every duration in
pallet-storage-providerand the registry pallets —ChallengeTimeout,SettlementTimeout,RequestTimeout, checkpoint interval/grace,DeregisterAnnouncementPeriod,MaxNonceAge, agreement durations/extensions, drive expiry, replicamin_sync_interval— is now measured in relay chain blocks instead of parachain blocks, so nothing silently rescales when the parachain moves to 2s blocks (#113 / #131).Closes #233.
On-chain
Config::BlockNumberProvideronpallet-storage-provider; all block-number reads route throughPallet::current_anchor_block(). The drive/s3 registries reuse it via theirConfigsupertrait — no new Config items there. Runtimes wirecumulus_pallet_parachain_system::RelaychainDataProvider; mocks wireSystem, so existing tests drive the clock unchanged.current_anchor_block()(the clock) andanchor_block_time_millis()(its tick, backed by#[pallet::constant] Config::AnchorBlockTimeMillis, declared in each runtime right next to theBlockNumberProviderit describes;integrity_testrejects zero).on_finalize(n)point-lookup breaks because relay numbers advance by a variable amount (including zero) between consecutive parachain blocks. Replaced with a drain-range sweep inon_initializetracking aLastSweptChallengeBlockcursor. It drains only deadline keys strictly below the previous block's relay parent (provably unrespondable — the validation-data inherent hasn't run yet inon_initialize, and every later block's relay parent is ≥ that value), and returns the exact weight consumed instead of the old reserve-in-on_init/ work-in-on_finalizesplit, which cannot be kept sound under the two-clock asymmetry. Two independent bounds protect the block budget: a 32-key span cap on probing, and a per-block slash budget ofmin(MaxChallengesPerDeadline, MAX_SWEEP_SLASH_BUDGET = 100)so one fully-loaded deadline cannot eat the block's PoV. On budget exhaustion the cursor parks below the partially drained key and carries over.relay_timemodule anchored toRELAY_CHAIN_SLOT_DURATION_MILLISrather thanMILLISECS_PER_BLOCK— numerically identical today (6s == 6s), but Support for 3 async cores #131's 2s switch no longer touches them.RelaychainDataProvider::set_block_numberis available underruntime-benchmarks); the paseo runtime tests gained aset_clockshelper for the same reason.Off-chain (same PR to keep e2e green)
chain_state.current_anchor_blocktracks the pallet's clock at each finalized block via thecurrent_anchor_blockruntime API (rawstate_call+ manual decode, so the API needn't be in the node's metadata snapshot), so/negotiate'svalid_untiland the checkpoint window math stay consistent with the pallet.ChallengerClient::analyze_providermeasureslast_checkpoint_ageon the anchor clock (sharedfetch_current_anchor_blockhelper inclient/src/substrate.rs) instead of subtracting a relay-denominatedcheckpoint_blockfrom the parachain head. Against a pre-anchor runtime this now errors loudly rather than producing a silently wrong Challenge/Monitor/Skip recommendation.currentRelayBlock/waitForRelayBlockin@web3-storage/layer0. Demos and e2e suites snapshot the relay block forCommitmentPayloadnonces (aSystem.Numbernonce would be rejected as stale on any network where relay numbers dwarf parachain heights — e.g. Paseo) and wait out expiries/windows on the relay clock. The terms replay nonce is counter-based (ReplayWindow) and needed no change.anchorBlock$, refreshed per finalized block from thecurrent_anchor_blockruntime API via PAPI'sgetUnsafeApi()(live metadata — no descriptor regeneration; pre-anchor runtimes fall back to the parachain height, which is their pallet clock). All pallet-clock comparisons read it: agreement expiry and challenge status (whose previous source was additionally a never-fed subject, so both always compared against 0), checkpoint-overdue detection, the deregister cooldown, earnings progress, and the s3-ui challenge countdown.formatDurationconverts anchor-denominated durations withAnchorBlockTimeMillisinstead ofAura.SlotDuration, which would have silently skewed when Support for 3 async cores #131 changes the parachain tick.Ordering vs #131
Not a hard prerequisite, but landing this first is the better order: #131's rebase then no longer needs to touch storage timing constants (its checkpoint-interval rework becomes unnecessary), off-chain consumers re-denominate once instead of twice, and shipping #131 to PreviewNet first would force a live-state migration here later.
Migration & deployment notes
Fresh chains need nothing (
LastSweptChallengeBlockself-anchors on first block). Upgrading a live chain with in-flight state is NOT covered: existing parachain-denominatedChallengeskeys would sit below the cursor (never swept) and old expiries would read as long past — for testnets, reset; otherwise a one-shotOnRuntimeUpgradere-inserting pending challenges atrelay_now + ChallengeTimeout(and clearing their strandedNextChallengeIndexentries) is the minimum. On live networks the runtime and provider node must ship together: an old runtime with a new provider node 503s on/negotiate(missing runtime API), a new runtime with an old provider node signs instantly-expired terms.Remaining follow-up per the issue: user-facing duration inputs (duration pickers, payment-per-block display), and a
pnpm papi:generatedescriptor refresh so the UIs can move fromgetUnsafeApi()to typed runtime-API calls.Verified
runtime-benchmarks/try-runtimefeature checks; clippy-D warnings; JS typecheck + unit tests.just demo(all 8 steps — relay-denominated challenge deadlines, both defenses, expiry + payout),just fs-demo-ci,just s3-demo-ci.anchor_blockrename, anchor-tick API, UI wiring): workspace clippy-D warnings, pallet/registry/client test suites,tsc -b+ vitest for the provider and s3 UIs.