Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Update MMR Runtime API with functionality to generate MMR proof for a series of leaf indices #10635

Conversation

Wizdave97
Copy link
Contributor

@Wizdave97 Wizdave97 commented Jan 11, 2022

This PR adds new functions to the MMR Runtime API to accept an array of leaf indexes, this would allow generating a proof for multiple leaf indices in one run if needed

Also added functions for verifying these kind of proof

Previous implementation of the PR here #10625
@rphmeier this is an updated fix for #10625 following your recommendation

polkadot companion: paritytech/polkadot#4700

@Wizdave97 Wizdave97 marked this pull request as ready for review January 12, 2022 08:55
@acatangiu acatangiu self-requested a review January 13, 2022 12:02
@rphmeier rphmeier added the A0-please_review Pull request needs code review. label Jan 14, 2022
@stale
Copy link

stale bot commented Mar 5, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 5, 2022
@seunlanlege
Copy link
Contributor

Bump

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 5, 2022
@stale
Copy link

stale bot commented Apr 4, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Apr 4, 2022
@Wizdave97
Copy link
Contributor Author

bump

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Apr 5, 2022
@seunlanlege
Copy link
Contributor

@acatangiu please could you take a look at this PR? The API it adds will be useful to parachain teams

@acatangiu acatangiu requested a review from andresilva April 7, 2022 07:55
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

I think most of these functions could be deduplicated if you just used primitives::BatchProof everywhere.

primitives::Proof is just a primitives::BatchProof with inner leaf_indices.len() == 1.

Since we don't want to break the API we can keep the non-batched runtime API functions at the runtime API level (we can also keep both versions for RPC), but everything underneath (frame/merkle-mountain-range/src/mmr/mmr.rs, frame/merkle-mountain-range/src/lib.rs) can have single implementation that works with BatchProof.

frame/merkle-mountain-range/src/lib.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/primitives/src/lib.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/src/mmr/mmr.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/src/mmr/mmr.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/src/mmr/mmr.rs Outdated Show resolved Hide resolved
@acatangiu
Copy link
Contributor

acatangiu commented Apr 12, 2022

@Wizdave97 @seunlanlege sorry for the delay in review, I've only recently got acquainted with the MMR pallet and library.

Also FYI the MMR primitives are moved in #11183 ; so this PR should also rebase on top of that to get clean merge.

@Wizdave97 Wizdave97 force-pushed the david/allow-batch-verification-of-leaves branch from 5f002ab to 4132c67 Compare April 13, 2022 14:40
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

🚀

frame/merkle-mountain-range/rpc/src/lib.rs Outdated Show resolved Hide resolved
frame/merkle-mountain-range/src/lib.rs Outdated Show resolved Hide resolved
primitives/merkle-mountain-range/src/lib.rs Outdated Show resolved Hide resolved
primitives/merkle-mountain-range/src/lib.rs Outdated Show resolved Hide resolved
@acatangiu acatangiu requested a review from svyatonik April 18, 2022 10:54
Copy link
Contributor

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

Couple of suggestions, otherwise seems good

frame/merkle-mountain-range/rpc/src/lib.rs Show resolved Hide resolved
frame/merkle-mountain-range/src/mmr/mmr.rs Show resolved Hide resolved
frame/merkle-mountain-range/src/mmr/mmr.rs Show resolved Hide resolved
@acatangiu acatangiu added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Apr 19, 2022
@acatangiu
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#4700

@Wizdave97 Wizdave97 force-pushed the david/allow-batch-verification-of-leaves branch from 26a16e1 to 590eae9 Compare April 27, 2022 12:15
@Wizdave97 Wizdave97 requested a review from acatangiu April 27, 2022 14:52
@acatangiu
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#4700

@acatangiu
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#4700

Comment on lines 366 to 367
/// Will panic if leaf indices length is less than 1
pub fn into_single_leaf_proof(proof: BatchProof<Hash>) -> Proof<Hash> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed this and think we should do checked access and return Error::InvalidLeafIndices instead of unchecked access and panicking.

@Wizdave97 Wizdave97 requested a review from acatangiu May 3, 2022 15:35
@acatangiu
Copy link
Contributor

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 636ffa4 into paritytech:master May 4, 2022
@Wizdave97 Wizdave97 deleted the david/allow-batch-verification-of-leaves branch May 4, 2022 11:40
godcodehunter pushed a commit to sensoriumxr/substrate that referenced this pull request Jun 22, 2022
… series of leaf indices (paritytech#10635)

* updated mmr rpc api with functions for batch generation of proof

* update code comments

* fix build errors

* added tests to mmr-rpc

* add tests to pallet-mmr

* update comments

* minor comment fix

* remove unused variables

* fix rust doc errors

* refactor mmr runtime api

* fix tests

* minor fix

* minor fix

* fix node-runtime

* revert to initial api

* impl from proof fot batchproof

* minor fix

* minor fix

* use explicit functions to convert btw batch proof and single proof

* minor fix

* add new variant to mmr error

* fmt

* update conversion to single leaf proof

* fix style nit

Co-authored-by: Adrian Catangiu <adrian@parity.io>
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
… series of leaf indices (paritytech#10635)

* updated mmr rpc api with functions for batch generation of proof

* update code comments

* fix build errors

* added tests to mmr-rpc

* add tests to pallet-mmr

* update comments

* minor comment fix

* remove unused variables

* fix rust doc errors

* refactor mmr runtime api

* fix tests

* minor fix

* minor fix

* fix node-runtime

* revert to initial api

* impl from proof fot batchproof

* minor fix

* minor fix

* use explicit functions to convert btw batch proof and single proof

* minor fix

* add new variant to mmr error

* fmt

* update conversion to single leaf proof

* fix style nit

Co-authored-by: Adrian Catangiu <adrian@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
… series of leaf indices (paritytech#10635)

* updated mmr rpc api with functions for batch generation of proof

* update code comments

* fix build errors

* added tests to mmr-rpc

* add tests to pallet-mmr

* update comments

* minor comment fix

* remove unused variables

* fix rust doc errors

* refactor mmr runtime api

* fix tests

* minor fix

* minor fix

* fix node-runtime

* revert to initial api

* impl from proof fot batchproof

* minor fix

* minor fix

* use explicit functions to convert btw batch proof and single proof

* minor fix

* add new variant to mmr error

* fmt

* update conversion to single leaf proof

* fix style nit

Co-authored-by: Adrian Catangiu <adrian@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants