Integrate asynchronous backing for 6000ms block time#1447
Conversation
WalkthroughAdds async backing and relay-timestamp wiring, halves block time from 12s→6s across runtimes, updates workspace/runtime dependencies and migrations, adds migration scaffolding and benchmark/weight wiring, introduces test-net synchronization helpers and extensive integration-test/script/tooling changes. Changes
Sequence Diagram(s)sequenceDiagram
participant RelayChain as Relay Chain
participant RelayTS as RelayTimestamp Wrapper
participant Consensus as ConsensusHook<br/>(FixedVelocity)
participant Runtime as Runtime<br/AsyncBacking
Note over RelayChain,Runtime: Block validation with relay timestamp injection
RelayChain->>RelayTS: provide state_proof (contains TIMESTAMP_NOW)
RelayTS->>RelayTS: read_entry(TIMESTAMP_NOW) from proof
alt TIMESTAMP_NOW present
RelayTS->>RelayTS: store RelayTimestampNow
else Absent/fallback
RelayTS->>RelayTS: use last-known or 0 (warn)
end
RelayTS->>Consensus: call on_state_proof(wrapped)
Consensus->>Runtime: query slot via SlotProvider -> AsyncBacking::slot_info()
Runtime->>Runtime: AsyncBacking may update slot_info
Consensus-->>RelayTS: return (Weight + 1 write)
RelayTS-->>RelayChain: accept/reject decision
sequenceDiagram
participant Test as Test Suite
participant Mutex as TEST_NET_MUTEX
participant Reset as reset_test_net()
participant Wrapper as with_...()
participant Para as Para::execute_with
Note over Test,Wrapper: Serialized test-net usage to avoid races
Test->>Mutex: lock (TEST_NET_MUTEX)
Mutex-->>Reset: guard acquired
Reset->>Reset: reset test network
Test->>Wrapper: invoke wrapper(closure)
Wrapper->>Para: Para::execute_with(closure)
Para->>Para: run closure
Para-->>Wrapper: result
Wrapper->>Wrapper: optional clear_async_backing_slot_info()
Wrapper-->>Test: return result
Test->>Mutex: drop guard (unlock)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas to focus review on:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
Chralt98
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
🧹 Nitpick comments (1)
runtime/common/src/lib.rs (1)
689-697: Consider logging when slot_info is None.The
unwrap_or_default()on line 695 silently falls back to the default slot value whenslot_info()returnsNone. This might hide issues during runtime.Consider adding logging or a debug assertion to track when slot information is unavailable:
impl Get<Slot> for RelayChainSlotProvider { fn get() -> Slot { let slot_info = pallet_async_backing::pallet::Pallet::<Runtime>::slot_info(); - slot_info.unwrap_or_default().0 + slot_info + .map(|(slot, _)| slot) + .unwrap_or_else(|| { + log::debug!("Relay slot info unavailable, using default"); + Slot::default() + }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(1 hunks)Makefile(1 hunks)node/src/command.rs(2 hunks)primitives/src/constants.rs(2 hunks)runtime/battery-station/Cargo.toml(4 hunks)runtime/battery-station/src/parachain_params.rs(3 hunks)runtime/common/src/lib.rs(8 hunks)runtime/common/src/relay_timestamp.rs(1 hunks)runtime/zeitgeist/Cargo.toml(4 hunks)runtime/zeitgeist/src/parachain_params.rs(3 hunks)
🔇 Additional comments (10)
node/src/command.rs (2)
514-525: LGTM: Consistent async backing enablement.The change correctly mirrors the Zeitgeist runtime configuration, ensuring both runtimes have async backing enabled consistently.
501-512: I need to investigate further whether theMultiplyRoundLenBy2migration is actually required and how pallet parachain staking is being handled.Let me search for more context on whether this migration is specific to this project or an actual requirement.
Let me search the codebase directly for migration-related code and runtime configuration:
Based on my comprehensive verification of the codebase and external documentation, I can now provide you with the rewritten review comment:
Verify async backing activation prerequisites.
The change to enable async backing (setting the 4th parameter to
true) is correct and the runtime infrastructure is properly configured. The async backing setup includes pallet_aura configured with AllowMultipleBlocksPerSlot = true, UNINCLUDED_SEGMENT_CAPACITY = 3, BLOCK_PROCESSING_VELOCITY = 1, and MILLISECS_PER_BLOCK = 6000.All configuration components verified:
- ✓ pallet_async_backing is integrated with FixedVelocityConsensusHook
- ✓ MILLISECS_PER_BLOCK: 6000ms configured
- ✓ UNINCLUDED_SEGMENT_CAPACITY: 3 configured
- ✓ BLOCK_PROCESSING_VELOCITY: 1 configured
However, the
MultiplyRoundLenBy2migration for pallet-parachain-staking referenced in issue #1435 was not found in the codebase. With parachain-staking using 600 blocks per round, halving the block time from 12s to 6s would reduce round duration from 2 hours to 1 hour, which may require adjustments to ensure the staking round schedule remains appropriate.Please confirm:
- Whether the
MultiplyRoundLenBy2migration has been implemented elsewhere or if it's being handled through a different mechanism- That pallet-parachain-staking round length is correctly configured for the new 6-second block time
- That integration tests pass with the updated block timing
runtime/common/src/lib.rs (8)
62-63: LGTM! Relay timestamp module properly integrated.The module is correctly feature-gated for parachain builds and is appropriately used in the ConsensusHook configuration below.
406-406: LGTM! AsyncBacking pallet properly declared.The pallet index (114) is correctly sequenced, and the included components (Pallet, Storage) are appropriate for this system pallet.
463-463: Critical change for async backing support.Switching from
RelayNumberStrictlyIncreasestoRelayNumberMonotonicallyIncreasesis essential for async backing, as it allows multiple parachain blocks to be produced within the same relay chain slot without rejection.
726-726: LGTM! SlotProvider correctly updated.The update to use
RelayChainSlotProvideraligns with the slot provider implementation changes and ensures parachain staking uses relay chain slot information from the async backing pallet.
2210-2217: Critical implementation for async backing functionality.The change from a hardcoded
falsereturn to delegating toConsensusHook::can_build_upon(included_hash, slot)enables the async backing consensus mechanism. This is essential for the feature to work correctly.
464-478: Verified: Issue #1426 is properly tracked.Issue #1426 exists and is open in the zeitgeistpm/zeitgeist repository with the title "Use production consensus hook for xcm integration tests." The issue documents the technical constraints: using
BLOCK_PROCESSING_VELOCITY = 1with the relay timestamp wrapper fails in XCM integration tests because the environment cannot produce multiple parachain blocks per relaychain block. The test-specific configuration withBLOCK_PROCESSING_VELOCITY = 2is a pragmatic interim solution pending resolution of the underlying integration test environment limitations.
572-577: The parameter types are properly defined and in scope.The
AllowMultipleBlocksPerSlotandExpectedBlockTimetypes are correctly brought into the battery-station runtime scope via thepub use crate::parachain_params::*;andpub use crate::parameters::*;statements (lines 42–43), which occurs beforecreate_runtime_with_additional_pallets!()is invoked. When thecreate_runtime!macro expands at the call site, these identifiers are available and resolve correctly. TheExpectedBlockTimeis set to6_000milliseconds as required.Likely an incorrect or invalid review comment.
446-451: Verify constant definitions are imported.The
ConsensusHooktype alias referencesBLOCK_PROCESSING_VELOCITYandUNINCLUDED_SEGMENT_CAPACITYconstants. Ensure these constants are properly defined and imported into scope.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
runtime/common/src/lib.rs (1)
119-125: Wire the staking round-length migrationDropping the block time to 6 s halves the effective staking round unless storage is migrated. Issue #1435 explicitly calls for
MultiplyRoundLenBy2, butMigrationsstill omits it. Ship the runtime with that migration (or equivalent logic) so existing on-chain rounds are doubled during the upgrade.#[cfg(feature = "parachain")] type Migrations = ( RemoveDmpQueue, pallet_xcm::migration::MigrateToLatestXcmVersion<Runtime>, + pallet_parachain_staking::migration::MultiplyRoundLenBy2<Runtime>, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
primitives/src/constants.rs(2 hunks)runtime/common/src/lib.rs(8 hunks)runtime/common/src/relay_timestamp.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
…nsaction version to 35
…tor snapshot weight function
…handling - Added a function to determine the Polkadot release version, allowing for overrides. - Enhanced the binary download process with improved logging and error handling. - Updated the script to ensure binaries are executable after download. - Changed the copyright year in the script to 2025. - Removed hardcoded Polkadot release version and replaced it with dynamic determination. fix(tests): Adjust XCM fee upper limit for forked networks - Increased the upper limit for XCM fees in common tests to accommodate forked networks. test: Add logging to runtime upgrade tests for better traceability - Introduced logging hooks in the runtime upgrade tests to capture key steps and state changes during execution. chore: Update zombienet configuration for RPC ports - Modified the produce-blocks.toml file to specify unique RPC ports for relaychain nodes.
Chralt98
added
s:review-needed
…matting in relay timestamp module
Chralt98
added
s:review-needed
mergify
bot
added
s:in-progress
Chralt98
added
s:review-needed
mergify
bot
added
s:in-progress
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
runtime/common/src/relay_timestamp.rs (1)
43-101: Missingcan_build_upondelegation blocks async backing.As noted in the previous review, the
ConsensusHookWrapperForRelayTimestampdoesn't overridecan_build_upon, which means it defaults tofalseand prevents async backing from functioning properly. The wrapper must delegate this call toInner::can_build_uponto preserve the intended behavior.Please add the delegation as suggested in the previous review.
🧹 Nitpick comments (2)
integration-tests/README.md (1)
92-92: Add rationale for--runInBandflag in documentation.The three test commands now consistently use the
--runInBandflag to run tests sequentially rather than in parallel. While this change aligns with the async backing integration and block time reduction, the documentation doesn't explain why sequential execution is necessary.Consider adding a brief note in the relevant sections (e.g., in a new subsection or inline comment) explaining that this flag is required due to timing-sensitive block time changes introduced by async backing and that running tests sequentially prevents race conditions.
This could be added as a comment or note, for example:
#### Test the upgrade to the WASM from `./target/release/wbuild/zeitgeist-runtime` on zombienet: +Note: The `--runInBand` flag runs tests sequentially to handle timing-sensitive block time changes from async backing. + ```bash -pnpm exec moonwall test zombienet_zeitgeist_upgrade +pnpm exec moonwall test zombienet_zeitgeist_upgrade --runInBandPlease verify that this flag change is consistently applied in the actual test configuration (e.g., `integration-tests.yml` or test execution scripts) and confirm whether other test commands in the integration test suite should also be updated. Also applies to: 98-98, 104-104 </blockquote></details> <details> <summary>integration-tests/scripts/download-polkadot.sh (1)</summary><blockquote> `73-88`: **Clarify asymmetric fallback behavior and improve error state management.** The download logic has two concerns: 1. **Asymmetric fallback:** For tagged/branch releases (version != "latest"), only GitHub is attempted; if GitHub fails, the entire script exits. For "latest", only moonwall is attempted. This may be intentional (GitHub releases may not exist for "latest", or moonwall may not support specific tags), but the asymmetry is not documented. If a GitHub release is misconfigured or temporarily unavailable, there's no fallback. 2. **Global error state:** `LAST_ERROR_LOG` (line 85) is a global variable set only when `run_moonwall_download()` fails. Using a global to pass error messages between functions is fragile and can cause issues if additional calls are added in the future. A more robust pattern would capture stderr directly in the caller or return error messages via stdout. If fallback behavior is desired for tagged releases, consider updating `download_binary()` to fallback to moonwall if GitHub fails: ```bash download_binary() { local name="$1" local version="$2" if [[ "${version}" != "latest" ]]; then if download_from_github "${name}" "${version}"; then return fi echo "GitHub download failed for ${name} ${version}, attempting moonwall fallback..." if run_moonwall_download "${name}" "${version}"; then return fi echo "Failed to download ${name} ${version} from GitHub and moonwall." echo "${LAST_ERROR_LOG}" exit 1 fi if run_moonwall_download "${name}" "${version}"; then return fi echo "Failed to download ${name}:" echo "${LAST_ERROR_LOG}" exit 1 }Alternatively, to improve error handling, capture stderr directly without relying on a global:
run_moonwall_download() { local name="$1" local version="$2" if pnpm moonwall download "${name}" "${version}" tmp 2>&1; then chmod +x "tmp/${name}" return 0 fi return 1 } # In the caller: if ! run_moonwall_download "${name}" "${version}"; then local error_msg error_msg=$(run_moonwall_download "${name}" "${version}" 2>&1 || true) echo "Failed: ${error_msg}" exit 1 fiAlso applies to: 90-105, 107-126
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
integration-tests/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.github/workflows/integration-tests.yml(3 hunks)integration-tests/README.md(1 hunks)integration-tests/configs/hydradx.yml(1 hunks)integration-tests/configs/polkadot.yml(1 hunks)integration-tests/configs/zeitgeist.yml(1 hunks)integration-tests/configs/zombieZeitgeist.json(3 hunks)integration-tests/moonwall.config.json(1 hunks)integration-tests/package.json(2 hunks)integration-tests/scripts/download-polkadot.sh(2 hunks)integration-tests/tests/common-tests.ts(1 hunks)integration-tests/tests/rt-upgrade-zeitgeist-chopsticks/test-zeitgeist-chopsticks-runtime-upgrade.ts(1 hunks)integration-tests/zombienet/produce-blocks.toml(2 hunks)runtime/common/Cargo.toml(1 hunks)runtime/common/src/relay_timestamp.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- integration-tests/configs/polkadot.yml
🧰 Additional context used
🪛 GitHub Actions: Copyright
runtime/common/src/relay_timestamp.rs
[error] 2-2: check-license: expected copyright notice, found 'This file is part of Zeitgeist.'
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Coverage
- GitHub Check: Checks (parachain)
- GitHub Check: Checks (clippy)
- GitHub Check: Fuzz
- GitHub Check: Test parachain build
- GitHub Check: Checks (parachain)
- GitHub Check: Test standalone build
- GitHub Check: Checks (clippy)
- GitHub Check: Checks (standalone)
- GitHub Check: Quick check benchmarks
- GitHub Check: Format
- GitHub Check: Coverage
- GitHub Check: Summary
🔇 Additional comments (14)
.github/workflows/integration-tests.yml (1)
160-160: Verify the necessity and document the rationale for sequential test execution.The
--runInBandflag converts three test jobs from parallel to sequential execution. While this may be necessary given the 6s block time reduction and async backing changes, the rationale should be clearly documented.Please verify the following:
- Is sequential execution required for test correctness (to prevent timing/race conditions), or is it a temporary workaround?
- Are there documented performance implications for the CI pipeline?
- Does the integration-test README document this change and the underlying reason?
Additionally, ensure that all test jobs that might be affected by the block time reduction also include this flag if needed, to maintain consistency.
Also applies to: 208-208, 263-263
integration-tests/tests/rt-upgrade-zeitgeist-chopsticks/test-zeitgeist-chopsticks-runtime-upgrade.ts (2)
41-45: LGTM! Excellent addition for test observability.The
hookLoghelper provides consistent, prefixed logging to both console and test framework outputs, which will significantly improve debugging and tracing of the runtime upgrade flow—especially valuable when verifying async backing integration.
52-102: LGTM! Comprehensive logging coverage of the upgrade flow.The hookLog calls systematically trace each step of the runtime upgrade process—from initialization through API connections, spec validation, version capture, upgrade execution, and completion. This enhanced observability will be particularly helpful for diagnosing issues during async backing integration testing.
integration-tests/tests/common-tests.ts (1)
186-187: Verify the XCM fee bound increase is justified by the block time reduction.The upper bound for XCM fees was increased by 33% (from 1.5e9 to 2.0e9). While the comment mentions chopsticks variance, it doesn't explain whether this increase is directly related to the block time reduction from 12s to 6s or other async backing changes introduced in this PR.
Please clarify:
- Is this increase expected behavior due to the block time/async backing changes, or is it compensating for chopsticks-specific variance unrelated to this PR?
- What actual fee values were observed in testing that necessitated this adjustment?
- Could this overly generous bound mask potential fee calculation issues introduced by the runtime changes?
Consider running the XCM transfer tests multiple times to capture the actual fee range and set a bound that's tight enough to catch regressions but loose enough to accommodate expected variance.
integration-tests/configs/zeitgeist.yml (1)
1-2: Build-block-mode setting aligns with async backing objectives.The addition of
build-block-mode: Manualenables manual block production in tests, which is consistent with the PR's async backing support for 6000ms block time. This setting allows tests to control block production timing independently of runtime block time.integration-tests/configs/hydradx.yml (1)
2-2: Config additions for manual block production are consistent across test suites.The
build-block-mode: Manualandruntime-log-level: 5settings match those added to the Zeitgeist config, ensuring consistent test orchestration across chains. These settings support the async backing implementation with reduced (6s) block times.Also applies to: 6-6
integration-tests/zombienet/produce-blocks.toml (1)
12-25: LGTM! Port configuration updates are consistent.The migration from
ws_porttorpc_portacross all node configurations aligns with the similar updates inzombieZeitgeist.jsonand modernizes the test infrastructure.integration-tests/moonwall.config.json (1)
60-60: LGTM! Explicit path improves clarity.The change from
"polkadot"to"./configs/polkadot.yml"makes the configuration more explicit and aligns with the addition of the new Polkadot config file.runtime/common/Cargo.toml (1)
83-87: LGTM! Focused try-runtime feature addition.The new
try-runtimefeature enables testing and validation capabilities for the core runtime components. The limited scope (frame-system, frame-support, and pallet-timestamp) appears intentional, as other pallets likely handle their try-runtime features through their own dependency chains.integration-tests/configs/zombieZeitgeist.json (1)
13-31: LGTM! Consistent port configuration updates.The migration to
rpc_portfor all nodes (charlie, alice, bob) is consistent with the changes inproduce-blocks.tomland modernizes the Zombienet test configuration.integration-tests/scripts/download-polkadot.sh (2)
1-40: Script structure is solid; good defensive practices.Script initialization with
set -euo pipefail, early environment setup, and macOS check are all well-handled. The modular function design for release detection, binary validation, and download orchestration is clean and maintainable.
51-59: Cargo.lock git URL parsing works correctly.The regex successfully extracts the polkadot-sdk release tag from the Cargo.lock entry. The current format is properly handled and the tag extraction functions as intended.
integration-tests/package.json (2)
27-36: Clarify the necessity and scope of pnpm overrides.The new pnpm overrides block pins @PolkaDot and @polkadot/util versions to control transitive dependency resolution. However, @polkadot/types-support and @polkadot/util are not listed in devDependencies, meaning they are indirect dependencies. Verify that:
- These overrides are necessary to resolve conflicts (e.g., different transitive versions of @polkadot/types across chopsticks, @moonwall/cli, or other indirect consumers).
- The pinned versions don't conflict with the explicit versions in devDependencies or introduce breaking changes downstream in tests or runtime configurations.
Confirm that pnpm overrides for @PolkaDot packages are aligned with the PR's async backing integration and that pinning these transitive versions does not inadvertently downgrade or conflict with any other transitive requirements.
9-15: I need to gather more specific compatibility information. The initial search confirms no new breaking changes in the 16.1.1→16.5.2 range, but I need to verify the broader package ecosystem compatibility.I need to verify more details about Chopsticks 1.2.4 and @Moonwall compatibility. Let me search for package dependency information.
Version compatibility appears reasonable, but Chopsticks 1.2.4 and Moonwall 5.16.4 are pre-release versions—confirm they are intentional and test thoroughly.
Ensure @polkadot/util-crypto and @polkadot/keyring versions match with what the API depends on—your package.json correctly pairs both at 13.5.2, and @polkadot/api and @polkadot/types versions must match, which they do at 16.5.2.
The upgrade from @PolkaDot 16.1.1 to 16.5.2 carries no new breaking changes (both are in the Metadata v16 series). However, Chopsticks latest released version is 1.2.2, while your package.json specifies 1.2.4, and @moonwall/cli latest is 5.11.0, while you specify 5.16.4. These pre-release versions need verification—confirm whether these versions are intentional (from git dependencies or internal sources) and run your integration tests to validate async backing and relay timestamp handling work as expected.
Chralt98
added
s:review-needed
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1447 +/- ##
=======================================
Coverage 93.32% 93.32%
=======================================
Files 181 181
Lines 34769 34769
=======================================
Hits 32448 32448
Misses 2321 2321
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…locity for std builds
* Refactor relay timestamp logging and enhance parachain staking weight calculations - Updated logging in `relay_timestamp.rs` to improve readability of warning messages regarding missing TIMESTAMP_NOW. - Modified `migrate_old_collator_snapshot` function in `pallet_parachain_staking.rs` to accept a parameter for item count, adjusting weight calculations accordingly. - Added new features for legacy migration in `Cargo.toml` of the zeitgeist runtime. - Included migration functionality in the zeitgeist runtime library. * Remove legacy migration features and streamline parachain migration handling * Refactor parachain staking weights and add migration support - Removed the `migrate_old_collator_snapshot` function from the parachain staking weights file. - Updated `Cargo.toml` to include `pallet-migrations` in the Zeitgeist runtime. - Modified the Zeitgeist runtime to integrate migration logic and defined `MultiBlockMigrations`. - Enhanced benchmark configuration to include `pallet_migrations`. - Created `legacy_keys.rs` and `mod.rs` for runtime migrations in both battery-station and zeitgeist runtimes. - Implemented legacy migration structures for both battery-station and zeitgeist, allowing for the migration of legacy staking data. * Remove parachain feature flag from runtime migrations * feat: Add pallet_migrations support and update runtime configurations for benchmarks
Update copyright years in source files to include 2025 - Updated copyright notices in multiple Rust source files across the project to reflect the new year 2025. - Ensured consistency in copyright formatting, changing from "2023-2024" to "2023-2025" where applicable. - This change affects files in the `liquidity_tree`, `orderbook`, `parimutuel`, `prediction-markets`, `swaps`, and `styx` modules.
|
This pull request is now in conflicts. Could you fix it @Chralt98? 🙏 |
mergify
bot
added
s:revision-needed
mergify
bot
added
s:in-progress
Chralt98
added
s:review-needed
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
runtime/zeitgeist/src/integration_tests/xcm/tests/mod.rs (1)
48-55: Same panic safety consideration aswith_zeitgeist.This function has the same cleanup pattern as
with_zeitgeist(lines 39-46) and shares the same panic safety consideration. If adopting a guard-based pattern forwith_zeitgeist, apply the same pattern here.runtime/battery-station/Cargo.toml (1)
135-135: Use workspace dependency foronce_cell.Same issue as in
runtime/zeitgeist/Cargo.toml- theonce_celldependency is hardcoded to version "1.20.2" instead of using a workspace reference.Refer to the verification script in the zeitgeist Cargo.toml review comment.
runtime/common/src/lib.rs (1)
703-711: Already flagged:slot_info()returningNonehandled with silent default.This concern was raised in previous review. The
unwrap_or_default()silently returnsSlot(0)whenslot_info()isNone, which could occur during parachain startup or relay chain data unavailability.
🧹 Nitpick comments (7)
zrml/authorized/src/authorized_pallet_api.rs (1)
1-1: Header change is fine; consider normalizing year formatThe copyright update is harmless and consistent with the file’s intent. If you’re standardizing headers elsewhere (e.g., in PR #1449), you might want to switch to a range style like
2023-2025(and confirm whether 2024 should be included) for consistency, but it’s not required here.scripts/check-license/src/check_license/copyright.py (1)
17-29: Approve the logic; consider modern Python idioms.The parsing logic correctly handles both literal "(C)" and year-based copyright formats. The regex properly escapes parentheses, and the validation logic works correctly with the Years objects.
Recommended: Apply static analysis suggestions for modern Python (3.10+).
Apply this diff to use
itertools.pairwise()and addstrict=True:+import itertools + from __future__ import annotations import dataclassesyears_split = years.split(", ") years_ranges = [Years.from_string(y) for y in years_split] # Check that year ranges don't overlap and are ordered correctly. - for prev, curr in zip(years_ranges, years_ranges[1:]): + for prev, curr in itertools.pairwise(years_ranges): if curr.start <= prev.end: raise IllegalYearRange() return Copyright(holder, years_ranges)Note:
itertools.pairwise()requires Python 3.10+. If supporting older versions, keep the current approach but addstrict=Trueto thezip()call.Based on static analysis hints.
runtime/zeitgeist/src/integration_tests/xcm/tests/mod.rs (2)
27-31: Consider documenting the mutex guard pattern.The function returns
MutexGuardso callers can control lock lifetime, but this contract isn't documented. Adding a brief doc comment would clarify that callers must hold the guard for the duration of their test to maintain isolation.Example:
+/// Resets the test network and returns a mutex guard. +/// The caller must hold the guard for the duration of the test +/// to ensure exclusive access to the test network. pub(crate) fn reset_test_net() -> MutexGuard<'static, ()> { let guard = TEST_NET_MUTEX.lock().expect("Test net mutex poisoned"); crate::integration_tests::xcm::test_net::TestNet::reset(); guard }
39-46: Consider panic safety for slot info cleanup.If the closure
f()panics, the conditional slot info cleanup won't execute, potentially affecting subsequent tests. While test panics typically fail fast, using a guard-based cleanup pattern would ensure cleanup even on panic.Example using a guard pattern:
pub(crate) fn with_zeitgeist<R>(f: impl FnOnce() -> R) -> R { struct SlotInfoGuard; impl Drop for SlotInfoGuard { fn drop(&mut self) { #[cfg(feature = "parachain")] clear_async_backing_slot_info(); } } crate::integration_tests::xcm::test_net::ZeitgeistPara::execute_with(|| { let _guard = SlotInfoGuard; f() }) }However, the current implementation is acceptable for test code if test isolation is handled at the process level.
Makefile (1)
26-28: Formatting inconsistency and verify idempotency check disabling.Line 28 has mixed indentation (spaces + tab) while line 27 uses a tab only. This inconsistency could cause issues in some Make implementations.
Additionally, disabling idempotency checks (
--disable-idempotency-checks) is reasonable during initial async backing integration, but consider adding a comment explaining why this is necessary or if it should be re-enabled after the migration stabilizes.--blocktime=6000 \ --disable-idempotency-checks \ - --mbm-max-blocks 128 \ + --mbm-max-blocks 128 \runtime/zeitgeist/Cargo.toml (1)
133-133: Use workspace dependency foronce_cell.The
once_celldependency is hardcoded to version "1.20.2" instead of using a workspace reference. This can lead to version inconsistencies across the codebase.Verify whether
once_cellis defined in the workspace Cargo.toml and use it if available:#!/bin/bash # Check if once_cell is defined in workspace dependencies rg -n "once_cell.*=" Cargo.toml | head -5runtime/common/src/weights/pallet_migrations.rs (1)
178-189: Weight function ignores thenparameter despite variable storage access.The
clear_historicfunction takes_n: u32but the weight calculation uses fixed values (256 reads/writes) regardless ofn. The comment on line 183 indicates that measured size varies withn(992 + n * (269 ±0)).While returning the worst-case weight is safe (over-estimates), it's inefficient for smaller values of
n.If more accurate weight accounting is desired:
- fn clear_historic(_n: u32) -> Weight { + fn clear_historic(n: u32) -> Weight { // Proof Size summary in bytes: // Measured: `992 + n * (269 ±0)` // Estimated: `702686` // Minimum execution time: 17_890 nanoseconds. - Weight::from_parts(262_410_000, 702686) - .saturating_add(T::DbWeight::get().reads(256)) - .saturating_add(T::DbWeight::get().writes(256)) + Weight::from_parts(17_890_000, 992) + .saturating_add(Weight::from_parts(0, 269).saturating_mul(n.into())) + .saturating_add(T::DbWeight::get().reads(n.into())) + .saturating_add(T::DbWeight::get().writes(n.into())) }Note: The exact per-item weight should be derived from proper benchmarks with sufficient steps/repeats.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
Cargo.toml(4 hunks)Makefile(1 hunks)integration-tests/tests/common-tests.ts(2 hunks)integration-tests/tests/rt-upgrade-battery-station-chopsticks/test-battery-station-chopsticks-runtime-upgrade.ts(2 hunks)integration-tests/tests/rt-upgrade-zeitgeist-chopsticks/test-zeitgeist-chopsticks-runtime-upgrade.ts(3 hunks)integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts(2 hunks)integration-tests/zombienet/0001-balance-transfer.ts(1 hunks)macros/src/lib.rs(1 hunks)node/build.rs(1 hunks)node/src/chain_spec/additional_chain_spec.rs(1 hunks)primitives/src/constants/ztg.rs(1 hunks)primitives/src/hybrid_router_api_types.rs(1 hunks)primitives/src/lib.rs(1 hunks)primitives/src/math/mod.rs(1 hunks)primitives/src/max_runtime_usize.rs(1 hunks)primitives/src/outcome_report.rs(1 hunks)primitives/src/proxy_type.rs(1 hunks)primitives/src/serde_wrapper.rs(1 hunks)primitives/src/traits/complete_set_operations_api.rs(1 hunks)primitives/src/traits/deploy_pool_api.rs(1 hunks)primitives/src/traits/dispute_api.rs(1 hunks)primitives/src/traits/distribute_fees.rs(1 hunks)primitives/src/traits/hybrid_router_amm_api.rs(1 hunks)primitives/src/traits/hybrid_router_orderbook_api.rs(1 hunks)primitives/src/traits/market_builder.rs(1 hunks)primitives/src/traits/market_id.rs(1 hunks)primitives/src/traits/swaps.rs(1 hunks)primitives/src/traits/zeitgeist_asset.rs(1 hunks)runtime/battery-station/Cargo.toml(10 hunks)runtime/battery-station/src/integration_tests/xcm/mod.rs(1 hunks)runtime/battery-station/src/lib.rs(3 hunks)runtime/battery-station/src/runtime_migrations/mod.rs(1 hunks)runtime/battery-station/src/xcm_config/asset_registry.rs(1 hunks)runtime/common/Cargo.toml(2 hunks)runtime/common/src/lib.rs(13 hunks)runtime/common/src/weights/mod.rs(1 hunks)runtime/common/src/weights/pallet_assets.rs(1 hunks)runtime/common/src/weights/pallet_grandpa.rs(1 hunks)runtime/common/src/weights/pallet_migrations.rs(1 hunks)runtime/zeitgeist/Cargo.toml(10 hunks)runtime/zeitgeist/src/integration_tests/xcm/mod.rs(1 hunks)runtime/zeitgeist/src/integration_tests/xcm/tests/mod.rs(2 hunks)runtime/zeitgeist/src/lib.rs(3 hunks)runtime/zeitgeist/src/runtime_migrations/mod.rs(1 hunks)runtime/zeitgeist/src/xcm_config/asset_registry.rs(1 hunks)scripts/benchmarks/configuration.sh(1 hunks)scripts/check-license/src/check_license/copyright.py(1 hunks)zrml/authorized/src/authorized_pallet_api.rs(1 hunks)zrml/authorized/src/benchmarks.rs(1 hunks)zrml/authorized/src/migrations.rs(1 hunks)zrml/authorized/src/tests.rs(1 hunks)zrml/court/src/benchmarks.rs(1 hunks)zrml/court/src/court_pallet_api.rs(1 hunks)zrml/court/src/migrations.rs(1 hunks)zrml/court/src/types.rs(1 hunks)zrml/global-disputes/src/benchmarks.rs(1 hunks)zrml/global-disputes/src/migrations.rs(1 hunks)zrml/global-disputes/src/types.rs(1 hunks)zrml/global-disputes/src/utils.rs(1 hunks)zrml/hybrid-router/src/benchmarking.rs(1 hunks)zrml/hybrid-router/src/tests/mod.rs(1 hunks)zrml/hybrid-router/src/types.rs(1 hunks)zrml/hybrid-router/src/utils.rs(1 hunks)zrml/market-commons/src/migrations.rs(1 hunks)zrml/market-commons/src/tests.rs(1 hunks)zrml/market-commons/src/types/market_builder.rs(1 hunks)zrml/market-commons/src/types/mod.rs(1 hunks)zrml/neo-swaps/src/consts.rs(1 hunks)zrml/neo-swaps/src/helpers.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/macros.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/mod.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/tests/deposit_fees.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/tests/exit.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/tests/join.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/tests/shares_of.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/tests/total_shares.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/tests/withdraw_fees.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/traits/liquidity_tree_helper.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/types/liquidity_tree_child_indices.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/types/liquidity_tree_error.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/types/liquidity_tree_max_nodes.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/types/mod.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/types/node.rs(1 hunks)zrml/neo-swaps/src/liquidity_tree/types/update_descendant_stake_operation.rs(1 hunks)zrml/neo-swaps/src/tests/liquidity_tree_interactions.rs(1 hunks)zrml/neo-swaps/src/traits/liquidity_shares_manager.rs(1 hunks)zrml/neo-swaps/src/types/fee_distribution.rs(1 hunks)zrml/neo-swaps/src/types/max_assets.rs(1 hunks)zrml/orderbook/src/migrations.rs(1 hunks)zrml/orderbook/src/tests.rs(1 hunks)zrml/orderbook/src/utils.rs(1 hunks)zrml/parimutuel/src/tests/assets.rs(1 hunks)zrml/parimutuel/src/tests/buy.rs(1 hunks)zrml/parimutuel/src/tests/claim.rs(1 hunks)zrml/parimutuel/src/tests/mod.rs(1 hunks)zrml/parimutuel/src/tests/refund.rs(1 hunks)zrml/parimutuel/src/utils.rs(1 hunks)zrml/prediction-markets/runtime-api/src/lib.rs(1 hunks)zrml/prediction-markets/src/benchmarks.rs(1 hunks)zrml/prediction-markets/src/migrations.rs(1 hunks)zrml/prediction-markets/src/tests/admin_move_market_to_closed.rs(1 hunks)zrml/prediction-markets/src/tests/admin_move_market_to_resolved.rs(1 hunks)zrml/prediction-markets/src/tests/approve_market.rs(1 hunks)zrml/prediction-markets/src/tests/buy_complete_set.rs(1 hunks)zrml/prediction-markets/src/tests/close_trusted_market.rs(1 hunks)zrml/prediction-markets/src/tests/create_market.rs(1 hunks)zrml/prediction-markets/src/tests/create_market_and_deploy_pool.rs(1 hunks)
⛔ Files not processed due to max files limit (36)
- zrml/prediction-markets/src/tests/dispute.rs
- zrml/prediction-markets/src/tests/dispute_early_close.rs
- zrml/prediction-markets/src/tests/edit_market.rs
- zrml/prediction-markets/src/tests/manually_close_market.rs
- zrml/prediction-markets/src/tests/on_initialize.rs
- zrml/prediction-markets/src/tests/on_market_close.rs
- zrml/prediction-markets/src/tests/redeem_shares.rs
- zrml/prediction-markets/src/tests/reject_early_close.rs
- zrml/prediction-markets/src/tests/reject_market.rs
- zrml/prediction-markets/src/tests/report.rs
- zrml/prediction-markets/src/tests/request_edit.rs
- zrml/prediction-markets/src/tests/schedule_early_close.rs
- zrml/prediction-markets/src/tests/sell_complete_set.rs
- zrml/prediction-markets/src/tests/start_global_dispute.rs
- zrml/styx/src/benchmarks.rs
- zrml/styx/src/lib.rs
- zrml/swaps/fuzz/create_pool.rs
- zrml/swaps/fuzz/pool_exit.rs
- zrml/swaps/fuzz/pool_exit_with_exact_asset_amount.rs
- zrml/swaps/fuzz/pool_exit_with_exact_pool_amount.rs
- zrml/swaps/fuzz/pool_join.rs
- zrml/swaps/fuzz/pool_join_with_exact_asset_amount.rs
- zrml/swaps/fuzz/pool_join_with_exact_pool_amount.rs
- zrml/swaps/fuzz/swap_exact_amount_in.rs
- zrml/swaps/fuzz/swap_exact_amount_out.rs
- zrml/swaps/fuzz/utils.rs
- zrml/swaps/runtime-api/src/lib.rs
- zrml/swaps/src/benchmarks.rs
- zrml/swaps/src/events.rs
- zrml/swaps/src/fixed.rs
- zrml/swaps/src/lib.rs
- zrml/swaps/src/math.rs
- zrml/swaps/src/migrations.rs
- zrml/swaps/src/types/mod.rs
- zrml/swaps/src/types/pool.rs
- zrml/swaps/src/utils.rs
✅ Files skipped from review due to trivial changes (80)
- zrml/prediction-markets/src/migrations.rs
- zrml/neo-swaps/src/liquidity_tree/traits/liquidity_tree_helper.rs
- zrml/neo-swaps/src/traits/liquidity_shares_manager.rs
- runtime/zeitgeist/src/xcm_config/asset_registry.rs
- zrml/court/src/benchmarks.rs
- zrml/prediction-markets/src/tests/close_trusted_market.rs
- zrml/prediction-markets/src/tests/create_market_and_deploy_pool.rs
- primitives/src/traits/hybrid_router_amm_api.rs
- zrml/neo-swaps/src/liquidity_tree/tests/join.rs
- primitives/src/lib.rs
- primitives/src/traits/deploy_pool_api.rs
- primitives/src/max_runtime_usize.rs
- zrml/hybrid-router/src/utils.rs
- primitives/src/proxy_type.rs
- primitives/src/traits/hybrid_router_orderbook_api.rs
- zrml/global-disputes/src/benchmarks.rs
- zrml/market-commons/src/types/mod.rs
- zrml/authorized/src/tests.rs
- zrml/neo-swaps/src/liquidity_tree/tests/shares_of.rs
- zrml/orderbook/src/migrations.rs
- zrml/parimutuel/src/tests/assets.rs
- zrml/neo-swaps/src/consts.rs
- zrml/parimutuel/src/tests/refund.rs
- zrml/orderbook/src/tests.rs
- primitives/src/traits/distribute_fees.rs
- primitives/src/serde_wrapper.rs
- zrml/parimutuel/src/tests/buy.rs
- zrml/prediction-markets/src/tests/create_market.rs
- zrml/hybrid-router/src/types.rs
- zrml/prediction-markets/src/tests/buy_complete_set.rs
- primitives/src/traits/complete_set_operations_api.rs
- zrml/prediction-markets/src/tests/admin_move_market_to_resolved.rs
- zrml/neo-swaps/src/types/max_assets.rs
- primitives/src/traits/market_builder.rs
- zrml/orderbook/src/utils.rs
- zrml/neo-swaps/src/liquidity_tree/tests/exit.rs
- zrml/court/src/types.rs
- primitives/src/outcome_report.rs
- zrml/neo-swaps/src/liquidity_tree/types/liquidity_tree_error.rs
- primitives/src/hybrid_router_api_types.rs
- zrml/neo-swaps/src/types/fee_distribution.rs
- zrml/parimutuel/src/utils.rs
- zrml/neo-swaps/src/liquidity_tree/tests/total_shares.rs
- integration-tests/zombienet/0001-balance-transfer.ts
- primitives/src/constants/ztg.rs
- zrml/global-disputes/src/utils.rs
- zrml/neo-swaps/src/liquidity_tree/types/node.rs
- zrml/parimutuel/src/tests/mod.rs
- primitives/src/traits/market_id.rs
- zrml/neo-swaps/src/helpers.rs
- zrml/market-commons/src/tests.rs
- zrml/neo-swaps/src/liquidity_tree/types/liquidity_tree_child_indices.rs
- macros/src/lib.rs
- zrml/neo-swaps/src/liquidity_tree/types/liquidity_tree_max_nodes.rs
- zrml/prediction-markets/src/benchmarks.rs
- integration-tests/tests/rt-upgrade-zombienet/test-zombienet-runtime-upgrade.ts
- zrml/prediction-markets/src/tests/admin_move_market_to_closed.rs
- zrml/market-commons/src/types/market_builder.rs
- zrml/court/src/migrations.rs
- primitives/src/math/mod.rs
- node/src/chain_spec/additional_chain_spec.rs
- zrml/authorized/src/benchmarks.rs
- runtime/common/src/weights/pallet_grandpa.rs
- zrml/court/src/court_pallet_api.rs
- zrml/parimutuel/src/tests/claim.rs
- runtime/common/src/weights/pallet_assets.rs
- integration-tests/tests/rt-upgrade-battery-station-chopsticks/test-battery-station-chopsticks-runtime-upgrade.ts
- runtime/battery-station/src/integration_tests/xcm/mod.rs
- primitives/src/traits/dispute_api.rs
- node/build.rs
- zrml/neo-swaps/src/liquidity_tree/tests/deposit_fees.rs
- zrml/market-commons/src/migrations.rs
- primitives/src/traits/zeitgeist_asset.rs
- zrml/hybrid-router/src/benchmarking.rs
- zrml/neo-swaps/src/tests/liquidity_tree_interactions.rs
- zrml/prediction-markets/src/tests/approve_market.rs
- zrml/hybrid-router/src/tests/mod.rs
- zrml/neo-swaps/src/liquidity_tree/types/mod.rs
- runtime/zeitgeist/src/integration_tests/xcm/mod.rs
- zrml/neo-swaps/src/liquidity_tree/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- integration-tests/tests/rt-upgrade-zeitgeist-chopsticks/test-zeitgeist-chopsticks-runtime-upgrade.ts
- runtime/zeitgeist/src/lib.rs
- integration-tests/tests/common-tests.ts
- Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (2)
runtime/battery-station/src/runtime_migrations/mod.rs (1)
runtime/zeitgeist/src/runtime_migrations/mod.rs (1)
get_key(19-21)
runtime/zeitgeist/src/runtime_migrations/mod.rs (1)
runtime/battery-station/src/runtime_migrations/mod.rs (1)
get_key(19-21)
🪛 Ruff (0.14.5)
scripts/check-license/src/check_license/copyright.py
26-26: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
26-26: Prefer itertools.pairwise() over zip() when iterating over successive pairs
Replace zip() with itertools.pairwise()
(RUF007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (42)
zrml/global-disputes/src/migrations.rs (1)
1-1: LGTM!Copyright header correctly updated to include 2025, reflecting the current modifications.
zrml/global-disputes/src/types.rs (1)
1-1: LGTM!Copyright header correctly updated to extend the year range through 2025.
zrml/neo-swaps/src/liquidity_tree/tests/withdraw_fees.rs (1)
1-1: LGTM!Copyright header update to include 2025 is appropriate. No functional changes to the test code.
zrml/neo-swaps/src/liquidity_tree/types/update_descendant_stake_operation.rs (1)
1-1: LGTM!Copyright header update to include 2025. No changes to the
UpdateDescendantStakeOperationenum.zrml/neo-swaps/src/liquidity_tree/macros.rs (1)
1-1: LGTM!Copyright header update to include 2025. The
assert_liquidity_tree_state!macro remains unchanged.scripts/check-license/src/check_license/copyright.py (5)
4-4: LGTM!The
datetimeimport is correctly added to supportdatetime.MAXYEARused in theendproperty.
12-12: LGTM!The
literalfield is well-designed as an optional parameter with clear semantics for storing literal copyright markers like "(C)".
35-39: LGTM!The
__str__method correctly handles both literal and year-based copyright formats, producing properly formatted copyright strings.
41-45: LGTM!The
endproperty correctly returnsdatetime.MAXYEARfor literal copyrights, which appropriately treats them as having no expiration. This design allows comparison logic to work correctly without special cases.
47-57: LGTM!The
push_yearmethod correctly prevents modification of literal copyrights by raisingIllegalYearRange. This is proper defensive programming that prevents invalid operations on copyrights without year ranges.runtime/zeitgeist/src/integration_tests/xcm/tests/mod.rs (4)
1-1: LGTM: Copyright year updated appropriately.The copyright year update to include 2025 is correct.
25-25: LGTM: Proper lazy mutex initialization for test coordination.The lazy static mutex pattern is appropriate for preventing race conditions in concurrent test execution.
33-37: LGTM: Proper cleanup of async backing slot info.The feature-gated function correctly clears the SlotInfo storage for test isolation.
21-23:once_cellis properly declared as a dev-dependency.The
once_cell = "1.20.2"dependency is correctly listed in the[dev-dependencies]section ofruntime/zeitgeist/Cargo.toml(line 132), which is appropriate for integration tests.primitives/src/traits/swaps.rs (1)
1-1: LGTM!Copyright year update is appropriate for ongoing maintenance.
runtime/battery-station/src/xcm_config/asset_registry.rs (1)
1-1: LGTM!Copyright year update is appropriate.
zrml/prediction-markets/runtime-api/src/lib.rs (1)
1-1: LGTM!Copyright year update is appropriate.
zrml/authorized/src/migrations.rs (1)
1-1: LGTM!Copyright year update is appropriate.
scripts/benchmarks/configuration.sh (1)
9-10: LGTM!The addition of
pallet_migrationsto the benchmark configuration is correct and consistent with the pallet being added to the runtime.runtime/common/Cargo.toml (3)
24-24: LGTM!The
pallet-migrationsdependency is correctly added with workspace configuration.
104-104: LGTM!The std feature properly includes
pallet-migrations/std.
111-116: LGTM!The try-runtime feature block is correctly configured with the necessary pallets.
runtime/zeitgeist/Cargo.toml (2)
22-22: LGTM!The new pallet dependencies (
pallet-migrations,sp-timestamp,pallet-async-backing) are correctly added with workspace configuration.Also applies to: 47-47, 75-75
169-169: LGTM!Feature flag wiring for the new pallets is comprehensive and correct across
parachain,runtime-benchmarks,std, andtry-runtimefeatures.Also applies to: 212-212, 255-255, 263-263, 288-288, 368-368, 372-372, 415-415
runtime/battery-station/src/runtime_migrations/mod.rs (1)
1-30: LGTM with verification needed.The migration implementation is consistent with the Zeitgeist runtime pattern and properly uses a unique pallet_id "bstk" for Battery Station. The batch size of 2,000 matches the Zeitgeist configuration.
Same verification as Zeitgeist runtime - ensure the batch size is appropriate for the expected migration workload.
runtime/battery-station/Cargo.toml (2)
22-22: LGTM!The new pallet dependencies are correctly added with workspace configuration.
Also applies to: 48-48, 75-75
171-171: LGTM!Feature flag wiring is comprehensive and correct across all feature sets.
Also applies to: 215-215, 258-259, 266-266, 292-292, 379-379, 383-383, 426-426
runtime/common/src/weights/mod.rs (1)
44-44: LGTM!The
pallet_migrationsmodule export is correctly added alongside other pallet weight modules.runtime/battery-station/src/lib.rs (4)
87-88: LGTM!The conditional module declaration correctly excludes
runtime_migrationsduring benchmarks while enabling it for parachain builds.
124-129: LGTM!The conditional type aliases for
MultiBlockMigrationsproperly separate:
- Mock migrations for benchmarks
- Legacy migrations for parachain production
- No-op for standalone mode
124-139: I'll help you verify the concerns about migration cursor and identifier max lengths. Let me search the codebase to understand how these parameters are used and compare them with Moonbeam's configuration.
<function_calls>
#!/bin/bashSearch for usage of CursorMaxLen and IdentifierMaxLen in the codebase
rg -n "CursorMaxLen|IdentifierMaxLen" --type rust -A 5 -B 2
</function_calls>
<function_calls>
#!/bin/bashSearch for pallet-migrations to understand the constraints
rg -n "pallet_migrations" --type rust -A 3 -B 1 | head -100
</function_calls>
<function_calls>
#!/bin/bashLook for Moonbeam references or migration-related documentation
rg -n "Moonbeam|moonbeam" --type rust -i -A 2 -B 2 | head -50
</function_calls>
<function_calls>
#!/bin/bashSearch for any migration cursor or identifier usage patterns
rg -n "migration.*cursor|cursor.*migration" --type rust -i -A 3 -B 1 | head -80
</function_calls>
97-100: Version bumps are appropriate and align with migrations and async backing features.The verification confirms the spec_version bump from 61 to 62 and transaction_version bump from 34 to 35 are correctly aligned with the introduction of:
- Async backing: Confirmed in runtime Cargo.toml and implemented in runtime/common/src/lib.rs with pallet_async_backing and async-backing-primitives configurations
- Migrations: Confirmed in battery-station/src/lib.rs line 88 with the runtime_migrations module and associated type aliases
Both runtimes (battery-station and zeitgeist) were updated with identical version numbers, indicating coordinated changes. The version increments follow appropriate semantics—spec_version for breaking changes and transaction_version for transaction format changes.
runtime/zeitgeist/src/runtime_migrations/mod.rs (1)
1-26: Based on my verification of the codebase:Migration implementation is sound with consistent batch sizing across runtimes.
The verification confirms:
- Batch size consistency: Both zeitgeist (2_000) and battery-station (2_000) use identical batch sizes, indicating deliberate parity
- Implementation correctness: Proper struct implementation, correct pallet_ids ("zstk"), and valid version transitions (0→1)
- Scale appropriateness: For 94,365 legacy keys, a batch size of 2,000 results in ~47 processing batches, which is reasonable and manageable
- Pattern compliance: Implementation follows established patterns from pallet_parachain_staking migrations
The original review's concern about verifying appropriateness for "block time constraints" cannot be empirically validated without actual runtime metrics, but the batch size shows no signs of being problematic. The consistency with battery-station and the reasonable scale relative to the key count (94,365 keys) suggest the value was thoughtfully chosen.
runtime/common/src/weights/pallet_migrations.rs (1)
21-22: Benchmark parameters appear insufficient for production weights.The benchmarks were run with
STEPS: 2andREPEAT: 0, which produces unreliable weight estimates. Production benchmarks typically requireSTEPS >= 50andREPEAT >= 20for statistically meaningful results.If this is intentional for initial setup, please confirm and consider re-running benchmarks with proper parameters before mainnet deployment.
Also applies to: 35-36
runtime/common/src/lib.rs (8)
62-64: LGTM!The
relay_timestampmodule is correctly gated behind theparachainfeature flag.
119-130: LGTM!The
MigrationsMaxServiceWeightcorrectly uses the full block weight for migrations, and theMultiplyRoundLenBy2migration is appropriately included as required by the async backing integration (per PR objectives referencing Moonbeam's approach).
456-479: LGTM!The
ConsensusHookconfiguration follows the established async backing pattern:
FixedVelocityConsensusHookwith appropriate velocity/capacity constantsRelayNumberMonotonicallyIncreasesis the correct choice for async backing- Wrapping with
ConsensusHookWrapperForRelayTimestampensures relay timestamps are properly synchronized
573-589: LGTM!Both pallet configurations are properly set up:
pallet_migrationsusesFreezeChainOnFailedMigrationwhich is a safe default that halts the chain on critical migration failurespallet_async_backingcorrectly usesRelaySlotfor slot verification
2470-2478: LGTM!The
can_build_uponimplementation now correctly delegates toConsensusHook::can_build_uponinstead of returningfalse. This enables proper async backing functionality by allowing the relay chain to query whether a new block can be built on a given included hash and slot.
1847-1847: LGTM!The
pallet_migrationsis correctly added to the benchmark metadata list.
2097-2097: LGTM!The
pallet_migrationsis correctly added to the dispatch benchmark execution.
518-526: LGTM!The conditional compilation correctly ensures:
NoopMessageProcessoris used during benchmarks (but not tests) to avoid XCM execution overheadProcessXcmMessageis used for actual runtime operation and tests
mergify
bot
added
s:in-progress
Chralt98
added
s:review-needed
Chralt98
added
s:accepted
3 participants
What does it do?
Fixes #1435
Fixes #1426
What important points should reviewers know?
It follows this approach: moonbeam-foundation/moonbeam#2805 for Moonbeam and for Moonriver: moonbeam-foundation/moonbeam#2776 and this moonbeam-foundation/moonbeam#2804
Asynchronous Backing was enabled with this runtime release for Moonbeam: https://github.com/moonbeam-foundation/moonbeam/releases/tag/runtime-3000. It is therefore not necessary to do any other migration than the
MultiplyRoundLenBy2. Evidence: https://github.com/moonbeam-foundation/moonbeam/blob/614b9937382409cf7004b6a9f5305ffdb4295798/runtime/common/src/migrations.rs#L53-L199 and https://github.com/moonbeam-foundation/moonbeam/blob/614b9937382409cf7004b6a9f5305ffdb4295798/runtime/moonbeam/src/lib.rs#L1128-L1131 and https://github.com/moonbeam-foundation/moonbeam/blob/614b9937382409cf7004b6a9f5305ffdb4295798/runtime/moonbeam/src/migrations.rs#L41This PR includes the merge of #1448 and #1449, because the migrations of
AtStakewere built upon the new asynchronous backing feature to check whether all migrations work together using try-runtime.Migration outputs: zeitgeist-try-runtime-output.txt, battery-station-try-runtime-output.txt
Is there something left for follow-up PRs?
This feature requires to update all occurrences of block based time calculation to be updated in the zeitgeist runtime pallets!
What alternative implementations were considered?
Are there relevant PRs or issues?
References
moonbeam-foundation/moonbeam#3305
#1434
moonbeam-foundation/moonbeam#3298
moonbeam-foundation/moonbeam#2593
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.