[MMR] fix MmrLeaf::next_auth_set for session-boundary blocks

[acatangiu]

paritytech/polkadot#6577 implemented paritytech/substrate#11797 in an attempt to decrease light-client complexity, but inadvertently opened up a corner-case that can end up bricking on-chain light clients (such as bridges):

The offending change is:

// contents for leaf index <N-1> added by block <N>
MmrLeaf {
    version: <leaf-data-format-version>,
    (
        parent_num: <N-1>,
        parent_hash: <hash_of_<N-1>>,
    ),
    extra_data: <para_heads_of_<N-1>>,
-   next_auth_set: <next_auth_set_of<N>>,
+   next_auth_set: <next_auth_set_of<N-1>>,
}

Which is a problem when combined with the following property of the BEEFY protocol:

• BEEFY will finalize at least one block in each session, called "mandatory commitment", this block has been chosen to be the first block of the session (the one that also signals the auth set change).

When combining the two, we see that mandatory block N contains MMR root for MMR with latest leaf providing
next_auth_set: <next_auth_set_of<N-1>>, representing the currently active validator set and not the next validator set as required (in order to know who are the authorities in the next session).
In other, simplified, words: Mandatory BEEFY commitment ends up providing proof for current validator set instead of next one - which is useless in practice.

We need to change MmrLeaf contents (back) to:

// contents for leaf index <N-1> added by block <N>
MmrLeaf {
    version: <leaf-data-format-version>,
    (
        parent_num: <N-1>,
        parent_hash: <hash_of_<N-1>>,
    ),
    extra_data: <para_heads_of_<N-1>>,
-   next_auth_set: <next_auth_set_of<N-1>>,
+   next_auth_set: <next_auth_set_of<N>>,
}

We can implement above fix by reverting paritytech/polkadot#6577 for both Kusama and Polkadot.

[acatangiu]

ping @vgeddes and @seunlanlege as the current known users of MMR

[seunlanlege]

I've written my light client to be independent of all of this

[Lederstrumpf]

Context for this issue is bridge deployment stalling this week: paritytech/polkadot-sdk#3080

We might have to investigate this issue and change approach (again ^^), so here are the three possibilities I see for addressing this (more may exist):

1. revert construct mmr leaf prior to session pallet hook paritytech/polkadot#6577, as mentioned by @acatangiu above
2. implement pallet-mmr: improve offchain storage, relax LeafData requirements paritytech/substrate#12446 (comment), but now in the client gadget.
3. change the mandatory block voted on to be the second block of the session. It could also be any other block in the session.

Assessment:

1. This has the advantage that it reverts to an operation mode we've used in the past, hence is the simplest to implement. Has the disadvantage of mixing state of <N> & <N-1>.
2. This would be the cleanest solution for consumers of BEEFY commitments (leaf of block<N> only references information about block<N>), but has the overhead mentioned in MMR Leaf Data: off-by-one error in beefy_next_authority_set paritytech/substrate#11797 (comment), hence not worth it.
3. This would retain the cohesive <N-1> structure, so would be simpler for consumer of BEEFY commitments than 1.. Has the disadvantage that commitments to the previously mandatory blocks (non-mandatory assuming 3. is implemented) would be rejected by Snowfork's current implementation unless the check here is loosened to also allow leaf.nextAuthoritySetID == nextValidatorSet.id (trivial to implement, and we'll likely loosen checks on authority id to be greater than or equal anyway - both for current & next authority set): https://github.com/snowfork/snowbridge/blob/13db09317fad428af4e2bb8faf590cb3f17ad97c/contracts/src/BeefyClient.sol#L362-L364.

[vgeddes]

ping vgeddes and seunlanlege as the current known users of MMR

+1 from Snowfork - We've gone over our protocol & code and the fix here will solve the problem for us.

Luckily, we don't need to make any code changes on our side to support the fix either.

[acatangiu]

fixed in #169