substrate

substrate copied to clipboard

Description of the changes

A continuation for #11230

This PR modifies the mmr_generateProof() and mmr_generateBatchProof() MMR RPC methods by adding the best_known_block as an argument.

In addition to supporting the current use cases, the modified methods can be also used to generate a historical MMR proof (a MMR proof for when the best known block is older than the best block of the chain).

Why it's needed

1. BEEFY light client needs to import explicit commitment + leaf + leaf proof for blocks that are changing validator set. If the chain has progressed since such block, the state of validator-set-change-block may have already been discarded (unless you're using archive node, which we want to avoid) and the call of mmr_generateProof(use-state-at-validator-set-change-block) will obviously fail.
2. if block B is known to the light client, then we can't simply generate some MMR-related proof using MMR state at block B+10. So e.g. to prove some storage (read: deliver message), we need to generate proof using MMR state at block B. If state is discarded, we'll need to import B+10 first => extra bridge cost.

Note

Marking this as an RFC since I'm not sure if modifying the MMR runtime API and MMR RPC is acceptable. Also having best_known_block as an argument might be confusing since the RuntimeApi macros add the at argument automatically. However this seems the simplest solution to me. Adding a different method for this seems a bit redundant.

If the feedback is positive, I can implement unit tests for this version of the code. If not I'm also ok with taking the approach in #11230 and having a separate mmr_generateHistoricalProof method.

I pushed an updated, but I didn't get to test it yet. Leaving it as draft.

@acatangiu @svyatonik

Sorry, I have a question before moving on with the PR. I just realized that the interface that I started with for generate_historical_batch_proof() probably doesn't make much sense and I was thinking what would be the right one. Right now generate_historical_batch_proof() takes 2 arguments:

leaf_indices: Vec<LeafIndex>,
best_known_block: <T as frame_system::Config>::BlockNumber,

I think it would make sense to either provide block numbers and best known block:

block_numbers: Vec<<T as frame_system::Config>::BlockNumber>,
best_known_block: <T as frame_system::Config>::BlockNumber,

or leaf indices and leaves count:

leaf_indices: Vec<LeafIndex>,
leaves_count: LeafIndex,

At first sight to me it would seem more natural to provide block_numbers and best_known_block. I guess if I were to write a light client I would like to deal only with block numbers, which I probably need to manage anyway and avoid managing leaf indexes and the conversion from block number => leaf index. But since I'm not very familiar with light clients and since generate_batch_proof receives leaf indices I would like to double check this with you. What do you think ?

Adrian is the best person to answer here, but imo let's make it similar to other APIs perhaps - i.e. deal with leafs?

If I were to design the pallet's API now, I would go for using only block numbers and having the pallet internally map those to leaves, but since we're past that point I propose we be consistent with the existing API and stick to LeafIndexes.

I suggest using

leaf_indices: Vec<LeafIndex>,
leaves_count: LeafIndex,

in this PR for consistency, and we can explore switching all APIs to block numbers in a dedicated issue/PR (opened https://github.com/paritytech/substrate/issues/12339).

Since pallet-mmr is only deployed on Rococo as of yet, I believe we can afford to make such a disruptive change if we decide to.

Ok, thanks for the input ! Then I'll use leaves for the moment and I'll follow the discussion on #12339 to see if we can switch to block numbers in the future.

I addressed all the comments so far. This PR is ready for review. Sorry I rebased by mistake and then I had to force push.

@Lederstrumpf thank you for the improvements to the unit tests. I added your commits in this PR. And I created #12391 to continue the discussion on whether we want to have 2 separate methods generate_batch_proof() and generate_historical_batch_proof(). Could you PTAL when you have time ? Thanks !

@Lederstrumpf please use bot merge next time :) It'll take care of merging the companion automatically.

@Lederstrumpf please use bot merge next time :) It'll take care of merging the companion automatically.

oops - noted - thanks @ordian!

@Lederstrumpf please use bot merge next time :) It'll take care of merging the companion automatically.

Yea... Now we have to fast-track https://github.com/paritytech/polkadot/pull/6061 since it is blocking the CI of other Polkadot MRs :see_no_evil:

@ggwpez I'm sorry for the trouble that's caused :( won't let it happen again with a companion