Storage Proofs: BlockDigest -> DomainRuntimeUpgrades & merge AllowList #3329
Conversation
teor2345
added
execution
teor2345
requested review from
NingLin-P,
vedhavyas and
nazar-pc
as code owners
crates/subspace-runtime/src/lib.rs
Outdated
| @@ -1048,7 +1048,9 @@ impl FraudProofStorageKeyProvider<NumberFor<Block>> for StorageKeyProvider { | |||
| FraudProofStorageKeyRequest::DomainAllowlistUpdates(domain_id) => { | |||
| Messenger::domain_allow_list_update_storage_key(domain_id) | |||
| } | |||
| FraudProofStorageKeyRequest::BlockDigest => sp_domains::system_digest_final_key(), | |||
| FraudProofStorageKeyRequest::DomainRuntimeUpgrades => { | |||
| pallet_domains::DomainRuntimeUpgrades::<Runtime>::hashed_key().to_vec() | |||
There was a problem hiding this comment.
Should this be using DomainRuntimeUpgradeRecords or DomainRuntimeUpgrades for the parent block instead?
(pallet_domains::Hooks::on_initialize() clears the contents of DomainRuntimeUpgrades using take().)
There was a problem hiding this comment.
The test is failing with Invalid Domain extrinsic root proof verification failed: StorageProof(DomainRuntimeUpgradesStorageProof(MissingValue)), and yes this is because we clear the contents of DomainRuntimeUpgrades using take() and this value does not exist in the Merkle tree thus we are unable to generate a storage proof (which is a proof of existence).
To fix it we need to instead reset the DomainRuntimeUpgrades to an empty vec so we can generate a proof-of-empty-value.
There was a problem hiding this comment.
How did you find this failure in the logs?
All I can see is a test hang (not a test failure):
https://github.com/autonomys/subspace/actions/runs/12459186519/job/34775706616?pr=3329#step:11:1535
There was a problem hiding this comment.
I run the test locally cargo test -p domain-client-operator -- --exact tests::test_invalid_domain_extrinsics_root_proof_creation
There was a problem hiding this comment.
To fix it we need to instead reset the
DomainRuntimeUpgradesto an empty vec so we can generate a proof-of-empty-value.
But an empty value doesn't work with the rest of the code, which needs a list of upgraded runtime IDs:
https://github.com/autonomys/subspace/pull/3329/files#diff-4828b750acc5f6c044ce5281811e58a5135ab6b80541bd805a1dde9281159f9fR344-R346
So I think we should be using DomainRuntimeUpgradeRecords instead.
There was a problem hiding this comment.
An empty list (or a list that doesn't contain the domain's runtime id) means there is no domain runtime upgrade happening, so we don't need to generate the runtime code storage proof. In this way, if a bad ER claims there is a domain runtime upgrade happened, the FP can prove that there is actually not and prune the bad ER and slash the submitter.
The problem with using DomainRuntimeUpgradeRecords is if a domain runtime upgrade happens in #N then the record is added to the state at #N+1 and the record is clear from the state at #N+1+challenge period.
teor2345
force-pushed
the
fraud-proof-data-digest
branch
from
f657e5c to
2f3a9b6
Compare
teor2345
changed the title
teor2345
force-pushed
the
fraud-proof-data-digest
branch
from
2f3a9b6 to
a0a7dd3
Compare
teor2345
force-pushed
the
fraud-proof-data-digest
branch
from
a0a7dd3 to
0c39996
Compare
| let domain_chain_allowlist = | ||
| T::DomainAllowlistUpdates::domain_allowlist_updates(domain_id) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Is it because of the allowlist update we need to store the InvalidInherentExtrinsicData for each domain separately?
While most of the data are the same for all the domain I think it makes more sense to store them once and shared by all the domains, for the allowlist update we can store Map<domain_id, AllowlistUpdate> which may bring redundant data in the FP but it should be fine as FP and the allowlist update are rare.
| // At genesis, avoid block number underflow, and use the default zero genesis parent hash. | ||
| let parent_number = block_number.checked_sub(&One::one()); |
There was a problem hiding this comment.
IIUC there should be no on_initialize/on_finalize for the genesis block, the genesis block is built from the chain spec with genesis_build, also see https://github.com/paritytech/polkadot-sdk/blob/ca7817922148c1e6f6856138998f7135f42f3f4f/substrate/frame/system/src/lib.rs#L1812.
cc @vedhavyas @nazar-pc in case I'm wrong.
| #[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, |
There was a problem hiding this comment.
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.
This PR replaces the block digest storage proof with the existing runtime upgrade records storage proof, and merges the allow list storage proof into the common storage proof.
It also cleans up the associated struct hierarchy, and bumps the runtime version.
Part of ticket #3281.
Code contributor checklist: