Front-Load HotStuff Verification Pt. 1: Update Compliance Engines

[jordanschalm]

This PR modifies compliance engines to front-load HotStuff verification:

• Once a block connects to the finalized state, we validate the proposal using HotStuff first, and only then attempt to extend the state (compliance checks)
• This means, all blocks in storage have been fully validated (there is no need for a separate MarkValid step)

⚠️ Depends on https://github.com/onflow/flow-go/pull/3248 👉 Removing MarkValid will be done in a separate PR: https://github.com/onflow/flow-go/pull/3303

[codecov-commenter]

Codecov Report

Merging #3294 (fb3f66e) into feature/active-pacemaker (95f7277) will increase coverage by 0.34%. The diff coverage is 0.00%.

@@                     Coverage Diff                      @@
##           feature/active-pacemaker    #3294      +/-   ##
============================================================
+ Coverage                     53.83%   54.17%   +0.34%     
============================================================
  Files                           553      600      +47     
  Lines                         50225    52744    +2519     
============================================================
+ Hits                          27037    28576    +1539     
- Misses                        20922    21843     +921     
- Partials                       2266     2325      +59     

Flag	Coverage Δ
unittests	54.17% <0.00%> (+0.34%)	:arrow_up:

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

Impacted Files	Coverage Δ
cmd/execution_builder.go	0.00% <0.00%> (ø)
cmd/verification_builder.go	0.00% <0.00%> (ø)
state/cluster/badger/mutator.go	73.36% <ø> (ø)
fvm/environment/contract_updater.go	66.54% <0.00%> (-0.72%)	:arrow_down:
admin/command_runner.go	79.88% <0.00%> (ø)
network/channels/topic.go	0.00% <0.00%> (ø)
network/test/testUtil.go	94.83% <0.00%> (ø)
network/topology/empty.go	100.00% <0.00%> (ø)
network/validator/target_validator.go	0.00% <0.00%> (ø)
network/p2p/connection/peerManager.go	75.40% <0.00%> (ø)
... and 43 more

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

[jordanschalm]

bors merge

[bors[bot]]

Build failed:

• Unit Tests (consensus)

[jordanschalm]

bors retry

[bors[bot]]

Build failed:

• Unit Tests (consensus)

[AlexHentschel]

Btw, also created issue https://github.com/onflow/flow-go/issues/3374 where I noted suggestions for potential future revisions. Feel free to address points, if you find that they fit nicely within the scope of the PR (we can also always follow up with subsequent PRs)

[AlexHentschel]

Generally, I find the terminology "HotStuff validation" and "compliance checks" ambiguous. https://github.com/onflow/flow-go/blob/b4a7dbb4a6b7cf09a481f7eaa49c5bd8057f2427/engine/collection/compliance/core_test.go#L324-L325

This is probably a legacy from our old design, where the compliance engine would check the payload and hotstuff would check the header. However, now the compliance engine checks everything, which makes the terminology confusing in my opinion.

suggestion

Use terminology:

• "header validation"
• and "payload validation"

I would appreciate if we could update the naming and documentation of the tests:

• rename TestOnBlockProposal_InvalidProposal() to TestOnBlockProposal_InvalidHeader() and update the godoc
  • "fails HotStuff validation" -> "fails Header validation"
  • "compliance checks" -> "payload checks"
• analogous for TestOnBlockProposal_InvalidExtension()

[AlexHentschel]

Thank you. Very nice work.

I have created PR #3408 with some stylistic revisions targeting your branch.

[jordanschalm]

suggestion Use terminology: "header validation" and "payload validation"

This suggestion is not much more accurate, IMO. It's true that "HotStuff validation" is limited to the header, but it does not validate all parts of the header, and state.Extend validates parts of the header (height, parent ID, chain ID) in addition to the payload.

The separation is reflective of different layers of the logic, than parts of the data structure, to me. HotStuff validation, as in the portion of validation relevant to the HotStuff layer, seems right to me. We used to call the layer above HotStuff, which mainly enforces validity of the payloads, the "compliance layer".

For now I went with "protocol state validation" and "hotstuff validation": https://github.com/onflow/flow-go/pull/3294/commits/6c6b630ea6662ec10a74c4c10e05c821eb1a2f53

[jordanschalm]

I think the failing consensus tests here would be fixed by the addition of a block message queue to VoteAggregator in https://github.com/onflow/flow-go/pull/3277

[durkmurder]

I think the failing consensus tests here would be fixed by the addition of a block message queue to VoteAggregator in #3277

Yup, both https://github.com/onflow/flow-go/pull/3277 and https://github.com/onflow/flow-go/pull/3357 fix flaky behavior in different tests.

[AlexHentschel]

Thanks @jordanschalm for explaining the motivation behind the naming convention/ terminology. Makes sense.

Very nice PR altogether!

[jordanschalm]

bors merge

[bors[bot]]

Build failed:

• Unit Tests (engine)