Pallet-MMR should use custom `block_hash` getter instead of using `frame_system::block_hash` for non-canonicalised blocks

[vedhavyas]

Is there an existing issue?

• I have searched the existing issues

Experiencing problems? Have you tried our Stack Exchange first?

• This is not a support question.

Motivation

Currently pallet_mmr uses frame_system::block_hash to get the hash of a given block number while constructing the non-canonicalised keys. This works fine with Polkadot and other blockchains where finality is quicker. Currently, polkadot stores 4096 number of block hashes on the runtime and before the block_hashes are pruned, the blocks are already finalized.

At subspace, we do not have a quick finality and we cannot put a static number at which we can gaurantee block finalization happens. With this context, when an MMR proof is being verified, if the particular block is not canonicalised yet, it will look at frame_system::block_hashes but by then our runtime already prunes the block hash due to frame_system::Config::BlockHashCount and the storage returns an Default value for the Hash since the frame_system::block_hash is a ValueQuery. With the default value given, the MMR proof verification fails on our end.

Request

pallet_mmr should ideally define a custom getter to fetch the block_hash instead of directly using the frame_system. This will provide the projects using the pallet_mmr can impl the custom getter to fetch the said block_hash.

For polkadot and likes, the impl will just wrap frame_system::block_hashes function and should not change anything while Subspace and other project can impl custom logic to fetch the correct block_hash for a given non-canonicalised block number.

Solution

Solution would be introduce a new type for pallet_mmr::Config that can fetch block_hash for a given block_number

Are you willing to help with this request?

Yes!

[acatangiu]

IIUC this

polkadot-sdk/substrate/frame/merkle-mountain-range/src/mmr/storage.rs

Line 90 in 817870e

let ancestor_parent_hash = <frame_system::Pallet<T>>::block_hash(ancestor_parent_block_num);

is what is causing problems for you.

So instead of <frame_system::Pallet<T>>::block_hash (mapping block-number -> block-hash on current fork), I am guessing you will use some other on-chain map block_number -> some-unique-identifier that runs deeper than 4096, right?

Solution would be introduce a new type for pallet_mmr::Config that can fetch block_hash for a given block_number

Sounds ok to me. I would not call it block_hash anymore, maybe block_uid or something to avoid confusions since it can be something else than the actual "block hash".

[vedhavyas]

So instead of <frame_system::Pallet>::block_hash (mapping block-number -> block-hash on current fork), I am guessing you will use some other on-chain map block_number -> some-unique-identifier that runs deeper than 4096, right?

Yes, this is causing the issue.
But instead of block_number -> some-unique-identifier we would still be using block-number -> block-hash but instead of using frame_system::block_hash we would like to provide a custom trait to config that fetches this map so that we can plug in different way of fetching block_hash instead of directly from frame_system. For polkadot or other chains, this trait will still use frame_system underneath.

I hope this makes sense

[acatangiu]

Ok, sounds good to me. I can help review the PR and push it through once you post it.

[vedhavyas]

@acatangiu PR for the above proposed solution path is here - #4080