[MD-1520] Handle commitment record separately for v1 and v2 - #1473
Merged
Conversation
📝 WalkthroughWalkthroughAdds versioned (v1/v2) outbound message commitment recording: new RecordedCommitmentV2 storage and APIs, record_commitment_root_v2/prove_message_v2, merged Merkle-root computation when both versions exist, runtime/runtime-api wiring, tests, Cargo deps, and TypeScript query/type updates. Changes
Sequence Diagram(s)sequenceDiagram
participant QueueV1 as OutboundQueueV1
participant QueueV2 as OutboundQueueV2
participant Recorder as OutboundMessageCommitmentRecorder
participant StorageV1 as RecordedCommitmentV1
participant StorageV2 as RecordedCommitmentV2
participant Merkle as MerkleRootCalculator
QueueV1->>Recorder: OnNewCommitment(commitment_v1)
Recorder->>QueueV1: read message_leaves_v1
Recorder->>StorageV1: record_commitment_root(commitment_v1, message_leaves_v1)
Recorder-->>QueueV1: emit NewCommitmentRootRecorded (v1)
QueueV2->>Recorder: OnNewCommitment(commitment_v2)
Recorder->>QueueV2: read message_leaves_v2
Recorder->>StorageV2: record_commitment_root_v2(commitment_v2, message_leaves_v2)
Recorder-->>QueueV2: emit NewCommitmentRootRecorded (v2)
Caller->>Recorder: take_commitment_root()
Recorder->>StorageV1: read maybe_v1
Recorder->>StorageV2: read maybe_v2
alt both present
Recorder->>Merkle: merkle_root(concat(maybe_v1.leaves, maybe_v2.leaves))
Merkle-->>Recorder: unified_root
Recorder->>StorageV1: clear
Recorder->>StorageV2: clear
Recorder-->>Caller: emit CommitmentRootRead(unified_root) / return unified_root
else only v1
Recorder->>StorageV1: clear
Recorder-->>Caller: emit CommitmentRootRead(v1_root) / return v1_root
else only v2
Recorder->>StorageV2: clear
Recorder-->>Caller: emit CommitmentRootRead(v2_root) / return v2_root
else none
Recorder-->>Caller: return None
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
WASM runtime size check:Compared to target branchdancebox runtime: 1900 KB (no changes) ✅ flashbox runtime: 1124 KB (no changes) ✅ dancelight runtime: 2656 KB (no changes) 🚨 starlight runtime: 2580 KB (-4 KB) 🚨 container chain template simple runtime: 1516 KB (no changes) ✅ container chain template frontier runtime: 1848 KB (no changes) ✅ |
![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 (1)
pallets/outbound-message-commitment-recorder/src/lib.rs (1)
34-86: Avoid merging commitments that may come from different blocks.Line 63-86 merges v1/v2 whenever both are present, but the storage doesn’t track block numbers. If the relayer misses a block (or a pre-upgrade v1 value lingers after the STORAGE_VERSION bump on Line 34), you can merge commitments from different blocks and emit a root that never existed, which breaks Ethereum validation. Consider storing the block number with each commitment and only merging when they match (or clearing stale values on upgrade). This would require a storage migration.
💡 Possible direction (block-number guard)
- pub type RecordedCommitment<T: Config> = StorageValue<_, H256, OptionQuery>; + pub type RecordedCommitment<T: Config> = StorageValue<_, (T::BlockNumber, H256), OptionQuery>; - pub type RecordedCommitmentV2<T: Config> = StorageValue<_, H256, OptionQuery>; + pub type RecordedCommitmentV2<T: Config> = StorageValue<_, (T::BlockNumber, H256), OptionQuery>; pub fn record_commitment_root(commitment: H256) { - RecordedCommitment::<T>::put(commitment); + let block = frame_system::Pallet::<T>::block_number(); + RecordedCommitment::<T>::put((block, commitment)); Pallet::<T>::deposit_event(Event::<T>::NewCommitmentRootRecorded { commitment }); } pub fn record_commitment_root_v2(commitment: H256) { - RecordedCommitmentV2::<T>::put(commitment); + let block = frame_system::Pallet::<T>::block_number(); + RecordedCommitmentV2::<T>::put((block, commitment)); Pallet::<T>::deposit_event(Event::<T>::NewCommitmentRootRecorded { commitment }); } - (Some(v1_commit), Some(v2_commit)) => { + (Some((b1, v1_commit)), Some((b2, v2_commit))) if b1 == b2 => { // merge as before } + (Some((b1, v1_commit)), Some((b2, v2_commit))) => { + let commitment = if b1 >= b2 { v1_commit } else { v2_commit }; + Pallet::<T>::deposit_event(Event::<T>::CommitmentRootRead { commitment }); + Some(commitment) + }
🤖 Fix all issues with AI agents
In `@pallets/outbound-message-commitment-recorder/Cargo.toml`:
- Line 20: Add "snowbridge-merkle-tree/std" to the std feature list in this
crate's Cargo.toml so the workspace dependency is forwarded for
no-default-features builds; specifically, update the std features array that
currently lists other dependencies (the std feature block for this crate) to
include "snowbridge-merkle-tree/std" alongside the existing entries so std
builds can enable the merkle-tree's std feature like other pallets (e.g.,
pallet-external-validators-rewards).
tmpolaczyk
reviewed
Jan 21, 2026
tmpolaczyk
reviewed
Jan 21, 2026
tmpolaczyk
reviewed
Jan 21, 2026
evgeny-s
marked this pull request as ready for review
January 21, 2026 11:07
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pallets/outbound-message-commitment-recorder/src/lib.rs`:
- Around line 82-105: take_commitment_root currently consumes both
RecordedCommitment and RecordedCommitmentV2 (using .take()), which removes the
leaves needed by runtime proof endpoints like prove_message_* and causes proofs
to be unavailable after MMR construction; change take_commitment_root to
preserve leaves for proof generation by only removing/marking the commitment
roots while retaining the associated leaves (or move the extracted leaves into a
new storage like PendingProofLeaves / LastLeavesForProofs inside
take_commitment_root) and update references to
RecordedCommitment/RecordedCommitmentV2 and get_combined_leaves so proofs still
read leaves from the preserved storage until the next commitment replaces them.
- Around line 123-147: The code in prove_message_v2 and prove_message can
overflow or be out-of-bounds: replace the unchecked addition in prove_message_v2
(leaves.len() as u64 + leaf_index) with a checked_add and return None on
overflow; in prove_message, after building combined_leaves compute combined_len
safely (as u64 using try_from or checked conversion) and verify
combined_leaf_index < combined_len before calling merkle_proof, returning None
if out-of-bounds; update references to RecordedCommitment::prove_message_v2,
prove_message, get_combined_leaves and the merkle_proof invocation accordingly.
tmpolaczyk
reviewed
Jan 21, 2026
Author
|
/cmd generate-ts-api |
1 similar comment
Author
|
/cmd generate-ts-api |
Contributor
Coverage Report@@ Coverage Diff @@
## master evgeny-commitment-recorder +/- ##
==============================================================
+ Coverage 73.84% 73.91% +0.07%
+ Files 568 569 +1
+ Lines 82805 83085 +280
==============================================================
+ Hits 61141 61410 +269
+ Misses 21664 21675 +11
|
tmpolaczyk
reviewed
Jan 21, 2026
tmpolaczyk
approved these changes
Jan 21, 2026
evgeny-s
commented
Jan 22, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pallets/outbound-message-commitment-recorder/src/lib.rs`:
- Around line 119-135: prove_message_v1 and prove_message_v2 must first check
which RecordedCommitment variant is stored and return None when the requested
version isn't recorded; update both helpers to pattern-match
RecordedCommitment::<T>::get() against the expected variant
(RecordedCommitment::V1 vs RecordedCommitment::V2) before calling
Self::prove_message, and in prove_message_v2 only compute the combined index
(using the existing checked_add on leaves.len() and leaf_index) when the stored
commitment is the V2 variant—otherwise return None to avoid producing proofs for
the wrong version or out‑of‑range indices.
🧹 Nitpick comments (1)
pallets/outbound-message-commitment-recorder/src/lib.rs (1)
82-105: Document the ephemeral lifetime of recorded commitments.
take_commitment_rootclears both v1/v2 storages; proof data is only available in the recording block. A short doc comment here (and/or onprove_message_v*) would prevent relayer misuse.📝 Suggested doc clarification
- pub fn take_commitment_root() -> Option<H256> { + /// Takes (and clears) recorded commitments for the current block. + /// Proofs must be queried in the same block via the runtime API. + pub fn take_commitment_root() -> Option<H256> {Based on learnings, add explicit guidance about the per-block availability of proofs.
ParthDesai
force-pushed
the
evgeny-commitment-recorder
branch
from
January 30, 2026 14:12
ae55c35 to
0627102
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pallets/outbound-message-commitment-recorder/src/lib.rs`:
- Around line 28-41: The STORAGE_VERSION constant was not bumped after adding
new storage items; update the storage version by changing the declaration of
STORAGE_VERSION (const STORAGE_VERSION: StorageVersion = StorageVersion::new(0))
to the next version number (e.g. StorageVersion::new(1)) so the runtime
recognizes the schema change for this pallet.
🧹 Nitpick comments (1)
chains/orchestrator-relays/runtime/dancelight/src/lib.rs (1)
3143-3156: Document proof timing for relayers.
Since proof data is ephemeral, consider adding a brief doc/comment on these runtime APIs noting proofs are only available for the block where the commitment was recorded.Based on learnings In the
pallet-outbound-message-commitment-recorderpallet, commitment data (including leaves) is intentionally removed in the next block after being recorded viatake_commitment_root. Relayers must call theprove_messageruntime API at a specific block to obtain proofs before the commitment data is cleared. This ephemeral design is intentional for storage efficiency.
Co-authored-by: tmpolaczyk <44604217+tmpolaczyk@users.noreply.github.com>
ParthDesai
force-pushed
the
evgeny-commitment-recorder
branch
from
January 30, 2026 16:29
f350238 to
ece6a22
Compare
tmpolaczyk
approved these changes
Feb 3, 2026
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
3 participants
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.
Description
This PR implements unified commitment root handling for Snowbridge outbound queues in the Dancelight runtime. The OutboundMessageCommitmentRecorder pallet now stores commitments from v1 and v2 queues separately and computes a Merkle tree root when both are present, ensuring the relayer receives a single, unified commitment for Ethereum smart contract validation. Changes include adding new storage for v2 commitments, updating the commitment logic, and adding comprehensive integration tests. The pallet version has been bumped to 1 to reflect the new storage addition.