[Dynamic Protocol State] Refactor Epoch Commitment Deadline Enforcement #5108

jordanschalm · 2023-12-05T16:54:11Z

This PR addresses #5104, using this suggestion:

Alternatively, we could implement the check entirely in the constructor or the mutator. We would essentially just add another case, where we instantiate an epochFallbackStateMachine here:

Changes

EFM caused by exceeding the epoch commitment deadline is observed strictly in the protocol state stateMutator module
Adds tests for the stateMutator constructor

1. Update bootstrapping to use DynamicProtocolState rather than protocol.Epoch API, resolving a TODO and simplifying that method 2. Removes the committed_epoch_final_view metric. We originally added this metric with the idea that it would provide a way to see how much time you have minimally, until the next time when EFM could possibly be triggered. However: - it is not used - it is somewhat misleading, as EFM can actually be triggered at other points than the final view of the committed epoch in some circumstances - if we do want to alert on something like this in the future, it can be created from other metrics: - cur_epoch_final_view, cur_view, cur_epoch_phase

- remove expectation of removed metric - fix call to indexFirstHeight in bootstrapping - fix boolean conditional in EpochPhase method

Co-authored-by: Yurii Oleksyshyn <yuraolex@gmail.com>

…or-epoch-commit-deadline

codecov-commenter · 2023-12-11T21:46:25Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (1668c30) 56.51% compared to head (6ba3c78) 56.50%.

Files	Patch %	Lines
...sensus/hotstuff/cruisectl/block_time_controller.go	0.00%	2 Missing ⚠️
utils/unittest/fixtures.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@                        Coverage Diff                         @@
##           feature/dynamic-protocol-state    #5108      +/-   ##
==================================================================
- Coverage                           56.51%   56.50%   -0.01%     
==================================================================
  Files                                 987      987              
  Lines                               92539    92531       -8     
==================================================================
- Hits                                52298    52289       -9     
- Misses                              36396    36400       +4     
+ Partials                             3845     3842       -3

Flag	Coverage Δ
unittests	`56.50% <87.50%> (-0.01%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

state/protocol/protocol_state/mutator_test.go

durkmurder

Very nice PR, great tests. 🏅

Co-authored-by: Yurii Oleksyshyn <yuraolex@gmail.com>

AlexHentschel

Very clean and amazing tests. Thank you 🙇

AlexHentschel · 2023-12-15T02:55:22Z

state/protocol/badger/mutator_test.go

There a lots of mentions of "EECC" in this file. Not sure, did we want to replace those with "EFM" 😅 (?)

Did a project-wide replace in 8e69496

AlexHentschel · 2023-12-15T02:58:53Z

state/protocol/protocol_state/mutator.go

@@ -269,3 +273,16 @@ func (m *stateMutator) transitionToEpochFallbackMode(results []*flow.ExecutionRe
 	}
 	return dbUpdates, nil
 }
+
+// epochFallbackTriggeredByIncorporatingCandidate checks whether incorporating the input block
+// would trigger epoch fallback mode (EFM) along the current fork. In particular, we trigger epoch


in scientific literature, it is common practise to set abbreviations su ch as EFM in square brackets. Would suggest to adopt this convention also in our code:

// would trigger epoch fallback mode [EFM] along the current fork.

AlexHentschel · 2023-12-15T03:04:54Z

state/protocol/protocol_state/mutator.go

+// epochFallbackTriggeredByIncorporatingCandidate checks whether incorporating the input block
+// would trigger epoch fallback mode (EFM) along the current fork. In particular, we trigger epoch
+// fallback mode when:
+//  1. The next epoch has not been committed as of B (EpochPhase < flow.EpochPhaseCommitted) AND


would be inclined to use the same representation as in code:

flow-go/state/protocol/protocol_state/mutator.go

Line 284 in 59165d9

if phase == flow.EpochPhaseCommitted { // Requirement 1

Suggested change

// 1. The next epoch has not been committed as of B (EpochPhase < flow.EpochPhaseCommitted) AND

// 1. The next epoch has not been committed as of B (EpochPhase ≠ flow.EpochPhaseCommitted) AND

This holds independently of the internal order of EpochPhase

AlexHentschel · 2023-12-15T03:45:55Z

state/protocol/protocol_state/mutator.go

+// epochFallbackTriggeredByIncorporatingCandidate checks whether incorporating the input block
+// would trigger epoch fallback mode (EFM) along the current fork. In particular, we trigger epoch
+// fallback mode when:
+//  1. The next epoch has not been committed as of B (EpochPhase < flow.EpochPhaseCommitted) AND
+//  2. B is the first incorporated block with view greater than or equal to the epoch commitment
+//     deadline for the current epoch
+func epochFallbackTriggeredByIncorporatingCandidate(candidateView, safetyThreshold, finalView uint64, phase flow.EpochPhase) bool {
+	if phase == flow.EpochPhaseCommitted { // Requirement 1
+		return false
+	}
+	return candidateView+safetyThreshold >= finalView // Requirement 2
+}


Algorithmically it is correct I think, but I got stuck on it because there were a few details that I wasn't aware of. I think we can increase code clarity and documentation coverage notably if we changed the signature of the method just a bit.

Suggested change

// epochFallbackTriggeredByIncorporatingCandidate checks whether incorporating the input block

// would trigger epoch fallback mode (EFM) along the current fork. In particular, we trigger epoch

// fallback mode when:

// 1. The next epoch has not been committed as of B (EpochPhase < flow.EpochPhaseCommitted) AND

// 2. B is the first incorporated block with view greater than or equal to the epoch commitment

// deadline for the current epoch

func epochFallbackTriggeredByIncorporatingCandidate(candidateView, safetyThreshold, finalView uint64, phase flow.EpochPhase) bool {

if phase == flow.EpochPhaseCommitted { // Requirement 1

return false

}

return candidateView+safetyThreshold >= finalView // Requirement 2

}

// epochFallbackTriggeredByIncorporatingCandidate checks whether incorporating the input block B

// would trigger epoch fallback mode [EFM] along the current fork. We trigger epoch fallback mode

// when:

// 1. The next epoch has not been committed as of B (EpochPhase ≠ flow.EpochPhaseCommitted) AND

// 2. B is the first incorporated block with view greater than or equal to the epoch commitment

// deadline for the current epoch

//

// In protocol terms, condition 1 means that an EpochCommit service event for the upcoming epoch has

// not yet been sealed as of block B. Formally, a service event S is considered sealed as of block B if:

// - S was emitted during execution of some block A, s.t. A is an ancestor of B.

// - The seal for block A was included in some block C, s.t C is an ancestor of B.

//

// For further details see `params.EpochCommitSafetyThreshold()`.

func epochFallbackTriggeredByIncorporatingCandidate(candidateView uint64, params protocol.GlobalParams, parentState *flow.RichProtocolStateEntry) bool {

if parentState.EpochPhase() == flow.EpochPhaseCommitted { // Requirement 1

return false

}

return candidateView+params.EpochCommitSafetyThreshold() >= parentState.CurrentEpochSetup.FinalView // Requirement 2

}

Here, I have used params and parentState instead of the derived values, because then I can easily refer to them in the documentation

Done in 6ba3c78

AlexHentschel · 2023-12-15T03:46:53Z

state/protocol/protocol_state/mutator.go

+		//         epoch service event or an invalid state transition previously in this fork.
+		// Case 2: Incorporating the candidate block is itself an invalid epoch transition.
+		//
+		// In either case, Epoch Fallback Mode (EFM) has been tentatively triggered on this fork,


Suggested change

// In either case, Epoch Fallback Mode (EFM) has been tentatively triggered on this fork,

// In either case, Epoch Fallback Mode [EFM] has been tentatively triggered on this fork,

AlexHentschel · 2023-12-15T03:52:35Z

state/protocol/protocol_state/mutator_test.go

+	// if a state machine constructor returns an error, the stateMutator constructor should fail
+	// and propagate the error to the caller
+	s.Run("state machine constructor returns error", func() {


thank you for the also testing the error path 🤩

…or-epoch-commit-deadline

jordanschalm and others added 21 commits November 29, 2023 13:12

wip

24c4e21

add EpophPhase to protocol state entry

1925f3e

add todos for refactor

76cac32

replace usages of epoch status

3b807c8

remove metric emission

72c1491

remove last usage of EpochStatus from proto state api

bce5a80

remove EpochStatus getter

c84a092

update adapter tests

90ebb1b

remove EpochStatus type

1d540d3

move indexing of first-height for epochs

0b527ba

update mocks

4c0e635

remove old mock

78cb4f1

lint

9b51366

update tests

af741e3

- remove expectation of removed metric - fix call to indexFirstHeight in bootstrapping - fix boolean conditional in EpochPhase method

remove todo comments

36db66d

lint

df6cc1a

Apply suggestions from code review

e4bb3fb

Co-authored-by: Yurii Oleksyshyn <yuraolex@gmail.com>

lint

04b42dc

move epoch commit deadline check into state machine

b2f9cf9

remove previous deadline check fn

ba41f4b

jordanschalm changed the base branch from master to jordan/4649-epoch-status December 5, 2023 16:54

jordanschalm added 6 commits December 5, 2023 12:29

update protocol state tests

27b7468

update tests

725f8fa

adjust godocs

000fd1b

typo

fd57454

add test cases for mutator constructor

df5e23d

use bool over chan

957c840

jordanschalm marked this pull request as ready for review December 6, 2023 22:34

jordanschalm requested a review from durkmurder December 6, 2023 22:35

jordanschalm requested a review from AlexHentschel December 6, 2023 22:35

Base automatically changed from jordan/4649-epoch-status to feature/dynamic-protocol-state December 11, 2023 19:22

jordanschalm added 2 commits December 11, 2023 16:40

Merge branch 'feature/dynamic-protocol-state' into jordan/5104-refact…

45671d8

…or-epoch-commit-deadline

lint

6a737f3

durkmurder reviewed Dec 13, 2023

View reviewed changes

state/protocol/protocol_state/mutator_test.go Outdated Show resolved Hide resolved

durkmurder approved these changes Dec 13, 2023

View reviewed changes

Apply suggestions from code review

59165d9

Co-authored-by: Yurii Oleksyshyn <yuraolex@gmail.com>

AlexHentschel approved these changes Dec 15, 2023

View reviewed changes

jordanschalm added 4 commits December 15, 2023 13:44

Merge branch 'feature/dynamic-protocol-state' into jordan/5104-refact…

2613c28

…or-epoch-commit-deadline

rename in comments: EECC->EFM

8e69496

comment structure: (EFM) -> [EFM]

d3cee86

modify signature of epochFallbackTriggeredByIncorporatingCandidate

6ba3c78

jordanschalm merged commit 292b541 into feature/dynamic-protocol-state Dec 15, 2023
53 checks passed

jordanschalm deleted the jordan/5104-refactor-epoch-commit-deadline branch December 15, 2023 20:27

jordanschalm mentioned this pull request Apr 26, 2024

[Dynamic Protocol State] passing the epoch commitment deadline without an EpochCommit event #5104

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dynamic Protocol State] Refactor Epoch Commitment Deadline Enforcement #5108

[Dynamic Protocol State] Refactor Epoch Commitment Deadline Enforcement #5108

jordanschalm commented Dec 5, 2023 •

edited

Loading

codecov-commenter commented Dec 11, 2023 •

edited

Loading

durkmurder left a comment

AlexHentschel left a comment

AlexHentschel Dec 15, 2023

jordanschalm Dec 15, 2023

AlexHentschel Dec 15, 2023

jordanschalm Dec 15, 2023

AlexHentschel Dec 15, 2023

AlexHentschel Dec 15, 2023

jordanschalm Dec 15, 2023

AlexHentschel Dec 15, 2023

AlexHentschel Dec 15, 2023

	// 1. The next epoch has not been committed as of B (EpochPhase < flow.EpochPhaseCommitted) AND
	// 1. The next epoch has not been committed as of B (EpochPhase ≠ flow.EpochPhaseCommitted) AND

	// In either case, Epoch Fallback Mode (EFM) has been tentatively triggered on this fork,
	// In either case, Epoch Fallback Mode [EFM] has been tentatively triggered on this fork,

[Dynamic Protocol State] Refactor Epoch Commitment Deadline Enforcement #5108

[Dynamic Protocol State] Refactor Epoch Commitment Deadline Enforcement #5108

Conversation

jordanschalm commented Dec 5, 2023 • edited Loading

Changes

codecov-commenter commented Dec 11, 2023 • edited Loading

Codecov Report

durkmurder left a comment

Choose a reason for hiding this comment

AlexHentschel left a 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

jordanschalm commented Dec 5, 2023 •

edited

Loading

codecov-commenter commented Dec 11, 2023 •

edited

Loading