enforce slot based and make offset dynamic - #3740
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
📝 WalkthroughWalkthroughRelay-parent offset sourcing moved from hardcoded constants to the runtime AsyncBacking provider. Runtimes now configure ChangesRelay Parent Offset Async Backing Migration
Sequence DiagramsequenceDiagram
participant Node as Node Service
participant Client as Client/Block Backend
participant RuntimeAPI as RuntimeApi
participant Builder as RelayStateBuilder
participant Parachain as ParachainInherentData
Node->>Client: request block handle
Client->>RuntimeAPI: runtime_api().relay_parent_offset(block)
RuntimeAPI-->>Client: relay_parent_offset (u32)
Client-->>Node: relay_parent_offset (unwrap_or_default)
Node->>Builder: into_state_root_proof_and_descendants(u64::from(relay_parent_offset))
Builder-->>Node: (storage_root, chain_state, relay_parent_descendants)
Node->>Parachain: construct ParachainInherentData{ relay_parent_descendants, relay_offset = relay_parent_offset + additional }
Parachain-->>Node: inherent data ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
WASM runtime size check:Compared to target branchMoonbase runtime: 2064 KB (+8 KB) Moonbeam runtime: 2196 KB (+12 KB) Moonriver runtime: 2196 KB (+12 KB) Compared to latest release (runtime-4300)Moonbase runtime: 2064 KB (+204 KB compared to latest release) Moonbeam runtime: 2196 KB (+244 KB compared to latest release) Moonriver runtime: 2196 KB (+244 KB compared to latest release) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
node/service/src/lib.rs (1)
1552-1569: 💤 Low valueConsider logging the runtime API error before defaulting to 0.
relay_parent_offset(block).unwrap_or_default()silently falls back to0if the runtime API call fails. In the manual-seal dev path this is mostly diagnostic, but failures here are still worth alog::error!(mirroring the pattern used a few lines above forcollect_collation_info) so a misconfigured runtime doesn't quietly produce blocks with the wrongrelay_offset.♻️ Suggested change
- let relay_parent_offset = client_for_xcm - .runtime_api() - .relay_parent_offset(block) - .unwrap_or_default(); + let relay_parent_offset = match client_for_xcm + .runtime_api() + .relay_parent_offset(block) + { + Ok(offset) => offset, + Err(e) => { + log::error!( + "Failed to query relay_parent_offset, defaulting to 0: {:?}", + e + ); + 0 + } + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node/service/src/lib.rs` around lines 1552 - 1569, The call to client_for_xcm.runtime_api().relay_parent_offset(block).unwrap_or_default() silently drops errors; change it to capture the Result/Option, log the error with log::error! when it fails, and then use 0 as a fallback so misconfigured runtimes are visible; locate the call to relay_parent_offset in the code that constructs MockValidationDataInherentDataProvider (the relay_offset assignment) and replace the unwrap_or_default usage with a pattern that logs the runtime API error before defaulting (e.g., match/if let Err or .map_err/log and then .unwrap_or(0)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@node/service/src/lib.rs`:
- Around line 1552-1569: The call to
client_for_xcm.runtime_api().relay_parent_offset(block).unwrap_or_default()
silently drops errors; change it to capture the Result/Option, log the error
with log::error! when it fails, and then use 0 as a fallback so misconfigured
runtimes are visible; locate the call to relay_parent_offset in the code that
constructs MockValidationDataInherentDataProvider (the relay_offset assignment)
and replace the unwrap_or_default usage with a pattern that logs the runtime API
error before defaulting (e.g., match/if let Err or .map_err/log and then
.unwrap_or(0)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d33f0f2d-4544-4a18-964a-a8f5ca93051c
📒 Files selected for processing (3)
node/service/src/lazy_loading/mod.rsnode/service/src/lib.rsnode/service/src/rpc.rs
Coverage Report@@ Coverage Diff @@
## master elois/dynamic-offset +/- ##
=======================================================
Coverage 77.11% 77.11% 0.00%
Files 389 389
+ Lines 77230 77242 +12
=======================================================
+ Hits 59554 59563 +9
+ Misses 17676 17679 +3
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
runtime/moonbase/src/lib.rs (1)
760-773:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftVerify the
relay_parent_offsetstorage default in the external Moonkit pallet before deployment
pallet_async_backingis registered without aConfig<T>genesis entry, sorelay_parent_offsetis uninitialized after the runtime upgrade. TheGet<u32>impl will read from storage, which defaults to the item'sValueQuerydefault.If that default is
0(Rust's native u32 zero) instead of1, then immediately after upgrade:
cumulus_pallet_parachain_systemenforces relay-parent offset 0- Collators still use offset 1 (from the prior API contract)
- Every block produced is rejected → network halt
The PR description states "the effective default remains 1" in Moonkit commit
8fae4417637f8e55ea04b3548809d7f1c22daf64. This must be confirmed in thepallet_async_backingstorage declaration (verify theQueryKindor explicitDefaultValue). Test code across all three runtimes already callsAsyncBacking::relay_parent_offset()in production paths, which is encouraging; however, the actual pallet source is external and cannot be fully inspected from this repository.Also note:
spec_version: 4400is consistent across moonbase, moonbeam, and moonriver and was not marked as changed in this diff. Confirm that spec_version has been or will be incremented separately before the on-chain runtime upgrade, as this is a breaking consensus change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@runtime/moonbase/src/lib.rs` around lines 760 - 773, The runtime registers pallet_async_backing as AsyncBacking and uses it for RelayParentOffset in the cumulus_pallet_parachain_system impl, but the pallet's storage key relay_parent_offset may default to 0 if the pallet's StorageValue QueryKind/DefaultValue isn't explicitly set; confirm that the external pallet_async_backing declares relay_parent_offset with a ValueQuery/default of 1 (or an explicit DefaultValue => 1) or add an explicit genesis Config entry setting AsyncBacking::relay_parent_offset = 1 for this runtime so RelayParentOffset returns 1 after upgrade; also ensure spec_version is bumped before the on‑chain upgrade.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@runtime/moonbase/src/lib.rs`:
- Around line 760-773: The runtime registers pallet_async_backing as
AsyncBacking and uses it for RelayParentOffset in the
cumulus_pallet_parachain_system impl, but the pallet's storage key
relay_parent_offset may default to 0 if the pallet's StorageValue
QueryKind/DefaultValue isn't explicitly set; confirm that the external
pallet_async_backing declares relay_parent_offset with a ValueQuery/default of 1
(or an explicit DefaultValue => 1) or add an explicit genesis Config entry
setting AsyncBacking::relay_parent_offset = 1 for this runtime so
RelayParentOffset returns 1 after upgrade; also ensure spec_version is bumped
before the on‑chain upgrade.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0719544-87f6-4371-80a9-6cfce7fb40a7
📒 Files selected for processing (3)
runtime/moonbase/src/lib.rsruntime/moonbeam/src/lib.rsruntime/moonriver/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- runtime/moonbeam/src/lib.rs
- runtime/moonriver/src/lib.rs
@coderabbitai The external Moonkit pallet already declares RelayParentOffset as ValueQuery with DefaultRelayParentOffset returning 1 |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
node/service/src/lib.rs (1)
1552-1569:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t silently fall back to relay offset
0here.If
relay_parent_offset(block)fails, this path now authors mocked validation data withrelay_offset = 0 + additional_relay_offset, which recreates the exact mismatch this PR is trying to remove. Please surface the runtime API error instead of defaulting so dev-authored blocks don’t silently use an invalid relay parent offset.node/service/src/lazy_loading/mod.rs (1)
660-676:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid masking lazy-loading offset lookup failures with
0.This has the same problem as the dev path: on any
relay_parent_offset(block)error, lazy-loading will keep building mocked inherent data with the old implicit offset0. Since this mode is already more exposed to remote-state failures, please propagate the error instead of fabricating a relay offset that may no longer match the runtime configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node/service/src/lazy_loading/mod.rs` around lines 660 - 676, The code masks failures from client_for_xcm.runtime_api().relay_parent_offset(block) by using unwrap_or_default() which silently substitutes 0 into MockValidationDataInherentDataProvider.relay_offset (combined with additional_relay_offset.load), risking incorrect relay offsets; change the call to propagate the error instead of swallowing it—replace unwrap_or_default() with proper error propagation (e.g., using ? or map_err and propagating the Result from the function that builds the MockValidationDataInherentDataProvider) so that failures from relay_parent_offset are returned to the caller and the mock is not constructed with a fabricated offset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@node/service/src/rpc.rs`:
- Around line 244-247: The code currently swallows runtime API errors by calling
unwrap_or_default() on client_for_cidp.runtime_api().relay_parent_offset(block),
causing a synthesized offset 0 and incorrect relay_parent_descendants for
pending RPC simulation; replace unwrap_or_default() with proper error
propagation (e.g., use ? or map_err to return an error) so failures from
relay_parent_offset bubble up to the RPC caller, and apply the same change to
the other occurrences mentioned (the block handling around the 269-284 region)
so no runtime API error is converted into a default offset.
---
Outside diff comments:
In `@node/service/src/lazy_loading/mod.rs`:
- Around line 660-676: The code masks failures from
client_for_xcm.runtime_api().relay_parent_offset(block) by using
unwrap_or_default() which silently substitutes 0 into
MockValidationDataInherentDataProvider.relay_offset (combined with
additional_relay_offset.load), risking incorrect relay offsets; change the call
to propagate the error instead of swallowing it—replace unwrap_or_default() with
proper error propagation (e.g., using ? or map_err and propagating the Result
from the function that builds the MockValidationDataInherentDataProvider) so
that failures from relay_parent_offset are returned to the caller and the mock
is not constructed with a fabricated offset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50593e51-bd34-4980-97e8-20d980252c97
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
node/service/src/lazy_loading/mod.rsnode/service/src/lib.rsnode/service/src/rpc.rsruntime/common/src/apis.rsruntime/moonbase/src/lib.rsruntime/moonbase/tests/common/mod.rsruntime/moonbeam/src/lib.rsruntime/moonbeam/tests/common/mod.rsruntime/moonriver/src/lib.rsruntime/moonriver/tests/common/mod.rstest/configs/zombieAlphanet.jsontest/configs/zombieMoonbeam.jsontest/configs/zombieMoonriver.jsontest/suites/dev/common/test-block/test-block-mocked-relay.tstest/suites/dev/moonbase/test-precompile/test-precompile-relay-verifier.tstest/suites/lazy-loading/common/test-runtime-upgrade.tszombienet/configs/moonbeam-polkadot.tomlzombienet/configs/moonriver-kusama.toml
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
This reverts commit ce8944f.
cumulus_pallet_parachain_system::Config::RelayParentOffset.What does it do?
Updates the runtimes to use the
AsyncBackingpallet as the single source of truth for the relay parent offset.The runtime API now returns
AsyncBacking::relay_parent_offset(), and Moonbeam, Moonriver, and Moonbase wirecumulus_pallet_parachain_system::Config::RelayParentOffsettoAsyncBackinginstead ofConstU32<0>.This makes the slot-based collation offset both dynamic and enforced by runtime consensus checks.
What important points should reviewers know?
Cargo.lockpins Moonkit to8fae4417637f8e55ea04b3548809d7f1c22daf64, which adds theAsyncBackingstorage value and Root setter for the relay parent offset.1, while parachain-system consensus checks used0; this PR removes that mismatch.1, but governance can update it through Moonkit’spallet_async_backing::set_relay_parent_offset.Is there something left for follow-up PRs?
No immediate follow-up required.
What alternative implementations were considered?
Keeping a runtime constant would enforce the offset, but would not allow governance to adjust it without a runtime upgrade. Using
AsyncBackingdirectly keeps runtime API behavior and consensus validation aligned around the same storage value.Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?
8fae4417637f8e55ea04b3548809d7f1c22daf64What value does it bring to the blockchain users?
It enforces the slot-based collation relay parent offset at runtime level, reducing the risk of accepting blocks that do not follow the intended async-backing consensus configuration.