[Collection] Compliance engine message queues

[durkmurder]

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

Context

This PR implements message queues for collection compliance engine(former proposal engine). Design and implementation is inspired by consensus compliance engine, basically same engine is used with needed modifications for collection clusters.

Previously we named it as proposal engine to follow naming of consensus engines I have renamed it to compliance engine

[zhangchiqing]

Looks good

[codecov-commenter]

Codecov Report

Merging #1270 (208c3f0) into master (386f157) will increase coverage by 0.22%.
The diff coverage is 74.54%.

@@            Coverage Diff             @@
##           master    #1270      +/-   ##
==========================================
+ Coverage   54.58%   54.81%   +0.22%     
==========================================
  Files         500      501       +1     
  Lines       31659    31721      +62     
==========================================
+ Hits        17281    17387     +106     
+ Misses      12023    11970      -53     
- Partials     2355     2364       +9     

Flag	Coverage Δ
unittests	54.81% <74.54%> (+0.22%)	⬆️

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

Impacted Files	Coverage Δ
engine/consensus/compliance/core.go	80.86% <ø> (ø)
engine/collection/compliance/engine.go	70.16% <70.16%> (ø)
engine/consensus/compliance/engine.go	72.41% <77.77%> (-0.21%)	⬇️
engine/collection/compliance/core.go	78.62% <78.62%> (ø)
consensus/hotstuff/event_loop.go	37.83% <100.00%> (+5.48%)	⬆️
...sus/approvals/assignment_collector_statemachine.go	45.19% <0.00%> (-6.74%)	⬇️
...ngine/common/synchronization/finalized_snapshot.go	68.75% <0.00%> (-4.17%)	⬇️
engine/collection/synchronization/engine.go	62.90% <0.00%> (ø)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 386f157...208c3f0. Read the comment docs.

[AlexHentschel]

LGTM.

What really bothers me is the amount of code duplication between the consensus and the collector compliance engine. If golang only had generics ... 😠.
I guess that is technical debt we can address once go 1.18 comes around.

P.S. took the liberty up slightly update some goDoc (commit 26bb30f). No functional changes

[AlexHentschel]

I feel like LifecycleManager and Unit supply largely redundant functionality here (?)
The only component from unit that we utilize here seems to be the WaitGroup:

flow-go/engine/unit.go

Line 13 in 1101316

wg sync.WaitGroup // tracks in-progress functions

Is there any reason to not replace unit by a WaitGroup. Would remove some of the clutter.

[durkmurder]

I really hope to see one day LifecycleManager fused with Unit so that's why I would leave it as it in and just remove unit completely then.

[AlexHentschel]

BTW, I think we can actually have a single implementation of the compliance.Engine and compliance.Core for both collectors and consensus nodes:

• Engine and Core are agnostic about the payload type
• The only thing that cares about the payload is the MutableState implementation, which we anyway already inject into Core

My thoughts are that we could:

• introduce an interface like

  type AbstractBlock interface {
    Header() *flow.Header
  }

  and a similar abstraction for votes
• introduce a config object that holds all the metric identifiers and such
• thereby, we would also only need one implementation of PendingBlockBuffer / PendingClusterBlockBuffer

While it seems doable on a high-level, there are probably a lot of details that would need to be aligned. Not sure if it is worth the effort at this time ...

[durkmurder]

BTW, I think we can actually have a single implementation of the compliance.Engine and compliance.Core for both collectors and consensus nodes:

• Engine and Core are agnostic about the payload type
• The only thing that cares about the payload is the MutableState implementation, which we anyway already inject into Core

My thoughts are that we could:

• introduce an interface like

  type AbstractBlock interface {
    Header() *flow.Header
  }

  and a similar abstraction for votes

• introduce a config object that holds all the metric identifiers and such

• thereby, we would also only need one implementation of PendingBlockBuffer / PendingClusterBlockBuffer

While it seems doable on a high-level, there are probably a lot of details that would need to be aligned. Not sure if it is worth the effort at this time ...

I was trying to do the same for sync engine and in the end it appears that it's a good idea for later refactoring especially with go 1.18 but for time being it's not a priority. So I would indeed leave it for later.

[durkmurder]

bors merge

[bors]

Build failed:

• Unit Tests (1.15)

[durkmurder]

bors merge

[bors]

Build succeeded:

• Integration Tests (1.15)
• Integration Tests (1.16)
• Lint
• Unit Tests (1.15)
• Unit Tests (1.16)