Bump polkadot-sdk to stable2603#95
Open
manuelmauro wants to merge 11 commits into
Open
Conversation
Updates moonkit's polkadot-sdk dependency from stable2512 to stable2603. Source-level migrations driven by upstream API drift: - RuntimeDebug is no longer re-exported from sp_core / sp_runtime; switched derives to core::fmt::Debug. - ExecuteBlock trait split into verify_and_remove_seal + execute_verified_block; pallet-author-inherent's BlockExecutor delegates accordingly. - ConsensusDataProvider lost its Proof associated type and now takes a concrete StorageProof in append_block_import. Dropped the now unused P generic from NimbusManualSealConsensusDataProvider. - cumulus-client-consensus-proposer was removed upstream (paritytech/polkadot-sdk#9947). Vendored a moonkit-local ProposerInterface inside nimbus-consensus that wraps the new sp_consensus::ProposeArgs API. The collator now owns the ProofRecorder, registers ProofSizeExt, and drains the proof after proposing — same proof-recording semantics as before. - ProposerFactory dropped its EnableProofRecording generic. - find_potential_parents / PotentialParent are gone; switched to find_parent_for_building / ParentSearchResult. ParentSearchParams no longer takes max_depth or ignore_alternative_branches; depth is now derived from header numbers via Saturating. - ParachainInherentDataProvider::create_at takes RelayProofRequest instead of Vec<Vec<u8>>; we wrap raw keys as RelayStorageKey::Top. - SubmitCollationParams gained scheduling_parent (Some -> V3, None preserves V2 semantics). - AssetClaims removed from xcm Config; the new TrapAndClaimAssets bound on AssetTrap covers both. - AssetsInHolding now tracks ImbalanceAccounting credits, no longer From<Vec<Asset>>. xcm-utils precompile uses xcm_executor::test_helpers::mock_asset_to_holding for its fee-estimation read path and unwraps credits via .amount(). - sp_session::SessionKeys runtime API moved to v2: generate_session_keys(seed) -> Vec<u8> became generate_session_keys(owner, seed) -> OpaqueGeneratedSessionKeys. - sc_service::BuildNetworkParams gained spawn_essential_handle and new_full_parts gained pruning_filters; threaded through. - MockValidationDataInherentDataProvider gained relay_parent_offset (defaulted to 0 in the template). frontier deps temporarily point to moonbeam-foundation/frontier mb/polkadot-sdk-stable2603 since polkadot-evm/frontier#1892 is still open. Once that lands, we can flip back to upstream stable2603.
|
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:
📝 WalkthroughWalkthroughThis PR upgrades workspace dependencies to stable2603, adds a local ProposerInterface/Error, refactors collator parent-finding and proof-recording (including ProofRecorder usage), simplifies manual-seal generics, updates derives and mock XCM/EVM holdings, and syncs template runtime/service APIs with the new SDK. ChangesSubstrate SDK Upgrade and ProposerInterface Abstraction
🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
manuelmauro
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
template/runtime/src/lib.rs (1)
861-865:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
relay_parent_offset()returns2butParachainSystem::RelayParentOffsetisConstU32<0>.These two values are semantically tied: the collator uses the runtime API to derive how far back to anchor the relay parent, while the parachain-system pallet enforces the on-chain offset. Returning
2here while configuringConstU32<0>at line 423 will cause the collator and pallet to disagree, which can lead to inherent/validation mismatches when block-building. The PR description also states the template default should be0. Please align them (likely return0, or expose a singleparameter_types!constant used in both places).🔧 Suggested fix
impl cumulus_primitives_core::RelayParentOffsetApi<Block> for Runtime { fn relay_parent_offset() -> u32 { - 2 + 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 `@template/runtime/src/lib.rs` around lines 861 - 865, The runtime API impl for cumulus_primitives_core::RelayParentOffsetApi currently returns 2 while the parachain-system pallet sets ParachainSystem::RelayParentOffset = ConstU32<0>, causing a mismatch; update the impl for Runtime::relay_parent_offset() to return 0 (or refactor both to read the same parameter by introducing a shared parameter type/constant) so the value used by the collator (relay_parent_offset) and the on-chain ParachainSystem::RelayParentOffset are identical.client/consensus/nimbus-consensus/src/collators/slot_based/mod.rs (1)
30-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale doc link to removed
find_potential_parents.Per the stable2603 API rename (
find_potential_parents→find_parent_for_building,PotentialParent→ParentSearchResult), this intra-doc link is now stale and will fail rustdoc resolution. Update to point to the new symbol.📝 Suggested fix
-//! 2. Retrieves the current best relay chain block and identifies a valid parent block (see -//! [find_potential_parents][cumulus_client_consensus_common::find_potential_parents] for parent -//! selection criteria) +//! 2. Retrieves the current best relay chain block and identifies a valid parent block (see +//! [find_parent_for_building][cumulus_client_consensus_common::find_parent_for_building] for +//! parent selection criteria)🤖 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 `@client/consensus/nimbus-consensus/src/collators/slot_based/mod.rs` around lines 30 - 32, Update the stale intra-doc link that references cumul_us_client_consensus_common::find_potential_parents to the new API names: replace references to find_potential_parents with find_parent_for_building and any mention of PotentialParent with ParentSearchResult in the doc comment block (e.g., the comment in slot_based::mod.rs that links to the parent-selection helper), ensuring the intra-doc link target is cumul_us_client_consensus_common::find_parent_for_building (and update the bracketed link text if needed) so rustdoc resolves correctly.
🧹 Nitpick comments (6)
client/consensus/nimbus-consensus/src/collators.rs (2)
254-289: 💤 Low value
find_parentmigration LGTM, but consider distinguishing transient from terminal failures in logs.The
Ok(None)branch logs awarn!saying "Could not find parent to build upon". This can fire frequently and benignly during startup or when the relay chain hasn't progressed enough yet, which makes warn-level noisy. Consider downgrading todebug!or qualifying the message; keeperror!only for theErr(_)case which already represents a real fetch failure.📝 Suggested fix
- Ok(None) => { - tracing::warn!( - target: crate::LOG_TARGET, - ?relay_parent, - "Could not find parent to build upon.", - ); - None - } + Ok(None) => { + tracing::debug!( + target: crate::LOG_TARGET, + ?relay_parent, + "No suitable parent to build upon at this relay parent.", + ); + None + }🤖 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 `@client/consensus/nimbus-consensus/src/collators.rs` around lines 254 - 289, The warning log in the Ok(None) branch of the find_parent flow (around the call to cumulus_client_consensus_common::find_parent_for_building::<Block>) should be downgraded to a lower-severity log because missing a parent can be transient; change the tracing::warn! that logs "Could not find parent to build upon." to tracing::debug! (or add a qualifier like "transient/no parents yet") so only real failures remain at error level (the Err(e) branch remains unchanged); update the log target and captured ?relay_parent usage exactly where the Ok(None) branch returns None.
54-61: 💤 Low value
PARENT_SEARCH_DEPTHis unused and should be removed.The constant at line 61 is not referenced anywhere in the codebase. The
ParentSearchParamsstruct now derives depth viaancestry_lookback(calculated fromscheduling_lookahead().saturating_sub(1)), eliminating the need for this hardcoded limit. Remove the constant and its explanatory comment to clean up the code.🤖 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 `@client/consensus/nimbus-consensus/src/collators.rs` around lines 54 - 61, Remove the unused constant and its comment: delete the PARENT_SEARCH_DEPTH constant declaration and its explanatory comment block since ParentSearchParams now derives depth via ancestry_lookback (from scheduling_lookahead().saturating_sub(1)); ensure no other code references PARENT_SEARCH_DEPTH and leave ParentSearchParams/ancestry_lookback logic intact.Cargo.toml (1)
221-223: ⚡ Quick winPin the Frontier fork to a
revand track migration back to upstream.These three deps now point to
moonbeam-foundation/frontieron a feature branch (mb/polkadot-sdk-stable2603). Branch-only git refs are reproducibility hazards: the branch can be force-pushed, deleted, or rebased, which silently changes consumer builds and underminesCargo.lockguarantees for downstream consumers. Please pin arev = "<commit-sha>"for each of these three crates until upstreampolkadot-evm/frontierlands stable2603 and you can revert. Consider tracking the revert with aTODO/issue link next to these entries.🔧 Suggested change
-fp-evm = { git = "https://github.com/moonbeam-foundation/frontier", branch = "mb/polkadot-sdk-stable2603", default-features = false } -pallet-evm = { git = "https://github.com/moonbeam-foundation/frontier", branch = "mb/polkadot-sdk-stable2603", default-features = false } -precompile-utils = { git = "https://github.com/moonbeam-foundation/frontier", branch = "mb/polkadot-sdk-stable2603", default-features = false } +# TODO: revert to polkadot-evm/frontier once stable2603 lands upstream. +fp-evm = { git = "https://github.com/moonbeam-foundation/frontier", branch = "mb/polkadot-sdk-stable2603", rev = "<pin-sha>", default-features = false } +pallet-evm = { git = "https://github.com/moonbeam-foundation/frontier", branch = "mb/polkadot-sdk-stable2603", rev = "<pin-sha>", default-features = false } +precompile-utils = { git = "https://github.com/moonbeam-foundation/frontier", branch = "mb/polkadot-sdk-stable2603", rev = "<pin-sha>", default-features = false }🤖 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 `@Cargo.toml` around lines 221 - 223, The three git dependencies fp-evm, pallet-evm, and precompile-utils are using a branch ref which is a reproducibility risk; replace the branch-only refs with pinned commit SHAs by adding rev = "<commit-sha>" (e.g. fp-evm = { git = "...", rev = "<sha>", default-features = false }) for each crate, optionally keep branch for human context, then run cargo update to refresh Cargo.lock; also add a short TODO comment next to each entry indicating this is a temporary pin and include an issue/link to revert when upstream polkadot-evm/frontier lands stable2603.precompiles/xcm-utils/src/lib.rs (1)
22-22: 💤 Low valueRemove the unused
ImbalanceAccountingimport.The import on line 22 is not used in any actual code. The
.amount()method called on line 185 is inherent to theCredittype and does not require the trait to be in scope. The import only appears in comments documenting the change in stable2603.🤖 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 `@precompiles/xcm-utils/src/lib.rs` at line 22, Remove the unused import ImbalanceAccounting from the precompiles xcm-utils lib (it's not referenced by code and only appears in comments); delete the use frame_support::traits::tokens::imbalance::ImbalanceAccounting; line so the compiler no longer warns, and leave the Credit type and its .amount() usage intact (no trait in-scope change needed).client/consensus/nimbus-consensus/src/proposer.rs (1)
38-58: ⚡ Quick winAdd
#[source]toinnerto guarantee correctError::source()delegation.
#[error(transparent)]on a struct is shown in thiserror's canonical example as a tuple structpub struct PublicError(#[from] ErrorRepr)— the#[from](or#[source]) annotation explicitly marks the transparent field. For a named-field struct without either annotation,thiserrormay not wireError::source()to delegate throughinner. Without this, error-chain reporters that traversesource()will stop atErrorand drop the fullanyhowcontext chain.🔧 Proposed fix
#[derive(thiserror::Error, Debug)] #[error(transparent)] pub struct Error { - inner: anyhow::Error, + #[source] + inner: anyhow::Error, }🤖 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 `@client/consensus/nimbus-consensus/src/proposer.rs` around lines 38 - 58, The Error struct's inner anyhow::Error field lacks the #[source] annotation so thiserror may not delegate Error::source() through inner; update the struct declaration for Error to annotate the inner field with #[source] (i.e., mark the inner field as the source) so that source() and error-chain traversal correctly delegate through inner; keep the existing deriving and the proposer_creation and proposing constructors unchanged aside from this annotation.client/consensus/nimbus-consensus/src/collators/lookahead.rs (1)
24-24: 💤 Low valueRedundant import —
ProposerInterfaceis already in scope via theuse crate::*glob on line 17.This explicit re-import at Line 24 is a no-op but may produce an
unused_importwarning under stricter lint settings.🔧 Suggested fix
-use crate::ProposerInterface;🤖 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 `@client/consensus/nimbus-consensus/src/collators/lookahead.rs` at line 24, Remove the redundant explicit import of ProposerInterface (the `use crate::ProposerInterface;` line) because it is already brought into scope by the existing `use crate::*;` glob; delete that single import to avoid unused_import warnings and keep the module imports consistent.
🤖 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 `@pallets/author-inherent/src/exec.rs`:
- Around line 98-100: Remove the redundant call to
I::verify_and_remove_seal(block) inside the BlockExecutor flow: BlockExecutor
already performs Nimbus seal verification/removal, so delete the delegation to
I::verify_and_remove_seal before calling execute_verified_block; ensure the code
only relies on the single seal removal performed by BlockExecutor (and does not
assume frame_executive::Executive::verify_and_remove_seal is a no-op) so any
ExecuteBlock implementation will receive an already-unsealed block when
execute_verified_block is invoked.
---
Outside diff comments:
In `@client/consensus/nimbus-consensus/src/collators/slot_based/mod.rs`:
- Around line 30-32: Update the stale intra-doc link that references
cumul_us_client_consensus_common::find_potential_parents to the new API names:
replace references to find_potential_parents with find_parent_for_building and
any mention of PotentialParent with ParentSearchResult in the doc comment block
(e.g., the comment in slot_based::mod.rs that links to the parent-selection
helper), ensuring the intra-doc link target is
cumul_us_client_consensus_common::find_parent_for_building (and update the
bracketed link text if needed) so rustdoc resolves correctly.
In `@template/runtime/src/lib.rs`:
- Around line 861-865: The runtime API impl for
cumulus_primitives_core::RelayParentOffsetApi currently returns 2 while the
parachain-system pallet sets ParachainSystem::RelayParentOffset = ConstU32<0>,
causing a mismatch; update the impl for Runtime::relay_parent_offset() to return
0 (or refactor both to read the same parameter by introducing a shared parameter
type/constant) so the value used by the collator (relay_parent_offset) and the
on-chain ParachainSystem::RelayParentOffset are identical.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 221-223: The three git dependencies fp-evm, pallet-evm, and
precompile-utils are using a branch ref which is a reproducibility risk; replace
the branch-only refs with pinned commit SHAs by adding rev = "<commit-sha>"
(e.g. fp-evm = { git = "...", rev = "<sha>", default-features = false }) for
each crate, optionally keep branch for human context, then run cargo update to
refresh Cargo.lock; also add a short TODO comment next to each entry indicating
this is a temporary pin and include an issue/link to revert when upstream
polkadot-evm/frontier lands stable2603.
In `@client/consensus/nimbus-consensus/src/collators.rs`:
- Around line 254-289: The warning log in the Ok(None) branch of the find_parent
flow (around the call to
cumulus_client_consensus_common::find_parent_for_building::<Block>) should be
downgraded to a lower-severity log because missing a parent can be transient;
change the tracing::warn! that logs "Could not find parent to build upon." to
tracing::debug! (or add a qualifier like "transient/no parents yet") so only
real failures remain at error level (the Err(e) branch remains unchanged);
update the log target and captured ?relay_parent usage exactly where the
Ok(None) branch returns None.
- Around line 54-61: Remove the unused constant and its comment: delete the
PARENT_SEARCH_DEPTH constant declaration and its explanatory comment block since
ParentSearchParams now derives depth via ancestry_lookback (from
scheduling_lookahead().saturating_sub(1)); ensure no other code references
PARENT_SEARCH_DEPTH and leave ParentSearchParams/ancestry_lookback logic intact.
In `@client/consensus/nimbus-consensus/src/collators/lookahead.rs`:
- Line 24: Remove the redundant explicit import of ProposerInterface (the `use
crate::ProposerInterface;` line) because it is already brought into scope by the
existing `use crate::*;` glob; delete that single import to avoid unused_import
warnings and keep the module imports consistent.
In `@client/consensus/nimbus-consensus/src/proposer.rs`:
- Around line 38-58: The Error struct's inner anyhow::Error field lacks the
#[source] annotation so thiserror may not delegate Error::source() through
inner; update the struct declaration for Error to annotate the inner field with
#[source] (i.e., mark the inner field as the source) so that source() and
error-chain traversal correctly delegate through inner; keep the existing
deriving and the proposer_creation and proposing constructors unchanged aside
from this annotation.
In `@precompiles/xcm-utils/src/lib.rs`:
- Line 22: Remove the unused import ImbalanceAccounting from the precompiles
xcm-utils lib (it's not referenced by code and only appears in comments); delete
the use frame_support::traits::tokens::imbalance::ImbalanceAccounting; line so
the compiler no longer warns, and leave the Credit type and its .amount() usage
intact (no trait in-scope change needed).
🪄 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: b3137481-78ea-4e9f-b352-644ac177cc65
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
Cargo.tomlclient/consensus/nimbus-consensus/Cargo.tomlclient/consensus/nimbus-consensus/src/collator.rsclient/consensus/nimbus-consensus/src/collators.rsclient/consensus/nimbus-consensus/src/collators/basic.rsclient/consensus/nimbus-consensus/src/collators/lookahead.rsclient/consensus/nimbus-consensus/src/collators/slot_based/block_builder_task.rsclient/consensus/nimbus-consensus/src/collators/slot_based/collation_task.rsclient/consensus/nimbus-consensus/src/collators/slot_based/mod.rsclient/consensus/nimbus-consensus/src/lib.rsclient/consensus/nimbus-consensus/src/manual_seal.rsclient/consensus/nimbus-consensus/src/proposer.rspallets/author-inherent/src/exec.rspallets/author-mapping/src/mock.rspallets/randomness/src/types.rsprecompiles/xcm-utils/src/lib.rsprimitives/session-keys/src/digest.rstemplate/node/Cargo.tomltemplate/node/src/service.rstemplate/runtime/src/lib.rs
💤 Files with no reviewable changes (1)
- template/node/Cargo.toml
Fix the rustfmt drift and the dead-code error on PARENT_SEARCH_DEPTH that was blocking build, build-features, and rust-test (the latter at compile time, masking several dormant blockers in the workspace). With the lint unblocked, address the now-surfaced issues: - Switch to the new BlockLength builder, scale_info String for the benchmark API, and ProposerFactory::new (deprecation removals). - Add TransactionGasLimit to all pallet_evm mocks (new stable2603 item). - Update xcm-utils and pallet-xcm mocks for the new TransactAsset and WeightTrader signatures, AssetsInHolding API, and removed AssetClaims. - Recalibrate assets-erc20 expect_cost values for the new pallet-assets weights. - Drop pre-existing unused imports (fp_evm::ExitError, sp_std::vec) surfaced by the unblocked build. Review feedback: - Drop the redundant I::verify_and_remove_seal call in author-inherent; BlockExecutor already strips the seal before delegating. - Align template runtime relay_parent_offset() with ConstU32<0> to match the parachain-system config. - Update stale find_potential_parents intra-doc link to find_parent_for_building. - Remove redundant `use crate::ProposerInterface` re-imports already brought in via `use crate::*`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@precompiles/xcm-utils/src/lib.rs`:
- Around line 161-183: The call to
xcm_executor::test_helpers::mock_asset_to_holding in the trader.buy_weight path
is gated behind the xcm-executor "runtime-benchmarks" feature and is unavailable
with the current dependency features; either enable that feature in the crate's
Cargo.toml for xcm-executor (add features = ["runtime-benchmarks"]) so
mock_asset_to_holding can be used, or replace the helper by constructing an
AssetsInHolding equivalent directly (build an AssetsInHolding/holding that maps
multiasset.id to an ImbalanceAccounting-like value or a holding type whose
.amount() yields the u128 balance) and pass that into trader.buy_weight (the
variables to change are the mock_asset_to_holding call, the unused binding
returned by trader.buy_weight, and subsequent use of
unused.fungible.get(&multiasset.id).map(|credit| credit.amount())).
🪄 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: ceb144c0-cd90-4997-b126-87c1a8d668e8
📒 Files selected for processing (20)
client/consensus/nimbus-consensus/src/collator.rsclient/consensus/nimbus-consensus/src/collators.rsclient/consensus/nimbus-consensus/src/collators/basic.rsclient/consensus/nimbus-consensus/src/collators/lookahead.rsclient/consensus/nimbus-consensus/src/collators/slot_based/block_builder_task.rsclient/consensus/nimbus-consensus/src/collators/slot_based/mod.rspallets/author-inherent/src/exec.rspallets/author-mapping/src/mock.rsprecompiles/assets-erc20/src/lib.rsprecompiles/assets-erc20/src/mock.rsprecompiles/assets-erc20/src/tests.rsprecompiles/balances-erc20/src/mock.rsprecompiles/batch/src/mock.rsprecompiles/call-permit/src/mock.rsprecompiles/pallet-xcm/src/mock.rsprecompiles/proxy/src/mock.rsprecompiles/xcm-utils/src/lib.rsprecompiles/xcm-utils/src/mock.rstemplate/node/src/service.rstemplate/runtime/src/lib.rs
💤 Files with no reviewable changes (1)
- client/consensus/nimbus-consensus/src/collators/basic.rs
✅ Files skipped from review due to trivial changes (3)
- precompiles/assets-erc20/src/mock.rs
- client/consensus/nimbus-consensus/src/collators/slot_based/mod.rs
- precompiles/assets-erc20/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- pallets/author-mapping/src/mock.rs
- template/runtime/src/lib.rs
- client/consensus/nimbus-consensus/src/collators/slot_based/block_builder_task.rs
- client/consensus/nimbus-consensus/src/collators/lookahead.rs
- client/consensus/nimbus-consensus/src/collators.rs
Make `RelayParentOffsetApi::relay_parent_offset()` defer to `<Runtime as cumulus_pallet_parachain_system::Config>::RelayParentOffset::get()` so the off-chain advertised offset and the on-chain enforced offset share a single source of truth. Previously the two values had to be kept in sync by convention, which is what allowed the `2` vs `ConstU32<0>` mismatch that 0df632f had to correct.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
template/runtime/src/lib.rs (1)
425-426: ⚡ Quick winConfirm intentional behavioral change: template
relay_parent_offset()now returns0(was hardcoded2).With
type RelayParentOffset = ConstU32<0>;at Line 425, theRelayParentOffsetApi::relay_parent_offset()runtime API now returns0rather than the previously hardcoded2. This is consistent with the PR commit message (single source of truth) and the template node update torelay_parent_offset: 0inMockValidationDataInherentDataProvider.Worth calling out explicitly in release/migration notes since downstream users copying this template (or relying on the previous offset of
2) will see different async-backing behavior. Downstream runtimes (e.g., Moonbeam) should setRelayParentOffsetto whatever value they actually want advertised by the runtime API.Also applies to: 863-867
🤖 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 `@template/runtime/src/lib.rs` around lines 425 - 426, The runtime now sets RelayParentOffset = ConstU32<0>, so RelayParentOffsetApi::relay_parent_offset() returns 0 instead of the previous hardcoded 2; either revert the type alias to ConstU32<2> in the runtime (change type RelayParentOffset = ConstU32<0> back to ConstU32<2> if you want the old behavior) or, if 0 is intended, add an explicit migration/release note and update MockValidationDataInherentDataProvider’s relay_parent_offset value and any downstream docs/examples to call out this breaking behavioral change so integrators (e.g., Moonbeam) can choose their desired RelayParentOffset.
🤖 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 `@template/runtime/src/lib.rs`:
- Around line 425-426: The runtime now sets RelayParentOffset = ConstU32<0>, so
RelayParentOffsetApi::relay_parent_offset() returns 0 instead of the previous
hardcoded 2; either revert the type alias to ConstU32<2> in the runtime (change
type RelayParentOffset = ConstU32<0> back to ConstU32<2> if you want the old
behavior) or, if 0 is intended, add an explicit migration/release note and
update MockValidationDataInherentDataProvider’s relay_parent_offset value and
any downstream docs/examples to call out this breaking behavioral change so
integrators (e.g., Moonbeam) can choose their desired RelayParentOffset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c075ed8a-9167-41f4-bb22-77c0811a4ae9
📒 Files selected for processing (1)
template/runtime/src/lib.rs
`polkadot-evm/frontier#1892` merged, cutting `stable2603` from the canonical upstream. Drop the temporary `moonbeam-foundation/frontier: mb/polkadot-sdk-stable2603` fork in favour of `polkadot-evm/frontier: stable2603` for `fp-evm`, `pallet-evm`, and `precompile-utils`.
- ProposerInterface::propose: drop the always-`Some` Option from the return type. The impl never yields `Ok(None)`, so the caller's `match` arm was dead code; collator.rs simplified accordingly. - lookahead collator: log at debug level when `find_parent_for_building` returns no parent, instead of silently skipping the slot. - xcm-utils precompile: note that `xcm_executor::test_helpers` is a test-only upstream module and should be replaced with a stable API before relying on it long-term.
npmjs.com returns HTTP 403 to the link-checker bot, which failed the check-links job on otherwise-valid links in template/README.md.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@precompiles/xcm-utils/src/lib.rs`:
- Around line 168-176: The code is calling the internal test helper
xcm_executor::test_helpers::mock_asset_to_holding from trader.buy_weight, which
is unstable for production; replace that call with a local stable adapter (e.g.,
a new function in this crate) that converts the MultiAsset (multiasset) into the
required Holding type and use that adapter when calling trader.buy_weight with
the same Weight and &ctx arguments; implement the adapter to mirror the minimal
behavior needed by trader.buy_weight so the precompile no longer depends on
xcm_executor::test_helpers::mock_asset_to_holding.
🪄 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: 79b14d7d-6560-4496-96a6-a8980461c486
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.linkspector.ymlCargo.tomlclient/consensus/nimbus-consensus/src/collator.rsclient/consensus/nimbus-consensus/src/collators/lookahead.rsclient/consensus/nimbus-consensus/src/proposer.rsprecompiles/xcm-utils/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- client/consensus/nimbus-consensus/src/collator.rs
- client/consensus/nimbus-consensus/src/collators/lookahead.rs
… adapter Stop depending on xcm_executor::test_helpers::mock_asset_to_holding in the get_units_per_second precompile. The upstream test_helpers module is internal- only and may change or be removed without a deprecation cycle. Add a moonkit- local HoldingCredit + asset_to_holding adapter that builds the AssetsInHolding needed by WeightTrader::buy_weight using only stable public traits.
…lSealConsensusDataProvider The `_phantom: PhantomData<P>` field existed to carry the `P` generic. `P` was removed during the stable2603 manual-seal type simplification and the field became `PhantomData<()>` — a no-op zero-sized field with no variance, drop, or lifetime info to carry. Drop the field and its template construction site.
…odule The trait-glue needed to feed WeightTrader::buy_weight in the fee-estimation precompile is mostly type-system boilerplate, not precompile logic. Pull it out into `holding.rs` so lib.rs stays focused on the precompile surface.
Replace the npmjs.com package link in the template README with the Zombienet docs install page anchored at the "Using NPM" section, which documents the same install command. Revert the linkspector ignorePatterns hack — the remaining links resolve cleanly.
The file only set `dirs: - .`, which is linkspector's default. The CI step invokes the action without a `config_file:` parameter, so removing the file falls back to the same default scan.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Updates moonkit's
polkadot-sdkdependency fromstable2512tostable2603(polkadot-stable2603-1). Required for downstream consumers (Moonbeam) to track the latest stable cycle.Migrations driven by upstream API drift
sp_core/sp_runtime. Switched derives and imports tocore::fmt::Debug.verify_and_remove_seal+execute_verified_block.pallet-author-inherent'sBlockExecutornow implements both and delegates seal-stripping appropriately.Proofassociated type and now takes a concreteStorageProof. Dropped the now-unusedPgeneric fromNimbusManualSealConsensusDataProvider.cumulus-client-consensus-proposerremoved upstream (paritytech/polkadot-sdk#9947). Vendored a moonkit-localProposerInterfaceinsidenimbus-consensusthat wraps the newsp_consensus::ProposeArgsAPI. The collator now owns theProofRecorder, registersProofSizeExt, and drains the proof after proposing — preserves prior proof-recording semantics.ProposerFactorydropped itsEnableProofRecordinggeneric.find_potential_parents/PotentialParentremoved; switched tofind_parent_for_building/ParentSearchResult.ParentSearchParamslostmax_depthandignore_alternative_branches; "depth" now derived from header numbers viaSaturating.ParachainInherentDataProvider::create_attakesRelayProofRequestinstead ofVec<Vec<u8>>. Raw additional keys wrapped asRelayStorageKey::Top.SubmitCollationParams.scheduling_parentadded; passingNonepreserves V2 candidate-descriptor semantics (set toSome(hash)to opt into V3).Config::AssetClaimsremoved; the newTrapAndClaimAssetsbound onAssetTrapcovers both.AssetsInHoldingnow tracksImbalanceAccountingcredits, no longerFrom<Vec<Asset>>. Thexcm-utilsprecompile usesxcm_executor::test_helpers::mock_asset_to_holdingfor its fee-estimation read path and unwraps credit values via.amount().sp_session::SessionKeysruntime API v2:generate_session_keys(seed) -> Vec<u8>becamegenerate_session_keys(owner, seed) -> OpaqueGeneratedSessionKeys.sc_service::BuildNetworkParamsgainedspawn_essential_handle;new_full_parts/new_full_parts_record_importgained apruning_filtersparameter. Threaded through the template node.MockValidationDataInherentDataProvidergainedrelay_parent_offset(template defaults to0).Frontier
fp-evm,pallet-evm, andprecompile-utilspoint at the officialpolkadot-evm/frontier:stable2603branch — polkadot-evm/frontier#1892 (the stable2603 base bump) has merged and upstream cutstable2603from it.evmcontinues to trackrust-ethereum/evm:v0.x.Test plan
cargo check --workspace --all-featuresis clean (a handful ofBlockLength::max_with_normal_ratioandRuntimeStringdeprecation warnings, scheduled for removal after July 2026).cargo test --workspacein CI.--devboot, parachain block production via slot-based collator.ProofRecorderflow).