fix(consensus): fix the hashes-in-blocks feature implementation

[kpop-dfinity]

Problem

Currently, with the feature flag enabled, when a node sends a block proposal to their peers it first removes all the ingress messages from the block's payload, leaving behind only the IngressMessageIds which identify the ingress messages. When a node receives such a "stripped" block proposal it reconstructs the block by retrieving the referenced ingress messages from:

1. either their local ingress pools if there exists an ingress message with the given IngressMessageId there,
2. or by requesting the missing ingress messages from peers who are advertising the block.

The problem with the approach is that the IngressMessageIds do not uniquely identify the the ingress messages and two ingresses with the same content but with different signatures have the same id. We thus could end up in a situation where peers reconstruct blocks different than what the block maker created.

Note

The feature is disabled on the Mainnet so no subnet is currently affected by the issue

Proposal

To address this problem we introduce a new type SignedIngressId (which contains both IngressMessageId and the hash of a original ingress message bytes) which uniquely identifies a SignedIngress, which is used only within the the block proposal assembler module. Everything else remains basically the same but we add several checks to make sure we put the correct ingress message in the block:

1. when retrieving ingress messages from ingress pools by IngressMessageIds we double check that the SignedIngressId of the ingress message matches what we are expecting;
2. same for when we get an ingress message from a peer.

Alternatives considered

Instead of introducing a new type, we could modify the existing IngressMessageId so that it uniquely identifies ingress messages.

disadvantages over the proposed solution:

1. without any changes to the ingress pool we would end up with duplicates of the same ingress messages (but different signatures) in the pool & which would be further broadcasted to the other peers
2. the duplication detection in the ingress selector would have to be modified
3. computing IngressMessageId would be more expensive and we currently do it quite often
4. the change would affect multiple more components

advantages over the proposed solution:

1. in some corner-case situations in the proposed solution a node might have to fetch more ingress message from their peers

[derlerd-dfinity]

Interface changes LGTM.

[derlerd-dfinity]

One more thing that just crossed my mind: IIRC SignedIngress has more fields than the bytes we include in the hash. Can we somewhere document why it is safe to exclude these fields?

[kpop-dfinity]

One more thing that just crossed my mind: IIRC SignedIngress has more fields than the bytes we include in the hash. Can we somewhere document why it is safe to exclude these fields?

Sure, I'll add a comment to the type definition