Storage proof: change BlockDigest to DomainRuntimeUpgrades #3329

teor2345 · 2024-12-23T00:48:32Z

This PR replaces the block digest storage proof with the existing runtime upgrade records storage proof.

It also cleans up the fraud proof struct hierarchy, and fixes up some comments.

Breaking changes:

moving structs and proofs around in the fraud proof data layout
add/delete enum variants which are passed to runtime functions, which is a breaking runtime change.

Part of ticket #3281.

Code contributor checklist:

I have read, understood and followed contributing guide

crates/subspace-runtime/src/lib.rs

crates/pallet-domains/src/lib.rs

NingLin-P · 2024-12-24T11:55:54Z

crates/subspace-runtime/src/lib.rs

 #[sp_version::runtime_version]
 pub const VERSION: RuntimeVersion = RuntimeVersion {
    spec_name: Cow::Borrowed("subspace"),
    impl_name: Cow::Borrowed("subspace"),
    authoring_version: 0,
-    spec_version: 1,


The consensus runtime version is already bumped in 4ba268a.

As a good practice, I think we should bump the runtime version when we are going to do runtime upgrade instead of when there are runtime code change to avoid bumping the version multiple times by accident.

The consensus runtime version is already bumped in 4ba268a.

I deliberately put the same version bump commit in both PRs.

This PR was created before that PR merged, so we had to bump the runtime version in both. Otherwise, it would be possible to merge an incompatible runtime version change without bumping the version.

But it is exactly the same commit in both PRs. So when this PR is merged into main, there will be no change to the runtime version, because the commit was already merged in PR #3303.

As a good practice, I think we should bump the runtime version when we are going to do runtime upgrade instead of when there are runtime code change to avoid bumping the version multiple times by accident.

I'm not sure about this. I thought the team decided to bump the version in the history seeding removal PR, rather than waiting until we wanted to do a runtime upgrade:
#3303 (comment)

There's a few different ways we can do runtime version bumps:

Bump the version in each PR that makes a breaking runtime change, or in the first PR that makes a breaking change

we could accidentally bump multiple times, wasting a few version numbers

Bump the version before each runtime upgrade

we could accidentally forget, leading to a rejected upgrade

if we decide not to upgrade, we could accidentally bump multiple times, wasting a few version numbers

Bump the version after each runtime upgrade

(I can't see anything that's likely to go wrong here, what do you think?)

If we had a checklist and followed it every time, it wouldn't matter which option we chose. But until we do that, it seems like option 3 (or option 1) is the least likely to cause significant issues.

Let's talk about it with the team, and come up with a consistent way of doing it?

I thought the team decided to bump the version in the history seeding removal PR, rather than waiting until we wanted to do a runtime upgrade:
#3303 (comment)

Yeah, I saw that, this is just a thought that I think can make the runtime upgrade process better.

If we had a checklist and followed it every time, it wouldn't matter which option we chose. But until we do that, it seems like option 3 (or option 1) is the least likely to cause significant issues.

We should certainly have a checklist (including proper testing) for the runtime upgrade. Since the only onchain check for upgrade is the runtime version, we need to do more offchain checks to the runtime code, things that I'm most concerned about are not rejected upgrades due to runtime version (which only wastes some tx fee) but successful upgrades with incompatible runtime (e.g. with host function change or incompatible storage change) which can do much damage to the network and may even need a hard fork to fix. So I think our goal is not to make the upgrade to pass the runtime version check as easy and convenient as possible.

Let's talk about it with the team, and come up with a consistent way of doing it?

Yeah, we can discuss this on the next sync, definitely not a blocker for this PR.

I dropped this commit when I was rebasing, because it is already in the main branch.

teor2345

I'm still working my way through these suggestions. But I'm wondering if it would be easier to just leave the allow lists the way they are.

teor2345 · 2025-01-02T00:48:18Z

crates/subspace-runtime/src/lib.rs

 #[sp_version::runtime_version]
 pub const VERSION: RuntimeVersion = RuntimeVersion {
    spec_name: Cow::Borrowed("subspace"),
    impl_name: Cow::Borrowed("subspace"),
    authoring_version: 0,
-    spec_version: 1,


The consensus runtime version is already bumped in 4ba268a.

I deliberately put the same version bump commit in both PRs.

This PR was created before that PR merged, so we had to bump the runtime version in both. Otherwise, it would be possible to merge an incompatible runtime version change without bumping the version.

But it is exactly the same commit in both PRs. So when this PR is merged into main, there will be no change to the runtime version, because the commit was already merged in PR #3303.

As a good practice, I think we should bump the runtime version when we are going to do runtime upgrade instead of when there are runtime code change to avoid bumping the version multiple times by accident.

I'm not sure about this. I thought the team decided to bump the version in the history seeding removal PR, rather than waiting until we wanted to do a runtime upgrade:
#3303 (comment)

There's a few different ways we can do runtime version bumps:

Bump the version in each PR that makes a breaking runtime change, or in the first PR that makes a breaking change

we could accidentally bump multiple times, wasting a few version numbers

Bump the version before each runtime upgrade

we could accidentally forget, leading to a rejected upgrade

if we decide not to upgrade, we could accidentally bump multiple times, wasting a few version numbers

Bump the version after each runtime upgrade

(I can't see anything that's likely to go wrong here, what do you think?)

If we had a checklist and followed it every time, it wouldn't matter which option we chose. But until we do that, it seems like option 3 (or option 1) is the least likely to cause significant issues.

Let's talk about it with the team, and come up with a consistent way of doing it?

crates/pallet-domains/src/lib.rs

teor2345

Alright, that should be everything.

I also added some timeouts to the tests, to make any errors easier to diagnose.

teor2345 · 2025-01-02T23:01:53Z

crates/subspace-runtime/src/lib.rs

 #[sp_version::runtime_version]
 pub const VERSION: RuntimeVersion = RuntimeVersion {
    spec_name: Cow::Borrowed("subspace"),
    impl_name: Cow::Borrowed("subspace"),
    authoring_version: 0,
-    spec_version: 1,


I dropped this commit when I was rebasing, because it is already in the main branch.

crates/pallet-domains/src/lib.rs

NingLin-P

Make sense overall, just a few nits, thanks!

crates/pallet-domains/src/lib.rs

NingLin-P · 2025-01-03T09:48:17Z

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

            &self.storage_key_provider,
            self.consensus_client.as_ref(),
-            domain_id,
            consensus_block_hash,
            maybe_runtime_id,


We can also replace BlockDigest with DomainRuntimeUpgrades to determine if runtime upgrades happen in is_domain_runtime_upgraded_at

I don't know how to do this, sorry! For the next few months, can provide more details about how to do things in tickets and reviews?

You can see what I tried in commit 6077082 of https://github.com/autonomys/subspace/tree/fraud-proof-data-digest-upgraded-at-via-proof

I couldn't get the value of DomainRuntimeUpgrades in is_domain_runtime_upgraded_at(), because there is no runtime API for it. So I tried creating and verifying a storage proof to get that value, but I think that didn't work because the proof hadn't been submitted yet.

Do you want me to add a new runtime API?

Do you want me to add a new runtime API?

Yeah, that is what I mean, sorry for not being clear, let's leave this to the next PR.

crates/pallet-domains/src/lib.rs

teor2345

This PR has been open for a while, and it's getting pretty big.

If there aren't any blockers, can we merge it as it is, then do any further cleanups in a new PR?

crates/pallet-domains/src/lib.rs

teor2345 · 2025-01-06T04:17:46Z

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

            &self.storage_key_provider,
            self.consensus_client.as_ref(),
-            domain_id,
            consensus_block_hash,
            maybe_runtime_id,


I don't know how to do this, sorry! For the next few months, can provide more details about how to do things in tickets and reviews?

You can see what I tried in commit 6077082 of https://github.com/autonomys/subspace/tree/fraud-proof-data-digest-upgraded-at-via-proof

I couldn't get the value of DomainRuntimeUpgrades in is_domain_runtime_upgraded_at(), because there is no runtime API for it. So I tried creating and verifying a storage proof to get that value, but I think that didn't work because the proof hadn't been submitted yet.

Do you want me to add a new runtime API?

NingLin-P

LGTM, thanks!

teor2345 added execution Subspace execution breaking-runtime This PR introduces breaking changes to the runtime labels Dec 23, 2024

teor2345 self-assigned this Dec 23, 2024

teor2345 requested review from NingLin-P, vedhavyas and nazar-pc as code owners December 23, 2024 00:48

teor2345 mentioned this pull request Dec 23, 2024

Merge DomainAllowlistUpdates into common storage proof #3327

Closed

1 task

teor2345 commented Dec 23, 2024

View reviewed changes

crates/subspace-runtime/src/lib.rs Show resolved Hide resolved

teor2345 enabled auto-merge December 23, 2024 08:00

teor2345 force-pushed the fraud-proof-data-digest branch from f657e5c to 2f3a9b6 Compare December 23, 2024 23:46

teor2345 changed the title ~~Replace BlockDigest with DomainRuntimeUpgrades storage proof~~ Storage Proofs: BlockDigest -> DomainRuntimeUpgrades & merge AllowList Dec 23, 2024

teor2345 force-pushed the fraud-proof-data-digest branch 2 times, most recently from a0a7dd3 to 0c39996 Compare December 24, 2024 02:21

NingLin-P reviewed Dec 24, 2024

View reviewed changes

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

NingLin-P reviewed Dec 24, 2024

View reviewed changes

teor2345 commented Jan 2, 2025

View reviewed changes

teor2345 force-pushed the fraud-proof-data-digest branch from 0c39996 to 96cc741 Compare January 2, 2025 23:39

teor2345 commented Jan 2, 2025

View reviewed changes

Fix typos

c992083

teor2345 force-pushed the fraud-proof-data-digest branch from 96cc741 to d712dec Compare January 2, 2025 23:46

teor2345 commented Jan 2, 2025

View reviewed changes

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

teor2345 changed the title ~~Storage Proofs: BlockDigest -> DomainRuntimeUpgrades & merge AllowList~~ Storage proof: change BlockDigest to DomainRuntimeUpgrades Jan 2, 2025

NingLin-P reviewed Jan 3, 2025

View reviewed changes

teor2345 added 6 commits January 6, 2025 14:13

Storage proof: change BlockDigest to DomainRuntimeUpgrades

f6397d8

Skip on_initialize() and on_finalize() for genesis in tests

693b087

Always have an explicit value in DomainRuntimeUpgrades

c26e6a8

Add a timeout to fraud proof tests to improve diagnostics

2aa8a42

Fail fraud proof tests on timeout

ba0a6f8

Clarify fraud proof test comments

4c366f5

teor2345 force-pushed the fraud-proof-data-digest branch from d712dec to 4c366f5 Compare January 6, 2025 04:13

teor2345 commented Jan 6, 2025

View reviewed changes

teor2345 added this pull request to the merge queue Jan 6, 2025

NingLin-P approved these changes Jan 6, 2025

View reviewed changes

Merged via the queue into main with commit 0992ef6 Jan 6, 2025
8 checks passed

teor2345 deleted the fraud-proof-data-digest branch January 6, 2025 09:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Storage proof: change BlockDigest to DomainRuntimeUpgrades #3329

Storage proof: change BlockDigest to DomainRuntimeUpgrades #3329

teor2345 commented Dec 23, 2024 •

edited

Loading

NingLin-P Dec 24, 2024

teor2345 Jan 2, 2025

NingLin-P Jan 2, 2025

teor2345 Jan 2, 2025

teor2345 left a comment

teor2345 Jan 2, 2025

teor2345 left a comment

teor2345 Jan 2, 2025

NingLin-P left a comment

NingLin-P Jan 3, 2025

teor2345 Jan 6, 2025

NingLin-P Jan 6, 2025

teor2345 left a comment

teor2345 Jan 6, 2025

NingLin-P left a comment

Storage proof: change BlockDigest to DomainRuntimeUpgrades #3329

Storage proof: change BlockDigest to DomainRuntimeUpgrades #3329

Conversation

teor2345 commented Dec 23, 2024 • edited Loading

Code contributor checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NingLin-P left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NingLin-P left a comment

Choose a reason for hiding this comment

teor2345 commented Dec 23, 2024 •

edited

Loading