Versioned bundle and Execution receipt #3623

vedhavyas · 2025-07-09T10:11:49Z

First of all, changes may appear big but most of the changes are cosmetic.
I did not change everything at once but rather inroduced necessary intermediate types so that I'm aware which parts require direct changes, like runtime, and which parts require an actual compatilibility code. These intermediate types would have be removed in the following commits.

Notable changes

New Bundle V1 version
New ER version V0
New FraudProof V1 version that will hold both old and new Bundle Versions.

For the reviewers, you can either follow commit by commit to see how the migration is applied or look at overall changes. Overall changes are surprisingly easy to understand.

Please let me know @NingLin-P if there are changes that missed that may cause incomaptibility on current Taurus.

Cleanup

We can safely remove

Bundle V0
FraudProof V0
Once the new traurus is deployed. As for the mainnet, assuming, this PR lands before we instantiate domains this should be safe to remove as well.

Closes: #2942

Code contributor checklist:

I have read, understood and followed contributing guide

…y code for previous fraud proof version

teor2345

Looks good overall, but it's a large change. What is our testing and risk mitigation strategy?

There are a lot of new functions and types without documentation. Can you add a short description to the production ones? (Tests aren't as important, but it can still be useful to say what a test is meant to check.)

What does a version upgrade look like? Do we need documentation that says how to do it, and tests to make sure it works? (Or a documented manual test process.)

There are some duplicated code blocks and types we could fix up, to avoid confusion.

teor2345 · 2025-07-09T20:40:23Z

crates/pallet-domains/src/block_tree.rs

 ) -> ReceiptType {
-    let receipt_number = execution_receipt.domain_block_number;
+    let ExecutionReceiptRef::V0(ExecutionReceiptV0 {


How will this hard-coded V0 work when we upgrade the version?

We currently has only one version of execution receipt on runtime. If we introduce a new version, then we should handle it at that point based on new changes.

crates/sp-domains-fraud-proof/benches/fraud_proof_verification.rs

domains/client/domain-operator/src/aux_schema.rs

vedhavyas · 2025-07-10T02:09:24Z

What is our testing and risk mitigation strategy?

Same as usual.

Deploy Local testnet with latest client but older runtime and do an upgrade.
Sync taurus node from scratch with latest client.

Both should cover all the cases

What does a version upgrade look like? Do we need documentation that says how to do it, and tests to make sure it works? (Or a documented manual test process.)

no, its purely defined by the runtime and client uses whatever runtime expects. Only thing we should aware is the node release should go in first followed by runtime upgrade for Taurus. For mainnet, since there are no domains, we can release a client as soon as taurus is tested.

teor2345 · 2025-07-10T02:38:37Z

no, its purely defined by the runtime and client uses whatever runtime expects. Only thing we should aware is the node release should go in first followed by runtime upgrade for Taurus.

These things seem important to document somewhere, because we'll be doing another upgrade on mainnet with domains at some point in the future.

vedhavyas · 2025-07-10T06:20:09Z

These things seem important to document somewhere, because we'll be doing another upgrade on mainnet with domains at some point in the future.

Not necessarily. Take a look at how XDM is versioned. This is same as that. Runtime defines the versions that need to be used and client uses that.

As for protocol specs, there are no changes to the specs since nothing has changed but rather made type versioned

…emove unnecessary clone

crates/pallet-domains/src/lib.rs

NingLin-P

There are a lot of changes and a lot of duplicated code, I don't have the confidence to find every issue (if any) with my bare eye, thus let's see how the test goes.

crates/pallet-domains/src/lib.rs

vedhavyas · 2025-07-14T06:51:32Z

@NingLin-P I think there maybe confusion on Bundle version and Er version when it comes to storing them

A runtime will only accept the Bundle version that is defined on the Runtime's CurrentVersion.
If a runtime says it expects V1 bundle version, then bundle submission must be of version V1 and not any other.

Now coming to ER version, it changes a bit.
ER version is defined based on the consensus block number at which ER is derived
So if the Runtime's ER version at the time of deriving block is V0, then runtime will accept V0 ER when that ER is submitted through the bundle or singleton receipt since between ER is derived and Submitted there maybe a Runtime upgrade and ER version would have changed.

now coming to how we store the versions, we hook into the set_code
When we initiate the runtime upgrade, example spec_version 2, set_code is called on runtime with spec_version 1
Spec version 1's current versions are stored against the block number which included set_code extrinsics. So any ERs derived until the upgraded number should come in the same ER version as spec version 1 ER version
From upgraded_number + 1, ER version may have changed, and all the ER derived from that number should come with version accepted by new Runtime.

I have added tests to explain this better.

Let me know if this is clear or we can do a sync call

Note: conflicts will be fixed once we have an approval from the team else merge commits will pollute the actual commits from this PR

vedhavyas

Apologies again for force pushes.

@jfrank-summit and I decided to remove the compatibility code and instead run a devent to verify this in the interest of time.

Due to that, I just simply removed the old versions instead of re-working from ground up as this might take more time.

For the reviewers, for this PR alone, please look at the full diff instead of individual commits since this will be much more cleaner due to no compatibility code.

Appreciate your patience and understanding on this.

Note: Now that we dont need compatibility code, there will be a follow-up PR based on this PR to remove any comaptibility code we had for Domains in Taurus

crates/sp-domains-fraud-proof/benches/fraud_proof_verification.rs

crates/pallet-domains/src/block_tree.rs

NingLin-P · 2025-07-16T22:36:21Z

I actually prefer a hook when the runtime is upgraded over on_initialize. This to me is much more easy to understand when the versions actually change ie... at when runtime is actually upgraded.

As for the tests you have written, will take a look why its failing later today

I don't have a strong opinion on either using set_code or on_initialize, as long as @teor2345 is okay, since he will also maintain this code.

As for the test in 00a4c43, I have investigated the failure and did find an issue with the implementation in this PR.

More specifically, when the ER version is upgraded from V0 to V1 in consensus block #N, the runtime will assume using V0 for block #N and using V1 starting from block #N+1. While on the client side, when querying the ER versions in block #N it will return V1, thus it will use V1 to construct ER derived from #N. This inconsistency will cause the bundle to be rejected and the domain chain to stop progressing.

FYI, in 048fd2b, this edge case is handled by:

subspace/crates/pallet-domains/src/lib.rs

Lines 2100 to 2104 in 048fd2b

    
           // NOTE: the new runtime code is take affect in the next block, thus we pass the 
        
           // `parent_number` since which brings the new runtime code. And most important, in the 
        
           // client side while using the runtime API at `parent_hash` to query the bundle/ER 
        
           // version, it will also return the bundle/ER version in the new runtime code. 
        
           Self::record_new_bundle_and_er_version(parent_number);

NingLin-P

Rest LGTM

teor2345

Summary

This is a good change, and we should merge this PR eventually. But I do not know the best time to merge it.

I wonder if it is better to do ongoing maintenance of the current code. I think there are risks in merging within the next week (or two). And risks and costs in delaying other plans to fit it in.

Review of PR Complexity

Different members of the team have different ideas of how complex this PR is.

I think this PR is now one of our most complex PRs, because of:

the number of reviews/comments/questions
the number changed lines
- I appreciate we've reduced that by about 25%
the number of PR changes after opening the PR
the number of bugs found during review and reviewer testing
- including broken benchmarks

Suggested Next Steps

We're already doing some things to reduce the risk of this PR.

Another way to reduce the risk is splitting the PR into:

refactors that do not change functionality at all
- renames, with the exact replacement commands in the commit message
- turning fields into functions, with replacements in the commit
- code movement
- any tests that work on the old code
everything else that changes functionality
- including tests for those changes

Then the first PR would be easy to review and merge, and we could focus our review efforts on the second PR.

I know this is extra work, but it might be the fastest way forward?

crates/pallet-domains/src/benchmarking.rs

crates/pallet-domains/src/lib.rs

crates/pallet-domains/src/tests.rs

crates/sp-domains-fraud-proof/src/lib.rs

crates/sp-domains-fraud-proof/src/verification.rs

crates/sp-domains/src/bundle/bundle_v0.rs

domains/client/domain-operator/src/domain_bundle_producer.rs

…ck at which rutime upgrade did takes place

vedhavyas

More specifically, when the ER version is upgraded from V0 to V1 in consensus block #N, the runtime will assume using V0 for block #N and using V1 starting from block #N+1. While on the client side, when querying the ER versions in block #N it will return V1, thus it will use V1 to construct ER derived from #N. This inconsistency will cause the bundle to be rejected and the domain chain to stop progressing.

Thanks for pointing it out @NingLin-P . This was indeed missed from my end. Should be fixed in the new commits.

I wonder if it is better to do ongoing maintenance of the current code. I think there are risks in merging within the next week (or two). And risks and costs in delaying other plans to fit it in.

@teor2345 The whole reason why the compatibility code is removed to actually push to devnet and subsequently to mainnet since we dont have a tarus beyond this point.

Another way to reduce the risk is splitting the PR into:

I do not agree on this. IF a PR changes or introduces somethings, then its tests and everything affected by it should be part of the PR. We did this earlier and it did not bode well especially its multiple steps of opening a new PR and context switching to something else.

I would rather have reviewers take as much as they need to understand the changes and the side-effects it brings, if any, before merging the PR or else its a No-go from my end.

crates/pallet-domains/src/benchmarking.rs

crates/pallet-domains/src/block_tree.rs

crates/sp-domains-fraud-proof/src/lib.rs

teor2345 · 2025-07-18T00:20:01Z

Another way to reduce the risk is splitting the PR into:

I do not agree on this. IF a PR changes or introduces somethings, then its tests and everything affected by it should be part of the PR. We did this earlier and it did not bode well especially its multiple steps of opening a new PR and context switching to something else.

Hmm, I'm not sure if there's been a misunderstanding here.

Why are tests needed for "refactors that do not change functionality at all"?

teor2345

I have some non-blocking code style questions.

There will be some residual risk no matter how much we review this. Let's do some initial testing, and see if any bugs remain?

That will give us a better idea of the risk of this change, and we can decide what to do next when we know more.

crates/pallet-domains/src/lib.rs

teor2345 · 2025-07-18T00:35:26Z

crates/pallet-domains/src/lib.rs

+        let versions = PreviousBundleAndExecutionReceiptVersions::<T>::get();
+        match versions.get(&block_number) {
+            // no upgrade happened at this number, so safe to return the current version
+            None => T::CurrentBundleAndExecutionReceiptVersion::get(),
+            // upgrade did happen at this block, so return the version stored at this number.
+            Some(version) => *version,
+        }


This code seems to be the same as:

bundle_and_execution_receipt_version_for_consensus_number(block_number, ...)

Would it be better to remove the code duplication? (Or explain in comments why they are different.)

hmm why you think this would be similar here. are the code comments not clear ?

Because both functions return the current version if there is no upgrade in this block, and the previous version if there is?

fair! But we cannot remove this function anyway since the block_number unlike other would require use to get current block number and is straightforward getter where in other function introduces maybe even though we always get the Version.

I would still prefer to keep this as is for easier read

Note: the other function does a loop iteration to find the current version for any block number while this one does a direct lookup.

crates/pallet-domains/src/lib.rs

teor2345 · 2025-07-18T00:48:58Z

crates/pallet-domains/src/lib.rs

+    pub(crate) fn set_previous_bundle_and_execution_receipt_version<SV, PV, BEV>(
+        block_number: BlockNumberFor<T>,
+        set_version: SV,
+        previous_versions: PV,
+        current_version: BEV,
+    ) where
+        SV: Fn(BTreeMap<BlockNumberFor<T>, BEV>),
+        PV: Fn() -> BTreeMap<BlockNumberFor<T>, BEV>,
+        BEV: PartialEq,
+    {
+        let mut versions = previous_versions();


This is an unusual code style.

Normally we would just pass a BTreeMap<BlockNumberFor<T>, BEV> directly to the function, return a BTreeMap<BlockNumberFor<T>, BEV> from the function, and then use the return value to set the storage.

Since the get and set are called unconditionally, I'm not sure why we're passing Fns to this function.

Passing Fns will reduce the amount of inlining and optimisation the compiler can do, potentially reducing performance (or increasing code size).

explained why I did above

teor2345 · 2025-07-18T00:49:52Z

crates/pallet-domains/src/lib.rs

+    pub(crate) fn bundle_and_execution_receipt_version_for_consensus_number<PV, BEV>(
+        er_derived_number: BlockNumberFor<T>,
+        previous_versions: PV,
+        current_version: BEV,
+    ) -> Option<BEV>
+    where
+        PV: Fn() -> BTreeMap<BlockNumberFor<T>, BEV>,
+        BEV: Copy + Clone,
+    {
+        let versions = previous_versions();


Similar feedback here, normally we would just pass BTreeMap<BlockNumberFor<T>, BEV> to this function directly.

Passing a Fn will reduce the amount of inlining and optimisation the compiler can do, potentially reducing performance (or increasing code size).

This is done specifically for testing. PTAL at testing this logic with mock versions and mock storage but reusing the same the same logic to pick the correct versions

I made these comments based on the test code.

What stops us calling the production code as:

bundle_and_execution_receipt_version_for_consensus_number( er_derived_number, CurrentBundleAndExecutionReceiptVersion::get(), PreviousBundleAndExecutionReceiptVersions::get(), )

And the test code as:

bundle_and_execution_receipt_version_for_consensus_number( er_derived_number, MockCurrentBundleAndExecutionReceiptVersion::get(), MockPreviousBundleAndExecutionReceiptVersions::get(), )

This is due to the change in type between production and testing for BundleAndExecutionReceiptVersion and in the inner types.

teor2345

It appears 3 benchmarks are broken now:

- pallet_messenger_from_domains_extension::from_domains_relay_message
- pallet_messenger_from_domains_extension::from_domains_relay_message_channel_open
- pallet_messenger_from_domains_extension::from_domains_relay_message_response

https://github.com/autonomys/subspace/actions/runs/16361523792/job/46230168298?pr=3623#step:6:5460

vedhavyas · 2025-07-18T03:52:46Z

Nice! very helpful. Thanks 🙏🏼

vedhavyas added 9 commits July 3, 2025 14:19

introduce v1 bundle version and rename current bundle type to BundleV1

dbf7599

introduce fraud proof v1 and update fraud proof api with compatibilit…

c1fc6d6

…y code for previous fraud proof version

move fraud proof v0 to its own module

15b37a3

refactor bundle versions and fraud proof versions to specific modules

7aa3c95

move execution receipt type to its own module

3361109

define current execution receipt version and update getters

8af40f8

rename execution receipt to v0

05b9593

cleanup intermediate code and add V0 bundle version to the bundle type

7367a2c

add v0 of execution receipt and versioned Er for Bundle V1

8469518

vedhavyas requested review from NingLin-P and nazar-pc as code owners July 9, 2025 10:11

vedhavyas requested a review from teor2345 July 9, 2025 10:12

vedhavyas force-pushed the versioned_bundle branch from 5e66e75 to 335e1d6 Compare July 9, 2025 10:13

teor2345 reviewed Jul 9, 2025

View reviewed changes

add runtime compatibility code in decoding ER and BlockTreeNode and r…

77abccd

…emove unnecessary clone

vedhavyas force-pushed the versioned_bundle branch from 335e1d6 to 77abccd Compare July 10, 2025 06:38

update docs

4980b06

teor2345 added the needs benchmarking Significant benchmark or runtime code changes, re-run benchmarks before the runtime release label Jul 10, 2025

NingLin-P reviewed Jul 11, 2025

View reviewed changes

crates/pallet-domains/src/lib.rs Outdated Show resolved Hide resolved

crates/pallet-domains/src/lib.rs Show resolved Hide resolved

vedhavyas added 3 commits July 11, 2025 15:57

store previous bundle and er versions on runtime upgrade

87885e4

use execution receipt version on client when deriving new ER

93f0d08

check expected execution receipt version

3fa7be6

vedhavyas requested a review from NingLin-P July 11, 2025 11:27

NingLin-P reviewed Jul 11, 2025

View reviewed changes

crates/pallet-domains/src/lib.rs Outdated Show resolved Hide resolved

crates/pallet-domains/src/lib.rs Outdated Show resolved Hide resolved

crates/pallet-domains/src/lib.rs Outdated Show resolved Hide resolved

add tests for storing versions and quering versions

fea69d7

vedhavyas force-pushed the versioned_bundle branch from 86f298b to fea69d7 Compare July 14, 2025 06:53

Merge branch 'main' into versioned_bundle

86d1e23

vedhavyas commented Jul 16, 2025

View reviewed changes

crates/sp-domains-fraud-proof/benches/fraud_proof_verification.rs Show resolved Hide resolved

vedhavyas requested review from NingLin-P and teor2345 July 16, 2025 06:23

vedhavyas added breaking-execution This PR introduces breaking changes to the execution and removed needs benchmarking Significant benchmark or runtime code changes, re-run benchmarks before the runtime release labels Jul 16, 2025

jfrank-summit reviewed Jul 16, 2025

View reviewed changes

crates/pallet-domains/src/block_tree.rs Show resolved Hide resolved

crates/pallet-domains/src/block_tree.rs Outdated Show resolved Hide resolved

crates/pallet-domains/src/block_tree.rs Outdated Show resolved Hide resolved

NingLin-P reviewed Jul 16, 2025

View reviewed changes

teor2345 reviewed Jul 16, 2025

View reviewed changes

vedhavyas added 2 commits July 17, 2025 09:50

fix benchmarks

b9bae84

ensure correct execution and bundle versions are returned for the blo…

0391dcb

…ck at which rutime upgrade did takes place

vedhavyas force-pushed the versioned_bundle branch from 96df613 to 40c14ef Compare July 17, 2025 04:42

update comments, optimisations, and code reverts

5ed52a2

vedhavyas force-pushed the versioned_bundle branch from 40c14ef to 5ed52a2 Compare July 17, 2025 04:54

vedhavyas commented Jul 17, 2025

View reviewed changes

crates/pallet-domains/src/benchmarking.rs Show resolved Hide resolved

crates/pallet-domains/src/block_tree.rs Show resolved Hide resolved

crates/sp-domains-fraud-proof/src/lib.rs Show resolved Hide resolved

Merge branch 'main' into versioned_bundle

75bf1b4

vedhavyas requested review from teor2345, NingLin-P and jfrank-summit July 17, 2025 05:09

teor2345 previously approved these changes Jul 18, 2025

View reviewed changes

refactor variable names

4bafe4b

vedhavyas dismissed teor2345’s stale review via 4bafe4b July 18, 2025 03:33

vedhavyas enabled auto-merge July 18, 2025 03:33

vedhavyas requested a review from teor2345 July 18, 2025 03:34

teor2345 reviewed Jul 18, 2025

View reviewed changes

vedhavyas added 2 commits July 18, 2025 09:22

Merge branch 'main' into versioned_bundle

2840651

update benchmark fixtures for messenger

a38c00e

Versioned bundle and Execution receipt #3623

Are you sure you want to change the base?

Versioned bundle and Execution receipt #3623

Uh oh!

Conversation

vedhavyas commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notable changes

Cleanup

Code contributor checklist:

Uh oh!

teor2345 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vedhavyas commented Jul 10, 2025

Uh oh!

teor2345 commented Jul 10, 2025

Uh oh!

vedhavyas commented Jul 10, 2025

Uh oh!

Uh oh!

Uh oh!

NingLin-P left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vedhavyas commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vedhavyas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NingLin-P commented Jul 16, 2025

Uh oh!

NingLin-P left a comment

Choose a reason for hiding this comment

Uh oh!

teor2345 left a comment

Choose a reason for hiding this comment

Summary

Review of PR Complexity

Suggested Next Steps

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vedhavyas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

teor2345 commented Jul 18, 2025

Uh oh!

teor2345 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vedhavyas commented Jul 9, 2025 •

edited

Loading

vedhavyas commented Jul 14, 2025 •

edited

Loading

vedhavyas left a comment •

edited

Loading

vedhavyas Jul 18, 2025 •

edited

Loading