pallet-mmr: improve offchain storage, relax LeafData requirements

[acatangiu]

Issue

PR #11594 introduced fork-resistant offchain storage for MMR leaves.

Leaves are now saved offchain using (parent_hash, pos)-based key to be fork-resistant and later canonicalized to pos-based key.

Relying on (parent_hash, pos)-based keys means that conflicts could now only happen on 1-block deep forks, where two forks with identical line of ancestors compete to write at the same offchain key. This introduced a requirement on the LeafData to only reference data coming from ancestor blocks, so in the corner case above, the conflicting writes end up writing same values anyway.

While the current behavior works for polkadot/kusama BEEFY+MMR deployment, I think we can improve and eliminate the LeafData requirement shown above.

Proposed solution

Leaves are added to the MMR offchain storage by runtime during pallet_mmr::<Pallet as Hooks>::on_initialize(BlockNumber) here using indexing API. At that time block_hash is unavailable (block is still under construction) so we're using parent_hash-based key instead to uniquely identify this leaf in offchain db (only it's not unique for 1-block deep forks, ergo the LeafData limitation).

Idea is to drop using the indexing API and instead store the new leaf in a placeholder Pallet::NewLeaf runtime storage item, and have the offchain worker function move/copy the leaf to offchain db after the block is built and truly unique (block_hash, pos) is available to be used as offchain key.

The same Pallet::NewLeaf storage is reused for temporarily holding the new leaf at every block.

[seunlanlege]

Will pick this up on monday

[acatangiu]

Doing this will normally require an offchain database migration for any runtime deploying #11594 (Rococo runtime >= 9260).

Right now though, pallet-mmr is only deployed on Rococo which is still running runtime version 9250; if we get this change in soon, then we might get away upgrading directly from 9250 without needing database migration.

[iTranscend]

Hi, I've gone ahead to make the changes suggested in the Issue spec.

I haven't updated the tests yet, but I'll do that over the next few days.

I've opened a draft PR here.

[acatangiu]

Hi, I've gone ahead to make the changes suggested in the Issue spec.

I haven't updated the tests yet, but I'll do that over the next few days.

I've opened a draft PR here.

Do you need any help here?

[iTranscend]

Hi @acatangiu.
I've started working on the changes you suggested but I don't think I'll be able to finish them up as quick as they are needed.
I'd love it if someone else can pick up from where I stopped so we can get away with upgrading directly from 9250 without needing a database migration.

[acatangiu]

Unfortunately offchain_worker is not reliable enough to allow for this.. see #12446 (comment)

Closing the issue as "Not possible".