[EFM] Recoverable Random Beacon State Machine follow up updates #6815

durkmurder · 2024-12-13T15:54:49Z

Context

This PR addresses some open comments from PR #6771.
Specifically:

By far the biggest change is how we insert/upsert keys, now caller needs to provide evidence in form of EpochCommit so state machine can sanity check if it's inserting a valid private key.

…m beacon key.

…for evidence. Updated tests

codecov-commenter · 2024-12-13T16:06:27Z

Codecov Report

Attention: Patch coverage is 45.94595% with 60 lines in your changes missing coverage. Please review.

Project coverage is 41.74%. Comparing base (a35632f) to head (caef7c3).

Files with missing lines	Patch %	Lines
storage/badger/dkg_state.go	58.22%	23 Missing and 10 partials ⚠️
storage/mock/dkg_state.go	0.00%	12 Missing ⚠️
cmd/consensus/main.go	0.00%	6 Missing ⚠️
engine/consensus/dkg/reactor_engine.go	44.44%	4 Missing and 1 partial ⚠️
storage/mock/epoch_recovery_my_beacon_key.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/efm-recovery    #6815      +/-   ##
========================================================
- Coverage                 41.81%   41.74%   -0.08%     
========================================================
  Files                      1588     2033     +445     
  Lines                    144098   181300   +37202     
========================================================
+ Hits                      60251    75677   +15426     
- Misses                    78887    99385   +20498     
- Partials                   4960     6238    +1278

Flag	Coverage Δ
unittests	`41.74% <45.94%> (-0.08%)`	⬇️

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.

into yurii/6725-follow-up-changes

…rrect key

jordanschalm · 2024-12-18T22:00:11Z

engine/consensus/dkg/reactor_engine_test.go


 	// start up the engine
-	unittest.AssertClosesBefore(suite.T(), suite.engine.Ready(), time.Second)
+	unittest.AssertClosesBefore(suite.T(), suite.engine.Ready(), 100*time.Second)


Is it really not starting up within a second? 100s seems like a lot.. Maybe 5s or 10s?

jordanschalm · 2024-12-18T22:10:40Z

storage/badger/dkg_state.go

+		// verify that the key is part of the EpochCommit
+		if err = ensureKeyIncludedInEpoch(epochCounter, key, commit); err != nil {
+			return storage.NewInvalidDKGStateTransitionErrorf(currentState, flow.RandomBeaconKeyCommitted,
+				"previously storred key has not been found in epoch commit event: %w", err)


Suggested change

"previously storred key has not been found in epoch commit event: %w", err)

"previously stored key has not been found in epoch commit event: %w", err)

jordanschalm · 2024-12-18T22:12:59Z

storage/badger/dkg_state.go

+// CommitMyBeaconPrivateKey commits the previously inserted random beacon private key for an epoch.
+// Effectively, this method transitions the state machine into the [flow.RandomBeaconKeyCommitted] state if the current state is [flow.DKGStateCompleted].
+// Caller needs to supply the [flow.EpochCommit] which is an evidence that the key has been indeed included for the given epoch.
+// No errors are expected during normal operations.


Suggested change

// No errors are expected during normal operations.

// If the current state is already [flow.RandomBeaconKeyCommitted], this function is a no-op regardless of input.

// No errors are expected during normal operations.

Describing the behaviour on line 238.

regarding Jordan's suggestion:

If the current state is already [flow.RandomBeaconKeyCommitted], this function is a no-op regardless of input.

This is probably the most minimal implementation, but I am not sure it would be the most robust. I think there are two non-happy path scenarios I think we should consider for the CommitMyBeaconPrivateKey:

A random beacon key was previously committed (i.e. we are in the flow.RandomBeaconKeyCommitted state). If there is a call to CommitMyBeaconPrivateKey with an flow.EpochCommit that is inconsistent with the committed key, we have a clear failure condition.

A random beacon key was previously committed (i.e. we are in the flow.RandomBeaconKeyCommitted state). The state machine receives a call to CommitMyBeaconPrivateKey with an flow.EpochCommit that is consistent with the committed key. Here, we could either return an exception and say that repeated calls are in principle not allowed. Alternatively, we can treat this call as confirming information that the state machine already has and ignore this call (as information is idempotent). The latter is my preference.

What makes me nervous is taking the response strategy for 2. and also applying it to 1., because we would be proceeding in a clear failure case. I think it would be comparatively easy to be more strict in this case.

jordanschalm · 2024-12-18T22:30:27Z

storage/badger/dkg_state_test.go

@@ -344,9 +515,16 @@ func TestDKGState_RandomBeaconKeyCommittedState(t *testing.T) {
 		store, err := NewRecoverableRandomBeaconStateMachine(metrics, db)
 		require.NoError(t, err)

+		var evidence *flow.EpochCommit
+		var pkey crypto.PrivateKey


Suggested change

var pkey crypto.PrivateKey

var privKey crypto.PrivateKey

Nitpick: avoiding just the "p" prefix (because public also starts with "p" 😄)

jordanschalm · 2024-12-18T22:30:51Z

storage/badger/dkg_state_test.go

+			pkey = unittest.StakingPrivKeyFixture()
+			evidence = unittest.EpochCommitFixture(func(commit *flow.EpochCommit) {
+				commit.Counter = epochCounter
+				commit.DKGParticipantKeys[0] = pkey.PublicKey()
+			})
+			err = store.UpsertMyBeaconPrivateKey(epochCounter, pkey, evidence)


Suggested change

pkey = unittest.StakingPrivKeyFixture()

evidence = unittest.EpochCommitFixture(func(commit *flow.EpochCommit) {

commit.Counter = epochCounter

commit.DKGParticipantKeys[0] = pkey.PublicKey()

})

err = store.UpsertMyBeaconPrivateKey(epochCounter, pkey, evidence)

privKey = unittest.StakingPrivKeyFixture()

evidence = unittest.EpochCommitFixture(func(commit *flow.EpochCommit) {

commit.Counter = epochCounter

commit.DKGParticipantKeys[0] = privKey.PublicKey()

})

err = store.UpsertMyBeaconPrivateKey(epochCounter, privKey, evidence)

AlexHentschel

Looks great. Thanks for the detailed work and solid tests. My comments mostly fall into two categories:

how we represent the underlying logic in code - in particular, we want to make it as easy as possible for somebody reading the code to convince themself of correctness (and which parts can be modified / extended without breaking correctness)
clearly documenting under which conditions operations are idempotent and structuring the code accordingly for clarity.

AlexHentschel · 2025-01-03T01:04:43Z

storage/badger/dkg_state.go

+// CommitMyBeaconPrivateKey commits the previously inserted random beacon private key for an epoch.
+// Effectively, this method transitions the state machine into the [flow.RandomBeaconKeyCommitted] state if the current state is [flow.DKGStateCompleted].
+// Caller needs to supply the [flow.EpochCommit] which is an evidence that the key has been indeed included for the given epoch.


Suggested change

// CommitMyBeaconPrivateKey commits the previously inserted random beacon private key for an epoch.

// Effectively, this method transitions the state machine into the [flow.RandomBeaconKeyCommitted] state if the current state is [flow.DKGStateCompleted].

// Caller needs to supply the [flow.EpochCommit] which is an evidence that the key has been indeed included for the given epoch.

// CommitMyBeaconPrivateKey commits the previously inserted random beacon private key for an epoch. Effectively, this method

// transitions the state machine into the [flow.RandomBeaconKeyCommitted] state if the current state is [flow.DKGStateCompleted].

// The caller needs to supply the [flow.EpochCommit] as evidence that the stored key is valid for the specified epoch. Repeated

// calls for the same epoch are accepted (idempotent operation), if and only if the provided EpochCommit confirms the already

// committed key.

AlexHentschel · 2025-01-03T01:19:16Z

storage/badger/dkg_state.go

+		// if we are in committed state then there is nothing to do
+		if currentState == flow.RandomBeaconKeyCommitted {
+			return nil
+		}
+		key, err := ds.keyCache.Get(epochCounter)(tx.DBTxn)
+		if err != nil {
+			return storage.NewInvalidDKGStateTransitionErrorf(currentState, flow.RandomBeaconKeyCommitted, "cannot transition without a valid random beacon key: %w", err)
+		}
+
+		// verify that the key is part of the EpochCommit
+		if err = ensureKeyIncludedInEpoch(epochCounter, key, commit); err != nil {
+			return storage.NewInvalidDKGStateTransitionErrorf(currentState, flow.RandomBeaconKeyCommitted,
+				"previously storred key has not been found in epoch commit event: %w", err)
+		}
+		return ds.processStateTransition(epochCounter, currentState, flow.RandomBeaconKeyCommitted)(tx)


here, I would suggest to always enforce that the stored random beacon key is consistent with the provided EpochCommit. Only the state transition itself should be omitted, if this is a repeated call. In a nutshell, the block if currentState == flow.RandomBeaconKeyCommitted would just move down.

Suggested change

// if we are in committed state then there is nothing to do

if currentState == flow.RandomBeaconKeyCommitted {

return nil

}

key, err := ds.keyCache.Get(epochCounter)(tx.DBTxn)

if err != nil {

return storage.NewInvalidDKGStateTransitionErrorf(currentState, flow.RandomBeaconKeyCommitted, "cannot transition without a valid random beacon key: %w", err)

}

// verify that the key is part of the EpochCommit

if err = ensureKeyIncludedInEpoch(epochCounter, key, commit); err != nil {

return storage.NewInvalidDKGStateTransitionErrorf(currentState, flow.RandomBeaconKeyCommitted,

"previously storred key has not been found in epoch commit event: %w", err)

}

return ds.processStateTransition(epochCounter, currentState, flow.RandomBeaconKeyCommitted)(tx)

// Repeated calls for same epoch are idempotent, but only if they consistently confirm the stored private key. We explicitly

// enforce consistency with the already committed key here. Repetitions are considered rare, so performance overhead is acceptable.

key, err := ds.keyCache.Get(epochCounter)(tx.DBTxn)

if err != nil {

return storage.NewInvalidDKGStateTransitionErrorf(currentState, flow.RandomBeaconKeyCommitted, "cannot transition without a valid random beacon key: %w", err)

}

// verify that the key is part of the EpochCommit

if err = ds.ensureKeyIncludedInEpoch(epochCounter, key, commit); err != nil {

return storage.NewInvalidDKGStateTransitionErrorf(currentState, flow.RandomBeaconKeyCommitted,

"according to EpochCommit event, my stored random beacon key is not valid for signing: %w", err)

}

// transition to RandomBeaconKeyCommitted, unless this is a repeated call, in which case there is nothing else to do

if currentState == flow.RandomBeaconKeyCommitted {

return nil

}

return ds.processStateTransition(epochCounter, currentState, flow.RandomBeaconKeyCommitted)(tx)

AlexHentschel · 2025-01-03T01:28:23Z

storage/badger/dkg_state.go

+		// verify that the key is part of the EpochCommit
+		if err = ensureKeyIncludedInEpoch(epochCounter, key, commit); err != nil {
+			return storage.NewInvalidDKGStateTransitionErrorf(currentState, flow.RandomBeaconKeyCommitted,
+				"previously storred key has not been found in epoch commit event: %w", err)


Suggested change

"previously storred key has not been found in epoch commit event: %w", err)

"according to EpochCommit event, the input random beacon key is not valid for signing: %w", err)

AlexHentschel · 2025-01-03T01:30:40Z

storage/badger/dkg_state.go

+		err := operation.RetrieveDKGStateForEpoch(epochCounter, &currentState)(txn)
+		if err != nil && !errors.Is(err, storage.ErrNotFound) {
+			return currentState, fmt.Errorf("could not retrieve current state for epoch %d: %w", epochCounter, err)
+


Suggested change

AlexHentschel · 2025-01-03T01:38:15Z

storage/badger/dkg_state.go

+		// if we are in committed state, we cannot overwrite the key, but we can ignore this input iff the provided key is the same
+		if currentState == flow.RandomBeaconKeyCommitted {
+			// check if the stored key is equal to the provided key


Suggested change

// if we are in committed state, we cannot overwrite the key, but we can ignore this input iff the provided key is the same

if currentState == flow.RandomBeaconKeyCommitted {

// check if the stored key is equal to the provided key

// Repeated calls for same epoch are idempotent, but only if they consistently confirm the stored private key. We explicitly

// enforce consistency with the already committed key here. Repetitions are considered rare, so performance overhead is acceptable.

if currentState == flow.RandomBeaconKeyCommitted {

AlexHentschel · 2025-01-03T03:22:21Z

storage/dkg.go

+	// CommitMyBeaconPrivateKey commits the previously inserted random beacon private key for an epoch.
+	// Effectively, this method transitions the state machine into the [flow.RandomBeaconKeyCommitted] state if the current state is [flow.DKGStateCompleted].
+	// Caller needs to supply the [flow.EpochCommit] which is an evidence that the key has been indeed included for the given epoch.


Suggested change

// CommitMyBeaconPrivateKey commits the previously inserted random beacon private key for an epoch.

// Effectively, this method transitions the state machine into the [flow.RandomBeaconKeyCommitted] state if the current state is [flow.DKGStateCompleted].

// Caller needs to supply the [flow.EpochCommit] which is an evidence that the key has been indeed included for the given epoch.

// CommitMyBeaconPrivateKey commits the previously inserted random beacon private key for an epoch. Effectively, this method

// transitions the state machine into the [flow.RandomBeaconKeyCommitted] state if the current state is [flow.DKGStateCompleted].

// The caller needs to supply the [flow.EpochCommit] as evidence that the stored key is valid for the specified epoch. Repeated

// calls for the same epoch are accepted (idempotent operation),if and only if the provided EpochCommit confirms the already

// committed key.

AlexHentschel · 2025-01-03T03:25:08Z

storage/dkg.go

 	// from Epoch Fallback Mode. State transitions are allowed if and only if the current state is not equal to
 	// [flow.RandomBeaconKeyCommitted]. The resulting state of this method call is [flow.RandomBeaconKeyCommitted].


Suggested change

// from Epoch Fallback Mode. State transitions are allowed if and only if the current state is not equal to

// [flow.RandomBeaconKeyCommitted]. The resulting state of this method call is [flow.RandomBeaconKeyCommitted].

// from Epoch Fallback Mode. The resulting state of this method call is [flow.RandomBeaconKeyCommitted].

// State transitions are allowed if and only if the current state is not equal to [flow.RandomBeaconKeyCommitted].

// Repeated calls for the same epoch are idempotent, if and only if the provided EpochCommit confirms the already

// committed key (error otherwise).

AlexHentschel · 2025-01-03T03:25:27Z

storage/badger/dkg_state.go

 // from Epoch Fallback Mode. State transitions are allowed if and only if the current state is not equal to
 // [flow.RandomBeaconKeyCommitted]. The resulting state of this method call is [flow.RandomBeaconKeyCommitted].


Suggested change

// from Epoch Fallback Mode. State transitions are allowed if and only if the current state is not equal to

// [flow.RandomBeaconKeyCommitted]. The resulting state of this method call is [flow.RandomBeaconKeyCommitted].

// from Epoch Fallback Mode. The resulting state of this method call is [flow.RandomBeaconKeyCommitted].

// State transitions are allowed if and only if the current state is not equal to [flow.RandomBeaconKeyCommitted].

// Repeated calls for the same epoch are idempotent, if and only if the provided EpochCommit confirms the already

// committed key (error otherwise).

AlexHentschel · 2025-01-03T03:28:07Z

cmd/consensus/main.go

@@ -316,7 +316,7 @@ func main() {
 				return fmt.Errorf("could not load beacon key file: %w", err)
 			}

-			rootEpoch := node.State.AtBlockID(node.FinalizedRootBlock.ID()).Epochs().Current()
+			rootEpoch := rootSnapshot.Epochs().Current()
 			epochCounter, err := rootEpoch.Counter()


for consistency, could you rename this variable

Suggested change

epochCounter, err := rootEpoch.Counter()

rootEpochCounter, err := rootEpoch.Counter()

AlexHentschel · 2025-01-03T03:32:29Z

cmd/consensus/main.go

@@ -346,7 +346,11 @@ func main() {
 			// perform this only if state machine is in initial state


would suggest documenting the logical block of lines 341 to 357 as a whole. Something like

// store my beacon key for the first epoch post-spork (only if we haven't run this logic before, i.e. state machine is in initial state) started, err := myBeaconKeyStateMachine.IsDKGStarted(epochCounter) if err != nil { return fmt.Errorf("could not get DKG started flag for root epoch %d: %w", epochCounter, err) } if !started { epochProtocolState, err := rootSnapshot.EpochProtocolState() ⋮

durkmurder added 4 commits December 12, 2024 16:40

Implemented evidence based state transitions for committing the rando…

8fbd1ea

…m beacon key.

Updated logic to how commit and overwrite private keys. Added checks …

fafb959

…for evidence. Updated tests

Updated mocks. Updated usages of state machine. Fixed tests

8a67288

Linted

6f4eb62

durkmurder added 6 commits December 16, 2024 14:38

Merge branch 'feature/efm-recovery' of https://github.com/onflow/flow-go

4940df3

into yurii/6725-follow-up-changes

Fixed inclussion of beacon key in epoch commit sanity check

ff65251

Fixed broken unit tests

5dfc4c7

Added a separate test for testing failure state in another setup

3da41d6

Added extra checks to verify that we have indeed inserted/upserted co…

0acb3f4

…rrect key

Linted

fc908b4

durkmurder assigned jordanschalm and AlexHentschel Dec 17, 2024

durkmurder requested review from jordanschalm and AlexHentschel December 17, 2024 14:29

durkmurder marked this pull request as ready for review December 17, 2024 14:29

Merge branch 'feature/efm-recovery' into yurii/6725-follow-up-changes

b953a43

jordanschalm approved these changes Dec 18, 2024

View reviewed changes

Merge branch 'feature/efm-recovery' into yurii/6725-follow-up-changes

caef7c3

AlexHentschel approved these changes Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[EFM] Recoverable Random Beacon State Machine follow up updates #6815

[EFM] Recoverable Random Beacon State Machine follow up updates #6815

durkmurder commented Dec 13, 2024 •

edited

Loading

codecov-commenter commented Dec 13, 2024 •

edited

Loading

jordanschalm Dec 18, 2024

jordanschalm Dec 18, 2024

jordanschalm Dec 18, 2024

AlexHentschel Dec 19, 2024

jordanschalm Dec 18, 2024

jordanschalm Dec 18, 2024

AlexHentschel left a comment •

edited

Loading

AlexHentschel Jan 3, 2025

AlexHentschel Jan 3, 2025

AlexHentschel Jan 3, 2025

AlexHentschel Jan 3, 2025

AlexHentschel Jan 3, 2025

AlexHentschel Jan 3, 2025

AlexHentschel Jan 3, 2025

AlexHentschel Jan 3, 2025

AlexHentschel Jan 3, 2025

AlexHentschel Jan 3, 2025

	"previously storred key has not been found in epoch commit event: %w", err)
	"previously stored key has not been found in epoch commit event: %w", err)

	// No errors are expected during normal operations.
	// If the current state is already [flow.RandomBeaconKeyCommitted], this function is a no-op regardless of input.
	// No errors are expected during normal operations.

-// CommitMyBeaconPrivateKey commits the previously inserted random beacon private key for an epoch.
-// Effectively, this method transitions the state machine into the [flow.RandomBeaconKeyCommitted] state if the current state is [flow.DKGStateCompleted].
-// Caller needs to supply the [flow.EpochCommit] which is an evidence that the key has been indeed included for the given epoch.
+// CommitMyBeaconPrivateKey commits the previously inserted random beacon private key for an epoch. Effectively, this method
+// transitions the state machine into the [flow.RandomBeaconKeyCommitted] state if the current state is [flow.DKGStateCompleted].
+// The caller needs to supply the [flow.EpochCommit] as evidence that the stored key is valid for the specified epoch. Repeated
+// calls for the same epoch are accepted (idempotent operation), if and only if the provided EpochCommit confirms the already
+// committed key.

	"previously storred key has not been found in epoch commit event: %w", err)
	"according to EpochCommit event, the input random beacon key is not valid for signing: %w", err)

		// from Epoch Fallback Mode. State transitions are allowed if and only if the current state is not equal to
		// [flow.RandomBeaconKeyCommitted]. The resulting state of this method call is [flow.RandomBeaconKeyCommitted].

-	// from Epoch Fallback Mode. State transitions are allowed if and only if the current state is not equal to
-	// [flow.RandomBeaconKeyCommitted]. The resulting state of this method call is [flow.RandomBeaconKeyCommitted].
+	// from Epoch Fallback Mode. The resulting state of this method call is [flow.RandomBeaconKeyCommitted].
+	// State transitions are allowed if and only if the current state is not equal to [flow.RandomBeaconKeyCommitted].
+	// Repeated calls for the same epoch are idempotent, if and only if the provided EpochCommit confirms the already
+	// committed key (error otherwise).

	epochCounter, err := rootEpoch.Counter()
	rootEpochCounter, err := rootEpoch.Counter()

		@@ -346,7 +346,11 @@ func main() {
		// perform this only if state machine is in initial state

[EFM] Recoverable Random Beacon State Machine follow up updates #6815

Are you sure you want to change the base?

[EFM] Recoverable Random Beacon State Machine follow up updates #6815

Conversation

durkmurder commented Dec 13, 2024 • edited Loading

Context

codecov-commenter commented Dec 13, 2024 • 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

AlexHentschel left a comment • edited Loading

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

durkmurder commented Dec 13, 2024 •

edited

Loading

codecov-commenter commented Dec 13, 2024 •

edited

Loading

AlexHentschel left a comment •

edited

Loading