Consensus Compliance Engine updated to conform to `Component` interface #3121

jordanschalm · 2022-08-29T20:27:21Z

This PR updates consensus/compliance engine to implement component.Component.

The epochmgr and collection/compliance engine will be updated in a subsequent PR.

Other Changes

Replaces type-unsafe communication between compliance, sync, and provider engines with specific interfaces
Changes provider engine from a deprecated Engine to a MessageProcessor
Refactors Stopper slightly to work with context cancellation

* no longer conforms to deprecated Engine interface

…omponent-pattern

engine/consensus/compliance/engine.go

engine/consensus/provider/engine.go

…:onflow/flow-go into jordan/6263-compliance-component-pattern

codecov-commenter · 2022-09-01T17:23:40Z

Codecov Report

Merging #3121 (97d3714) into feature/active-pacemaker (1da5cdc) will decrease coverage by 1.62%.
The diff coverage is 61.07%.

@@                     Coverage Diff                      @@
##           feature/active-pacemaker    #3121      +/-   ##
============================================================
- Coverage                     54.84%   53.21%   -1.63%     
============================================================
  Files                           487      694     +207     
  Lines                         39550    65832   +26282     
============================================================
+ Hits                          21692    35035   +13343     
- Misses                        16068    27870   +11802     
- Partials                       1790     2927    +1137

Flag	Coverage Δ
unittests	`53.21% <61.07%> (-1.63%)`	⬇️

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

Impacted Files	Coverage Δ
engine/enqueue.go	`71.42% <ø> (ø)`
utils/unittest/irrecoverable.go	`0.00% <0.00%> (ø)`
utils/unittest/unittest.go	`7.60% <0.00%> (ø)`
engine/consensus/compliance/core.go	`74.60% <41.66%> (ø)`
engine/consensus/provider/engine.go	`34.32% <42.10%> (ø)`
engine/common/synchronization/engine.go	`72.76% <62.50%> (ø)`
engine/collection/compliance/engine.go	`63.56% <66.66%> (ø)`
engine/consensus/compliance/engine.go	`61.41% <77.77%> (ø)`
engine/collection/compliance/core.go	`80.73% <100.00%> (ø)`
engine/common/follower/engine.go	`52.47% <100.00%> (ø)`
... and 329 more

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

consensus/integration/integration_test.go

consensus/integration/nodes_test.go

consensus/integration/stopper_test.go

engine/consensus/compliance/engine.go

durkmurder · 2022-09-05T10:19:00Z

engine/consensus/compliance/engine_test.go

-	err = cs.engine.BroadcastProposalWithDelay(header, 0)
-	require.Error(cs.T(), err, "should fail with missing payload")
-	header.View--
+	<-util.AllClosed(broadcasted, submitted)


Suggested change

<-util.AllClosed(broadcasted, submitted)

unittest.AssertClosesBefore(cs.T(), util.AllClosed(broadcasted, submitted), time.Second)

durkmurder · 2022-09-05T10:19:23Z

engine/consensus/compliance/engine_test.go

-	require.Error(cs.T(), err, "should fail with missing payload")
-	header.View--
+	<-util.AllClosed(broadcasted, submitted)
+	cs.cancel()


Same question here, I don't think we need to stop manually

durkmurder · 2022-09-05T10:19:37Z

engine/consensus/compliance/engine_test.go

+		return cs.engine.pendingVotes.(*engine.FifoMessageStore).Len() == 0
+	}, time.Second, time.Millisecond*10)
+	// stop the engine
+	cs.cancel()


Do we need to stop manually?

durkmurder · 2022-09-05T10:22:36Z

consensus/integration/integration_test.go


 	allViews := allFinalizedViews(t, nodes)
+	for i, views := range allViews {


is this for debugging ?

Yes - will remove this...

* catch irrecoverable errors in separate goroutine * don't cancel engine context with individual cases

AlexHentschel · 2022-09-15T21:36:01Z

utils/unittest/unittest.go

@@ -149,6 +149,13 @@ func AssertReturnsBefore(t *testing.T, f func(), duration time.Duration, msgAndA
 	}
 }

+// ClosedChannel returns a closed channel.
+func ClosedChannel() <-chan struct{} {


AlexHentschel · 2022-09-15T21:39:02Z

engine/enqueue.go

 // MessageStore is the interface to abstract how messages are buffered in memory before
-// being handled by the engine
+// being handled by the engine.


Suggested change

// MessageStore is the interface to abstract how messages are buffered in memory before

// being handled by the engine

// being handled by the engine.

// MessageStore is the interface to abstract how messages are buffered in memory

// while waiting to be processed.

engine/enqueue.go

AlexHentschel · 2022-09-15T21:50:15Z

engine/consensus/compliance/engine.go

+	mempool  module.MempoolMetrics
+	metrics  module.EngineMetrics


I personally find the variable naming here a bit ambiguous, because mempool is not an actual mempool but the metrics collector for mempools. I think it would improve clarity, if this was reflected in the variable name.

Suggested change

mempool module.MempoolMetrics

metrics module.EngineMetrics

mempoolMetrics module.MempoolMetrics

componentMetrics module.EngineMetrics

would suggest to also rename the corresponding variables in compliance.Core respectively

flow-go/engine/consensus/compliance/core.go

Line 37 in f7b85fb

metrics module.EngineMetrics

flow-go/engine/consensus/compliance/core.go

Line 37 in f7b85fb

metrics module.EngineMetrics

engine/consensus/compliance/engine.go

AlexHentschel · 2022-09-16T00:22:24Z

consensus/integration/stopper_test.go

@@ -47,11 +51,17 @@ func (s *Stopper) AddNode(n *Node) *StopperConsumer {
 	s.Lock()


could we add a brief goDoc to the AddNode method please? In particular, a comment regarding the intended usage of the return value would be great. To be honest, I don't understand why we return this consumer here and I also don't understand why it is called a StopperConsumer. Is this legacy code? I feel we could just remove it ... but not sure.

Good point - as far as I can tell the StopperConsumer does nothing at all. Going to remove it.

AlexHentschel · 2022-09-16T00:23:59Z

consensus/integration/stopper_test.go

 	stopConsumer := &StopperConsumer{}
 	return stopConsumer
 }

+// WithStopFunc adds a function to use to stop all nodes (typically the cancel function of the context used to start them).
+func (s *Stopper) WithStopFunc(stop func()) {


This function is not concurrency safe, which should be highlighted in the goDoc.

AlexHentschel · 2022-09-16T00:32:39Z

engine/consensus/compliance/engine.go

@@ -454,15 +434,15 @@ func (e *Engine) BroadcastTimeout(timeout *model.TimeoutObject) error {
 			return
 		}
 		if err != nil {
-			log.Error().Err(err).Msg("could not broadcast timeout")
+			log.Err(err).Msg("could not broadcast timeout")
 			return
 		}
 		log.Info().Msg("consensus timeout broadcast")


Not quite sure, but my understanding is that the commonly used past tense of "broadcast" is also "broadcast". I think it would avoid ambiguity, if we clarify that the broadcast has completed:

Suggested change

log.Info().Msg("consensus timeout broadcast")

log.Info().Msg("consensus timeout was broadcast")

similarly here, I would suggest to use a consistent working

flow-go/engine/consensus/compliance/engine.go

Line 536 in f7b85fb

log.Info().Msg("block proposal broadcasted")

AlexHentschel · 2022-09-16T07:01:54Z

engine/consensus/compliance/engine_test.go

-	// submit to broadcast proposal
-	err := cs.engine.BroadcastProposalWithDelay(block.Header, 0)
-	require.NoError(cs.T(), err, "header broadcast should pass")
+	// unset chain and height to make sure they are correctly reconstructed


Could we put lines 141 - 172 into their own cs.Run method to increase the structure of the test?

AlexHentschel · 2022-09-16T07:07:10Z

engine/consensus/compliance/engine_test.go

 // TestOnFinalizedBlock tests if finalized block gets processed when send through `Engine`.
 // Tests the whole processing pipeline.
-func (cs *ComplianceSuite) TestOnFinalizedBlock() {
+func (cs *EngineSuite) TestOnFinalizedBlock() {


For my learning: is the old implementation (below) actually buggy or just unnecessarily verbose?

flow-go/engine/consensus/compliance/engine_test.go

Lines 227 to 230 in f7c9481

require.Eventually(cs.T(),

func() bool {

return cs.pending.AssertCalled(cs.T(), "PruneByView", finalizedBlock.View)

}, time.Second, time.Millisecond*20)

AlexHentschel · 2022-09-16T07:15:29Z

engine/consensus/provider/engine.go

+	if e.me.NodeID() != proposal.Header.ProposerID {
+		return fmt.Errorf("sanity check failed: attempted to provide another node's proposal (%x!=%x)", e.me.NodeID(), proposal.Header.ProposerID)


this sanity check is also in the compliance Engine:

flow-go/engine/consensus/compliance/engine.go

Lines 454 to 457 in f7b85fb

// first, check that we are the proposer of the block

if header.ProposerID != e.me.NodeID() {

return fmt.Errorf("cannot broadcast proposal with non-local proposer (%x)", header.ProposerID)

}

do we need it in both places?
... the check is probably cheap enough that it makes no practical difference, so why not 🤷

Yeah I'm more inclined to just leave it

AlexHentschel

Thank you for the nice refactoring, the great documentation and the test revisions.

In response to Yurii's comment, I have created PR #3243 (targeting this branch) that adds the missing error documentation.

durkmurder

LGTM, great effort.

* • documented expected error returns in compliance.Core and compliance.Engine • compactified code by removing unnecessary line breaks * Update engine/consensus/compliance/core.go Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>

…omponent-pattern

jordanschalm · 2022-09-20T14:11:50Z

bors merge

bors · 2022-09-20T14:41:11Z

Build succeeded:

3248: Collection`compliance`, `epochmgr` Engines Implement `component.Component` r=jordanschalm a=jordanschalm ⚠️ Depends on #3121 This PR updates `collection/compliance.Engine` and `collection/epochmgr.Engine` to implement `component.Component` (#3121, but for collection nodes). Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

jordanschalm added 10 commits August 29, 2022 13:20

feat: compliance implements Component

1556e7b

remove engine.Launch usage

a32a2d3

cache ready/done, remove unused channels

2f60f5f

add todos for tests, to remove deprecated engine

175bd6b

refactor: update provider engine

7e1f099

* no longer conforms to deprecated Engine interface

doc: short godoc for MessageStore

52d956f

update mocks

1620125

update compliance engine tests

d58d77e

update sync engine tests

990d40a

Merge branch 'feature/active-pacemaker' into jordan/6263-compliance-c…

587a37f

…omponent-pattern

jordanschalm commented Aug 30, 2022

View reviewed changes

engine/consensus/compliance/engine.go Outdated Show resolved Hide resolved

engine/consensus/provider/engine.go Outdated Show resolved Hide resolved

Apply suggestions from code review

f13e742

jordanschalm changed the title ~~Compliance Engines updated to conform to Component interface~~ Consensus Compliance Engine updated to conform to Component interface Aug 30, 2022

jordanschalm added 5 commits September 1, 2022 11:13

follower implements ComplianceProcessor

6dfed14

Merge branch 'jordan/6263-compliance-component-pattern' of github.com…

f5ee53d

…:onflow/flow-go into jordan/6263-compliance-component-pattern

todo for follower engine

c1b92ef

fix: re-order lifecycle of hotstuff integration tests

eeb9e23

fix: attach consensus in compliance test

288c7d3

jordanschalm added 3 commits September 1, 2022 15:17

docs: stopper condition

8cd997c

docs: add note about ready/done

1ae804a

doc: consensus integration node creation

633df57

jordanschalm commented Sep 1, 2022

View reviewed changes

jordanschalm added 2 commits September 1, 2022 15:36

Apply suggestions from code review

5fe0adb

lint

f7925a4

jordanschalm marked this pull request as ready for review September 1, 2022 19:42

jordanschalm requested review from AlexHentschel and zhangchiqing as code owners September 1, 2022 19:42

jordanschalm removed request for zhangchiqing and AlexHentschel September 1, 2022 19:42

durkmurder reviewed Sep 5, 2022

View reviewed changes

jordanschalm added 3 commits September 5, 2022 20:00

remove debug log

d6583c3

fix: address review feedback

355dfb2

* catch irrecoverable errors in separate goroutine * don't cancel engine context with individual cases

fix: use MessageProcessor for sync->comp messages

f7b85fb

AlexHentschel reviewed Sep 16, 2022

View reviewed changes

AlexHentschel mentioned this pull request Sep 16, 2022

suggested amendments to PR #3121 #3243

Merged

AlexHentschel reviewed Sep 16, 2022

View reviewed changes

AlexHentschel approved these changes Sep 16, 2022

View reviewed changes

durkmurder approved these changes Sep 16, 2022

View reviewed changes

Alexander Hentschel and others added 10 commits September 16, 2022 15:24

suggested amendments to PR #3121 (#3243)

c6bb8a8

* • documented expected error returns in compliance.Core and compliance.Engine • compactified code by removing unnecessary line breaks * Update engine/consensus/compliance/core.go Co-authored-by: Jordan Schalm <jordan@dapperlabs.com>

Apply suggestions from code review

e82ca42

Co-authored-by: Alexander Hentschel <alex.hentschel@axiomzen.co>

Merge branch 'feature/active-pacemaker' into jordan/6263-compliance-c…

28c2493

…omponent-pattern

style: wording

fb49eff

style: metric field naming

ed05ccc

perf: avoid func call in loop

8dc0b66

fix: rm unused code, update docs

96d8a53

style: log broadcast tense

e44f34a

style: subtest structure

6a1eb81

update mocks

98d8686

jordanschalm mentioned this pull request Sep 19, 2022

Collectioncompliance, epochmgr Engines Implement component.Component #3248

Merged

Merge branch 'feature/active-pacemaker' into jordan/6263-compliance-c…

97d3714

…omponent-pattern

jordanschalm added the S-Consensus label Sep 20, 2022

bors bot merged commit b4c2fcc into feature/active-pacemaker Sep 20, 2022

bors bot deleted the jordan/6263-compliance-component-pattern branch September 20, 2022 14:41

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consensus Compliance Engine updated to conform to `Component` interface #3121

Consensus Compliance Engine updated to conform to `Component` interface #3121

jordanschalm commented Aug 29, 2022 •

edited

Loading

codecov-commenter commented Sep 1, 2022 •

edited

Loading

durkmurder Sep 5, 2022

durkmurder Sep 5, 2022

durkmurder Sep 5, 2022

durkmurder Sep 5, 2022

jordanschalm Sep 5, 2022

AlexHentschel Sep 15, 2022

AlexHentschel Sep 15, 2022

AlexHentschel Sep 15, 2022

AlexHentschel Sep 16, 2022

jordanschalm Sep 16, 2022

AlexHentschel Sep 16, 2022

AlexHentschel Sep 16, 2022

AlexHentschel Sep 16, 2022

AlexHentschel Sep 16, 2022

AlexHentschel Sep 16, 2022

jordanschalm Sep 16, 2022

AlexHentschel left a comment •

edited

Loading

durkmurder left a comment

jordanschalm commented Sep 20, 2022

bors bot commented Sep 20, 2022

	<-util.AllClosed(broadcasted, submitted)
	unittest.AssertClosesBefore(cs.T(), util.AllClosed(broadcasted, submitted), time.Second)


		allViews := allFinalizedViews(t, nodes)
		for i, views := range allViews {

		@@ -47,11 +51,17 @@ func (s Stopper) AddNode(n Node) *StopperConsumer {
		s.Lock()

	log.Info().Msg("consensus timeout broadcast")
	log.Info().Msg("consensus timeout was broadcast")

	require.Eventually(cs.T(),
	func() bool {
	return cs.pending.AssertCalled(cs.T(), "PruneByView", finalizedBlock.View)
	}, time.Second, time.Millisecond*20)

		if e.me.NodeID() != proposal.Header.ProposerID {
		return fmt.Errorf("sanity check failed: attempted to provide another node's proposal (%x!=%x)", e.me.NodeID(), proposal.Header.ProposerID)

	// first, check that we are the proposer of the block
	if header.ProposerID != e.me.NodeID() {
	return fmt.Errorf("cannot broadcast proposal with non-local proposer (%x)", header.ProposerID)
	}

Consensus Compliance Engine updated to conform to Component interface #3121

Consensus Compliance Engine updated to conform to Component interface #3121

Conversation

jordanschalm commented Aug 29, 2022 • edited Loading

Other Changes

codecov-commenter commented Sep 1, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel left a comment • edited Loading

Choose a reason for hiding this comment

durkmurder left a comment

Choose a reason for hiding this comment

jordanschalm commented Sep 20, 2022

bors bot commented Sep 20, 2022

Consensus Compliance Engine updated to conform to `Component` interface #3121

Consensus Compliance Engine updated to conform to `Component` interface #3121

jordanschalm commented Aug 29, 2022 •

edited

Loading

codecov-commenter commented Sep 1, 2022 •

edited

Loading

AlexHentschel left a comment •

edited

Loading