`height.increment()` not needed for Beefy

[livelybug]

Hello,

When implementing Beefy protocol, based on Grandpa consensus, there are 3 kinds of verifications in a hierarchical structure.

• The top tier verification is MMR root verification, by collecting sufficient validators' valid signatures.
• Based on a valid MMR root, the 2nd tier is block header verification.
• Based on a valid block header, the 3rd tier is on-chain storage verification, which calculates the correct on-chain storage by the state root, the storage proof, and the storage key. A successful verification needs the state root and storage proof to be from the same block. However, as the height is increased by one when composing proofs of connection, channel, and packet, the corresponding state root always belong to the header of block height N + 1, while the storage proof belongs to block height N, resulting in a failed verification

Understand the height.increment() may be for Tendermint consensus, is it possible to compose a trait for it, and allow consensus-specific implementation?

Thank you

[lyqingye]

I guess it's because the ConsensusState contains the app hash (merkle root) of the previous block, so PacketProof Height + 1 = ConsensusState Height

[adizere]

Hey @livelybug and @lyqingye, many thanks for documenting this!

We can add consensus-specific implementations for the relayer's logic when it is composing proofs of connection, channel, and packet.

Refining the problem

These three methods of trait ChainEndpoint have too many responsibilities:

1. build_connection_proofs_and_client_state
2. build_channel_proofs
3. build_packet_proofs

They each have two responsibilities: (A) preparing the different sub-elements of a Proof -- such as object, client, or consensus proofs -- from (B) assembling a coherent Proof from these sub-elements (including the correct height within the proof) by calling into Proof::new.

Part (A) is generic for all IBC-enabled chains as far as I can tell. Part (B) is chain (i.e., consensus-protocol) specific. So the fix would be to decouple (A) from (B).

Acceptance criteria

• [ ] The three methods of ChainEndpoint mentioned above should only implement part (A), they should call in to Proofs::new and handle height.increment() (this is (B)) since that is not their responsibility.
• [ ] For the final construction of a Proof (point (B)), these methods should delegate to a new method assemble_proof defined on the ChainEndpoint trait.
  • The CosmosSdk implementation of ChainEndpoint: assemble_proof method will simply call into Proof::new while incrementing the height.
  • Other chain implementations will assemble proofs without increasing the height.

@livelybug let me know if you agree with the scope & acceptance criteria.

[livelybug]

Thank you, Adi! I am fine with the scope & acceptance criteria.