Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stateless mmr proof for the consensus chain #2687

Merged
merged 8 commits into from
Apr 19, 2024
Merged

Conversation

NingLin-P
Copy link
Member

@NingLin-P NingLin-P commented Apr 17, 2024

This PR adds support for generating & verifying mmr proof in a stateless fashion for the consensus chain, so the consensus node (i.e. farmer) won't be required to maintain the offchain MMR leaf and resolves the conflict with fast-sync (fast-synced node won't has MMR data) and toward our goal to make the farmer lightweight. This is done by storing MmrRootHashCount number of mmr root in the consensus runtime, and use these roots in mmr proof verification instead of getting from offchain storage.

In this PR this is only used in the XDM verification of the consensus chain, while the domain is still and MUST be the same as before because the mmr proof verification in the domain chain is done by a runtime API call (via a host function) to its consensus node and the result very depends on the hash we used to this call (we use the parent hash of the best block currently) which means the result may be different since the best block may be different in different node thus the domain block execution can be indeterministic (Also, I run the test_cross_domains_messages_should_work test locally it passes, but this test is disabled in CI due flaky).

In the upcoming PR, the fraud proof verification will also be updated to use the stateless mmr proof.

Code contributor checklist:

Signed-off-by: linning <linningde25@gmail.com>
Signed-off-by: linning <linningde25@gmail.com>
…ck number at which the proof is generated

This will be used later in the consensus runtime to verify XDM stateless

Signed-off-by: linning <linningde25@gmail.com>
Signed-off-by: linning <linningde25@gmail.com>
@NingLin-P
Copy link
Member Author

Apology for the annoying force push, always forgot to run clippy with --features runtime-benchmarks 🤦‍♂️

Copy link
Contributor

@vedhavyas vedhavyas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally make sense

I was expecting domain also use the stateless verification since the MMR roots are avaible on consensus state. At the moment, even with these changes, host function domain's use will still do the proof verification statefully.

I believe we should enable cross domain tests. I dont think there would be any reason to disable it right now with the recent changes that went in

#[pallet::storage]
#[pallet::getter(fn mmr_root_hash)]
pub type MmrRootHashes<T: Config> =
StorageMap<_, Twox64Concat, BlockNumberFor<T>, T::MmrRootHash, OptionQuery>;
}

impl<T: Config> OnNewRoot<T::MmrRootHash> for Pallet<T> {
fn on_new_root(root: &T::MmrRootHash) {
let digest = DigestItem::new_mmr_root(*root);
<frame_system::Pallet<T>>::deposit_log(digest);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should remove this per our discussion since this is unneeded at this time
cc: @nazar-pc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in case we already deployed runtime with it (IDK) removing corresponding enum variant will be a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is already deployed to 3h so will leave a TODO for now.

/// root at `to_prove`.
// TODO: remove `dead_code` after it is used in fraud proof generation
#[allow(dead_code)]
pub(crate) fn generate_mmr_proof<CClient, CBlock>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will we be using this in the relayer while generating MMR prrofs for XDM ?

Copy link
Member Author

@NingLin-P NingLin-P Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use this for generating MMR proofs for the consensus XDM, but there are some slight differences between the proof of the consensus chain and the domain chain, so we still need to keep 2 versions of this anyway, I just distinguish them as XDM vs FP instead of consensus vs domain.

@NingLin-P
Copy link
Member Author

NingLin-P commented Apr 18, 2024

I was expecting domain also use the stateless verification since the MMR roots are avaible on consensus state. At the moment, even with these changes, host function domain's use will still do the proof verification statefully.

I did consider this but it seems problematic, because the mmr proof verification is done in the domain node via a host function to a runtime API call to the consensus chain's current best fork, while the proof is verified against the state of a specific domain block (i.e. the parent block) in the runtime, the consensus chain in the host function is not tied to a specific block, so as different domain nodes may execution domain block and verify the proof at a different time the consensus chain best block may be changed, as a result the execution may not be deterministic.

The proof verification in consensus chain doesn't has this problem because it is totally done in the runtime against a specific state, the current proof verification in the domain node (i.e. stateful verification) is also okay because we are generating proof at a finalized consensus block (not the best block like the stateless proof) so the result will be deterministic. Hope this makes sense.

I believe we should enable cross domain tests. I dont think there would be any reason to disable it right now with the recent changes that went in

Okay, will enable it.

Signed-off-by: linning <linningde25@gmail.com>
@NingLin-P
Copy link
Member Author

It seems the test test_cross_domains_messages_should_work is still flaky thus force push to disable it again...

Copy link
Contributor

@dastansam dastansam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some typos)

crates/pallet-subspace-mmr/src/lib.rs Outdated Show resolved Hide resolved
test/subspace-test-primitives/src/lib.rs Outdated Show resolved Hide resolved
domains/client/relayer/src/lib.rs Outdated Show resolved Hide resolved
vedhavyas
vedhavyas previously approved these changes Apr 19, 2024
Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>
NingLin-P and others added 2 commits April 19, 2024 16:45
Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>
Co-authored-by: Dastan <88332432+dastansam@users.noreply.github.com>
@NingLin-P NingLin-P added this pull request to the merge queue Apr 19, 2024
Merged via the queue into main with commit 13a6380 Apr 19, 2024
9 checks passed
@NingLin-P NingLin-P deleted the stateless-mmr-proof branch April 19, 2024 11:28
pub proof: MmrProof<MmrHash>,
}

// TODO: update upstream `EncodableOpaqueLeaf` to derive clone.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please send a PR upstream right away in such cases: paritytech/polkadot-sdk#5442

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants