[Consensus] `MessageHub` implementation

[durkmurder]

https://github.com/dapperlabs/flow-go/issues/6395

Context

This PR changes how incoming and outgoing consensus related messages are processed. Previously compliance engine was handling both incoming and outgoing messages + processing block proposals(validation, state extension). MessageHub takes responsibility of routing both incoming and outgoing messages, it doesn't do any processing. Incoming messages are routed in next way:

• vote -> VoteAggregator
• timeout -> TimeoutAggregator
• proposal -> ComplianceEngine

MessageHub implements hotstuff.Consumer interface which allows it to be subscriber of hotstuff events. By subscribing for published notifications it performs processing of outgoing consensus messages(vote, timeout, proposal).

Other notable changes:

• Removed proposal provider engine, moved that logic into consensus message hub.
• Updated structure of hotstuff.Consumer interface to have events for outgoing messages.
• EventHandler uses pub/sub to communicate with MessageHub

[codecov-commenter]

Codecov Report

Merging #3357 (135fd17) into feature/active-pacemaker (6ca919a) will decrease coverage by 1.12%. The diff coverage is 100.00%.

@@                     Coverage Diff                      @@
##           feature/active-pacemaker    #3357      +/-   ##
============================================================
- Coverage                     55.30%   54.17%   -1.13%     
============================================================
  Files                           748      600     -148     
  Lines                         69412    52731   -16681     
============================================================
- Hits                          38387    28568    -9819     
+ Misses                        27890    21839    -6051     
+ Partials                       3135     2324     -811     

Flag	Coverage Δ
unittests	54.17% <100.00%> (-1.13%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
consensus/config.go	0.00% <ø> (ø)
consensus/participant.go	18.84% <ø> (+0.13%)	:arrow_up:
consensus/hotstuff/eventhandler/event_handler.go	77.07% <100.00%> (+2.00%)	:arrow_up:
network/p2p/dns/resolver.go	91.51% <0.00%> (-4.85%)	:arrow_down:
...s/hotstuff/votecollector/staking_vote_processor.go	81.05% <0.00%> (-3.16%)	:arrow_down:
admin/command_runner.go	79.88% <0.00%> (-1.12%)	:arrow_down:
fvm/environment/contract_updater.go	66.54% <0.00%> (-0.72%)	:arrow_down:
engine/collection/compliance/core.go
engine/collection/compliance/engine.go
engine/collection/epochmgr/engine.go
... and 145 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[AlexHentschel]

High-level design thought:

• at the moment, there is some code in the compliance engines, which I feel is legacy from the time when the compliance engine was directly ingesting messages from the networking layer:
  • https://github.com/onflow/flow-go/blob/c20f2cf79c8b1707db9900c57b09ec1a40c132cf/engine/collection/compliance/engine.go#L186 this is exactly the `network.MessageProcessor interface
  • we have all this logic for packing network-level messages into the generic engine.Message type.

update: I realized now that there is also the synchronization engine, that might complicate things. Though, I am confused why it is possible for the consensus compliance core to inject the block sync engine (-> code) but not for the collectors compliance layer 🤔 Maybe we should leave this for later. Not sure my suggestion below is as simple as I originally thought. But still wanted to keep the comment here to invite thoughts.

Suggestion (potentially too complicated to include into PR):

I think through its generality, this code somewhat obfuscates the central logic. I'd prefer if we used strongly typed interfaces as for many other data flow components. Suggestions (using collector compliance engine as an example):

• in the collector compliance engine, introduce type-specific methods one for receiving OnClusterBlockProposal and another one for OnSyncedClusterBlock
• I think that OnClusterBlockProposal and OnSyncedClusterBlock do not need to receive the channel as parameter
• remove the generic message hander and move the respective processing logic into OnClusterBlockProposal and OnSyncedClusterBlock (note: you need to call the notifier than manually of course).
• add an interface specific to the compliance engine exposing methods OnClusterBlockProposal and OnSyncedClusterBlock
• I think it would improve clarity, if we used a type-specific structure in the compliance engine:

  type inboundBlock struct {
  	originID flow.Identifier
  	block    *messages.ClusterBlockProposal
  }
  

• in the MessageHub, change https://github.com/onflow/flow-go/blob/c20f2cf79c8b1707db9900c57b09ec1a40c132cf/engine/collection/message_hub/message_hub.go#L90 to the compliance-engine specific interface
• use the type-specific methods here in the message hub: https://github.com/onflow/flow-go/blob/c20f2cf79c8b1707db9900c57b09ec1a40c132cf/engine/collection/message_hub/message_hub.go#L432-L435

👉 same for consensus compliancy layer

[durkmurder]

bors merge

[durkmurder]

bors cancel

[bors[bot]]

Canceled.

[durkmurder]

High-level design thought:

• at the moment, there is some code in the compliance engines, which I feel is legacy from the time when the compliance engine was directly ingesting messages from the networking layer:

  • https://github.com/onflow/flow-go/blob/c20f2cf79c8b1707db9900c57b09ec1a40c132cf/engine/collection/compliance/engine.go#L186 this is exactly the `network.MessageProcessor interface
  • we have all this logic for packing network-level messages into the generic engine.Message type.

update: I realized now that there is also the synchronization engine, that might complicate things. Though, I am confused why it is possible for the consensus compliance core to inject the block sync engine (-> code) but not for the collectors compliance layer thinking Maybe we should leave this for later. Not sure my suggestion below is as simple as I originally thought. But still wanted to keep the comment here to invite thoughts.

Suggestion (potentially too complicated to include into PR):

I think through its generality, this code somewhat obfuscates the central logic. I'd prefer if we used strongly typed interfaces as for many other data flow components. Suggestions (using collector compliance engine as an example):

• in the collector compliance engine, introduce type-specific methods one for receiving OnClusterBlockProposal and another one for OnSyncedClusterBlock
• I think that OnClusterBlockProposal and OnSyncedClusterBlock do not need to receive the channel as parameter
• remove the generic message hander and move the respective processing logic into OnClusterBlockProposal and OnSyncedClusterBlock (note: you need to call the notifier than manually of course).
• add an interface specific to the compliance engine exposing methods OnClusterBlockProposal and OnSyncedClusterBlock
• I think it would improve clarity, if we used a type-specific structure in the compliance engine:

  type inboundBlock struct {
  	originID flow.Identifier
  	block    *messages.ClusterBlockProposal
  }
  

• in the MessageHub, change https://github.com/onflow/flow-go/blob/c20f2cf79c8b1707db9900c57b09ec1a40c132cf/engine/collection/message_hub/message_hub.go#L90 to the compliance-engine specific interface
• use the type-specific methods here in the message hub: https://github.com/onflow/flow-go/blob/c20f2cf79c8b1707db9900c57b09ec1a40c132cf/engine/collection/message_hub/message_hub.go#L432-L435

point_right same for consensus compliancy layer

@AlexHentschel Created an issue for tracking progress on extracting interface for compliance engine https://github.com/dapperlabs/flow-go/issues/6424

[durkmurder]

bors merge

[bors[bot]]

Build succeeded:

• Integration Tests (make -C integration access-tests)
• Integration Tests (make -C integration bft-tests)
• Integration Tests (make -C integration collection-tests)
• Integration Tests (make -C integration consensus-tests)
• Integration Tests (make -C integration epochs-tests)
• Integration Tests (make -C integration execution-tests)
• Integration Tests (make -C integration ghost-tests)
• Integration Tests (make -C integration mvp-tests)
• Integration Tests (make -C integration network-tests)
• Integration Tests (make -C integration verification-tests)
• Lint (./)
• Lint (./crypto/)
• Lint (./integration/)
• Unit Tests (access)
• Unit Tests (admin)
• Unit Tests (cmd)
• Unit Tests (consensus)
• Unit Tests (engine)
• Unit Tests (fvm)
• Unit Tests (ledger)
• Unit Tests (module)
• Unit Tests (network)
• Unit Tests (others)
• Unit Tests (utils)