[EFM] Recoverable Random Beacon State Machine follow up updates #6815
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/efm-recovery #6815 +/- ##
=====================================================
Coverage 41.71% 41.72%
=====================================================
Files 2033 2033
Lines 181062 181153 +91
=====================================================
+ Hits 75529 75578 +49
- Misses 99311 99346 +35
- Partials 6222 6229 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
||
| // start up the engine | ||
| unittest.AssertClosesBefore(suite.T(), suite.engine.Ready(), time.Second) | ||
| unittest.AssertClosesBefore(suite.T(), suite.engine.Ready(), 100*time.Second) |
There was a problem hiding this comment.
Is it really not starting up within a second? 100s seems like a lot.. Maybe 5s or 10s?
| // 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) |
There was a problem hiding this comment.
| "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) |
| // 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. |
There was a problem hiding this comment.
| // 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.
There was a problem hiding this comment.
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.RandomBeaconKeyCommittedstate). If there is a call toCommitMyBeaconPrivateKeywith anflow.EpochCommitthat 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.RandomBeaconKeyCommittedstate). The state machine receives a call toCommitMyBeaconPrivateKeywith anflow.EpochCommitthat 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.
| @@ -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 | |||
There was a problem hiding this comment.
| var pkey crypto.PrivateKey | |
| var privKey crypto.PrivateKey |
Nitpick: avoiding just the "p" prefix (because public also starts with "p" 😄)
| pkey = unittest.StakingPrivKeyFixture() | ||
| evidence = unittest.EpochCommitFixture(func(commit *flow.EpochCommit) { | ||
| commit.Counter = epochCounter | ||
| commit.DKGParticipantKeys[0] = pkey.PublicKey() | ||
| }) | ||
| err = store.UpsertMyBeaconPrivateKey(epochCounter, pkey, evidence) |
There was a problem hiding this comment.
| 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) |
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
EpochCommitso state machine can sanity check if it's inserting a valid private key.