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

pallet-mmr: Change runtime API and RPC API to work with block numbers instead of leaf indices #12339

Closed
Tracked by #1636
acatangiu opened this issue Sep 23, 2022 · 8 comments · Fixed by #12345
Closed
Tracked by #1636
Labels
D2-breaksapi I7-refactor Code needs refactoring. U2-some_time_soon Issue is worth doing soon. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.

Comments

@acatangiu
Copy link
Contributor

acatangiu commented Sep 23, 2022

Issue

This issue was triggered by a good observation made by @serban300 in #12324 (comment):

Current pallet-mmr APIs work with leaf indices, leaving the burden of translating block numbers to leaf indices to the users of the pallet.

Light clients might have an easier experience dealing only with block numbers, and avoiding managing leaf indices and the conversion from block numbers => leaf indices.

Suggested solution

pallet-mmr could expose APIs based on block numbers and internally map those to leaf indices.

Implications

  • Breaking API change, both for Runtime API and RPC API.
  • Pallet currently only deployed on Rococo so breaking change only for that. Kusama and Polkadot will directly deploy new API.

Feedback requests

cc @vgeddes and @seunlanlege as current (light-client) users of this pallet

@acatangiu acatangiu added I7-refactor Code needs refactoring. U2-some_time_soon Issue is worth doing soon. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. D2-breaksapi labels Sep 23, 2022
@Szegoo
Copy link
Contributor

Szegoo commented Sep 24, 2022

I will give this a try.

@acatangiu
Copy link
Contributor Author

As shown in #12345 there is also the option to change only RPC API to use block numbers, while leaving existing Runtime API unchanged.

@seunlanlege
Copy link
Contributor

And while this change is harmless, it's actually unneccessary. As light clients need to be aware of the leaf_index which they can use to derive the leaf_postion which is needed when verifying mmr proofs with the ckb-merkle-mountain-range lib

@vgeddes
Copy link
Contributor

vgeddes commented Sep 27, 2022

I'm in favour of the change. It does simplify our offchain relayers that call mmr_generateProof. They have needed to manually calculate the leaf indices. To compute the correct leaf indices on Kusama and Polkadot, they will need to know the block height when MMR leafs started being generated, and this is extra configuration and deployment hassle.

Our system also requires the MMR leaf index for a block. But since the output of mmr_generateProof(blockNumber) includes the leaf index we're happy. And we'd rather rely on that instead of calculating our own leaf indices.

@acatangiu
Copy link
Contributor Author

Seeing how pallet-mmr has only been deployed on Rococo so far, I'm leaning heavily to just break the API without backwards compatibility if that's acceptable to you.

@vgeddes
Copy link
Contributor

vgeddes commented Sep 28, 2022

sounds fine with us 👍

@Szegoo
Copy link
Contributor

Szegoo commented Sep 28, 2022

So is the final decision to have this implemented? If so should #12345 also change the runtime API? Initially, I didn't change the runtime API in the PR, but I don't actually see a reason why we wouldn't change that also.

@acatangiu
Copy link
Contributor Author

Yes, decision is to implement full breaking change. Let's change Runtime API too in #12345

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
D2-breaksapi I7-refactor Code needs refactoring. U2-some_time_soon Issue is worth doing soon. Z2-medium Can be fixed by a coder with good Rust knowledge but little knowledge of the codebase. Z6-mentor An easy task where a mentor is available. Please indicate in the issue who the mentor could be.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants