From 100a92bccd3852fecff3944d395ae1795e354aba Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 20 Nov 2024 20:08:05 +0200 Subject: [PATCH 01/34] Changed structure of interfaces and corresponding implementation for DKG storage --- cmd/consensus/main.go | 2 +- storage/badger/dkg_state.go | 78 +++++++++++------------------ storage/badger/dkg_state_test.go | 4 +- storage/dkg.go | 86 +++++++++++++++++--------------- 4 files changed, 78 insertions(+), 92 deletions(-) diff --git a/cmd/consensus/main.go b/cmd/consensus/main.go index 1c0d8934a20..091c7e4b889 100644 --- a/cmd/consensus/main.go +++ b/cmd/consensus/main.go @@ -127,7 +127,7 @@ func main() { committee *committees.Consensus epochLookup *epochs.EpochLookup hotstuffModules *consensus.HotstuffModules - dkgState *bstorage.DKGState + dkgState *bstorage.RecoverablePrivateBeaconKeyState safeBeaconKeys *bstorage.SafeBeaconPrivateKeys getSealingConfigs module.SealingConfigsGetter ) diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index 88edc495b9f..e236e0d7d56 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -16,15 +16,24 @@ import ( "github.com/onflow/flow-go/storage/badger/transaction" ) -// DKGState stores state information about in-progress and completed DKGs, including +// RecoverablePrivateBeaconKeyState stores state information about in-progress and completed DKGs, including // computed keys. Must be instantiated using secrets database. -type DKGState struct { +// RecoverablePrivateBeaconKeyState is a specific module that allows to overwrite the beacon private key for a given epoch. +// This module is used *ONLY* in the epoch recovery process and only by the consensus participants. +// Each consensus participant takes part in the DKG, and after successfully finishing the DKG protocol it obtains a +// random beacon private key, which is stored in the database along with DKG end state `flow.DKGEndStateSuccess`. +// If for any reason the DKG fails, then the private key will be nil and DKG end state will be `flow.DKGEndStateDKGFailure`. +// When the epoch recovery takes place, we need to query the last valid beacon private key for the current replica and +// also set it for use during the Recovery Epoch, otherwise replicas won't be able to vote for blocks during the Recovery Epoch. +type RecoverablePrivateBeaconKeyState struct { db *badger.DB keyCache *Cache[uint64, *encodable.RandomBeaconPrivKey] } -// NewDKGState returns the DKGState implementation backed by Badger DB. -func NewDKGState(collector module.CacheMetrics, db *badger.DB) (*DKGState, error) { +var _ storage.EpochRecoveryMyBeaconKey = (*RecoverablePrivateBeaconKeyState)(nil) + +// NewDKGState returns the RecoverablePrivateBeaconKeyState implementation backed by Badger DB. +func NewDKGState(collector module.CacheMetrics, db *badger.DB) (*RecoverablePrivateBeaconKeyState, error) { err := operation.EnsureSecretDB(db) if err != nil { return nil, fmt.Errorf("cannot instantiate dkg state storage in non-secret db: %w", err) @@ -48,7 +57,7 @@ func NewDKGState(collector module.CacheMetrics, db *badger.DB) (*DKGState, error withRetrieve(retrieveKey), ) - dkgState := &DKGState{ + dkgState := &RecoverablePrivateBeaconKeyState{ db: db, keyCache: cache, } @@ -56,11 +65,11 @@ func NewDKGState(collector module.CacheMetrics, db *badger.DB) (*DKGState, error return dkgState, nil } -func (ds *DKGState) storeKeyTx(epochCounter uint64, key *encodable.RandomBeaconPrivKey) func(tx *transaction.Tx) error { +func (ds *RecoverablePrivateBeaconKeyState) storeKeyTx(epochCounter uint64, key *encodable.RandomBeaconPrivKey) func(tx *transaction.Tx) error { return ds.keyCache.PutTx(epochCounter, key) } -func (ds *DKGState) retrieveKeyTx(epochCounter uint64) func(tx *badger.Txn) (*encodable.RandomBeaconPrivKey, error) { +func (ds *RecoverablePrivateBeaconKeyState) retrieveKeyTx(epochCounter uint64) func(tx *badger.Txn) (*encodable.RandomBeaconPrivKey, error) { return func(tx *badger.Txn) (*encodable.RandomBeaconPrivKey, error) { val, err := ds.keyCache.Get(epochCounter)(tx) if err != nil { @@ -75,7 +84,7 @@ func (ds *DKGState) retrieveKeyTx(epochCounter uint64) func(tx *badger.Txn) (*en // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned -func (ds *DKGState) InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { +func (ds *RecoverablePrivateBeaconKeyState) InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { if key == nil { return fmt.Errorf("will not store nil beacon key") } @@ -83,12 +92,12 @@ func (ds *DKGState) InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.Pri return operation.RetryOnConflictTx(ds.db, transaction.Update, ds.storeKeyTx(epochCounter, encodableKey)) } -// RetrieveMyBeaconPrivateKey retrieves the random beacon private key for an epoch. +// UnsafeRetrieveMyBeaconPrivateKey retrieves the random beacon private key for an epoch. // // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned -func (ds *DKGState) RetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) { +func (ds *RecoverablePrivateBeaconKeyState) UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) { tx := ds.db.NewTransaction(false) defer tx.Discard() encodableKey, err := ds.retrieveKeyTx(epochCounter)(tx) @@ -99,41 +108,29 @@ func (ds *DKGState) RetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.Priv } // SetDKGStarted sets the flag indicating the DKG has started for the given epoch. -func (ds *DKGState) SetDKGStarted(epochCounter uint64) error { +func (ds *RecoverablePrivateBeaconKeyState) SetDKGStarted(epochCounter uint64) error { return ds.db.Update(operation.InsertDKGStartedForEpoch(epochCounter)) } // GetDKGStarted checks whether the DKG has been started for the given epoch. -func (ds *DKGState) GetDKGStarted(epochCounter uint64) (bool, error) { +func (ds *RecoverablePrivateBeaconKeyState) GetDKGStarted(epochCounter uint64) (bool, error) { var started bool err := ds.db.View(operation.RetrieveDKGStartedForEpoch(epochCounter, &started)) return started, err } // SetDKGEndState stores that the DKG has ended, and its end state. -func (ds *DKGState) SetDKGEndState(epochCounter uint64, endState flow.DKGEndState) error { +func (ds *RecoverablePrivateBeaconKeyState) SetDKGEndState(epochCounter uint64, endState flow.DKGEndState) error { return ds.db.Update(operation.InsertDKGEndStateForEpoch(epochCounter, endState)) } // GetDKGEndState retrieves the DKG end state for the epoch. -func (ds *DKGState) GetDKGEndState(epochCounter uint64) (flow.DKGEndState, error) { +func (ds *RecoverablePrivateBeaconKeyState) GetDKGEndState(epochCounter uint64) (flow.DKGEndState, error) { var endState flow.DKGEndState err := ds.db.Update(operation.RetrieveDKGEndStateForEpoch(epochCounter, &endState)) return endState, err } -// SafeBeaconPrivateKeys is the safe beacon key storage backed by Badger DB. -type SafeBeaconPrivateKeys struct { - state *DKGState -} - -var _ storage.SafeBeaconKeys = (*SafeBeaconPrivateKeys)(nil) - -// NewSafeBeaconPrivateKeys returns a safe beacon key storage backed by Badger DB. -func NewSafeBeaconPrivateKeys(state *DKGState) *SafeBeaconPrivateKeys { - return &SafeBeaconPrivateKeys{state: state} -} - // RetrieveMyBeaconPrivateKey retrieves my beacon private key for the given // epoch, only if my key has been confirmed valid and safe for use. // @@ -143,8 +140,8 @@ func NewSafeBeaconPrivateKeys(state *DKGState) *SafeBeaconPrivateKeys { // -> no beacon key will ever be available for the epoch in this case // - (nil, false, storage.ErrNotFound) if the DKG has not ended // - (nil, false, error) for any unexpected exception -func (keys *SafeBeaconPrivateKeys) RetrieveMyBeaconPrivateKey(epochCounter uint64) (key crypto.PrivateKey, safe bool, err error) { - err = keys.state.db.View(func(txn *badger.Txn) error { +func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCounter uint64) (key crypto.PrivateKey, safe bool, err error) { + err = ds.db.View(func(txn *badger.Txn) error { // retrieve the end state var endState flow.DKGEndState @@ -159,7 +156,7 @@ func (keys *SafeBeaconPrivateKeys) RetrieveMyBeaconPrivateKey(epochCounter uint6 if endState == flow.DKGEndStateSuccess || endState == flow.RandomBeaconKeyRecovered { // retrieve the key - any storage error (including `storage.ErrNotFound`) is an exception var encodableKey *encodable.RandomBeaconPrivKey - encodableKey, err = keys.state.retrieveKeyTx(epochCounter)(txn) + encodableKey, err = ds.retrieveKeyTx(epochCounter)(txn) if err != nil { key = nil safe = false @@ -179,33 +176,16 @@ func (keys *SafeBeaconPrivateKeys) RetrieveMyBeaconPrivateKey(epochCounter uint6 return } -// EpochRecoveryMyBeaconKey is a specific module that allows to overwrite the beacon private key for a given epoch. -// This module is used *ONLY* in the epoch recovery process and only by the consensus participants. -// Each consensus participant takes part in the DKG, and after successfully finishing the DKG protocol it obtains a -// random beacon private key, which is stored in the database along with DKG end state `flow.DKGEndStateSuccess`. -// If for any reason the DKG fails, then the private key will be nil and DKG end state will be `flow.DKGEndStateDKGFailure`. -// When the epoch recovery takes place, we need to query the last valid beacon private key for the current replica and -// also set it for use during the Recovery Epoch, otherwise replicas won't be able to vote for blocks during the Recovery Epoch. -type EpochRecoveryMyBeaconKey struct { - *SafeBeaconPrivateKeys -} - -var _ storage.EpochRecoveryMyBeaconKey = (*EpochRecoveryMyBeaconKey)(nil) - -func NewEpochRecoveryMyBeaconKey(keys *SafeBeaconPrivateKeys) *EpochRecoveryMyBeaconKey { - return &EpochRecoveryMyBeaconKey{SafeBeaconPrivateKeys: keys} -} - // UpsertMyBeaconPrivateKey overwrites the random beacon private key for the epoch that recovers the protocol from // Epoch Fallback Mode. Effectively, this function overwrites whatever might be available in the database with // the given private key and sets the DKGEndState to `flow.DKGEndStateRecovered`. // No errors are expected during normal operations. -func (keys *EpochRecoveryMyBeaconKey) UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { +func (ds *RecoverablePrivateBeaconKeyState) UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { if key == nil { return fmt.Errorf("will not store nil beacon key") } encodableKey := &encodable.RandomBeaconPrivKey{PrivateKey: key} - err := keys.state.db.Update(func(txn *badger.Txn) error { + err := ds.db.Update(func(txn *badger.Txn) error { err := operation.UpsertMyBeaconPrivateKey(epochCounter, encodableKey)(txn) if err != nil { return err @@ -215,6 +195,6 @@ func (keys *EpochRecoveryMyBeaconKey) UpsertMyBeaconPrivateKey(epochCounter uint if err != nil { return fmt.Errorf("could not overwrite beacon key for epoch %d: %w", epochCounter, err) } - keys.state.keyCache.Insert(epochCounter, encodableKey) + ds.keyCache.Insert(epochCounter, encodableKey) return nil } diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index 5643b064d22..5ae78544806 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -60,7 +60,7 @@ func TestDKGState_BeaconKeys(t *testing.T) { assert.True(t, errors.Is(err, storage.ErrNotFound)) }) - // attempt to store a nil key should fail - use DKGState.SetEndState(flow.DKGEndStateNoKey) + // attempt to store a nil key should fail - use RecoverablePrivateBeaconKeyState.SetEndState(flow.DKGEndStateNoKey) t.Run("should fail to store a nil key instead)", func(t *testing.T) { err = store.InsertMyBeaconPrivateKey(epochCounter, nil) assert.Error(t, err) @@ -221,7 +221,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { }) } -// TestSecretDBRequirement tests that the DKGState constructor will return an +// TestSecretDBRequirement tests that the RecoverablePrivateBeaconKeyState constructor will return an // error if instantiated using a database not marked with the correct type. func TestSecretDBRequirement(t *testing.T) { unittest.RunWithBadgerDB(t, func(db *badger.DB) { diff --git a/storage/dkg.go b/storage/dkg.go index 4a13c1b92d3..5f9b573c50a 100644 --- a/storage/dkg.go +++ b/storage/dkg.go @@ -6,72 +6,78 @@ import ( "github.com/onflow/flow-go/model/flow" ) -// DKGState is the storage interface for storing all artifacts and state -// related to the DKG process, including the latest state of a running or -// completed DKG, and computed beacon keys. -type DKGState interface { +// SafeBeaconKeys is a safe way to access beacon keys. +type SafeBeaconKeys interface { - // SetDKGStarted sets the flag indicating the DKG has started for the given epoch. - // Error returns: storage.ErrAlreadyExists - SetDKGStarted(epochCounter uint64) error + // RetrieveMyBeaconPrivateKey retrieves my beacon private key for the given + // epoch, only if my key has been confirmed valid and safe for use. + // + // Returns: + // - (key, true, nil) if the key is present and confirmed valid + // - (nil, false, nil) if the key has been marked invalid or unavailable + // -> no beacon key will ever be available for the epoch in this case + // - (nil, false, storage.ErrNotFound) if the DKG has not ended + // - (nil, false, error) for any unexpected exception + RetrieveMyBeaconPrivateKey(epochCounter uint64) (key crypto.PrivateKey, safe bool, err error) +} + +// DKGStateReader ... +type DKGStateReader interface { + SafeBeaconKeys // GetDKGStarted checks whether the DKG has been started for the given epoch. // No errors expected during normal operation. GetDKGStarted(epochCounter uint64) (bool, error) - // SetDKGEndState stores that the DKG has ended, and its end state. - // Error returns: storage.ErrAlreadyExists - SetDKGEndState(epochCounter uint64, endState flow.DKGEndState) error - // GetDKGEndState retrieves the end state for the given DKG. // Error returns: storage.ErrNotFound GetDKGEndState(epochCounter uint64) (flow.DKGEndState, error) - // InsertMyBeaconPrivateKey stores the random beacon private key for an epoch. + // UnsafeRetrieveMyBeaconPrivateKey retrieves the random beacon private key for an epoch. // // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned + // Error returns: storage.ErrNotFound + UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) +} + +// DKGState is the storage interface for storing all artifacts and state +// related to the DKG process, including the latest state of a running or +// completed DKG, and computed beacon keys. +type DKGState interface { + DKGStateReader + + // SetDKGStarted sets the flag indicating the DKG has started for the given epoch. // Error returns: storage.ErrAlreadyExists - InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error + SetDKGStarted(epochCounter uint64) error + + // SetDKGEndState stores that the DKG has ended, and its end state. + // Error returns: storage.ErrAlreadyExists + SetDKGEndState(epochCounter uint64, endState flow.DKGEndState) error - // RetrieveMyBeaconPrivateKey retrieves the random beacon private key for an epoch. + // InsertMyBeaconPrivateKey stores the random beacon private key for an epoch. // // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned - // Error returns: storage.ErrNotFound - RetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) -} - -// SafeBeaconKeys is a safe way to access beacon keys. -type SafeBeaconKeys interface { - - // RetrieveMyBeaconPrivateKey retrieves my beacon private key for the given - // epoch, only if my key has been confirmed valid and safe for use. - // - // Returns: - // - (key, true, nil) if the key is present and confirmed valid - // - (nil, false, nil) if the key has been marked invalid or unavailable - // -> no beacon key will ever be available for the epoch in this case - // - (nil, false, storage.ErrNotFound) if the DKG has not ended - // - (nil, false, error) for any unexpected exception - RetrieveMyBeaconPrivateKey(epochCounter uint64) (key crypto.PrivateKey, safe bool, err error) + // Error returns: storage.ErrAlreadyExists + InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error } -// EpochRecoveryMyBeaconKey is a specific interface that allows to overwrite the beacon private key for a given epoch. +// EpochRecoveryMyBeaconKey is a specific interface that allows to overwrite the beacon private key for given epoch. // This interface is used *ONLY* in the epoch recovery process and only by the consensus participants. -// Each consensus participant takes part in the DKG, and after successfully finishing the DKG protocol it obtains a -// random beacon private key, which is stored in the database along with DKG end state `flow.DKGEndStateSuccess`. -// If for any reason the DKG fails, then the private key will be nil and DKG end state will be `flow.DKGEndStateDKGFailure`. -// When the epoch recovery takes place, we need to query the last valid beacon private key for the current replica and -// also set it for use during the Recovery Epoch, otherwise replicas won't be able to vote for blocks during the Recovery Epoch. +// Each consensus participant takes part in the DKG, after finishing the DKG protocol each replica obtains a random beacon +// private key which is stored in the database along with DKG end state which will be equal to flow.DKGEndStateSuccess. +// If for any reason DKG fails, then the private key will be nil and DKG end state will be equal to flow.DKGEndStateDKGFailure. +// It's not a problem by itself, but when the epoch recovery takes place, we need to query last valid beacon private key for +// the current replica and set it for recovered epoch, otherwise replicas won't be able to vote for blocks in the recovered epoch. type EpochRecoveryMyBeaconKey interface { - SafeBeaconKeys + DKGStateReader // UpsertMyBeaconPrivateKey overwrites the random beacon private key for the epoch that recovers the protocol from - // Epoch Fallback Mode. Effectively, this function overwrites whatever might be available in the database with - // the given private key and sets the DKGEndState to `flow.DKGEndStateRecovered`. + // epoch fallback mode. Effectively, this function overwrites whatever might be available in the database with + // given private key for current consensus participant. // No errors are expected during normal operations. UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error } From 719b6a61a8357e496e7cdefce1a190959c144138 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 22 Nov 2024 14:04:11 +0200 Subject: [PATCH 02/34] Removed 'DKG started' from storage. --- storage/badger/dkg_state.go | 10 +++++----- storage/badger/operation/dkg.go | 6 +++--- storage/badger/operation/dkg_test.go | 2 +- storage/dkg.go | 12 ++---------- storage/mock/dkg_state.go | 14 +++++++------- 5 files changed, 18 insertions(+), 26 deletions(-) diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index e236e0d7d56..805136b76f9 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -120,13 +120,13 @@ func (ds *RecoverablePrivateBeaconKeyState) GetDKGStarted(epochCounter uint64) ( } // SetDKGEndState stores that the DKG has ended, and its end state. -func (ds *RecoverablePrivateBeaconKeyState) SetDKGEndState(epochCounter uint64, endState flow.DKGEndState) error { +func (ds *RecoverablePrivateBeaconKeyState) SetDKGEndState(epochCounter uint64, endState flow.DKGState) error { return ds.db.Update(operation.InsertDKGEndStateForEpoch(epochCounter, endState)) } // GetDKGEndState retrieves the DKG end state for the epoch. -func (ds *RecoverablePrivateBeaconKeyState) GetDKGEndState(epochCounter uint64) (flow.DKGEndState, error) { - var endState flow.DKGEndState +func (ds *RecoverablePrivateBeaconKeyState) GetDKGEndState(epochCounter uint64) (flow.DKGState, error) { + var endState flow.DKGState err := ds.db.Update(operation.RetrieveDKGEndStateForEpoch(epochCounter, &endState)) return endState, err } @@ -144,7 +144,7 @@ func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCoun err = ds.db.View(func(txn *badger.Txn) error { // retrieve the end state - var endState flow.DKGEndState + var endState flow.DKGState err = operation.RetrieveDKGEndStateForEpoch(epochCounter, &endState)(txn) if err != nil { key = nil @@ -178,7 +178,7 @@ func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCoun // UpsertMyBeaconPrivateKey overwrites the random beacon private key for the epoch that recovers the protocol from // Epoch Fallback Mode. Effectively, this function overwrites whatever might be available in the database with -// the given private key and sets the DKGEndState to `flow.DKGEndStateRecovered`. +// the given private key and sets the DKGState to `flow.DKGEndStateRecovered`. // No errors are expected during normal operations. func (ds *RecoverablePrivateBeaconKeyState) UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { if key == nil { diff --git a/storage/badger/operation/dkg.go b/storage/badger/operation/dkg.go index 10a35355545..d360ce88eaa 100644 --- a/storage/badger/operation/dkg.go +++ b/storage/badger/operation/dkg.go @@ -69,7 +69,7 @@ func RetrieveDKGStartedForEpoch(epochCounter uint64, started *bool) func(*badger // InsertDKGEndStateForEpoch stores the DKG end state for the epoch. // Error returns: storage.ErrAlreadyExists -func InsertDKGEndStateForEpoch(epochCounter uint64, endState flow.DKGEndState) func(*badger.Txn) error { +func InsertDKGEndStateForEpoch(epochCounter uint64, endState flow.DKGState) func(*badger.Txn) error { return insert(makePrefix(codeDKGEnded, epochCounter), endState) } @@ -77,12 +77,12 @@ func InsertDKGEndStateForEpoch(epochCounter uint64, endState flow.DKGEndState) f // the given epoch counter already exists in the database or not. // CAUTION: this method has to be used only in the very specific edge-cases of epoch recovery. For storing the // DKG results obtained on the happy-path, please use method `InsertDKGEndStateForEpoch`. -func UpsertDKGEndStateForEpoch(epochCounter uint64, endState flow.DKGEndState) func(*badger.Txn) error { +func UpsertDKGEndStateForEpoch(epochCounter uint64, endState flow.DKGState) func(*badger.Txn) error { return upsert(makePrefix(codeDKGEnded, epochCounter), endState) } // RetrieveDKGEndStateForEpoch retrieves the DKG end state for the epoch. // Error returns: storage.ErrNotFound -func RetrieveDKGEndStateForEpoch(epochCounter uint64, endState *flow.DKGEndState) func(*badger.Txn) error { +func RetrieveDKGEndStateForEpoch(epochCounter uint64, endState *flow.DKGState) func(*badger.Txn) error { return retrieve(makePrefix(codeDKGEnded, epochCounter), endState) } diff --git a/storage/badger/operation/dkg_test.go b/storage/badger/operation/dkg_test.go index 03417e963f6..dcc2afa9711 100644 --- a/storage/badger/operation/dkg_test.go +++ b/storage/badger/operation/dkg_test.go @@ -88,7 +88,7 @@ func TestDKGEndStateForEpoch(t *testing.T) { assert.NoError(t, err) // should be able to read end state - var readEndState flow.DKGEndState + var readEndState flow.DKGState err = db.View(RetrieveDKGEndStateForEpoch(epochCounter, &readEndState)) assert.NoError(t, err) assert.Equal(t, endState, readEndState) diff --git a/storage/dkg.go b/storage/dkg.go index 5f9b573c50a..4f71dec8354 100644 --- a/storage/dkg.go +++ b/storage/dkg.go @@ -25,13 +25,9 @@ type SafeBeaconKeys interface { type DKGStateReader interface { SafeBeaconKeys - // GetDKGStarted checks whether the DKG has been started for the given epoch. - // No errors expected during normal operation. - GetDKGStarted(epochCounter uint64) (bool, error) - // GetDKGEndState retrieves the end state for the given DKG. // Error returns: storage.ErrNotFound - GetDKGEndState(epochCounter uint64) (flow.DKGEndState, error) + GetDKGEndState(epochCounter uint64) (flow.DKGState, error) // UnsafeRetrieveMyBeaconPrivateKey retrieves the random beacon private key for an epoch. // @@ -48,13 +44,9 @@ type DKGStateReader interface { type DKGState interface { DKGStateReader - // SetDKGStarted sets the flag indicating the DKG has started for the given epoch. - // Error returns: storage.ErrAlreadyExists - SetDKGStarted(epochCounter uint64) error - // SetDKGEndState stores that the DKG has ended, and its end state. // Error returns: storage.ErrAlreadyExists - SetDKGEndState(epochCounter uint64, endState flow.DKGEndState) error + SetDKGEndState(epochCounter uint64, endState flow.DKGState) error // InsertMyBeaconPrivateKey stores the random beacon private key for an epoch. // diff --git a/storage/mock/dkg_state.go b/storage/mock/dkg_state.go index 10451250fbf..73ee7b9b48e 100644 --- a/storage/mock/dkg_state.go +++ b/storage/mock/dkg_state.go @@ -15,22 +15,22 @@ type DKGState struct { } // GetDKGEndState provides a mock function with given fields: epochCounter -func (_m *DKGState) GetDKGEndState(epochCounter uint64) (flow.DKGEndState, error) { +func (_m *DKGState) GetDKGEndState(epochCounter uint64) (flow.DKGState, error) { ret := _m.Called(epochCounter) if len(ret) == 0 { panic("no return value specified for GetDKGEndState") } - var r0 flow.DKGEndState + var r0 flow.DKGState var r1 error - if rf, ok := ret.Get(0).(func(uint64) (flow.DKGEndState, error)); ok { + if rf, ok := ret.Get(0).(func(uint64) (flow.DKGState, error)); ok { return rf(epochCounter) } - if rf, ok := ret.Get(0).(func(uint64) flow.DKGEndState); ok { + if rf, ok := ret.Get(0).(func(uint64) flow.DKGState); ok { r0 = rf(epochCounter) } else { - r0 = ret.Get(0).(flow.DKGEndState) + r0 = ret.Get(0).(flow.DKGState) } if rf, ok := ret.Get(1).(func(uint64) error); ok { @@ -119,7 +119,7 @@ func (_m *DKGState) RetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.Priv } // SetDKGEndState provides a mock function with given fields: epochCounter, endState -func (_m *DKGState) SetDKGEndState(epochCounter uint64, endState flow.DKGEndState) error { +func (_m *DKGState) SetDKGEndState(epochCounter uint64, endState flow.DKGState) error { ret := _m.Called(epochCounter, endState) if len(ret) == 0 { @@ -127,7 +127,7 @@ func (_m *DKGState) SetDKGEndState(epochCounter uint64, endState flow.DKGEndStat } var r0 error - if rf, ok := ret.Get(0).(func(uint64, flow.DKGEndState) error); ok { + if rf, ok := ret.Get(0).(func(uint64, flow.DKGState) error); ok { r0 = rf(epochCounter, endState) } else { r0 = ret.Error(0) From 01bbdcf9ab112b2fc106c09ea685b77734e12cfb Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 22 Nov 2024 14:04:51 +0200 Subject: [PATCH 03/34] Updated DKG states to have extra states. They no more represent the end state but rather current --- model/flow/dkg.go | 52 +++++++++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/model/flow/dkg.go b/model/flow/dkg.go index f4ba41ab267..0548185304a 100644 --- a/model/flow/dkg.go +++ b/model/flow/dkg.go @@ -1,38 +1,46 @@ package flow -// DKGEndState captures the final state of a completed DKG. -type DKGEndState uint32 +// DKGState captures the final state of a completed DKG. +type DKGState uint32 const ( - // DKGEndStateUnknown - zero value for this enum, indicates unset value - DKGEndStateUnknown DKGEndState = iota - // DKGEndStateSuccess - the DKG completed, this node has a valid beacon key. - DKGEndStateSuccess - // DKGEndStateInconsistentKey - the DKG completed, this node has an invalid beacon key. - DKGEndStateInconsistentKey - // DKGEndStateNoKey - this node did not store a key, typically caused by a crash mid-DKG. - DKGEndStateNoKey - // DKGEndStateDKGFailure - the underlying DKG library reported an error. - DKGEndStateDKGFailure + // DKGStateUnknown - zero value for this enum, indicates unset value + DKGStateUnknown DKGState = iota + // DKGStateSuccess - the DKG completed, this node has a valid beacon key. + DKGStateStarted + // DKGStateCompleted + DKGStateCompleted + // DKGStateSuccess + DKGStateSuccess + // DKGStateInconsistentKey - the DKG completed, this node has an invalid beacon key. + DKGStateInconsistentKey + // DKGStateNoKey - this node did not store a key, typically caused by a crash mid-DKG. + DKGStateNoKey + // DKGStateDKGFailure - the underlying DKG library reported an error. + DKGStateDKGFailure // RandomBeaconKeyRecovered - this node has recovered its beacon key from a previous epoch. // This occurs only for epochs which are entered through the EFM Recovery process (`flow.EpochRecover` service event). RandomBeaconKeyRecovered ) -func (state DKGEndState) String() string { +func (state DKGState) String() string { switch state { - case DKGEndStateSuccess: - return "DKGEndStateSuccess" - case DKGEndStateInconsistentKey: - return "DKGEndStateInconsistentKey" - case DKGEndStateNoKey: - return "DKGEndStateNoKey" - case DKGEndStateDKGFailure: - return "DKGEndStateDKGFailure" + case DKGStateStarted: + return "DKGStateStarted" + case DKGStateCompleted: + return "DKGStateCompleted" + case DKGStateSuccess: + return "DKGStateSuccess" + case DKGStateInconsistentKey: + return "DKGStateInconsistentKey" + case DKGStateNoKey: + return "DKGStateNoKey" + case DKGStateDKGFailure: + return "DKGStateDKGFailure" case RandomBeaconKeyRecovered: return "RandomBeaconKeyRecovered" default: - return "DKGEndStateUnknown" + return "DKGStateUnknown" } } From 1d3c6a4766989dd3f50082e6c91bb2e3af8269f2 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 22 Nov 2024 14:08:04 +0200 Subject: [PATCH 04/34] Updated usages of DKG storage in reactor engine --- engine/consensus/dkg/reactor_engine.go | 35 +++++++++++---------- engine/consensus/dkg/reactor_engine_test.go | 6 ++-- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/engine/consensus/dkg/reactor_engine.go b/engine/consensus/dkg/reactor_engine.go index daedd8bac56..98ad2dc4256 100644 --- a/engine/consensus/dkg/reactor_engine.go +++ b/engine/consensus/dkg/reactor_engine.go @@ -154,24 +154,27 @@ func (e *ReactorEngine) startDKGForEpoch(currentEpochCounter uint64, first *flow Hex("first_block_id", firstID[:]). // id of first block in EpochSetup phase Logger() - // if we have started the dkg for this epoch already, exit - started, err := e.dkgState.GetDKGStarted(nextEpochCounter) + // if we have dkgState the dkg for this epoch already, exit + dkgState, err := e.dkgState.GetDKGEndState(nextEpochCounter) if err != nil { - // unexpected storage-level error - // TODO use irrecoverable context - log.Fatal().Err(err).Msg("could not check whether DKG is started") - } - if started { - log.Warn().Msg("DKG started before, skipping starting the DKG for this epoch") + if !errors.Is(err, storage.ErrNotFound) { + // unexpected storage-level error + // TODO use irrecoverable context + log.Fatal().Err(err).Msg("could not check whether DKG is dkgState") + } + + // there is no dkgState for this epoch, continue + } else { + log.Warn().Msgf("DKG started before, skipping starting the DKG for this epoch, current state: %s", dkgState) return } // flag that we are starting the dkg for this epoch - err = e.dkgState.SetDKGStarted(nextEpochCounter) + err = e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGStateStarted) if err != nil { // unexpected storage-level error // TODO use irrecoverable context - log.Fatal().Err(err).Msg("could not set dkg started") + log.Fatal().Err(err).Msg("could not set dkg dkgState") } curDKGInfo, err := e.getDKGInfo(firstID) @@ -289,10 +292,10 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin return } - myBeaconPrivKey, err := e.dkgState.RetrieveMyBeaconPrivateKey(nextEpochCounter) + myBeaconPrivKey, err := e.dkgState.UnsafeRetrieveMyBeaconPrivateKey(nextEpochCounter) if errors.Is(err, storage.ErrNotFound) { log.Warn().Msg("checking beacon key consistency: no key found") - err := e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGEndStateNoKey) + err := e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGStateNoKey) if err != nil { // TODO use irrecoverable context log.Fatal().Err(err).Msg("failed to set dkg end state") @@ -319,7 +322,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin Str("computed_beacon_pub_key", localPubKey.String()). Str("canonical_beacon_pub_key", nextDKGPubKey.String()). Msg("checking beacon key consistency: locally computed beacon public key does not match beacon public key for next epoch") - err := e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGEndStateInconsistentKey) + err := e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGStateNoKey) if err != nil { // TODO use irrecoverable context log.Fatal().Err(err).Msg("failed to set dkg end state") @@ -327,7 +330,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin return } - err = e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGEndStateSuccess) + err = e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGStateSuccess) if err != nil { // TODO use irrecoverable context e.log.Fatal().Err(err).Msg("failed to set dkg end state") @@ -427,10 +430,10 @@ func (e *ReactorEngine) end(nextEpochCounter uint64) func() error { // has already abandoned the happy path, because on the happy path the ReactorEngine is the only writer. // Then this function just stops and returns without error. e.log.Warn().Err(err).Msgf("node %s with index %d failed DKG locally", e.me.NodeID(), e.controller.GetIndex()) - err := e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGEndStateDKGFailure) + err := e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGStateDKGFailure) if err != nil { if errors.Is(err, storage.ErrAlreadyExists) { - return nil // DKGEndState already being set is expected in case of epoch recovery + return nil // DKGState already being set is expected in case of epoch recovery } return fmt.Errorf("failed to set dkg end state following dkg end error: %w", err) } diff --git a/engine/consensus/dkg/reactor_engine_test.go b/engine/consensus/dkg/reactor_engine_test.go index 7a0a1917c4c..92a6b1276d1 100644 --- a/engine/consensus/dkg/reactor_engine_test.go +++ b/engine/consensus/dkg/reactor_engine_test.go @@ -266,7 +266,7 @@ type ReactorEngineSuite_CommittedPhase struct { epochCounter uint64 // current epoch counter myLocalBeaconKey crypto.PrivateKey // my locally computed beacon key myGlobalBeaconPubKey crypto.PublicKey // my public key, as dictated by global DKG - dkgEndState flow.DKGEndState // backend for DGKState. + dkgEndState flow.DKGState // backend for DGKState. firstBlock *flow.Header // first block of EpochCommitted phase warnsLogged int // count # of warn-level logs @@ -313,12 +313,12 @@ func (suite *ReactorEngineSuite_CommittedPhase) SetupTest() { suite.dkgState.On("SetDKGEndState", suite.NextEpochCounter(), mock.Anything). Run(func(args mock.Arguments) { assert.Equal(suite.T(), flow.DKGEndStateUnknown, suite.dkgEndState) // must be unset - endState := args[1].(flow.DKGEndState) + endState := args[1].(flow.DKGState) suite.dkgEndState = endState }). Return(nil) suite.dkgState.On("GetDKGEndState", suite.NextEpochCounter()).Return( - func(_ uint64) flow.DKGEndState { return suite.dkgEndState }, + func(_ uint64) flow.DKGState { return suite.dkgEndState }, func(_ uint64) error { if suite.dkgEndState == flow.DKGEndStateUnknown { return storerr.ErrNotFound From 6c3488645c5544673da2e2299a121ca0a8d621f1 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 22 Nov 2024 14:18:09 +0200 Subject: [PATCH 05/34] Added back GetDKGStarted for easier usage in reactor engine. Updated internals of state machine --- engine/consensus/dkg/reactor_engine.go | 16 +++++++--------- storage/badger/dkg_state.go | 15 ++++++++------- storage/badger/operation/dkg.go | 12 ++++-------- storage/badger/operation/prefix.go | 2 +- storage/dkg.go | 4 ++++ 5 files changed, 24 insertions(+), 25 deletions(-) diff --git a/engine/consensus/dkg/reactor_engine.go b/engine/consensus/dkg/reactor_engine.go index 98ad2dc4256..9507b050f79 100644 --- a/engine/consensus/dkg/reactor_engine.go +++ b/engine/consensus/dkg/reactor_engine.go @@ -155,17 +155,15 @@ func (e *ReactorEngine) startDKGForEpoch(currentEpochCounter uint64, first *flow Logger() // if we have dkgState the dkg for this epoch already, exit - dkgState, err := e.dkgState.GetDKGEndState(nextEpochCounter) + started, err := e.dkgState.GetDKGStarted(nextEpochCounter) if err != nil { - if !errors.Is(err, storage.ErrNotFound) { - // unexpected storage-level error - // TODO use irrecoverable context - log.Fatal().Err(err).Msg("could not check whether DKG is dkgState") - } + // unexpected storage-level error + // TODO use irrecoverable context + log.Fatal().Err(err).Msg("could not check whether DKG is dkgState") - // there is no dkgState for this epoch, continue - } else { - log.Warn().Msgf("DKG started before, skipping starting the DKG for this epoch, current state: %s", dkgState) + } + if started { + log.Warn().Msg("DKG started before, skipping starting the DKG for this epoch") return } diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index 805136b76f9..caf586a5465 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -89,7 +89,13 @@ func (ds *RecoverablePrivateBeaconKeyState) InsertMyBeaconPrivateKey(epochCounte return fmt.Errorf("will not store nil beacon key") } encodableKey := &encodable.RandomBeaconPrivKey{PrivateKey: key} - return operation.RetryOnConflictTx(ds.db, transaction.Update, ds.storeKeyTx(epochCounter, encodableKey)) + return operation.RetryOnConflictTx(ds.db, transaction.Update, func(tx *transaction.Tx) error { + err := ds.storeKeyTx(epochCounter, encodableKey)(tx) + if err != nil { + return err + } + return operation.InsertDKGEndStateForEpoch(epochCounter, flow.DKGStateCompleted)(tx.DBTxn) + }) } // UnsafeRetrieveMyBeaconPrivateKey retrieves the random beacon private key for an epoch. @@ -107,11 +113,6 @@ func (ds *RecoverablePrivateBeaconKeyState) UnsafeRetrieveMyBeaconPrivateKey(epo return encodableKey.PrivateKey, nil } -// SetDKGStarted sets the flag indicating the DKG has started for the given epoch. -func (ds *RecoverablePrivateBeaconKeyState) SetDKGStarted(epochCounter uint64) error { - return ds.db.Update(operation.InsertDKGStartedForEpoch(epochCounter)) -} - // GetDKGStarted checks whether the DKG has been started for the given epoch. func (ds *RecoverablePrivateBeaconKeyState) GetDKGStarted(epochCounter uint64) (bool, error) { var started bool @@ -153,7 +154,7 @@ func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCoun } // for any end state besides success and recovery, the key is not safe - if endState == flow.DKGEndStateSuccess || endState == flow.RandomBeaconKeyRecovered { + if endState == flow.DKGStateSuccess || endState == flow.RandomBeaconKeyRecovered { // retrieve the key - any storage error (including `storage.ErrNotFound`) is an exception var encodableKey *encodable.RandomBeaconPrivKey encodableKey, err = ds.retrieveKeyTx(epochCounter)(txn) diff --git a/storage/badger/operation/dkg.go b/storage/badger/operation/dkg.go index d360ce88eaa..8dc79272385 100644 --- a/storage/badger/operation/dkg.go +++ b/storage/badger/operation/dkg.go @@ -41,19 +41,13 @@ func RetrieveMyBeaconPrivateKey(epochCounter uint64, info *encodable.RandomBeaco return retrieve(makePrefix(codeBeaconPrivateKey, epochCounter), info) } -// InsertDKGStartedForEpoch stores a flag indicating that the DKG has been started for the given epoch. -// Returns: storage.ErrAlreadyExists -// Error returns: storage.ErrAlreadyExists -func InsertDKGStartedForEpoch(epochCounter uint64) func(*badger.Txn) error { - return insert(makePrefix(codeDKGStarted, epochCounter), true) -} - // RetrieveDKGStartedForEpoch retrieves the DKG started flag for the given epoch. // If no flag is set, started is set to false and no error is returned. // No errors expected during normal operation. func RetrieveDKGStartedForEpoch(epochCounter uint64, started *bool) func(*badger.Txn) error { return func(tx *badger.Txn) error { - err := retrieve(makePrefix(codeDKGStarted, epochCounter), started)(tx) + var state flow.DKGState + err := RetrieveDKGEndStateForEpoch(epochCounter, &state)(tx) if errors.Is(err, storage.ErrNotFound) { // flag not set - therefore DKG not started *started = false @@ -62,6 +56,8 @@ func RetrieveDKGStartedForEpoch(epochCounter uint64, started *bool) func(*badger // storage error - set started to zero value *started = false return err + } else { + *started = true } return nil } diff --git a/storage/badger/operation/prefix.go b/storage/badger/operation/prefix.go index 6170cad34ec..7bbe97c1f00 100644 --- a/storage/badger/operation/prefix.go +++ b/storage/badger/operation/prefix.go @@ -72,7 +72,7 @@ const ( codeEpochSetup = 61 // EpochSetup service event, keyed by ID codeEpochCommit = 62 // EpochCommit service event, keyed by ID codeBeaconPrivateKey = 63 // BeaconPrivateKey, keyed by epoch counter - codeDKGStarted = 64 // flag that the DKG for an epoch has been started + _ = 64 // [DEPRECATED] flag that the DKG for an epoch has been started codeDKGEnded = 65 // flag that the DKG for an epoch has ended (stores end state) codeVersionBeacon = 67 // flag for storing version beacons codeEpochProtocolState = 68 diff --git a/storage/dkg.go b/storage/dkg.go index 4f71dec8354..cca28b33502 100644 --- a/storage/dkg.go +++ b/storage/dkg.go @@ -48,6 +48,10 @@ type DKGState interface { // Error returns: storage.ErrAlreadyExists SetDKGEndState(epochCounter uint64, endState flow.DKGState) error + // GetDKGStarted checks whether the DKG has been started for the given epoch. + // No errors expected during normal operation. + GetDKGStarted(epochCounter uint64) (bool, error) + // InsertMyBeaconPrivateKey stores the random beacon private key for an epoch. // // CAUTION: these keys are stored before they are validated against the From 8aac526b16c1f70f6410ad30120d579a3cd2e244 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 22 Nov 2024 16:32:48 +0200 Subject: [PATCH 06/34] Implemented allowed state transitions for recoverable random beacon state machine --- engine/consensus/dkg/reactor_engine.go | 1 - storage/badger/dkg_state.go | 40 ++++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/engine/consensus/dkg/reactor_engine.go b/engine/consensus/dkg/reactor_engine.go index 9507b050f79..bd3f7c372b6 100644 --- a/engine/consensus/dkg/reactor_engine.go +++ b/engine/consensus/dkg/reactor_engine.go @@ -160,7 +160,6 @@ func (e *ReactorEngine) startDKGForEpoch(currentEpochCounter uint64, first *flow // unexpected storage-level error // TODO use irrecoverable context log.Fatal().Err(err).Msg("could not check whether DKG is dkgState") - } if started { log.Warn().Msg("DKG started before, skipping starting the DKG for this epoch") diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index caf586a5465..6e987650aa2 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -2,6 +2,7 @@ package badger import ( "fmt" + "golang.org/x/exp/slices" "github.com/dgraph-io/badger/v2" "github.com/onflow/crypto" @@ -16,6 +17,16 @@ import ( "github.com/onflow/flow-go/storage/badger/transaction" ) +var allowedStateTransitions = map[flow.DKGState][]flow.DKGState{ + flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateNoKey, flow.DKGStateDKGFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, + flow.DKGStateCompleted: {flow.DKGStateSuccess, flow.DKGStateNoKey, flow.DKGStateDKGFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, + flow.DKGStateSuccess: {}, + flow.RandomBeaconKeyRecovered: {}, + flow.DKGStateDKGFailure: {flow.RandomBeaconKeyRecovered}, + flow.DKGStateInconsistentKey: {flow.RandomBeaconKeyRecovered}, + flow.DKGStateNoKey: {flow.RandomBeaconKeyRecovered}, +} + // RecoverablePrivateBeaconKeyState stores state information about in-progress and completed DKGs, including // computed keys. Must be instantiated using secrets database. // RecoverablePrivateBeaconKeyState is a specific module that allows to overwrite the beacon private key for a given epoch. @@ -94,7 +105,7 @@ func (ds *RecoverablePrivateBeaconKeyState) InsertMyBeaconPrivateKey(epochCounte if err != nil { return err } - return operation.InsertDKGEndStateForEpoch(epochCounter, flow.DKGStateCompleted)(tx.DBTxn) + return ds.processStateTransition(epochCounter, flow.DKGStateCompleted)(tx) }) } @@ -122,7 +133,32 @@ func (ds *RecoverablePrivateBeaconKeyState) GetDKGStarted(epochCounter uint64) ( // SetDKGEndState stores that the DKG has ended, and its end state. func (ds *RecoverablePrivateBeaconKeyState) SetDKGEndState(epochCounter uint64, endState flow.DKGState) error { - return ds.db.Update(operation.InsertDKGEndStateForEpoch(epochCounter, endState)) + return operation.RetryOnConflictTx(ds.db, transaction.Update, ds.processStateTransition(epochCounter, endState)) +} + +func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter uint64, newState flow.DKGState) func(*transaction.Tx) error { + return func(tx *transaction.Tx) error { + var currentState flow.DKGState + err := operation.RetrieveDKGEndStateForEpoch(epochCounter, ¤tState)(tx.DBTxn) + if err != nil { + return fmt.Errorf("could not retrieve current state for epoch %d: %w", epochCounter, err) + } + + allowedStates := allowedStateTransitions[currentState] + if slices.Index(allowedStates, newState) < 0 { + return fmt.Errorf("invalid state transition from %s to %s", currentState, newState) + } + + // ensure invariant holds and we still have a valid private key stored + if newState == flow.DKGStateSuccess { + _, err = ds.retrieveKeyTx(epochCounter)(tx.DBTxn) + if err != nil { + return fmt.Errorf("cannot transition to flow.DKGStateSuccess without a valid random beacon key: %w", err) + } + } + + return operation.InsertDKGEndStateForEpoch(epochCounter, newState)(tx.DBTxn) + } } // GetDKGEndState retrieves the DKG end state for the epoch. From 064b6514c1645395680e1e55f100c38af4191619 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 22 Nov 2024 16:38:43 +0200 Subject: [PATCH 07/34] Fixed unit test compilation. Updated allowed state transitions --- storage/badger/dkg_state.go | 8 +++++++- storage/badger/dkg_state_test.go | 35 ++++++++++++++++---------------- 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index 6e987650aa2..74640c020f8 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -1,6 +1,7 @@ package badger import ( + "errors" "fmt" "golang.org/x/exp/slices" @@ -25,6 +26,7 @@ var allowedStateTransitions = map[flow.DKGState][]flow.DKGState{ flow.DKGStateDKGFailure: {flow.RandomBeaconKeyRecovered}, flow.DKGStateInconsistentKey: {flow.RandomBeaconKeyRecovered}, flow.DKGStateNoKey: {flow.RandomBeaconKeyRecovered}, + flow.DKGStateUnknown: {flow.DKGStateStarted, flow.DKGStateNoKey, flow.DKGStateDKGFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, } // RecoverablePrivateBeaconKeyState stores state information about in-progress and completed DKGs, including @@ -141,7 +143,11 @@ func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter var currentState flow.DKGState err := operation.RetrieveDKGEndStateForEpoch(epochCounter, ¤tState)(tx.DBTxn) if err != nil { - return fmt.Errorf("could not retrieve current state for epoch %d: %w", epochCounter, err) + if errors.Is(err, storage.ErrNotFound) { + currentState = flow.DKGStateUnknown + } else { + return fmt.Errorf("could not retrieve current state for epoch %d: %w", epochCounter, err) + } } allowedStates := allowedStateTransitions[currentState] diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index 5ae78544806..ef034ed3d16 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -33,7 +33,7 @@ func TestDKGState_DKGStarted(t *testing.T) { // store dkg-started flag for epoch t.Run("should be able to set DKGStarted", func(t *testing.T) { - err = store.SetDKGStarted(epochCounter) + err = store.SetDKGEndState(epochCounter, flow.DKGStateStarted) assert.NoError(t, err) }) @@ -56,11 +56,11 @@ func TestDKGState_BeaconKeys(t *testing.T) { // attempt to get a non-existent key t.Run("should error if retrieving non-existent key", func(t *testing.T) { - _, err = store.RetrieveMyBeaconPrivateKey(epochCounter) + _, err = store.UnsafeRetrieveMyBeaconPrivateKey(epochCounter) assert.True(t, errors.Is(err, storage.ErrNotFound)) }) - // attempt to store a nil key should fail - use RecoverablePrivateBeaconKeyState.SetEndState(flow.DKGEndStateNoKey) + // attempt to store a nil key should fail - use RecoverablePrivateBeaconKeyState.SetEndState(flow.DKGStateNoKey) t.Run("should fail to store a nil key instead)", func(t *testing.T) { err = store.InsertMyBeaconPrivateKey(epochCounter, nil) assert.Error(t, err) @@ -75,7 +75,7 @@ func TestDKGState_BeaconKeys(t *testing.T) { // retrieve the key by epoch counter t.Run("should be able to retrieve stored key", func(t *testing.T) { - actual, err := store.RetrieveMyBeaconPrivateKey(epochCounter) + actual, err := store.UnsafeRetrieveMyBeaconPrivateKey(epochCounter) require.NoError(t, err) assert.Equal(t, expected, actual) }) @@ -95,7 +95,7 @@ func TestDKGState_EndState(t *testing.T) { require.NoError(t, err) epochCounter := rand.Uint64() - endState := flow.DKGEndStateNoKey + endState := flow.DKGStateNoKey t.Run("should be able to store an end state", func(t *testing.T) { err = store.SetDKGEndState(epochCounter, endState) @@ -115,11 +115,10 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { metrics := metrics.NewNoopCollector() dkgState, err := bstorage.NewDKGState(metrics, db) require.NoError(t, err) - safeKeys := bstorage.NewSafeBeaconPrivateKeys(dkgState) t.Run("non-existent key -> should return ErrNotFound", func(t *testing.T) { epochCounter := rand.Uint64() - key, safe, err := safeKeys.RetrieveMyBeaconPrivateKey(epochCounter) + key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) assert.Nil(t, key) assert.False(t, safe) assert.ErrorIs(t, err, storage.ErrNotFound) @@ -133,7 +132,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) assert.NoError(t, err) - key, safe, err := safeKeys.RetrieveMyBeaconPrivateKey(epochCounter) + key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) assert.Nil(t, key) assert.False(t, safe) assert.ErrorIs(t, err, storage.ErrNotFound) @@ -147,10 +146,10 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) assert.NoError(t, err) // mark dkg unsuccessful - err = dkgState.SetDKGEndState(epochCounter, flow.DKGEndStateInconsistentKey) + err = dkgState.SetDKGEndState(epochCounter, flow.DKGStateInconsistentKey) assert.NoError(t, err) - key, safe, err := safeKeys.RetrieveMyBeaconPrivateKey(epochCounter) + key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) assert.Nil(t, key) assert.False(t, safe) assert.NoError(t, err) @@ -164,10 +163,10 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) assert.NoError(t, err) // mark dkg result as inconsistent - err = dkgState.SetDKGEndState(epochCounter, flow.DKGEndStateInconsistentKey) + err = dkgState.SetDKGEndState(epochCounter, flow.DKGStateInconsistentKey) assert.NoError(t, err) - key, safe, err := safeKeys.RetrieveMyBeaconPrivateKey(epochCounter) + key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) assert.Nil(t, key) assert.False(t, safe) assert.NoError(t, err) @@ -177,10 +176,10 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { epochCounter := rand.Uint64() // mark dkg result as no key - err = dkgState.SetDKGEndState(epochCounter, flow.DKGEndStateNoKey) + err = dkgState.SetDKGEndState(epochCounter, flow.DKGStateNoKey) assert.NoError(t, err) - key, safe, err := safeKeys.RetrieveMyBeaconPrivateKey(epochCounter) + key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) assert.Nil(t, key) assert.False(t, safe) assert.NoError(t, err) @@ -194,10 +193,10 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) assert.NoError(t, err) // mark dkg successful - err = dkgState.SetDKGEndState(epochCounter, flow.DKGEndStateSuccess) + err = dkgState.SetDKGEndState(epochCounter, flow.DKGStateSuccess) assert.NoError(t, err) - key, safe, err := safeKeys.RetrieveMyBeaconPrivateKey(epochCounter) + key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) assert.NotNil(t, key) assert.True(t, expected.Equals(key)) assert.True(t, safe) @@ -208,10 +207,10 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { epochCounter := rand.Uint64() // mark dkg successful - err = dkgState.SetDKGEndState(epochCounter, flow.DKGEndStateSuccess) + err = dkgState.SetDKGEndState(epochCounter, flow.DKGStateSuccess) assert.NoError(t, err) - key, safe, err := safeKeys.RetrieveMyBeaconPrivateKey(epochCounter) + key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) assert.Nil(t, key) assert.False(t, safe) assert.Error(t, err) From 6bcaf38b2324bdb0b0524ba05b4a84c71e82cd15 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 22 Nov 2024 16:40:06 +0200 Subject: [PATCH 08/34] Renamed interface methods --- cmd/consensus/main.go | 2 +- engine/consensus/dkg/reactor_engine.go | 12 ++++++------ engine/consensus/dkg/reactor_engine_test.go | 12 ++++++------ storage/badger/dkg_state.go | 6 +++--- storage/badger/dkg_state_test.go | 16 ++++++++-------- storage/dkg.go | 4 ++-- storage/mock/dkg_state.go | 4 ++-- 7 files changed, 28 insertions(+), 28 deletions(-) diff --git a/cmd/consensus/main.go b/cmd/consensus/main.go index 091c7e4b889..8155c904734 100644 --- a/cmd/consensus/main.go +++ b/cmd/consensus/main.go @@ -350,7 +350,7 @@ func main() { return err } // mark the root DKG as successful, so it is considered safe to use the key - err = dkgState.SetDKGEndState(epochCounter, flow.DKGEndStateSuccess) + err = dkgState.SetDKGState(epochCounter, flow.DKGEndStateSuccess) if err != nil && !errors.Is(err, storage.ErrAlreadyExists) { return err } diff --git a/engine/consensus/dkg/reactor_engine.go b/engine/consensus/dkg/reactor_engine.go index bd3f7c372b6..9a4355b9d6e 100644 --- a/engine/consensus/dkg/reactor_engine.go +++ b/engine/consensus/dkg/reactor_engine.go @@ -167,7 +167,7 @@ func (e *ReactorEngine) startDKGForEpoch(currentEpochCounter uint64, first *flow } // flag that we are starting the dkg for this epoch - err = e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGStateStarted) + err = e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateStarted) if err != nil { // unexpected storage-level error // TODO use irrecoverable context @@ -271,7 +271,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin // This can happen if the DKG failed locally, if we failed to generate // a local private beacon key, or if we crashed while performing this // check previously. - endState, err := e.dkgState.GetDKGEndState(nextEpochCounter) + endState, err := e.dkgState.GetDKGState(nextEpochCounter) if err == nil { log.Warn().Msgf("checking beacon key consistency: exiting because dkg end state was already set: %s", endState.String()) return @@ -292,7 +292,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin myBeaconPrivKey, err := e.dkgState.UnsafeRetrieveMyBeaconPrivateKey(nextEpochCounter) if errors.Is(err, storage.ErrNotFound) { log.Warn().Msg("checking beacon key consistency: no key found") - err := e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGStateNoKey) + err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateNoKey) if err != nil { // TODO use irrecoverable context log.Fatal().Err(err).Msg("failed to set dkg end state") @@ -319,7 +319,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin Str("computed_beacon_pub_key", localPubKey.String()). Str("canonical_beacon_pub_key", nextDKGPubKey.String()). Msg("checking beacon key consistency: locally computed beacon public key does not match beacon public key for next epoch") - err := e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGStateNoKey) + err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateNoKey) if err != nil { // TODO use irrecoverable context log.Fatal().Err(err).Msg("failed to set dkg end state") @@ -327,7 +327,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin return } - err = e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGStateSuccess) + err = e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateSuccess) if err != nil { // TODO use irrecoverable context e.log.Fatal().Err(err).Msg("failed to set dkg end state") @@ -427,7 +427,7 @@ func (e *ReactorEngine) end(nextEpochCounter uint64) func() error { // has already abandoned the happy path, because on the happy path the ReactorEngine is the only writer. // Then this function just stops and returns without error. e.log.Warn().Err(err).Msgf("node %s with index %d failed DKG locally", e.me.NodeID(), e.controller.GetIndex()) - err := e.dkgState.SetDKGEndState(nextEpochCounter, flow.DKGStateDKGFailure) + err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateDKGFailure) if err != nil { if errors.Is(err, storage.ErrAlreadyExists) { return nil // DKGState already being set is expected in case of epoch recovery diff --git a/engine/consensus/dkg/reactor_engine_test.go b/engine/consensus/dkg/reactor_engine_test.go index 92a6b1276d1..375bb824f4f 100644 --- a/engine/consensus/dkg/reactor_engine_test.go +++ b/engine/consensus/dkg/reactor_engine_test.go @@ -310,14 +310,14 @@ func (suite *ReactorEngineSuite_CommittedPhase) SetupTest() { return nil }, ) - suite.dkgState.On("SetDKGEndState", suite.NextEpochCounter(), mock.Anything). + suite.dkgState.On("SetDKGState", suite.NextEpochCounter(), mock.Anything). Run(func(args mock.Arguments) { assert.Equal(suite.T(), flow.DKGEndStateUnknown, suite.dkgEndState) // must be unset endState := args[1].(flow.DKGState) suite.dkgEndState = endState }). Return(nil) - suite.dkgState.On("GetDKGEndState", suite.NextEpochCounter()).Return( + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return( func(_ uint64) flow.DKGState { return suite.dkgEndState }, func(_ uint64) error { if suite.dkgEndState == flow.DKGEndStateUnknown { @@ -438,7 +438,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_DKGS suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started but not ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGEndState", suite.NextEpochCounter()).Return(flow.DKGEndStateUnknown, storerr.ErrNotFound).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGEndStateUnknown, storerr.ErrNotFound).Once() // start up the engine unittest.AssertClosesBefore(suite.T(), suite.engine.Ready(), time.Second) @@ -461,7 +461,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_DKGE suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started and ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGEndState", suite.NextEpochCounter()).Return(flow.DKGEndStateNoKey, nil).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGEndStateNoKey, nil).Once() // start up the engine unittest.AssertClosesBefore(suite.T(), suite.engine.Ready(), time.Second) @@ -482,7 +482,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_Inco suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started but not ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGEndState", suite.NextEpochCounter()).Return(flow.DKGEndStateUnknown, storerr.ErrNotFound).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGEndStateUnknown, storerr.ErrNotFound).Once() // set our global pub key to a random value suite.myGlobalBeaconPubKey = unittest.RandomBeaconPriv().PublicKey() @@ -508,7 +508,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_Miss suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started but not ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGEndState", suite.NextEpochCounter()).Return(flow.DKGEndStateUnknown, storerr.ErrNotFound).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGEndStateUnknown, storerr.ErrNotFound).Once() // remove our key suite.myLocalBeaconKey = nil diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index 74640c020f8..3644636aefc 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -134,8 +134,8 @@ func (ds *RecoverablePrivateBeaconKeyState) GetDKGStarted(epochCounter uint64) ( } // SetDKGEndState stores that the DKG has ended, and its end state. -func (ds *RecoverablePrivateBeaconKeyState) SetDKGEndState(epochCounter uint64, endState flow.DKGState) error { - return operation.RetryOnConflictTx(ds.db, transaction.Update, ds.processStateTransition(epochCounter, endState)) +func (ds *RecoverablePrivateBeaconKeyState) SetDKGState(epochCounter uint64, newState flow.DKGState) error { + return operation.RetryOnConflictTx(ds.db, transaction.Update, ds.processStateTransition(epochCounter, newState)) } func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter uint64, newState flow.DKGState) func(*transaction.Tx) error { @@ -168,7 +168,7 @@ func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter } // GetDKGEndState retrieves the DKG end state for the epoch. -func (ds *RecoverablePrivateBeaconKeyState) GetDKGEndState(epochCounter uint64) (flow.DKGState, error) { +func (ds *RecoverablePrivateBeaconKeyState) GetDKGState(epochCounter uint64) (flow.DKGState, error) { var endState flow.DKGState err := ds.db.Update(operation.RetrieveDKGEndStateForEpoch(epochCounter, &endState)) return endState, err diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index ef034ed3d16..21750e0392b 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -33,7 +33,7 @@ func TestDKGState_DKGStarted(t *testing.T) { // store dkg-started flag for epoch t.Run("should be able to set DKGStarted", func(t *testing.T) { - err = store.SetDKGEndState(epochCounter, flow.DKGStateStarted) + err = store.SetDKGState(epochCounter, flow.DKGStateStarted) assert.NoError(t, err) }) @@ -98,12 +98,12 @@ func TestDKGState_EndState(t *testing.T) { endState := flow.DKGStateNoKey t.Run("should be able to store an end state", func(t *testing.T) { - err = store.SetDKGEndState(epochCounter, endState) + err = store.SetDKGState(epochCounter, endState) require.NoError(t, err) }) t.Run("should be able to read an end state", func(t *testing.T) { - readEndState, err := store.GetDKGEndState(epochCounter) + readEndState, err := store.GetDKGState(epochCounter) require.NoError(t, err) assert.Equal(t, endState, readEndState) }) @@ -146,7 +146,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) assert.NoError(t, err) // mark dkg unsuccessful - err = dkgState.SetDKGEndState(epochCounter, flow.DKGStateInconsistentKey) + err = dkgState.SetDKGState(epochCounter, flow.DKGStateInconsistentKey) assert.NoError(t, err) key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) @@ -163,7 +163,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) assert.NoError(t, err) // mark dkg result as inconsistent - err = dkgState.SetDKGEndState(epochCounter, flow.DKGStateInconsistentKey) + err = dkgState.SetDKGState(epochCounter, flow.DKGStateInconsistentKey) assert.NoError(t, err) key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) @@ -176,7 +176,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { epochCounter := rand.Uint64() // mark dkg result as no key - err = dkgState.SetDKGEndState(epochCounter, flow.DKGStateNoKey) + err = dkgState.SetDKGState(epochCounter, flow.DKGStateNoKey) assert.NoError(t, err) key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) @@ -193,7 +193,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) assert.NoError(t, err) // mark dkg successful - err = dkgState.SetDKGEndState(epochCounter, flow.DKGStateSuccess) + err = dkgState.SetDKGState(epochCounter, flow.DKGStateSuccess) assert.NoError(t, err) key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) @@ -207,7 +207,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { epochCounter := rand.Uint64() // mark dkg successful - err = dkgState.SetDKGEndState(epochCounter, flow.DKGStateSuccess) + err = dkgState.SetDKGState(epochCounter, flow.DKGStateSuccess) assert.NoError(t, err) key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) diff --git a/storage/dkg.go b/storage/dkg.go index cca28b33502..b954771aee6 100644 --- a/storage/dkg.go +++ b/storage/dkg.go @@ -27,7 +27,7 @@ type DKGStateReader interface { // GetDKGEndState retrieves the end state for the given DKG. // Error returns: storage.ErrNotFound - GetDKGEndState(epochCounter uint64) (flow.DKGState, error) + GetDKGState(epochCounter uint64) (flow.DKGState, error) // UnsafeRetrieveMyBeaconPrivateKey retrieves the random beacon private key for an epoch. // @@ -46,7 +46,7 @@ type DKGState interface { // SetDKGEndState stores that the DKG has ended, and its end state. // Error returns: storage.ErrAlreadyExists - SetDKGEndState(epochCounter uint64, endState flow.DKGState) error + SetDKGState(epochCounter uint64, newState flow.DKGState) error // GetDKGStarted checks whether the DKG has been started for the given epoch. // No errors expected during normal operation. diff --git a/storage/mock/dkg_state.go b/storage/mock/dkg_state.go index 73ee7b9b48e..a6740b3f77b 100644 --- a/storage/mock/dkg_state.go +++ b/storage/mock/dkg_state.go @@ -19,7 +19,7 @@ func (_m *DKGState) GetDKGEndState(epochCounter uint64) (flow.DKGState, error) { ret := _m.Called(epochCounter) if len(ret) == 0 { - panic("no return value specified for GetDKGEndState") + panic("no return value specified for GetDKGState") } var r0 flow.DKGState @@ -123,7 +123,7 @@ func (_m *DKGState) SetDKGEndState(epochCounter uint64, endState flow.DKGState) ret := _m.Called(epochCounter, endState) if len(ret) == 0 { - panic("no return value specified for SetDKGEndState") + panic("no return value specified for SetDKGState") } var r0 error From c13c7f61086ab869e621d8891df765e5da1b8c5b Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 22 Nov 2024 16:41:12 +0200 Subject: [PATCH 09/34] Updated mocks --- storage/mock/dkg_state.go | 81 +++++++----- storage/mock/dkg_state_reader.go | 124 +++++++++++++++++++ storage/mock/epoch_recovery_my_beacon_key.go | 60 +++++++++ 3 files changed, 234 insertions(+), 31 deletions(-) create mode 100644 storage/mock/dkg_state_reader.go diff --git a/storage/mock/dkg_state.go b/storage/mock/dkg_state.go index a6740b3f77b..84332eb4449 100644 --- a/storage/mock/dkg_state.go +++ b/storage/mock/dkg_state.go @@ -14,23 +14,23 @@ type DKGState struct { mock.Mock } -// GetDKGEndState provides a mock function with given fields: epochCounter -func (_m *DKGState) GetDKGEndState(epochCounter uint64) (flow.DKGState, error) { +// GetDKGStarted provides a mock function with given fields: epochCounter +func (_m *DKGState) GetDKGStarted(epochCounter uint64) (bool, error) { ret := _m.Called(epochCounter) if len(ret) == 0 { - panic("no return value specified for GetDKGState") + panic("no return value specified for GetDKGStarted") } - var r0 flow.DKGState + var r0 bool var r1 error - if rf, ok := ret.Get(0).(func(uint64) (flow.DKGState, error)); ok { + if rf, ok := ret.Get(0).(func(uint64) (bool, error)); ok { return rf(epochCounter) } - if rf, ok := ret.Get(0).(func(uint64) flow.DKGState); ok { + if rf, ok := ret.Get(0).(func(uint64) bool); ok { r0 = rf(epochCounter) } else { - r0 = ret.Get(0).(flow.DKGState) + r0 = ret.Get(0).(bool) } if rf, ok := ret.Get(1).(func(uint64) error); ok { @@ -42,23 +42,23 @@ func (_m *DKGState) GetDKGEndState(epochCounter uint64) (flow.DKGState, error) { return r0, r1 } -// GetDKGStarted provides a mock function with given fields: epochCounter -func (_m *DKGState) GetDKGStarted(epochCounter uint64) (bool, error) { +// GetDKGState provides a mock function with given fields: epochCounter +func (_m *DKGState) GetDKGState(epochCounter uint64) (flow.DKGState, error) { ret := _m.Called(epochCounter) if len(ret) == 0 { - panic("no return value specified for GetDKGStarted") + panic("no return value specified for GetDKGState") } - var r0 bool + var r0 flow.DKGState var r1 error - if rf, ok := ret.Get(0).(func(uint64) (bool, error)); ok { + if rf, ok := ret.Get(0).(func(uint64) (flow.DKGState, error)); ok { return rf(epochCounter) } - if rf, ok := ret.Get(0).(func(uint64) bool); ok { + if rf, ok := ret.Get(0).(func(uint64) flow.DKGState); ok { r0 = rf(epochCounter) } else { - r0 = ret.Get(0).(bool) + r0 = ret.Get(0).(flow.DKGState) } if rf, ok := ret.Get(1).(func(uint64) error); ok { @@ -89,7 +89,7 @@ func (_m *DKGState) InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.Pri } // RetrieveMyBeaconPrivateKey provides a mock function with given fields: epochCounter -func (_m *DKGState) RetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) { +func (_m *DKGState) RetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, bool, error) { ret := _m.Called(epochCounter) if len(ret) == 0 { @@ -97,8 +97,9 @@ func (_m *DKGState) RetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.Priv } var r0 crypto.PrivateKey - var r1 error - if rf, ok := ret.Get(0).(func(uint64) (crypto.PrivateKey, error)); ok { + var r1 bool + var r2 error + if rf, ok := ret.Get(0).(func(uint64) (crypto.PrivateKey, bool, error)); ok { return rf(epochCounter) } if rf, ok := ret.Get(0).(func(uint64) crypto.PrivateKey); ok { @@ -109,18 +110,24 @@ func (_m *DKGState) RetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.Priv } } - if rf, ok := ret.Get(1).(func(uint64) error); ok { + if rf, ok := ret.Get(1).(func(uint64) bool); ok { r1 = rf(epochCounter) } else { - r1 = ret.Error(1) + r1 = ret.Get(1).(bool) } - return r0, r1 + if rf, ok := ret.Get(2).(func(uint64) error); ok { + r2 = rf(epochCounter) + } else { + r2 = ret.Error(2) + } + + return r0, r1, r2 } -// SetDKGEndState provides a mock function with given fields: epochCounter, endState -func (_m *DKGState) SetDKGEndState(epochCounter uint64, endState flow.DKGState) error { - ret := _m.Called(epochCounter, endState) +// SetDKGState provides a mock function with given fields: epochCounter, newState +func (_m *DKGState) SetDKGState(epochCounter uint64, newState flow.DKGState) error { + ret := _m.Called(epochCounter, newState) if len(ret) == 0 { panic("no return value specified for SetDKGState") @@ -128,7 +135,7 @@ func (_m *DKGState) SetDKGEndState(epochCounter uint64, endState flow.DKGState) var r0 error if rf, ok := ret.Get(0).(func(uint64, flow.DKGState) error); ok { - r0 = rf(epochCounter, endState) + r0 = rf(epochCounter, newState) } else { r0 = ret.Error(0) } @@ -136,22 +143,34 @@ func (_m *DKGState) SetDKGEndState(epochCounter uint64, endState flow.DKGState) return r0 } -// SetDKGStarted provides a mock function with given fields: epochCounter -func (_m *DKGState) SetDKGStarted(epochCounter uint64) error { +// UnsafeRetrieveMyBeaconPrivateKey provides a mock function with given fields: epochCounter +func (_m *DKGState) UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) { ret := _m.Called(epochCounter) if len(ret) == 0 { - panic("no return value specified for SetDKGStarted") + panic("no return value specified for UnsafeRetrieveMyBeaconPrivateKey") } - var r0 error - if rf, ok := ret.Get(0).(func(uint64) error); ok { + var r0 crypto.PrivateKey + var r1 error + if rf, ok := ret.Get(0).(func(uint64) (crypto.PrivateKey, error)); ok { + return rf(epochCounter) + } + if rf, ok := ret.Get(0).(func(uint64) crypto.PrivateKey); ok { r0 = rf(epochCounter) } else { - r0 = ret.Error(0) + if ret.Get(0) != nil { + r0 = ret.Get(0).(crypto.PrivateKey) + } } - return r0 + if rf, ok := ret.Get(1).(func(uint64) error); ok { + r1 = rf(epochCounter) + } else { + r1 = ret.Error(1) + } + + return r0, r1 } // NewDKGState creates a new instance of DKGState. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. diff --git a/storage/mock/dkg_state_reader.go b/storage/mock/dkg_state_reader.go new file mode 100644 index 00000000000..429f4966fc5 --- /dev/null +++ b/storage/mock/dkg_state_reader.go @@ -0,0 +1,124 @@ +// Code generated by mockery v2.43.2. DO NOT EDIT. + +package mock + +import ( + crypto "github.com/onflow/crypto" + flow "github.com/onflow/flow-go/model/flow" + + mock "github.com/stretchr/testify/mock" +) + +// DKGStateReader is an autogenerated mock type for the DKGStateReader type +type DKGStateReader struct { + mock.Mock +} + +// GetDKGState provides a mock function with given fields: epochCounter +func (_m *DKGStateReader) GetDKGState(epochCounter uint64) (flow.DKGState, error) { + ret := _m.Called(epochCounter) + + if len(ret) == 0 { + panic("no return value specified for GetDKGState") + } + + var r0 flow.DKGState + var r1 error + if rf, ok := ret.Get(0).(func(uint64) (flow.DKGState, error)); ok { + return rf(epochCounter) + } + if rf, ok := ret.Get(0).(func(uint64) flow.DKGState); ok { + r0 = rf(epochCounter) + } else { + r0 = ret.Get(0).(flow.DKGState) + } + + if rf, ok := ret.Get(1).(func(uint64) error); ok { + r1 = rf(epochCounter) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// RetrieveMyBeaconPrivateKey provides a mock function with given fields: epochCounter +func (_m *DKGStateReader) RetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, bool, error) { + ret := _m.Called(epochCounter) + + if len(ret) == 0 { + panic("no return value specified for RetrieveMyBeaconPrivateKey") + } + + var r0 crypto.PrivateKey + var r1 bool + var r2 error + if rf, ok := ret.Get(0).(func(uint64) (crypto.PrivateKey, bool, error)); ok { + return rf(epochCounter) + } + if rf, ok := ret.Get(0).(func(uint64) crypto.PrivateKey); ok { + r0 = rf(epochCounter) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(crypto.PrivateKey) + } + } + + if rf, ok := ret.Get(1).(func(uint64) bool); ok { + r1 = rf(epochCounter) + } else { + r1 = ret.Get(1).(bool) + } + + if rf, ok := ret.Get(2).(func(uint64) error); ok { + r2 = rf(epochCounter) + } else { + r2 = ret.Error(2) + } + + return r0, r1, r2 +} + +// UnsafeRetrieveMyBeaconPrivateKey provides a mock function with given fields: epochCounter +func (_m *DKGStateReader) UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) { + ret := _m.Called(epochCounter) + + if len(ret) == 0 { + panic("no return value specified for UnsafeRetrieveMyBeaconPrivateKey") + } + + var r0 crypto.PrivateKey + var r1 error + if rf, ok := ret.Get(0).(func(uint64) (crypto.PrivateKey, error)); ok { + return rf(epochCounter) + } + if rf, ok := ret.Get(0).(func(uint64) crypto.PrivateKey); ok { + r0 = rf(epochCounter) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(crypto.PrivateKey) + } + } + + if rf, ok := ret.Get(1).(func(uint64) error); ok { + r1 = rf(epochCounter) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// NewDKGStateReader creates a new instance of DKGStateReader. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewDKGStateReader(t interface { + mock.TestingT + Cleanup(func()) +}) *DKGStateReader { + mock := &DKGStateReader{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/storage/mock/epoch_recovery_my_beacon_key.go b/storage/mock/epoch_recovery_my_beacon_key.go index cb8c4720675..8dd7311b72b 100644 --- a/storage/mock/epoch_recovery_my_beacon_key.go +++ b/storage/mock/epoch_recovery_my_beacon_key.go @@ -4,6 +4,8 @@ package mock import ( crypto "github.com/onflow/crypto" + flow "github.com/onflow/flow-go/model/flow" + mock "github.com/stretchr/testify/mock" ) @@ -12,6 +14,34 @@ type EpochRecoveryMyBeaconKey struct { mock.Mock } +// GetDKGState provides a mock function with given fields: epochCounter +func (_m *EpochRecoveryMyBeaconKey) GetDKGState(epochCounter uint64) (flow.DKGState, error) { + ret := _m.Called(epochCounter) + + if len(ret) == 0 { + panic("no return value specified for GetDKGState") + } + + var r0 flow.DKGState + var r1 error + if rf, ok := ret.Get(0).(func(uint64) (flow.DKGState, error)); ok { + return rf(epochCounter) + } + if rf, ok := ret.Get(0).(func(uint64) flow.DKGState); ok { + r0 = rf(epochCounter) + } else { + r0 = ret.Get(0).(flow.DKGState) + } + + if rf, ok := ret.Get(1).(func(uint64) error); ok { + r1 = rf(epochCounter) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // RetrieveMyBeaconPrivateKey provides a mock function with given fields: epochCounter func (_m *EpochRecoveryMyBeaconKey) RetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, bool, error) { ret := _m.Called(epochCounter) @@ -49,6 +79,36 @@ func (_m *EpochRecoveryMyBeaconKey) RetrieveMyBeaconPrivateKey(epochCounter uint return r0, r1, r2 } +// UnsafeRetrieveMyBeaconPrivateKey provides a mock function with given fields: epochCounter +func (_m *EpochRecoveryMyBeaconKey) UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) { + ret := _m.Called(epochCounter) + + if len(ret) == 0 { + panic("no return value specified for UnsafeRetrieveMyBeaconPrivateKey") + } + + var r0 crypto.PrivateKey + var r1 error + if rf, ok := ret.Get(0).(func(uint64) (crypto.PrivateKey, error)); ok { + return rf(epochCounter) + } + if rf, ok := ret.Get(0).(func(uint64) crypto.PrivateKey); ok { + r0 = rf(epochCounter) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(crypto.PrivateKey) + } + } + + if rf, ok := ret.Get(1).(func(uint64) error); ok { + r1 = rf(epochCounter) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // UpsertMyBeaconPrivateKey provides a mock function with given fields: epochCounter, key func (_m *EpochRecoveryMyBeaconKey) UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { ret := _m.Called(epochCounter, key) From 489c871fd20f5b423db1bfe04583420655cd53ae Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 22 Nov 2024 17:04:22 +0200 Subject: [PATCH 10/34] Fixed tests for reactor engine --- engine/consensus/dkg/reactor_engine.go | 2 +- engine/consensus/dkg/reactor_engine_test.go | 42 ++++++++++----------- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/engine/consensus/dkg/reactor_engine.go b/engine/consensus/dkg/reactor_engine.go index 9a4355b9d6e..379d7dfe86d 100644 --- a/engine/consensus/dkg/reactor_engine.go +++ b/engine/consensus/dkg/reactor_engine.go @@ -319,7 +319,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin Str("computed_beacon_pub_key", localPubKey.String()). Str("canonical_beacon_pub_key", nextDKGPubKey.String()). Msg("checking beacon key consistency: locally computed beacon public key does not match beacon public key for next epoch") - err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateNoKey) + err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateInconsistentKey) if err != nil { // TODO use irrecoverable context log.Fatal().Err(err).Msg("failed to set dkg end state") diff --git a/engine/consensus/dkg/reactor_engine_test.go b/engine/consensus/dkg/reactor_engine_test.go index 375bb824f4f..bc40e35fd86 100644 --- a/engine/consensus/dkg/reactor_engine_test.go +++ b/engine/consensus/dkg/reactor_engine_test.go @@ -139,7 +139,7 @@ func (suite *ReactorEngineSuite_SetupPhase) SetupTest() { // ensure that an attempt is made to insert the expected dkg private share // for the next epoch. suite.dkgState = new(storage.DKGState) - suite.dkgState.On("SetDKGStarted", suite.NextEpochCounter()).Return(nil).Once() + suite.dkgState.On("SetDKGState", suite.NextEpochCounter(), flow.DKGStateStarted).Return(nil).Once() suite.dkgState.On("InsertMyBeaconPrivateKey", mock.Anything, mock.Anything).Run( func(args mock.Arguments) { epochCounter := args.Get(0).(uint64) @@ -266,7 +266,7 @@ type ReactorEngineSuite_CommittedPhase struct { epochCounter uint64 // current epoch counter myLocalBeaconKey crypto.PrivateKey // my locally computed beacon key myGlobalBeaconPubKey crypto.PublicKey // my public key, as dictated by global DKG - dkgEndState flow.DKGState // backend for DGKState. + DKGState flow.DKGState // backend for DGKState. firstBlock *flow.Header // first block of EpochCommitted phase warnsLogged int // count # of warn-level logs @@ -290,7 +290,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) NextEpochCounter() uint64 { func (suite *ReactorEngineSuite_CommittedPhase) SetupTest() { suite.epochCounter = rand.Uint64() - suite.dkgEndState = flow.DKGEndStateUnknown + suite.DKGState = flow.DKGStateUnknown suite.me = new(module.Local) id := unittest.IdentifierFixture() @@ -301,7 +301,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) SetupTest() { suite.myGlobalBeaconPubKey = suite.myLocalBeaconKey.PublicKey() suite.dkgState = new(storage.DKGState) - suite.dkgState.On("RetrieveMyBeaconPrivateKey", suite.NextEpochCounter()).Return( + suite.dkgState.On("UnsafeRetrieveMyBeaconPrivateKey", suite.NextEpochCounter()).Return( func(_ uint64) crypto.PrivateKey { return suite.myLocalBeaconKey }, func(_ uint64) error { if suite.myLocalBeaconKey == nil { @@ -312,15 +312,15 @@ func (suite *ReactorEngineSuite_CommittedPhase) SetupTest() { ) suite.dkgState.On("SetDKGState", suite.NextEpochCounter(), mock.Anything). Run(func(args mock.Arguments) { - assert.Equal(suite.T(), flow.DKGEndStateUnknown, suite.dkgEndState) // must be unset + assert.Equal(suite.T(), flow.DKGStateUnknown, suite.DKGState) // must be unset endState := args[1].(flow.DKGState) - suite.dkgEndState = endState + suite.DKGState = endState }). Return(nil) suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return( - func(_ uint64) flow.DKGState { return suite.dkgEndState }, + func(_ uint64) flow.DKGState { return suite.DKGState }, func(_ uint64) error { - if suite.dkgEndState == flow.DKGEndStateUnknown { + if suite.DKGState == flow.DKGStateUnknown { return storerr.ErrNotFound } return nil @@ -382,7 +382,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestDKGSuccess() { suite.engine.EpochCommittedPhaseStarted(suite.epochCounter, suite.firstBlock) suite.Require().Equal(0, suite.warnsLogged) - suite.Assert().Equal(flow.DKGEndStateSuccess, suite.dkgEndState) + suite.Assert().Equal(flow.DKGStateSuccess, suite.DKGState) } // TestInconsistentKey tests the path where we are checking the global DKG @@ -397,7 +397,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestInconsistentKey() { suite.engine.EpochCommittedPhaseStarted(suite.epochCounter, suite.firstBlock) suite.Require().Equal(1, suite.warnsLogged) - suite.Assert().Equal(flow.DKGEndStateInconsistentKey, suite.dkgEndState) + suite.Assert().Equal(flow.DKGStateInconsistentKey, suite.DKGState) } // TestMissingKey tests the path where we are checking the global DKG results @@ -412,7 +412,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestMissingKey() { suite.engine.EpochCommittedPhaseStarted(suite.epochCounter, suite.firstBlock) suite.Require().Equal(1, suite.warnsLogged) - suite.Assert().Equal(flow.DKGEndStateNoKey, suite.dkgEndState) + suite.Assert().Equal(flow.DKGStateNoKey, suite.DKGState) } // TestLocalDKGFailure tests the path where we are checking the global DKG @@ -423,11 +423,11 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestMissingKey() { func (suite *ReactorEngineSuite_CommittedPhase) TestLocalDKGFailure() { // set dkg end state as failure - suite.dkgEndState = flow.DKGEndStateDKGFailure + suite.DKGState = flow.DKGStateDKGFailure suite.engine.EpochCommittedPhaseStarted(suite.epochCounter, suite.firstBlock) suite.Require().Equal(1, suite.warnsLogged) - suite.Assert().Equal(flow.DKGEndStateDKGFailure, suite.dkgEndState) + suite.Assert().Equal(flow.DKGStateDKGFailure, suite.DKGState) } // TestStartupInCommittedPhase_DKGSuccess tests that the dkg end state is correctly @@ -438,7 +438,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_DKGS suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started but not ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGEndStateUnknown, storerr.ErrNotFound).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateUnknown, storerr.ErrNotFound).Once() // start up the engine unittest.AssertClosesBefore(suite.T(), suite.engine.Ready(), time.Second) @@ -450,18 +450,18 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_DKGS mock.Anything, ) // should set DKG end state - suite.Assert().Equal(flow.DKGEndStateSuccess, suite.dkgEndState) + suite.Assert().Equal(flow.DKGStateSuccess, suite.DKGState) } // TestStartupInCommittedPhase_DKGSuccess tests that the dkg end state is correctly // set when starting in EpochCommitted phase and the DKG end state is already set. -func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_DKGEndStateAlreadySet() { +func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_DKGStateAlreadySet() { // we are in the EpochSetup phase suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started and ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGEndStateNoKey, nil).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateNoKey, nil).Once() // start up the engine unittest.AssertClosesBefore(suite.T(), suite.engine.Ready(), time.Second) @@ -482,7 +482,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_Inco suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started but not ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGEndStateUnknown, storerr.ErrNotFound).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateUnknown, storerr.ErrNotFound).Once() // set our global pub key to a random value suite.myGlobalBeaconPubKey = unittest.RandomBeaconPriv().PublicKey() @@ -497,7 +497,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_Inco mock.Anything, ) // should set DKG end state - suite.Assert().Equal(flow.DKGEndStateInconsistentKey, suite.dkgEndState) + suite.Assert().Equal(flow.DKGStateInconsistentKey, suite.DKGState) } // TestStartupInCommittedPhase_MissingKey tests that the dkg end state is correctly @@ -508,7 +508,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_Miss suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started but not ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGEndStateUnknown, storerr.ErrNotFound).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateUnknown, storerr.ErrNotFound).Once() // remove our key suite.myLocalBeaconKey = nil @@ -523,7 +523,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_Miss mock.Anything, ) // should set DKG end state - suite.Assert().Equal(flow.DKGEndStateNoKey, suite.dkgEndState) + suite.Assert().Equal(flow.DKGStateNoKey, suite.DKGState) } // utility function to track the number of warn-level calls to a logger From 10aab93447d46f844d4f886a1b0e3a249eed4399 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 25 Nov 2024 16:15:15 +0200 Subject: [PATCH 11/34] Updated godoc and reduced number of states for Recoverable state machine --- engine/consensus/dkg/reactor_engine.go | 4 +- engine/consensus/dkg/reactor_engine_test.go | 20 +++++----- model/flow/dkg.go | 42 ++++++++++----------- storage/badger/dkg_state.go | 18 ++++----- storage/badger/dkg_state_test.go | 4 +- 5 files changed, 43 insertions(+), 45 deletions(-) diff --git a/engine/consensus/dkg/reactor_engine.go b/engine/consensus/dkg/reactor_engine.go index 379d7dfe86d..77366e4fb83 100644 --- a/engine/consensus/dkg/reactor_engine.go +++ b/engine/consensus/dkg/reactor_engine.go @@ -327,7 +327,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin return } - err = e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateSuccess) + err = e.dkgState.SetDKGState(nextEpochCounter, flow.RandomBeaconKeyCommitted) if err != nil { // TODO use irrecoverable context e.log.Fatal().Err(err).Msg("failed to set dkg end state") @@ -427,7 +427,7 @@ func (e *ReactorEngine) end(nextEpochCounter uint64) func() error { // has already abandoned the happy path, because on the happy path the ReactorEngine is the only writer. // Then this function just stops and returns without error. e.log.Warn().Err(err).Msgf("node %s with index %d failed DKG locally", e.me.NodeID(), e.controller.GetIndex()) - err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateDKGFailure) + err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateFailure) if err != nil { if errors.Is(err, storage.ErrAlreadyExists) { return nil // DKGState already being set is expected in case of epoch recovery diff --git a/engine/consensus/dkg/reactor_engine_test.go b/engine/consensus/dkg/reactor_engine_test.go index bc40e35fd86..fabc619d303 100644 --- a/engine/consensus/dkg/reactor_engine_test.go +++ b/engine/consensus/dkg/reactor_engine_test.go @@ -290,7 +290,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) NextEpochCounter() uint64 { func (suite *ReactorEngineSuite_CommittedPhase) SetupTest() { suite.epochCounter = rand.Uint64() - suite.DKGState = flow.DKGStateUnknown + suite.DKGState = flow.DKGStateUninitialized suite.me = new(module.Local) id := unittest.IdentifierFixture() @@ -312,7 +312,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) SetupTest() { ) suite.dkgState.On("SetDKGState", suite.NextEpochCounter(), mock.Anything). Run(func(args mock.Arguments) { - assert.Equal(suite.T(), flow.DKGStateUnknown, suite.DKGState) // must be unset + assert.Equal(suite.T(), flow.DKGStateUninitialized, suite.DKGState) // must be unset endState := args[1].(flow.DKGState) suite.DKGState = endState }). @@ -320,7 +320,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) SetupTest() { suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return( func(_ uint64) flow.DKGState { return suite.DKGState }, func(_ uint64) error { - if suite.DKGState == flow.DKGStateUnknown { + if suite.DKGState == flow.DKGStateUninitialized { return storerr.ErrNotFound } return nil @@ -382,7 +382,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestDKGSuccess() { suite.engine.EpochCommittedPhaseStarted(suite.epochCounter, suite.firstBlock) suite.Require().Equal(0, suite.warnsLogged) - suite.Assert().Equal(flow.DKGStateSuccess, suite.DKGState) + suite.Assert().Equal(flow.RandomBeaconKeyCommitted, suite.DKGState) } // TestInconsistentKey tests the path where we are checking the global DKG @@ -423,11 +423,11 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestMissingKey() { func (suite *ReactorEngineSuite_CommittedPhase) TestLocalDKGFailure() { // set dkg end state as failure - suite.DKGState = flow.DKGStateDKGFailure + suite.DKGState = flow.DKGStateFailure suite.engine.EpochCommittedPhaseStarted(suite.epochCounter, suite.firstBlock) suite.Require().Equal(1, suite.warnsLogged) - suite.Assert().Equal(flow.DKGStateDKGFailure, suite.DKGState) + suite.Assert().Equal(flow.DKGStateFailure, suite.DKGState) } // TestStartupInCommittedPhase_DKGSuccess tests that the dkg end state is correctly @@ -438,7 +438,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_DKGS suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started but not ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateUnknown, storerr.ErrNotFound).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateUninitialized, storerr.ErrNotFound).Once() // start up the engine unittest.AssertClosesBefore(suite.T(), suite.engine.Ready(), time.Second) @@ -450,7 +450,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_DKGS mock.Anything, ) // should set DKG end state - suite.Assert().Equal(flow.DKGStateSuccess, suite.DKGState) + suite.Assert().Equal(flow.RandomBeaconKeyCommitted, suite.DKGState) } // TestStartupInCommittedPhase_DKGSuccess tests that the dkg end state is correctly @@ -482,7 +482,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_Inco suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started but not ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateUnknown, storerr.ErrNotFound).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateUninitialized, storerr.ErrNotFound).Once() // set our global pub key to a random value suite.myGlobalBeaconPubKey = unittest.RandomBeaconPriv().PublicKey() @@ -508,7 +508,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_Miss suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started but not ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateUnknown, storerr.ErrNotFound).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateUninitialized, storerr.ErrNotFound).Once() // remove our key suite.myLocalBeaconKey = nil diff --git a/model/flow/dkg.go b/model/flow/dkg.go index 0548185304a..278aa434ddd 100644 --- a/model/flow/dkg.go +++ b/model/flow/dkg.go @@ -1,23 +1,25 @@ package flow -// DKGState captures the final state of a completed DKG. +// DKGState captures all possible states of the Recoverable Random Beacon State Machine. type DKGState uint32 const ( - // DKGStateUnknown - zero value for this enum, indicates unset value - DKGStateUnknown DKGState = iota - // DKGStateSuccess - the DKG completed, this node has a valid beacon key. + // DKGStateUninitialized - zero value for this enum, indicates that there is no initialized state. + // Conceptually, this is the 'initial' state of a finite state machine before any transitions. + DKGStateUninitialized DKGState = iota + // DKGStateStarted - the DKG process has been started. This state is set when the node enters the [flow.EpochPhaseSetup] + // phase and starts the DKG process, which will on the happy path result in generating a random beacon key. DKGStateStarted - // DKGStateCompleted + // DKGStateCompleted - the DKG process has been locally completed by this node. This state is set when the node successfully + // completes the DKG process and has generated a random beacon key. + // ATTENTION: This state does not imply that there is a safe random beacon key available for the next epoch. Only after + // the node enters [flow.EpochPhaseCommitted] and the [flow.EpochCommit] service event has been finalized, we can be sure + // that our beacon key share is part of the Random Beacon Committee for the next epoch. DKGStateCompleted - // DKGStateSuccess - DKGStateSuccess - // DKGStateInconsistentKey - the DKG completed, this node has an invalid beacon key. - DKGStateInconsistentKey - // DKGStateNoKey - this node did not store a key, typically caused by a crash mid-DKG. - DKGStateNoKey - // DKGStateDKGFailure - the underlying DKG library reported an error. - DKGStateDKGFailure + // RandomBeaconKeyCommitted - + RandomBeaconKeyCommitted + // DKGStateFailure - the underlying DKG library reported an error. + DKGStateFailure // RandomBeaconKeyRecovered - this node has recovered its beacon key from a previous epoch. // This occurs only for epochs which are entered through the EFM Recovery process (`flow.EpochRecover` service event). RandomBeaconKeyRecovered @@ -29,18 +31,14 @@ func (state DKGState) String() string { return "DKGStateStarted" case DKGStateCompleted: return "DKGStateCompleted" - case DKGStateSuccess: - return "DKGStateSuccess" - case DKGStateInconsistentKey: - return "DKGStateInconsistentKey" - case DKGStateNoKey: - return "DKGStateNoKey" - case DKGStateDKGFailure: - return "DKGStateDKGFailure" + case RandomBeaconKeyCommitted: + return "RandomBeaconKeyCommitted" + case DKGStateFailure: + return "DKGStateFailure" case RandomBeaconKeyRecovered: return "RandomBeaconKeyRecovered" default: - return "DKGStateUnknown" + return "DKGStateUninitialized" } } diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index 3644636aefc..b278edec964 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -19,14 +19,14 @@ import ( ) var allowedStateTransitions = map[flow.DKGState][]flow.DKGState{ - flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateNoKey, flow.DKGStateDKGFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, - flow.DKGStateCompleted: {flow.DKGStateSuccess, flow.DKGStateNoKey, flow.DKGStateDKGFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, - flow.DKGStateSuccess: {}, + flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateNoKey, flow.DKGStateFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, + flow.DKGStateCompleted: {flow.RandomBeaconKeyCommitted, flow.DKGStateNoKey, flow.DKGStateFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, + flow.RandomBeaconKeyCommitted: {}, flow.RandomBeaconKeyRecovered: {}, - flow.DKGStateDKGFailure: {flow.RandomBeaconKeyRecovered}, + flow.DKGStateFailure: {flow.RandomBeaconKeyRecovered}, flow.DKGStateInconsistentKey: {flow.RandomBeaconKeyRecovered}, flow.DKGStateNoKey: {flow.RandomBeaconKeyRecovered}, - flow.DKGStateUnknown: {flow.DKGStateStarted, flow.DKGStateNoKey, flow.DKGStateDKGFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, + flow.DKGStateUninitialized: {flow.DKGStateStarted, flow.DKGStateNoKey, flow.DKGStateFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, } // RecoverablePrivateBeaconKeyState stores state information about in-progress and completed DKGs, including @@ -144,7 +144,7 @@ func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter err := operation.RetrieveDKGEndStateForEpoch(epochCounter, ¤tState)(tx.DBTxn) if err != nil { if errors.Is(err, storage.ErrNotFound) { - currentState = flow.DKGStateUnknown + currentState = flow.DKGStateUninitialized } else { return fmt.Errorf("could not retrieve current state for epoch %d: %w", epochCounter, err) } @@ -156,10 +156,10 @@ func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter } // ensure invariant holds and we still have a valid private key stored - if newState == flow.DKGStateSuccess { + if newState == flow.RandomBeaconKeyCommitted { _, err = ds.retrieveKeyTx(epochCounter)(tx.DBTxn) if err != nil { - return fmt.Errorf("cannot transition to flow.DKGStateSuccess without a valid random beacon key: %w", err) + return fmt.Errorf("cannot transition to flow.RandomBeaconKeyCommitted without a valid random beacon key: %w", err) } } @@ -196,7 +196,7 @@ func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCoun } // for any end state besides success and recovery, the key is not safe - if endState == flow.DKGStateSuccess || endState == flow.RandomBeaconKeyRecovered { + if endState == flow.RandomBeaconKeyCommitted || endState == flow.RandomBeaconKeyRecovered { // retrieve the key - any storage error (including `storage.ErrNotFound`) is an exception var encodableKey *encodable.RandomBeaconPrivKey encodableKey, err = ds.retrieveKeyTx(epochCounter)(txn) diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index 21750e0392b..d4c858f2c99 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -193,7 +193,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) assert.NoError(t, err) // mark dkg successful - err = dkgState.SetDKGState(epochCounter, flow.DKGStateSuccess) + err = dkgState.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) assert.NoError(t, err) key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) @@ -207,7 +207,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { epochCounter := rand.Uint64() // mark dkg successful - err = dkgState.SetDKGState(epochCounter, flow.DKGStateSuccess) + err = dkgState.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) assert.NoError(t, err) key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) From fb2824967d6ad88a5d7259a6f69a5080cace69d5 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 25 Nov 2024 16:28:40 +0200 Subject: [PATCH 12/34] Updated usages of DKG state. Updated naming, godocs --- cmd/consensus/main.go | 12 +++------ engine/consensus/dkg/reactor_engine.go | 4 +-- engine/consensus/dkg/reactor_engine_test.go | 10 ++++---- storage/badger/dkg_state.go | 20 +++++++-------- storage/badger/dkg_state_test.go | 8 +++--- storage/badger/operation/dkg.go | 28 ++++++++++----------- storage/badger/operation/dkg_test.go | 10 ++++---- storage/badger/operation/prefix.go | 2 +- 8 files changed, 43 insertions(+), 51 deletions(-) diff --git a/cmd/consensus/main.go b/cmd/consensus/main.go index 8155c904734..7ea266a4d4c 100644 --- a/cmd/consensus/main.go +++ b/cmd/consensus/main.go @@ -128,7 +128,6 @@ func main() { epochLookup *epochs.EpochLookup hotstuffModules *consensus.HotstuffModules dkgState *bstorage.RecoverablePrivateBeaconKeyState - safeBeaconKeys *bstorage.SafeBeaconPrivateKeys getSealingConfigs module.SealingConfigsGetter ) var deprecatedFlagBlockRateDelay time.Duration @@ -217,10 +216,6 @@ func main() { dkgState, err = bstorage.NewDKGState(node.Metrics.Cache, node.SecretsDB) return err }). - Module("beacon keys", func(node *cmd.NodeConfig) error { - safeBeaconKeys = bstorage.NewSafeBeaconPrivateKeys(dkgState) - return nil - }). Module("updatable sealing config", func(node *cmd.NodeConfig) error { setter, err := updatable_configs.NewSealingConfigs( requiredApprovalsForSealConstruction, @@ -350,7 +345,7 @@ func main() { return err } // mark the root DKG as successful, so it is considered safe to use the key - err = dkgState.SetDKGState(epochCounter, flow.DKGEndStateSuccess) + err = dkgState.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) if err != nil && !errors.Is(err, storage.ErrAlreadyExists) { return err } @@ -358,8 +353,7 @@ func main() { return nil }). Module("my beacon key epoch recovery", func(node *cmd.NodeConfig) error { - recoverMyBeaconKeyStorage := bstorage.NewEpochRecoveryMyBeaconKey(safeBeaconKeys) - myBeaconKeyRecovery, err := dkgmodule.NewBeaconKeyRecovery(node.Logger, node.Me, node.State, recoverMyBeaconKeyStorage) + myBeaconKeyRecovery, err := dkgmodule.NewBeaconKeyRecovery(node.Logger, node.Me, node.State, dkgState) if err != nil { return fmt.Errorf("could not initialize my beacon key epoch recovery: %w", err) } @@ -582,7 +576,7 @@ func main() { // wrap Main consensus committee with metrics wrappedCommittee := committees.NewMetricsWrapper(committee, mainMetrics) // wrapper for measuring time spent determining consensus committee relations - beaconKeyStore := hotsignature.NewEpochAwareRandomBeaconKeyStore(epochLookup, safeBeaconKeys) + beaconKeyStore := hotsignature.NewEpochAwareRandomBeaconKeyStore(epochLookup, dkgState) // initialize the combined signer for hotstuff var signer hotstuff.Signer diff --git a/engine/consensus/dkg/reactor_engine.go b/engine/consensus/dkg/reactor_engine.go index 77366e4fb83..958bf97498d 100644 --- a/engine/consensus/dkg/reactor_engine.go +++ b/engine/consensus/dkg/reactor_engine.go @@ -292,7 +292,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin myBeaconPrivKey, err := e.dkgState.UnsafeRetrieveMyBeaconPrivateKey(nextEpochCounter) if errors.Is(err, storage.ErrNotFound) { log.Warn().Msg("checking beacon key consistency: no key found") - err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateNoKey) + err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateFailure) if err != nil { // TODO use irrecoverable context log.Fatal().Err(err).Msg("failed to set dkg end state") @@ -319,7 +319,7 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin Str("computed_beacon_pub_key", localPubKey.String()). Str("canonical_beacon_pub_key", nextDKGPubKey.String()). Msg("checking beacon key consistency: locally computed beacon public key does not match beacon public key for next epoch") - err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateInconsistentKey) + err := e.dkgState.SetDKGState(nextEpochCounter, flow.DKGStateFailure) if err != nil { // TODO use irrecoverable context log.Fatal().Err(err).Msg("failed to set dkg end state") diff --git a/engine/consensus/dkg/reactor_engine_test.go b/engine/consensus/dkg/reactor_engine_test.go index fabc619d303..b59e5f6cc7e 100644 --- a/engine/consensus/dkg/reactor_engine_test.go +++ b/engine/consensus/dkg/reactor_engine_test.go @@ -397,7 +397,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestInconsistentKey() { suite.engine.EpochCommittedPhaseStarted(suite.epochCounter, suite.firstBlock) suite.Require().Equal(1, suite.warnsLogged) - suite.Assert().Equal(flow.DKGStateInconsistentKey, suite.DKGState) + suite.Assert().Equal(flow.DKGStateFailure, suite.DKGState) } // TestMissingKey tests the path where we are checking the global DKG results @@ -412,7 +412,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestMissingKey() { suite.engine.EpochCommittedPhaseStarted(suite.epochCounter, suite.firstBlock) suite.Require().Equal(1, suite.warnsLogged) - suite.Assert().Equal(flow.DKGStateNoKey, suite.DKGState) + suite.Assert().Equal(flow.DKGStateFailure, suite.DKGState) } // TestLocalDKGFailure tests the path where we are checking the global DKG @@ -461,7 +461,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_DKGS suite.snap.On("EpochPhase").Return(flow.EpochPhaseCommitted, nil).Once() // the dkg for this epoch has been started and ended suite.dkgState.On("GetDKGStarted", suite.NextEpochCounter()).Return(true, nil).Once() - suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateNoKey, nil).Once() + suite.dkgState.On("GetDKGState", suite.NextEpochCounter()).Return(flow.DKGStateFailure, nil).Once() // start up the engine unittest.AssertClosesBefore(suite.T(), suite.engine.Ready(), time.Second) @@ -497,7 +497,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_Inco mock.Anything, ) // should set DKG end state - suite.Assert().Equal(flow.DKGStateInconsistentKey, suite.DKGState) + suite.Assert().Equal(flow.DKGStateFailure, suite.DKGState) } // TestStartupInCommittedPhase_MissingKey tests that the dkg end state is correctly @@ -523,7 +523,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) TestStartupInCommittedPhase_Miss mock.Anything, ) // should set DKG end state - suite.Assert().Equal(flow.DKGStateNoKey, suite.DKGState) + suite.Assert().Equal(flow.DKGStateFailure, suite.DKGState) } // utility function to track the number of warn-level calls to a logger diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index b278edec964..de8a6ab0f0e 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -3,10 +3,10 @@ package badger import ( "errors" "fmt" - "golang.org/x/exp/slices" "github.com/dgraph-io/badger/v2" "github.com/onflow/crypto" + "golang.org/x/exp/slices" "github.com/onflow/flow-go/model/encodable" "github.com/onflow/flow-go/model/flow" @@ -19,14 +19,12 @@ import ( ) var allowedStateTransitions = map[flow.DKGState][]flow.DKGState{ - flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateNoKey, flow.DKGStateFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, - flow.DKGStateCompleted: {flow.RandomBeaconKeyCommitted, flow.DKGStateNoKey, flow.DKGStateFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, + flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateFailure, flow.RandomBeaconKeyRecovered}, + flow.DKGStateCompleted: {flow.RandomBeaconKeyCommitted, flow.DKGStateFailure, flow.RandomBeaconKeyRecovered}, flow.RandomBeaconKeyCommitted: {}, flow.RandomBeaconKeyRecovered: {}, flow.DKGStateFailure: {flow.RandomBeaconKeyRecovered}, - flow.DKGStateInconsistentKey: {flow.RandomBeaconKeyRecovered}, - flow.DKGStateNoKey: {flow.RandomBeaconKeyRecovered}, - flow.DKGStateUninitialized: {flow.DKGStateStarted, flow.DKGStateNoKey, flow.DKGStateFailure, flow.DKGStateInconsistentKey, flow.RandomBeaconKeyRecovered}, + flow.DKGStateUninitialized: {flow.DKGStateStarted, flow.DKGStateFailure, flow.RandomBeaconKeyRecovered}, } // RecoverablePrivateBeaconKeyState stores state information about in-progress and completed DKGs, including @@ -141,7 +139,7 @@ func (ds *RecoverablePrivateBeaconKeyState) SetDKGState(epochCounter uint64, new func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter uint64, newState flow.DKGState) func(*transaction.Tx) error { return func(tx *transaction.Tx) error { var currentState flow.DKGState - err := operation.RetrieveDKGEndStateForEpoch(epochCounter, ¤tState)(tx.DBTxn) + err := operation.RetrieveDKGStateForEpoch(epochCounter, ¤tState)(tx.DBTxn) if err != nil { if errors.Is(err, storage.ErrNotFound) { currentState = flow.DKGStateUninitialized @@ -163,14 +161,14 @@ func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter } } - return operation.InsertDKGEndStateForEpoch(epochCounter, newState)(tx.DBTxn) + return operation.InsertDKGStateForEpoch(epochCounter, newState)(tx.DBTxn) } } // GetDKGEndState retrieves the DKG end state for the epoch. func (ds *RecoverablePrivateBeaconKeyState) GetDKGState(epochCounter uint64) (flow.DKGState, error) { var endState flow.DKGState - err := ds.db.Update(operation.RetrieveDKGEndStateForEpoch(epochCounter, &endState)) + err := ds.db.Update(operation.RetrieveDKGStateForEpoch(epochCounter, &endState)) return endState, err } @@ -188,7 +186,7 @@ func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCoun // retrieve the end state var endState flow.DKGState - err = operation.RetrieveDKGEndStateForEpoch(epochCounter, &endState)(txn) + err = operation.RetrieveDKGStateForEpoch(epochCounter, &endState)(txn) if err != nil { key = nil safe = false @@ -233,7 +231,7 @@ func (ds *RecoverablePrivateBeaconKeyState) UpsertMyBeaconPrivateKey(epochCounte if err != nil { return err } - return operation.UpsertDKGEndStateForEpoch(epochCounter, flow.RandomBeaconKeyRecovered)(txn) + return operation.UpsertDKGStateForEpoch(epochCounter, flow.RandomBeaconKeyRecovered)(txn) }) if err != nil { return fmt.Errorf("could not overwrite beacon key for epoch %d: %w", epochCounter, err) diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index d4c858f2c99..8320a9e56e4 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -95,7 +95,7 @@ func TestDKGState_EndState(t *testing.T) { require.NoError(t, err) epochCounter := rand.Uint64() - endState := flow.DKGStateNoKey + endState := flow.DKGStateFailure t.Run("should be able to store an end state", func(t *testing.T) { err = store.SetDKGState(epochCounter, endState) @@ -146,7 +146,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) assert.NoError(t, err) // mark dkg unsuccessful - err = dkgState.SetDKGState(epochCounter, flow.DKGStateInconsistentKey) + err = dkgState.SetDKGState(epochCounter, flow.DKGStateFailure) assert.NoError(t, err) key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) @@ -163,7 +163,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) assert.NoError(t, err) // mark dkg result as inconsistent - err = dkgState.SetDKGState(epochCounter, flow.DKGStateInconsistentKey) + err = dkgState.SetDKGState(epochCounter, flow.DKGStateFailure) assert.NoError(t, err) key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) @@ -176,7 +176,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { epochCounter := rand.Uint64() // mark dkg result as no key - err = dkgState.SetDKGState(epochCounter, flow.DKGStateNoKey) + err = dkgState.SetDKGState(epochCounter, flow.DKGStateFailure) assert.NoError(t, err) key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) diff --git a/storage/badger/operation/dkg.go b/storage/badger/operation/dkg.go index 8dc79272385..8a6dbcd65bf 100644 --- a/storage/badger/operation/dkg.go +++ b/storage/badger/operation/dkg.go @@ -41,13 +41,13 @@ func RetrieveMyBeaconPrivateKey(epochCounter uint64, info *encodable.RandomBeaco return retrieve(makePrefix(codeBeaconPrivateKey, epochCounter), info) } -// RetrieveDKGStartedForEpoch retrieves the DKG started flag for the given epoch. +// RetrieveDKGStartedForEpoch retrieves whether DKG has started for the given epoch. // If no flag is set, started is set to false and no error is returned. // No errors expected during normal operation. func RetrieveDKGStartedForEpoch(epochCounter uint64, started *bool) func(*badger.Txn) error { return func(tx *badger.Txn) error { var state flow.DKGState - err := RetrieveDKGEndStateForEpoch(epochCounter, &state)(tx) + err := RetrieveDKGStateForEpoch(epochCounter, &state)(tx) if errors.Is(err, storage.ErrNotFound) { // flag not set - therefore DKG not started *started = false @@ -63,22 +63,22 @@ func RetrieveDKGStartedForEpoch(epochCounter uint64, started *bool) func(*badger } } -// InsertDKGEndStateForEpoch stores the DKG end state for the epoch. -// Error returns: storage.ErrAlreadyExists -func InsertDKGEndStateForEpoch(epochCounter uint64, endState flow.DKGState) func(*badger.Txn) error { - return insert(makePrefix(codeDKGEnded, epochCounter), endState) +// InsertDKGStateForEpoch stores the DKG current state of Random Beacon Recoverable State Machine for the epoch. +// Error returns: [storage.ErrAlreadyExists] +func InsertDKGStateForEpoch(epochCounter uint64, newState flow.DKGState) func(*badger.Txn) error { + return insert(makePrefix(codeDKGState, epochCounter), newState) } -// UpsertDKGEndStateForEpoch stores the DKG end state for the epoch, irrespective of whether an entry for +// UpsertDKGStateForEpoch stores the current state of Random Beacon Recoverable State Machine for the epoch, irrespective of whether an entry for // the given epoch counter already exists in the database or not. // CAUTION: this method has to be used only in the very specific edge-cases of epoch recovery. For storing the -// DKG results obtained on the happy-path, please use method `InsertDKGEndStateForEpoch`. -func UpsertDKGEndStateForEpoch(epochCounter uint64, endState flow.DKGState) func(*badger.Txn) error { - return upsert(makePrefix(codeDKGEnded, epochCounter), endState) +// DKG results obtained on the happy-path, please use method [InsertDKGStateForEpoch]. +func UpsertDKGStateForEpoch(epochCounter uint64, newState flow.DKGState) func(*badger.Txn) error { + return upsert(makePrefix(codeDKGState, epochCounter), newState) } -// RetrieveDKGEndStateForEpoch retrieves the DKG end state for the epoch. -// Error returns: storage.ErrNotFound -func RetrieveDKGEndStateForEpoch(epochCounter uint64, endState *flow.DKGState) func(*badger.Txn) error { - return retrieve(makePrefix(codeDKGEnded, epochCounter), endState) +// RetrieveDKGStateForEpoch retrieves the DKG end state for the epoch. +// Error returns: [storage.ErrNotFound] +func RetrieveDKGStateForEpoch(epochCounter uint64, newState *flow.DKGState) func(*badger.Txn) error { + return retrieve(makePrefix(codeDKGState, epochCounter), newState) } diff --git a/storage/badger/operation/dkg_test.go b/storage/badger/operation/dkg_test.go index dcc2afa9711..2cf50b74bb2 100644 --- a/storage/badger/operation/dkg_test.go +++ b/storage/badger/operation/dkg_test.go @@ -61,7 +61,7 @@ func TestDKGStartedForEpoch(t *testing.T) { epochCounter := rand.Uint64() // set the flag, ensure no error - err := db.Update(InsertDKGStartedForEpoch(epochCounter)) + err := db.Update(InsertDKGStateForEpoch(epochCounter, flow.DKGStateStarted)) assert.NoError(t, err) // read the flag, should be true now @@ -83,18 +83,18 @@ func TestDKGEndStateForEpoch(t *testing.T) { epochCounter := rand.Uint64() // should be able to write end state - endState := flow.DKGEndStateSuccess - err := db.Update(InsertDKGEndStateForEpoch(epochCounter, endState)) + endState := flow.DKGStateStarted + err := db.Update(InsertDKGStateForEpoch(epochCounter, endState)) assert.NoError(t, err) // should be able to read end state var readEndState flow.DKGState - err = db.View(RetrieveDKGEndStateForEpoch(epochCounter, &readEndState)) + err = db.View(RetrieveDKGStateForEpoch(epochCounter, &readEndState)) assert.NoError(t, err) assert.Equal(t, endState, readEndState) // attempting to overwrite should error - err = db.Update(InsertDKGEndStateForEpoch(epochCounter, flow.DKGEndStateDKGFailure)) + err = db.Update(InsertDKGStateForEpoch(epochCounter, flow.DKGStateFailure)) assert.ErrorIs(t, err, storage.ErrAlreadyExists) }) } diff --git a/storage/badger/operation/prefix.go b/storage/badger/operation/prefix.go index 7bbe97c1f00..ea74552933a 100644 --- a/storage/badger/operation/prefix.go +++ b/storage/badger/operation/prefix.go @@ -73,7 +73,7 @@ const ( codeEpochCommit = 62 // EpochCommit service event, keyed by ID codeBeaconPrivateKey = 63 // BeaconPrivateKey, keyed by epoch counter _ = 64 // [DEPRECATED] flag that the DKG for an epoch has been started - codeDKGEnded = 65 // flag that the DKG for an epoch has ended (stores end state) + codeDKGState = 65 // current state of Recoverable Random Beacon State Machine for given epoch codeVersionBeacon = 67 // flag for storing version beacons codeEpochProtocolState = 68 codeProtocolKVStore = 69 From fb161f7f3a42c19943a8b861b205d60c96234a05 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Thu, 28 Nov 2024 15:00:00 +0200 Subject: [PATCH 13/34] Removed flow.RandomBeaconKeyRecovered state. Cleanup --- model/flow/dkg.go | 5 ----- storage/badger/dkg_state.go | 31 +++++++++++++++---------------- 2 files changed, 15 insertions(+), 21 deletions(-) diff --git a/model/flow/dkg.go b/model/flow/dkg.go index 278aa434ddd..495112f6f1d 100644 --- a/model/flow/dkg.go +++ b/model/flow/dkg.go @@ -20,9 +20,6 @@ const ( RandomBeaconKeyCommitted // DKGStateFailure - the underlying DKG library reported an error. DKGStateFailure - // RandomBeaconKeyRecovered - this node has recovered its beacon key from a previous epoch. - // This occurs only for epochs which are entered through the EFM Recovery process (`flow.EpochRecover` service event). - RandomBeaconKeyRecovered ) func (state DKGState) String() string { @@ -35,8 +32,6 @@ func (state DKGState) String() string { return "RandomBeaconKeyCommitted" case DKGStateFailure: return "DKGStateFailure" - case RandomBeaconKeyRecovered: - return "RandomBeaconKeyRecovered" default: return "DKGStateUninitialized" } diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index de8a6ab0f0e..caace6b781e 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -19,12 +19,11 @@ import ( ) var allowedStateTransitions = map[flow.DKGState][]flow.DKGState{ - flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateFailure, flow.RandomBeaconKeyRecovered}, - flow.DKGStateCompleted: {flow.RandomBeaconKeyCommitted, flow.DKGStateFailure, flow.RandomBeaconKeyRecovered}, + flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted}, + flow.DKGStateCompleted: {flow.RandomBeaconKeyCommitted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted}, flow.RandomBeaconKeyCommitted: {}, - flow.RandomBeaconKeyRecovered: {}, - flow.DKGStateFailure: {flow.RandomBeaconKeyRecovered}, - flow.DKGStateUninitialized: {flow.DKGStateStarted, flow.DKGStateFailure, flow.RandomBeaconKeyRecovered}, + flow.DKGStateFailure: {flow.RandomBeaconKeyCommitted}, + flow.DKGStateUninitialized: {flow.DKGStateStarted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted}, } // RecoverablePrivateBeaconKeyState stores state information about in-progress and completed DKGs, including @@ -32,8 +31,8 @@ var allowedStateTransitions = map[flow.DKGState][]flow.DKGState{ // RecoverablePrivateBeaconKeyState is a specific module that allows to overwrite the beacon private key for a given epoch. // This module is used *ONLY* in the epoch recovery process and only by the consensus participants. // Each consensus participant takes part in the DKG, and after successfully finishing the DKG protocol it obtains a -// random beacon private key, which is stored in the database along with DKG end state `flow.DKGEndStateSuccess`. -// If for any reason the DKG fails, then the private key will be nil and DKG end state will be `flow.DKGEndStateDKGFailure`. +// random beacon private key, which is stored in the database along with DKG current state [flow.DKGStateCompleted]. +// If for any reason the DKG fails, then the private key will be nil and DKG current state will be [flow.DKGStateFailure]. // When the epoch recovery takes place, we need to query the last valid beacon private key for the current replica and // also set it for use during the Recovery Epoch, otherwise replicas won't be able to vote for blocks during the Recovery Epoch. type RecoverablePrivateBeaconKeyState struct { @@ -167,9 +166,9 @@ func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter // GetDKGEndState retrieves the DKG end state for the epoch. func (ds *RecoverablePrivateBeaconKeyState) GetDKGState(epochCounter uint64) (flow.DKGState, error) { - var endState flow.DKGState - err := ds.db.Update(operation.RetrieveDKGStateForEpoch(epochCounter, &endState)) - return endState, err + var currentState flow.DKGState + err := ds.db.Update(operation.RetrieveDKGStateForEpoch(epochCounter, ¤tState)) + return currentState, err } // RetrieveMyBeaconPrivateKey retrieves my beacon private key for the given @@ -185,16 +184,16 @@ func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCoun err = ds.db.View(func(txn *badger.Txn) error { // retrieve the end state - var endState flow.DKGState - err = operation.RetrieveDKGStateForEpoch(epochCounter, &endState)(txn) + var currentState flow.DKGState + err = operation.RetrieveDKGStateForEpoch(epochCounter, ¤tState)(txn) if err != nil { key = nil safe = false return err // storage.ErrNotFound or exception } - // for any end state besides success and recovery, the key is not safe - if endState == flow.RandomBeaconKeyCommitted || endState == flow.RandomBeaconKeyRecovered { + // a key is safe iff it was previously committed + if currentState == flow.RandomBeaconKeyCommitted { // retrieve the key - any storage error (including `storage.ErrNotFound`) is an exception var encodableKey *encodable.RandomBeaconPrivKey encodableKey, err = ds.retrieveKeyTx(epochCounter)(txn) @@ -219,7 +218,7 @@ func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCoun // UpsertMyBeaconPrivateKey overwrites the random beacon private key for the epoch that recovers the protocol from // Epoch Fallback Mode. Effectively, this function overwrites whatever might be available in the database with -// the given private key and sets the DKGState to `flow.DKGEndStateRecovered`. +// the given private key and sets the [flow.DKGState] to [flow.RandomBeaconKeyCommitted]. // No errors are expected during normal operations. func (ds *RecoverablePrivateBeaconKeyState) UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { if key == nil { @@ -231,7 +230,7 @@ func (ds *RecoverablePrivateBeaconKeyState) UpsertMyBeaconPrivateKey(epochCounte if err != nil { return err } - return operation.UpsertDKGStateForEpoch(epochCounter, flow.RandomBeaconKeyRecovered)(txn) + return operation.UpsertDKGStateForEpoch(epochCounter, flow.RandomBeaconKeyCommitted)(txn) }) if err != nil { return fmt.Errorf("could not overwrite beacon key for epoch %d: %w", epochCounter, err) From 650b8b849b4489e5f670bc9a901d3c7700a71ee6 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Thu, 28 Nov 2024 20:42:58 +0200 Subject: [PATCH 14/34] Updated how recovery happens in terms of inserting values --- storage/badger/dkg_state.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index caace6b781e..fdb64b8c65f 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -130,7 +130,8 @@ func (ds *RecoverablePrivateBeaconKeyState) GetDKGStarted(epochCounter uint64) ( return started, err } -// SetDKGEndState stores that the DKG has ended, and its end state. +// SetDKGState stores that the current state for the Recoverable Random Beacon State Machine. +// No errors are expected during normal operations. func (ds *RecoverablePrivateBeaconKeyState) SetDKGState(epochCounter uint64, newState flow.DKGState) error { return operation.RetryOnConflictTx(ds.db, transaction.Update, ds.processStateTransition(epochCounter, newState)) } @@ -225,12 +226,12 @@ func (ds *RecoverablePrivateBeaconKeyState) UpsertMyBeaconPrivateKey(epochCounte return fmt.Errorf("will not store nil beacon key") } encodableKey := &encodable.RandomBeaconPrivKey{PrivateKey: key} - err := ds.db.Update(func(txn *badger.Txn) error { - err := operation.UpsertMyBeaconPrivateKey(epochCounter, encodableKey)(txn) + err := operation.RetryOnConflictTx(ds.db, transaction.Update, func(tx *transaction.Tx) error { + err := operation.UpsertMyBeaconPrivateKey(epochCounter, encodableKey)(tx.DBTxn) if err != nil { return err } - return operation.UpsertDKGStateForEpoch(epochCounter, flow.RandomBeaconKeyCommitted)(txn) + return ds.processStateTransition(epochCounter, flow.RandomBeaconKeyCommitted)(tx) }) if err != nil { return fmt.Errorf("could not overwrite beacon key for epoch %d: %w", epochCounter, err) From 2386623bbfed6f33e36628569e3b6959bf2951eb Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 29 Nov 2024 12:47:41 +0200 Subject: [PATCH 15/34] Implemented test for enforcing invariants of the uninitialized state --- storage/badger/dkg_state_test.go | 78 +++++++++++++++++++++----------- 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index 8320a9e56e4..2e7e237a772 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -1,4 +1,4 @@ -package badger_test +package badger import ( "errors" @@ -12,44 +12,70 @@ import ( "github.com/onflow/flow-go/model/flow" "github.com/onflow/flow-go/module/metrics" "github.com/onflow/flow-go/storage" - bstorage "github.com/onflow/flow-go/storage/badger" "github.com/onflow/flow-go/utils/unittest" ) -func TestDKGState_DKGStarted(t *testing.T) { - unittest.RunWithTypedBadgerDB(t, bstorage.InitSecret, func(db *badger.DB) { +// TestDKGState_UninitializedState checks that invariants are enforced for uninitialized DKG state. +// This test is written in a way that we start with initial state of the Recoverable Random Beacon State Machine and +// try to perform all possible actions and transitions in it. +func TestDKGState_UninitializedState(t *testing.T) { + unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - store, err := bstorage.NewDKGState(metrics, db) + store, err := NewDKGState(metrics, db) require.NoError(t, err) epochCounter := rand.Uint64() - // check dkg-started flag for non-existent epoch - t.Run("DKGStarted should default to false", func(t *testing.T) { - started, err := store.GetDKGStarted(rand.Uint64()) - assert.NoError(t, err) - assert.False(t, started) - }) + started, err := store.GetDKGStarted(epochCounter) + require.NoError(t, err) + require.False(t, started) + + actualState, err := store.GetDKGState(epochCounter) + require.ErrorIs(t, err, storage.ErrNotFound) + require.Equal(t, flow.DKGStateUninitialized, actualState) - // store dkg-started flag for epoch - t.Run("should be able to set DKGStarted", func(t *testing.T) { + pk, err := store.UnsafeRetrieveMyBeaconPrivateKey(epochCounter) + require.ErrorIs(t, err, storage.ErrNotFound) + require.Nil(t, pk) + + pk, safe, err := store.RetrieveMyBeaconPrivateKey(epochCounter) + require.ErrorIs(t, err, storage.ErrNotFound) + require.False(t, safe) + require.Nil(t, pk) + + t.Run("-> flow.DKGStateStarted, should be allowed", func(t *testing.T) { + epochCounter++ err = store.SetDKGState(epochCounter, flow.DKGStateStarted) - assert.NoError(t, err) + require.NoError(t, err) }) - // retrieve flag for epoch - t.Run("should be able to read DKGStarted", func(t *testing.T) { - started, err := store.GetDKGStarted(epochCounter) - assert.NoError(t, err) - assert.True(t, started) + t.Run("-> flow.DKGStateFailure, should be allowed", func(t *testing.T) { + epochCounter++ + err = store.SetDKGState(epochCounter, flow.DKGStateFailure) + require.NoError(t, err) + }) + + t.Run("-> flow.DKGStateCompleted, not allowed", func(t *testing.T) { + epochCounter++ + err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) + require.Error(t, err, "should not be able to enter completed state without starting") + err = store.SetDKGState(epochCounter, flow.DKGStateCompleted) + require.Error(t, err, "should not be able to enter completed state without starting") + }) + + t.Run("-> flow.RandomBeaconKeyCommitted, should be allowed", func(t *testing.T) { + err = store.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) + require.Error(t, err, "should not be able to set DKG state to recovered, only using dedicated interface") + err = store.UpsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) + require.NoError(t, err) }) }) } func TestDKGState_BeaconKeys(t *testing.T) { - unittest.RunWithTypedBadgerDB(t, bstorage.InitSecret, func(db *badger.DB) { + unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - store, err := bstorage.NewDKGState(metrics, db) + store, err := NewDKGState(metrics, db) require.NoError(t, err) epochCounter := rand.Uint64() @@ -89,9 +115,9 @@ func TestDKGState_BeaconKeys(t *testing.T) { } func TestDKGState_EndState(t *testing.T) { - unittest.RunWithTypedBadgerDB(t, bstorage.InitSecret, func(db *badger.DB) { + unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - store, err := bstorage.NewDKGState(metrics, db) + store, err := NewDKGState(metrics, db) require.NoError(t, err) epochCounter := rand.Uint64() @@ -111,9 +137,9 @@ func TestDKGState_EndState(t *testing.T) { } func TestSafeBeaconPrivateKeys(t *testing.T) { - unittest.RunWithTypedBadgerDB(t, bstorage.InitSecret, func(db *badger.DB) { + unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - dkgState, err := bstorage.NewDKGState(metrics, db) + dkgState, err := NewDKGState(metrics, db) require.NoError(t, err) t.Run("non-existent key -> should return ErrNotFound", func(t *testing.T) { @@ -225,7 +251,7 @@ func TestSafeBeaconPrivateKeys(t *testing.T) { func TestSecretDBRequirement(t *testing.T) { unittest.RunWithBadgerDB(t, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - _, err := bstorage.NewDKGState(metrics, db) + _, err := NewDKGState(metrics, db) require.Error(t, err) }) } From bfe95b0a778ba32cba5f38bef36fbcbbeeed6330 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 29 Nov 2024 16:18:40 +0200 Subject: [PATCH 16/34] Added additional test cases. --- storage/badger/dkg_state.go | 1 + storage/badger/dkg_state_test.go | 77 ++++++++++++++++++++++---------- 2 files changed, 55 insertions(+), 23 deletions(-) diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index fdb64b8c65f..53ba4b8cd09 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -210,6 +210,7 @@ func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCoun } else { key = nil safe = false + return storage.ErrNotFound } return nil diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index 2e7e237a772..3f35090b605 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -1,7 +1,6 @@ package badger import ( - "errors" "math/rand" "testing" @@ -43,6 +42,12 @@ func TestDKGState_UninitializedState(t *testing.T) { require.False(t, safe) require.Nil(t, pk) + t.Run("-> flow.DKGStateUninitialized, not allowed", func(t *testing.T) { + epochCounter++ + err = store.SetDKGState(epochCounter, flow.DKGStateUninitialized) + require.Error(t, err) + }) + t.Run("-> flow.DKGStateStarted, should be allowed", func(t *testing.T) { epochCounter++ err = store.SetDKGState(epochCounter, flow.DKGStateStarted) @@ -64,6 +69,7 @@ func TestDKGState_UninitializedState(t *testing.T) { }) t.Run("-> flow.RandomBeaconKeyCommitted, should be allowed", func(t *testing.T) { + epochCounter++ err = store.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) require.Error(t, err, "should not be able to set DKG state to recovered, only using dedicated interface") err = store.UpsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) @@ -72,44 +78,69 @@ func TestDKGState_UninitializedState(t *testing.T) { }) } -func TestDKGState_BeaconKeys(t *testing.T) { +// TestDKGState_UninitializedState checks that invariants are enforced for uninitialized DKG state. +// This test is written in a way that we start with initial state of the Recoverable Random Beacon State Machine and +// try to perform all possible actions and transitions in it. +func TestDKGState_StartedState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() store, err := NewDKGState(metrics, db) require.NoError(t, err) - epochCounter := rand.Uint64() + setupState := func() uint64 { + epochCounter := rand.Uint64() + err = store.SetDKGState(epochCounter, flow.DKGStateStarted) + require.NoError(t, err) + return epochCounter + } + epochCounter := setupState() + + actualState, err := store.GetDKGState(epochCounter) + require.NoError(t, err, storage.ErrNotFound) + require.Equal(t, flow.DKGStateStarted, actualState) + + started, err := store.GetDKGStarted(epochCounter) + require.NoError(t, err) + require.True(t, started) + + pk, err := store.UnsafeRetrieveMyBeaconPrivateKey(epochCounter) + require.ErrorIs(t, err, storage.ErrNotFound) + require.Nil(t, pk) - // attempt to get a non-existent key - t.Run("should error if retrieving non-existent key", func(t *testing.T) { - _, err = store.UnsafeRetrieveMyBeaconPrivateKey(epochCounter) - assert.True(t, errors.Is(err, storage.ErrNotFound)) + pk, safe, err := store.RetrieveMyBeaconPrivateKey(epochCounter) + require.ErrorIs(t, err, storage.ErrNotFound) + require.False(t, safe) + require.Nil(t, pk) + + t.Run("-> flow.DKGStateUninitialized, not allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateUninitialized) + require.Error(t, err) }) - // attempt to store a nil key should fail - use RecoverablePrivateBeaconKeyState.SetEndState(flow.DKGStateNoKey) - t.Run("should fail to store a nil key instead)", func(t *testing.T) { - err = store.InsertMyBeaconPrivateKey(epochCounter, nil) - assert.Error(t, err) + t.Run("-> flow.DKGStateStarted, not allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateStarted) + require.Error(t, err) }) - // store a key in db - expected := unittest.RandomBeaconPriv() - t.Run("should be able to store and read a key", func(t *testing.T) { - err = store.InsertMyBeaconPrivateKey(epochCounter, expected) + t.Run("-> flow.DKGStateFailure, should be allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateFailure) require.NoError(t, err) }) - // retrieve the key by epoch counter - t.Run("should be able to retrieve stored key", func(t *testing.T) { - actual, err := store.UnsafeRetrieveMyBeaconPrivateKey(epochCounter) + t.Run("-> flow.DKGStateCompleted, should be allowed", func(t *testing.T) { + epochCounter := setupState() + err = store.SetDKGState(epochCounter, flow.DKGStateCompleted) + require.Error(t, err, "should not be able to enter completed state without providing a private key") + err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) require.NoError(t, err) - assert.Equal(t, expected, actual) }) - // test storing same key - t.Run("should fail to store a key twice", func(t *testing.T) { - err = store.InsertMyBeaconPrivateKey(epochCounter, expected) - require.True(t, errors.Is(err, storage.ErrAlreadyExists)) + t.Run("-> flow.RandomBeaconKeyCommitted, should be allowed", func(t *testing.T) { + epochCounter := setupState() + err = store.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) + require.Error(t, err, "should not be able to set DKG state to recovered, only using dedicated interface") + err = store.UpsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) + require.NoError(t, err) }) }) } From 79aac04138277b83f058c22095a7295e4a3b1c2d Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 29 Nov 2024 16:21:44 +0200 Subject: [PATCH 17/34] Updated logic for state transitions --- storage/badger/dkg_state.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index 53ba4b8cd09..16edd9bb573 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -154,14 +154,14 @@ func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter } // ensure invariant holds and we still have a valid private key stored - if newState == flow.RandomBeaconKeyCommitted { + if newState == flow.RandomBeaconKeyCommitted || newState == flow.DKGStateCompleted { _, err = ds.retrieveKeyTx(epochCounter)(tx.DBTxn) if err != nil { - return fmt.Errorf("cannot transition to flow.RandomBeaconKeyCommitted without a valid random beacon key: %w", err) + return fmt.Errorf("cannot transition to %s without a valid random beacon key: %w", newState, err) } } - return operation.InsertDKGStateForEpoch(epochCounter, newState)(tx.DBTxn) + return operation.UpsertDKGStateForEpoch(epochCounter, newState)(tx.DBTxn) } } From 1119c8c6a0dbfd9a763d8e27469d61a9e42335c8 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 29 Nov 2024 16:26:45 +0200 Subject: [PATCH 18/34] Added additional test for Completed state --- storage/badger/dkg_state_test.go | 67 ++++++++++++++++++++++++++++---- 1 file changed, 59 insertions(+), 8 deletions(-) diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index 3f35090b605..b8b7c77b931 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -145,24 +145,75 @@ func TestDKGState_StartedState(t *testing.T) { }) } -func TestDKGState_EndState(t *testing.T) { +// TestDKGState_UninitializedState checks that invariants are enforced for uninitialized DKG state. +// This test is written in a way that we start with initial state of the Recoverable Random Beacon State Machine and +// try to perform all possible actions and transitions in it. +func TestDKGState_CompletedState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() store, err := NewDKGState(metrics, db) require.NoError(t, err) - epochCounter := rand.Uint64() - endState := flow.DKGStateFailure + setupState := func() uint64 { + epochCounter := rand.Uint64() + err = store.SetDKGState(epochCounter, flow.DKGStateStarted) + require.NoError(t, err) + err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) + require.NoError(t, err) + return epochCounter + } + epochCounter := setupState() - t.Run("should be able to store an end state", func(t *testing.T) { - err = store.SetDKGState(epochCounter, endState) + actualState, err := store.GetDKGState(epochCounter) + require.NoError(t, err, storage.ErrNotFound) + require.Equal(t, flow.DKGStateCompleted, actualState) + + started, err := store.GetDKGStarted(epochCounter) + require.NoError(t, err) + require.True(t, started) + + pk, err := store.UnsafeRetrieveMyBeaconPrivateKey(epochCounter) + require.NoError(t, err) + require.NotNil(t, pk) + + pk, safe, err := store.RetrieveMyBeaconPrivateKey(epochCounter) + require.ErrorIs(t, err, storage.ErrNotFound) + require.False(t, safe) + require.Nil(t, pk) + + t.Run("-> flow.DKGStateUninitialized, not allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateUninitialized) + require.Error(t, err) + }) + + t.Run("-> flow.DKGStateStarted, not allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateStarted) + require.Error(t, err) + }) + + t.Run("-> flow.DKGStateFailure, should be allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateFailure) require.NoError(t, err) }) - t.Run("should be able to read an end state", func(t *testing.T) { - readEndState, err := store.GetDKGState(epochCounter) + t.Run("-> flow.DKGStateCompleted, not allowed", func(t *testing.T) { + epochCounter := setupState() + err = store.SetDKGState(epochCounter, flow.DKGStateCompleted) + require.Error(t, err, "already in this state") + err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) + require.Error(t, err, "already inserted private key") + }) + + t.Run("-> flow.RandomBeaconKeyCommitted, should be allowed", func(t *testing.T) { + epochCounter := setupState() + err = store.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) + require.NoError(t, err, "should be allowed since we have a stored private key") + }) + + t.Run("-> flow.RandomBeaconKeyCommitted(recovery), should be allowed", func(t *testing.T) { + epochCounter := setupState() + err = store.UpsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) require.NoError(t, err) - assert.Equal(t, endState, readEndState) }) }) } From c8dfca6cd6d4c3e2f9f498ba0eba83ad95620271 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 29 Nov 2024 16:52:05 +0200 Subject: [PATCH 19/34] Added tests for failure state --- storage/badger/dkg_state_test.go | 135 +++++++++++-------------------- 1 file changed, 46 insertions(+), 89 deletions(-) diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index b8b7c77b931..c26d49a636c 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/dgraph-io/badger/v2" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/onflow/flow-go/model/flow" @@ -218,113 +217,71 @@ func TestDKGState_CompletedState(t *testing.T) { }) } -func TestSafeBeaconPrivateKeys(t *testing.T) { +// TestDKGState_UninitializedState checks that invariants are enforced for uninitialized DKG state. +// This test is written in a way that we start with initial state of the Recoverable Random Beacon State Machine and +// try to perform all possible actions and transitions in it. +func TestDKGState_FailureState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - dkgState, err := NewDKGState(metrics, db) + store, err := NewDKGState(metrics, db) require.NoError(t, err) - t.Run("non-existent key -> should return ErrNotFound", func(t *testing.T) { + setupState := func() uint64 { epochCounter := rand.Uint64() - key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) - assert.Nil(t, key) - assert.False(t, safe) - assert.ErrorIs(t, err, storage.ErrNotFound) - }) + err = store.SetDKGState(epochCounter, flow.DKGStateFailure) + require.NoError(t, err) + return epochCounter + } + epochCounter := setupState() - t.Run("existent key, non-existent end state -> should return ErrNotFound", func(t *testing.T) { - epochCounter := rand.Uint64() + actualState, err := store.GetDKGState(epochCounter) + require.NoError(t, err) + require.Equal(t, flow.DKGStateFailure, actualState) - // store a key - expected := unittest.RandomBeaconPriv().PrivateKey - err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) - assert.NoError(t, err) + started, err := store.GetDKGStarted(epochCounter) + require.NoError(t, err) + require.True(t, started) - key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) - assert.Nil(t, key) - assert.False(t, safe) - assert.ErrorIs(t, err, storage.ErrNotFound) - }) + pk, safe, err := store.RetrieveMyBeaconPrivateKey(epochCounter) + require.ErrorIs(t, err, storage.ErrNotFound) + require.False(t, safe) + require.Nil(t, pk) - t.Run("existent key, unsuccessful end state -> not safe", func(t *testing.T) { - epochCounter := rand.Uint64() + pk, safe, err = store.RetrieveMyBeaconPrivateKey(epochCounter) + require.ErrorIs(t, err, storage.ErrNotFound) + require.False(t, safe) + require.Nil(t, pk) - // store a key - expected := unittest.RandomBeaconPriv().PrivateKey - err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) - assert.NoError(t, err) - // mark dkg unsuccessful - err = dkgState.SetDKGState(epochCounter, flow.DKGStateFailure) - assert.NoError(t, err) - - key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) - assert.Nil(t, key) - assert.False(t, safe) - assert.NoError(t, err) + t.Run("-> flow.DKGStateUninitialized, not allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateUninitialized) + require.Error(t, err) }) - t.Run("existent key, inconsistent key end state -> not safe", func(t *testing.T) { - epochCounter := rand.Uint64() - - // store a key - expected := unittest.RandomBeaconPriv().PrivateKey - err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) - assert.NoError(t, err) - // mark dkg result as inconsistent - err = dkgState.SetDKGState(epochCounter, flow.DKGStateFailure) - assert.NoError(t, err) - - key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) - assert.Nil(t, key) - assert.False(t, safe) - assert.NoError(t, err) + t.Run("-> flow.DKGStateStarted, not allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateStarted) + require.Error(t, err) }) - t.Run("non-existent key, no key end state -> not safe", func(t *testing.T) { - epochCounter := rand.Uint64() - - // mark dkg result as no key - err = dkgState.SetDKGState(epochCounter, flow.DKGStateFailure) - assert.NoError(t, err) - - key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) - assert.Nil(t, key) - assert.False(t, safe) - assert.NoError(t, err) + t.Run("-> flow.DKGStateFailure, not allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateFailure) + require.Error(t, err) }) - t.Run("existent key, successful end state -> safe", func(t *testing.T) { - epochCounter := rand.Uint64() - - // store a key - expected := unittest.RandomBeaconPriv().PrivateKey - err := dkgState.InsertMyBeaconPrivateKey(epochCounter, expected) - assert.NoError(t, err) - // mark dkg successful - err = dkgState.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) - assert.NoError(t, err) - - key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) - assert.NotNil(t, key) - assert.True(t, expected.Equals(key)) - assert.True(t, safe) - assert.NoError(t, err) + t.Run("-> flow.DKGStateCompleted, not allowed", func(t *testing.T) { + epochCounter := setupState() + err = store.SetDKGState(epochCounter, flow.DKGStateCompleted) + require.Error(t, err) + err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) + require.Error(t, err) }) - t.Run("non-existent key, successful end state -> exception!", func(t *testing.T) { - epochCounter := rand.Uint64() - - // mark dkg successful - err = dkgState.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) - assert.NoError(t, err) - - key, safe, err := dkgState.RetrieveMyBeaconPrivateKey(epochCounter) - assert.Nil(t, key) - assert.False(t, safe) - assert.Error(t, err) - assert.NotErrorIs(t, err, storage.ErrNotFound) + t.Run("-> flow.RandomBeaconKeyCommitted, should be allowed", func(t *testing.T) { + epochCounter := setupState() + err = store.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) + require.Error(t, err, "should not be able to set DKG state to recovered, only using dedicated interface") + err = store.UpsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) + require.NoError(t, err) }) - }) } From c9182c5fd86787aed89d2c1b97b9a4ca20aae466 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Fri, 29 Nov 2024 18:01:06 +0200 Subject: [PATCH 20/34] Added extra tests for Random Beacon Key Committed state --- storage/badger/dkg_state.go | 2 +- storage/badger/dkg_state_test.go | 77 ++++++++++++++++++++++++++++++-- 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index 16edd9bb573..11258d056db 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -21,7 +21,7 @@ import ( var allowedStateTransitions = map[flow.DKGState][]flow.DKGState{ flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted}, flow.DKGStateCompleted: {flow.RandomBeaconKeyCommitted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted}, - flow.RandomBeaconKeyCommitted: {}, + flow.RandomBeaconKeyCommitted: {flow.RandomBeaconKeyCommitted}, flow.DKGStateFailure: {flow.RandomBeaconKeyCommitted}, flow.DKGStateUninitialized: {flow.DKGStateStarted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted}, } diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index c26d49a636c..b8c7c7ef23b 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -242,12 +242,11 @@ func TestDKGState_FailureState(t *testing.T) { require.NoError(t, err) require.True(t, started) - pk, safe, err := store.RetrieveMyBeaconPrivateKey(epochCounter) + pk, err := store.UnsafeRetrieveMyBeaconPrivateKey(epochCounter) require.ErrorIs(t, err, storage.ErrNotFound) - require.False(t, safe) require.Nil(t, pk) - pk, safe, err = store.RetrieveMyBeaconPrivateKey(epochCounter) + pk, safe, err := store.RetrieveMyBeaconPrivateKey(epochCounter) require.ErrorIs(t, err, storage.ErrNotFound) require.False(t, safe) require.Nil(t, pk) @@ -279,6 +278,78 @@ func TestDKGState_FailureState(t *testing.T) { epochCounter := setupState() err = store.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) require.Error(t, err, "should not be able to set DKG state to recovered, only using dedicated interface") + expectedKey := unittest.RandomBeaconPriv() + err = store.UpsertMyBeaconPrivateKey(epochCounter, expectedKey) + require.NoError(t, err) + actualKey, safe, err := store.RetrieveMyBeaconPrivateKey(epochCounter) + require.NoError(t, err) + require.True(t, safe) + require.Equal(t, expectedKey, actualKey) + }) + }) +} + +// TestDKGState_UninitializedState checks that invariants are enforced for uninitialized DKG state. +// This test is written in a way that we start with initial state of the Recoverable Random Beacon State Machine and +// try to perform all possible actions and transitions in it. +func TestDKGState_RandomBeaconKeyCommittedState(t *testing.T) { + unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { + metrics := metrics.NewNoopCollector() + store, err := NewDKGState(metrics, db) + require.NoError(t, err) + + setupState := func() uint64 { + epochCounter := rand.Uint64() + err = store.UpsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) + require.NoError(t, err) + return epochCounter + } + epochCounter := setupState() + + actualState, err := store.GetDKGState(epochCounter) + require.NoError(t, err) + require.Equal(t, flow.RandomBeaconKeyCommitted, actualState) + + started, err := store.GetDKGStarted(epochCounter) + require.NoError(t, err) + require.True(t, started) + + pk, err := store.UnsafeRetrieveMyBeaconPrivateKey(epochCounter) + require.NoError(t, err) + require.NotNil(t, pk) + + pk, safe, err := store.RetrieveMyBeaconPrivateKey(epochCounter) + require.NoError(t, err) + require.True(t, safe) + require.NotNil(t, pk) + + t.Run("-> flow.DKGStateUninitialized, not allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateUninitialized) + require.Error(t, err) + }) + + t.Run("-> flow.DKGStateStarted, not allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateStarted) + require.Error(t, err) + }) + + t.Run("-> flow.DKGStateFailure, not allowed", func(t *testing.T) { + err = store.SetDKGState(setupState(), flow.DKGStateFailure) + require.Error(t, err) + }) + + t.Run("-> flow.DKGStateCompleted, not allowed", func(t *testing.T) { + epochCounter := setupState() + err = store.SetDKGState(epochCounter, flow.DKGStateCompleted) + require.Error(t, err) + err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) + require.Error(t, err) + }) + + t.Run("-> flow.RandomBeaconKeyCommitted, allowed", func(t *testing.T) { + epochCounter := setupState() + err = store.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) + require.NoError(t, err, "should be possible since we have a stored private key") err = store.UpsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) require.NoError(t, err) }) From 577712a68f5dcf2a0d309b315638b16d02d8f3ca Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 2 Dec 2024 12:30:26 +0200 Subject: [PATCH 21/34] Updated godoc for DKG tests --- storage/badger/dkg_state_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index b8c7c7ef23b..6b9597a9f66 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -13,7 +13,7 @@ import ( "github.com/onflow/flow-go/utils/unittest" ) -// TestDKGState_UninitializedState checks that invariants are enforced for uninitialized DKG state. +// TestDKGState_UninitializedState checks that invariants are enforced for [flow.DKGStateUninitialized] state. // This test is written in a way that we start with initial state of the Recoverable Random Beacon State Machine and // try to perform all possible actions and transitions in it. func TestDKGState_UninitializedState(t *testing.T) { @@ -77,8 +77,8 @@ func TestDKGState_UninitializedState(t *testing.T) { }) } -// TestDKGState_UninitializedState checks that invariants are enforced for uninitialized DKG state. -// This test is written in a way that we start with initial state of the Recoverable Random Beacon State Machine and +// TestDKGState_StartedState checks that invariants are enforced for [flow.DKGStateStarted] state. +// This test is written in a way that we start in [flow.DKGStateStarted] of the Recoverable Random Beacon State Machine and // try to perform all possible actions and transitions in it. func TestDKGState_StartedState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { @@ -144,9 +144,9 @@ func TestDKGState_StartedState(t *testing.T) { }) } -// TestDKGState_UninitializedState checks that invariants are enforced for uninitialized DKG state. -// This test is written in a way that we start with initial state of the Recoverable Random Beacon State Machine and -// try to perform all possible actions and transitions in it. +// TestDKGState_CompletedState checks that invariants are enforced for [flow.DKGStateCompleted] state. +// This test is written in a way that we start in [flow.DKGStateCompleted] of the Recoverable Random Beacon State Machine and +// try to perform all possible actions and transitions in it. We enter [flow.DKGStateCompleted] by inserting a mock private key. func TestDKGState_CompletedState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() @@ -217,8 +217,8 @@ func TestDKGState_CompletedState(t *testing.T) { }) } -// TestDKGState_UninitializedState checks that invariants are enforced for uninitialized DKG state. -// This test is written in a way that we start with initial state of the Recoverable Random Beacon State Machine and +// TestDKGState_FailureState checks that invariants are enforced for [flow.DKGStateFailure] state. +// This test is written in a way that we start with [flow.DKGStateFailure] of the Recoverable Random Beacon State Machine and // try to perform all possible actions and transitions in it. func TestDKGState_FailureState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { @@ -289,8 +289,8 @@ func TestDKGState_FailureState(t *testing.T) { }) } -// TestDKGState_UninitializedState checks that invariants are enforced for uninitialized DKG state. -// This test is written in a way that we start with initial state of the Recoverable Random Beacon State Machine and +// TestDKGState_RandomBeaconKeyCommittedState checks that invariants are enforced for [flow.RandomBeaconKeyCommitted] state. +// This test is written in a way that we start with [flow.RandomBeaconKeyCommitted] state of the Recoverable Random Beacon State Machine and // try to perform all possible actions and transitions in it. func TestDKGState_RandomBeaconKeyCommittedState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { From 7e728aa9b6492e9386ce989d7aa3f981859edfb5 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 2 Dec 2024 16:51:12 +0200 Subject: [PATCH 22/34] Godoc updates --- storage/badger/dkg_state.go | 41 +++++++++++++++---------------------- storage/dkg.go | 32 +++++++++++++++++------------ 2 files changed, 36 insertions(+), 37 deletions(-) diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index 11258d056db..81e10a12d8b 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -18,6 +18,7 @@ import ( "github.com/onflow/flow-go/storage/badger/transaction" ) +// allowedStateTransitions defines the allowed state transitions for the Recoverable Random Beacon State Machine. var allowedStateTransitions = map[flow.DKGState][]flow.DKGState{ flow.DKGStateStarted: {flow.DKGStateCompleted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted}, flow.DKGStateCompleted: {flow.RandomBeaconKeyCommitted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted}, @@ -67,26 +68,10 @@ func NewDKGState(collector module.CacheMetrics, db *badger.DB) (*RecoverablePriv withRetrieve(retrieveKey), ) - dkgState := &RecoverablePrivateBeaconKeyState{ + return &RecoverablePrivateBeaconKeyState{ db: db, keyCache: cache, - } - - return dkgState, nil -} - -func (ds *RecoverablePrivateBeaconKeyState) storeKeyTx(epochCounter uint64, key *encodable.RandomBeaconPrivKey) func(tx *transaction.Tx) error { - return ds.keyCache.PutTx(epochCounter, key) -} - -func (ds *RecoverablePrivateBeaconKeyState) retrieveKeyTx(epochCounter uint64) func(tx *badger.Txn) (*encodable.RandomBeaconPrivKey, error) { - return func(tx *badger.Txn) (*encodable.RandomBeaconPrivKey, error) { - val, err := ds.keyCache.Get(epochCounter)(tx) - if err != nil { - return nil, err - } - return val, nil - } + }, nil } // InsertMyBeaconPrivateKey stores the random beacon private key for an epoch. @@ -100,7 +85,7 @@ func (ds *RecoverablePrivateBeaconKeyState) InsertMyBeaconPrivateKey(epochCounte } encodableKey := &encodable.RandomBeaconPrivKey{PrivateKey: key} return operation.RetryOnConflictTx(ds.db, transaction.Update, func(tx *transaction.Tx) error { - err := ds.storeKeyTx(epochCounter, encodableKey)(tx) + err := ds.keyCache.PutTx(epochCounter, encodableKey)(tx) if err != nil { return err } @@ -113,10 +98,11 @@ func (ds *RecoverablePrivateBeaconKeyState) InsertMyBeaconPrivateKey(epochCounte // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned +// Error returns: storage.ErrNotFound func (ds *RecoverablePrivateBeaconKeyState) UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) { tx := ds.db.NewTransaction(false) defer tx.Discard() - encodableKey, err := ds.retrieveKeyTx(epochCounter)(tx) + encodableKey, err := ds.keyCache.Get(epochCounter)(tx) if err != nil { return nil, err } @@ -124,13 +110,18 @@ func (ds *RecoverablePrivateBeaconKeyState) UnsafeRetrieveMyBeaconPrivateKey(epo } // GetDKGStarted checks whether the DKG has been started for the given epoch. +// No errors expected during normal operation. func (ds *RecoverablePrivateBeaconKeyState) GetDKGStarted(epochCounter uint64) (bool, error) { var started bool err := ds.db.View(operation.RetrieveDKGStartedForEpoch(epochCounter, &started)) return started, err } -// SetDKGState stores that the current state for the Recoverable Random Beacon State Machine. +// SetDKGState performs a state transition for the Random Beacon Recoverable State Machine. +// Some state transitions may not be possible using this method. For instance, we might not be able to enter [flow.DKGStateCompleted] +// state directly from [flow.DKGStateStarted], even if such transition is valid. The reason for this is that some states require additional +// data to be processed by the state machine before the transition can be made. For such cases there are dedicated methods that should be used, ex. +// InsertMyBeaconPrivateKey and UpsertMyBeaconPrivateKey, which allow to store the needed data and perform the transition in one atomic operation. // No errors are expected during normal operations. func (ds *RecoverablePrivateBeaconKeyState) SetDKGState(epochCounter uint64, newState flow.DKGState) error { return operation.RetryOnConflictTx(ds.db, transaction.Update, ds.processStateTransition(epochCounter, newState)) @@ -155,7 +146,7 @@ func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter // ensure invariant holds and we still have a valid private key stored if newState == flow.RandomBeaconKeyCommitted || newState == flow.DKGStateCompleted { - _, err = ds.retrieveKeyTx(epochCounter)(tx.DBTxn) + _, err = ds.keyCache.Get(epochCounter)(tx.DBTxn) if err != nil { return fmt.Errorf("cannot transition to %s without a valid random beacon key: %w", newState, err) } @@ -165,7 +156,9 @@ func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter } } -// GetDKGEndState retrieves the DKG end state for the epoch. +// GetDKGState retrieves the current state of the state machine for the given epoch. +// If an error is returned, the state is undefined meaning that state machine is in initial state +// Error returns: storage.ErrNotFound. func (ds *RecoverablePrivateBeaconKeyState) GetDKGState(epochCounter uint64) (flow.DKGState, error) { var currentState flow.DKGState err := ds.db.Update(operation.RetrieveDKGStateForEpoch(epochCounter, ¤tState)) @@ -197,7 +190,7 @@ func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCoun if currentState == flow.RandomBeaconKeyCommitted { // retrieve the key - any storage error (including `storage.ErrNotFound`) is an exception var encodableKey *encodable.RandomBeaconPrivKey - encodableKey, err = ds.retrieveKeyTx(epochCounter)(txn) + encodableKey, err = ds.keyCache.Get(epochCounter)(txn) if err != nil { key = nil safe = false diff --git a/storage/dkg.go b/storage/dkg.go index b954771aee6..9aea4c61665 100644 --- a/storage/dkg.go +++ b/storage/dkg.go @@ -21,37 +21,43 @@ type SafeBeaconKeys interface { RetrieveMyBeaconPrivateKey(epochCounter uint64) (key crypto.PrivateKey, safe bool, err error) } -// DKGStateReader ... +// DKGStateReader is a ready-only interface for reading state of the Random Beacon Recoverable State Machine. type DKGStateReader interface { SafeBeaconKeys - // GetDKGEndState retrieves the end state for the given DKG. - // Error returns: storage.ErrNotFound + // GetDKGState retrieves the current state of the state machine for the given epoch. + // If an error is returned, the state is undefined meaning that state machine is in initial state + // Error returns: storage.ErrNotFound. GetDKGState(epochCounter uint64) (flow.DKGState, error) + // GetDKGStarted checks whether the DKG has been started for the given epoch. + // No errors expected during normal operation. + GetDKGStarted(epochCounter uint64) (bool, error) + // UnsafeRetrieveMyBeaconPrivateKey retrieves the random beacon private key for an epoch. // // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned - // Error returns: storage.ErrNotFound + // Error returns: storage.ErrNotFound. UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) } // DKGState is the storage interface for storing all artifacts and state -// related to the DKG process, including the latest state of a running or -// completed DKG, and computed beacon keys. +// related to the DKG process, including the latest state of a running or completed DKG, and computed beacon keys. +// It allows to initiate state transitions to the Random Beacon Recoverable State Machine by calling respective methods. +// It supports all state transitions for the happy path. Recovery from the epoch fallback mode is supported by the EpochRecoveryMyBeaconKey interface. type DKGState interface { DKGStateReader - // SetDKGEndState stores that the DKG has ended, and its end state. - // Error returns: storage.ErrAlreadyExists + // SetDKGState performs a state transition for the Random Beacon Recoverable State Machine. + // Some state transitions may not be possible using this method. For instance, we might not be able to enter [flow.DKGStateCompleted] + // state directly from [flow.DKGStateStarted], even if such transition is valid. The reason for this is that some states require additional + // data to be processed by the state machine before the transition can be made. For such cases there are dedicated methods that should be used, ex. + // InsertMyBeaconPrivateKey and UpsertMyBeaconPrivateKey, which allow to store the needed data and perform the transition in one atomic operation. + // No errors are expected during normal operations. SetDKGState(epochCounter uint64, newState flow.DKGState) error - // GetDKGStarted checks whether the DKG has been started for the given epoch. - // No errors expected during normal operation. - GetDKGStarted(epochCounter uint64) (bool, error) - // InsertMyBeaconPrivateKey stores the random beacon private key for an epoch. // // CAUTION: these keys are stored before they are validated against the @@ -64,7 +70,7 @@ type DKGState interface { // EpochRecoveryMyBeaconKey is a specific interface that allows to overwrite the beacon private key for given epoch. // This interface is used *ONLY* in the epoch recovery process and only by the consensus participants. // Each consensus participant takes part in the DKG, after finishing the DKG protocol each replica obtains a random beacon -// private key which is stored in the database along with DKG end state which will be equal to flow.DKGEndStateSuccess. +// private key which is stored in the database along with DKG state which will be equal to [flow.RandomBeaconKeyCommitted]. // If for any reason DKG fails, then the private key will be nil and DKG end state will be equal to flow.DKGEndStateDKGFailure. // It's not a problem by itself, but when the epoch recovery takes place, we need to query last valid beacon private key for // the current replica and set it for recovered epoch, otherwise replicas won't be able to vote for blocks in the recovered epoch. From 1fa11c628b358a9161883f01f57c2a9ecb842d15 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 2 Dec 2024 16:52:32 +0200 Subject: [PATCH 23/34] Updated mocks --- storage/mock/dkg_state_reader.go | 28 ++++++++++++++++++++ storage/mock/epoch_recovery_my_beacon_key.go | 28 ++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/storage/mock/dkg_state_reader.go b/storage/mock/dkg_state_reader.go index 429f4966fc5..eb62bf0ecab 100644 --- a/storage/mock/dkg_state_reader.go +++ b/storage/mock/dkg_state_reader.go @@ -14,6 +14,34 @@ type DKGStateReader struct { mock.Mock } +// GetDKGStarted provides a mock function with given fields: epochCounter +func (_m *DKGStateReader) GetDKGStarted(epochCounter uint64) (bool, error) { + ret := _m.Called(epochCounter) + + if len(ret) == 0 { + panic("no return value specified for GetDKGStarted") + } + + var r0 bool + var r1 error + if rf, ok := ret.Get(0).(func(uint64) (bool, error)); ok { + return rf(epochCounter) + } + if rf, ok := ret.Get(0).(func(uint64) bool); ok { + r0 = rf(epochCounter) + } else { + r0 = ret.Get(0).(bool) + } + + if rf, ok := ret.Get(1).(func(uint64) error); ok { + r1 = rf(epochCounter) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // GetDKGState provides a mock function with given fields: epochCounter func (_m *DKGStateReader) GetDKGState(epochCounter uint64) (flow.DKGState, error) { ret := _m.Called(epochCounter) diff --git a/storage/mock/epoch_recovery_my_beacon_key.go b/storage/mock/epoch_recovery_my_beacon_key.go index 8dd7311b72b..79c5a817a5a 100644 --- a/storage/mock/epoch_recovery_my_beacon_key.go +++ b/storage/mock/epoch_recovery_my_beacon_key.go @@ -14,6 +14,34 @@ type EpochRecoveryMyBeaconKey struct { mock.Mock } +// GetDKGStarted provides a mock function with given fields: epochCounter +func (_m *EpochRecoveryMyBeaconKey) GetDKGStarted(epochCounter uint64) (bool, error) { + ret := _m.Called(epochCounter) + + if len(ret) == 0 { + panic("no return value specified for GetDKGStarted") + } + + var r0 bool + var r1 error + if rf, ok := ret.Get(0).(func(uint64) (bool, error)); ok { + return rf(epochCounter) + } + if rf, ok := ret.Get(0).(func(uint64) bool); ok { + r0 = rf(epochCounter) + } else { + r0 = ret.Get(0).(bool) + } + + if rf, ok := ret.Get(1).(func(uint64) error); ok { + r1 = rf(epochCounter) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + // GetDKGState provides a mock function with given fields: epochCounter func (_m *EpochRecoveryMyBeaconKey) GetDKGState(epochCounter uint64) (flow.DKGState, error) { ret := _m.Called(epochCounter) From a92d8b79d600375394028873253ab30b5d5f5e0d Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 2 Dec 2024 18:45:21 +0200 Subject: [PATCH 24/34] Naming updates --- cmd/consensus/main.go | 4 ++-- integration/dkg/dkg_emulator_suite.go | 2 +- integration/dkg/dkg_whiteboard_test.go | 2 +- storage/badger/dkg_state.go | 30 +++++++++++++------------- storage/badger/dkg_state_test.go | 14 ++++++------ 5 files changed, 26 insertions(+), 26 deletions(-) diff --git a/cmd/consensus/main.go b/cmd/consensus/main.go index 7ea266a4d4c..c546a80ea3d 100644 --- a/cmd/consensus/main.go +++ b/cmd/consensus/main.go @@ -127,7 +127,7 @@ func main() { committee *committees.Consensus epochLookup *epochs.EpochLookup hotstuffModules *consensus.HotstuffModules - dkgState *bstorage.RecoverablePrivateBeaconKeyState + dkgState *bstorage.RecoverablePrivateBeaconKeyStateMachine getSealingConfigs module.SealingConfigsGetter ) var deprecatedFlagBlockRateDelay time.Duration @@ -213,7 +213,7 @@ func main() { return nil }). Module("dkg state", func(node *cmd.NodeConfig) error { - dkgState, err = bstorage.NewDKGState(node.Metrics.Cache, node.SecretsDB) + dkgState, err = bstorage.NewRecoverableRandomBeaconStateMachine(node.Metrics.Cache, node.SecretsDB) return err }). Module("updatable sealing config", func(node *cmd.NodeConfig) error { diff --git a/integration/dkg/dkg_emulator_suite.go b/integration/dkg/dkg_emulator_suite.go index 6d1677029e3..6360137f81d 100644 --- a/integration/dkg/dkg_emulator_suite.go +++ b/integration/dkg/dkg_emulator_suite.go @@ -449,7 +449,7 @@ func (s *EmulatorSuite) initEngines(node *node, ids flow.IdentityList) { // dkgState is used to store the private key resulting from the node's // participation in the DKG run - dkgState, err := badger.NewDKGState(core.Metrics, core.SecretsDB) + dkgState, err := badger.NewRecoverableRandomBeaconStateMachine(core.Metrics, core.SecretsDB) s.Require().NoError(err) // brokerTunnel is used to communicate between the messaging engine and the diff --git a/integration/dkg/dkg_whiteboard_test.go b/integration/dkg/dkg_whiteboard_test.go index 205dd454adb..c708bcfe11e 100644 --- a/integration/dkg/dkg_whiteboard_test.go +++ b/integration/dkg/dkg_whiteboard_test.go @@ -91,7 +91,7 @@ func createNode( // keyKeys is used to store the private key resulting from the node's // participation in the DKG run - dkgState, err := badger.NewDKGState(core.Metrics, core.SecretsDB) + dkgState, err := badger.NewRecoverableRandomBeaconStateMachine(core.Metrics, core.SecretsDB) require.NoError(t, err) // configure the state snapthost at firstBlock to return the desired diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index 81e10a12d8b..f47db15d979 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -27,24 +27,24 @@ var allowedStateTransitions = map[flow.DKGState][]flow.DKGState{ flow.DKGStateUninitialized: {flow.DKGStateStarted, flow.DKGStateFailure, flow.RandomBeaconKeyCommitted}, } -// RecoverablePrivateBeaconKeyState stores state information about in-progress and completed DKGs, including +// RecoverablePrivateBeaconKeyStateMachine stores state information about in-progress and completed DKGs, including // computed keys. Must be instantiated using secrets database. -// RecoverablePrivateBeaconKeyState is a specific module that allows to overwrite the beacon private key for a given epoch. +// RecoverablePrivateBeaconKeyStateMachine is a specific module that allows to overwrite the beacon private key for a given epoch. // This module is used *ONLY* in the epoch recovery process and only by the consensus participants. // Each consensus participant takes part in the DKG, and after successfully finishing the DKG protocol it obtains a // random beacon private key, which is stored in the database along with DKG current state [flow.DKGStateCompleted]. // If for any reason the DKG fails, then the private key will be nil and DKG current state will be [flow.DKGStateFailure]. // When the epoch recovery takes place, we need to query the last valid beacon private key for the current replica and // also set it for use during the Recovery Epoch, otherwise replicas won't be able to vote for blocks during the Recovery Epoch. -type RecoverablePrivateBeaconKeyState struct { +type RecoverablePrivateBeaconKeyStateMachine struct { db *badger.DB keyCache *Cache[uint64, *encodable.RandomBeaconPrivKey] } -var _ storage.EpochRecoveryMyBeaconKey = (*RecoverablePrivateBeaconKeyState)(nil) +var _ storage.EpochRecoveryMyBeaconKey = (*RecoverablePrivateBeaconKeyStateMachine)(nil) -// NewDKGState returns the RecoverablePrivateBeaconKeyState implementation backed by Badger DB. -func NewDKGState(collector module.CacheMetrics, db *badger.DB) (*RecoverablePrivateBeaconKeyState, error) { +// NewRecoverableRandomBeaconStateMachine returns the RecoverablePrivateBeaconKeyStateMachine implementation backed by Badger DB. +func NewRecoverableRandomBeaconStateMachine(collector module.CacheMetrics, db *badger.DB) (*RecoverablePrivateBeaconKeyStateMachine, error) { err := operation.EnsureSecretDB(db) if err != nil { return nil, fmt.Errorf("cannot instantiate dkg state storage in non-secret db: %w", err) @@ -68,7 +68,7 @@ func NewDKGState(collector module.CacheMetrics, db *badger.DB) (*RecoverablePriv withRetrieve(retrieveKey), ) - return &RecoverablePrivateBeaconKeyState{ + return &RecoverablePrivateBeaconKeyStateMachine{ db: db, keyCache: cache, }, nil @@ -79,7 +79,7 @@ func NewDKGState(collector module.CacheMetrics, db *badger.DB) (*RecoverablePriv // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned -func (ds *RecoverablePrivateBeaconKeyState) InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { +func (ds *RecoverablePrivateBeaconKeyStateMachine) InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { if key == nil { return fmt.Errorf("will not store nil beacon key") } @@ -99,7 +99,7 @@ func (ds *RecoverablePrivateBeaconKeyState) InsertMyBeaconPrivateKey(epochCounte // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned // Error returns: storage.ErrNotFound -func (ds *RecoverablePrivateBeaconKeyState) UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) { +func (ds *RecoverablePrivateBeaconKeyStateMachine) UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) { tx := ds.db.NewTransaction(false) defer tx.Discard() encodableKey, err := ds.keyCache.Get(epochCounter)(tx) @@ -111,7 +111,7 @@ func (ds *RecoverablePrivateBeaconKeyState) UnsafeRetrieveMyBeaconPrivateKey(epo // GetDKGStarted checks whether the DKG has been started for the given epoch. // No errors expected during normal operation. -func (ds *RecoverablePrivateBeaconKeyState) GetDKGStarted(epochCounter uint64) (bool, error) { +func (ds *RecoverablePrivateBeaconKeyStateMachine) GetDKGStarted(epochCounter uint64) (bool, error) { var started bool err := ds.db.View(operation.RetrieveDKGStartedForEpoch(epochCounter, &started)) return started, err @@ -123,11 +123,11 @@ func (ds *RecoverablePrivateBeaconKeyState) GetDKGStarted(epochCounter uint64) ( // data to be processed by the state machine before the transition can be made. For such cases there are dedicated methods that should be used, ex. // InsertMyBeaconPrivateKey and UpsertMyBeaconPrivateKey, which allow to store the needed data and perform the transition in one atomic operation. // No errors are expected during normal operations. -func (ds *RecoverablePrivateBeaconKeyState) SetDKGState(epochCounter uint64, newState flow.DKGState) error { +func (ds *RecoverablePrivateBeaconKeyStateMachine) SetDKGState(epochCounter uint64, newState flow.DKGState) error { return operation.RetryOnConflictTx(ds.db, transaction.Update, ds.processStateTransition(epochCounter, newState)) } -func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter uint64, newState flow.DKGState) func(*transaction.Tx) error { +func (ds *RecoverablePrivateBeaconKeyStateMachine) processStateTransition(epochCounter uint64, newState flow.DKGState) func(*transaction.Tx) error { return func(tx *transaction.Tx) error { var currentState flow.DKGState err := operation.RetrieveDKGStateForEpoch(epochCounter, ¤tState)(tx.DBTxn) @@ -159,7 +159,7 @@ func (ds *RecoverablePrivateBeaconKeyState) processStateTransition(epochCounter // GetDKGState retrieves the current state of the state machine for the given epoch. // If an error is returned, the state is undefined meaning that state machine is in initial state // Error returns: storage.ErrNotFound. -func (ds *RecoverablePrivateBeaconKeyState) GetDKGState(epochCounter uint64) (flow.DKGState, error) { +func (ds *RecoverablePrivateBeaconKeyStateMachine) GetDKGState(epochCounter uint64) (flow.DKGState, error) { var currentState flow.DKGState err := ds.db.Update(operation.RetrieveDKGStateForEpoch(epochCounter, ¤tState)) return currentState, err @@ -174,7 +174,7 @@ func (ds *RecoverablePrivateBeaconKeyState) GetDKGState(epochCounter uint64) (fl // -> no beacon key will ever be available for the epoch in this case // - (nil, false, storage.ErrNotFound) if the DKG has not ended // - (nil, false, error) for any unexpected exception -func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCounter uint64) (key crypto.PrivateKey, safe bool, err error) { +func (ds *RecoverablePrivateBeaconKeyStateMachine) RetrieveMyBeaconPrivateKey(epochCounter uint64) (key crypto.PrivateKey, safe bool, err error) { err = ds.db.View(func(txn *badger.Txn) error { // retrieve the end state @@ -215,7 +215,7 @@ func (ds *RecoverablePrivateBeaconKeyState) RetrieveMyBeaconPrivateKey(epochCoun // Epoch Fallback Mode. Effectively, this function overwrites whatever might be available in the database with // the given private key and sets the [flow.DKGState] to [flow.RandomBeaconKeyCommitted]. // No errors are expected during normal operations. -func (ds *RecoverablePrivateBeaconKeyState) UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { +func (ds *RecoverablePrivateBeaconKeyStateMachine) UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { if key == nil { return fmt.Errorf("will not store nil beacon key") } diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index 6b9597a9f66..7ab7a1b9b28 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -19,7 +19,7 @@ import ( func TestDKGState_UninitializedState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - store, err := NewDKGState(metrics, db) + store, err := NewRecoverableRandomBeaconStateMachine(metrics, db) require.NoError(t, err) epochCounter := rand.Uint64() @@ -83,7 +83,7 @@ func TestDKGState_UninitializedState(t *testing.T) { func TestDKGState_StartedState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - store, err := NewDKGState(metrics, db) + store, err := NewRecoverableRandomBeaconStateMachine(metrics, db) require.NoError(t, err) setupState := func() uint64 { @@ -150,7 +150,7 @@ func TestDKGState_StartedState(t *testing.T) { func TestDKGState_CompletedState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - store, err := NewDKGState(metrics, db) + store, err := NewRecoverableRandomBeaconStateMachine(metrics, db) require.NoError(t, err) setupState := func() uint64 { @@ -223,7 +223,7 @@ func TestDKGState_CompletedState(t *testing.T) { func TestDKGState_FailureState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - store, err := NewDKGState(metrics, db) + store, err := NewRecoverableRandomBeaconStateMachine(metrics, db) require.NoError(t, err) setupState := func() uint64 { @@ -295,7 +295,7 @@ func TestDKGState_FailureState(t *testing.T) { func TestDKGState_RandomBeaconKeyCommittedState(t *testing.T) { unittest.RunWithTypedBadgerDB(t, InitSecret, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - store, err := NewDKGState(metrics, db) + store, err := NewRecoverableRandomBeaconStateMachine(metrics, db) require.NoError(t, err) setupState := func() uint64 { @@ -356,12 +356,12 @@ func TestDKGState_RandomBeaconKeyCommittedState(t *testing.T) { }) } -// TestSecretDBRequirement tests that the RecoverablePrivateBeaconKeyState constructor will return an +// TestSecretDBRequirement tests that the RecoverablePrivateBeaconKeyStateMachine constructor will return an // error if instantiated using a database not marked with the correct type. func TestSecretDBRequirement(t *testing.T) { unittest.RunWithBadgerDB(t, func(db *badger.DB) { metrics := metrics.NewNoopCollector() - _, err := NewDKGState(metrics, db) + _, err := NewRecoverableRandomBeaconStateMachine(metrics, db) require.Error(t, err) }) } From f0be4ba6f72f8a26cbfa682f483546d1bd7e2d19 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 2 Dec 2024 19:01:37 +0200 Subject: [PATCH 25/34] Fixed broken tests --- integration/dkg/dkg_emulator_suite.go | 1 - integration/dkg/dkg_emulator_test.go | 4 ++-- integration/dkg/dkg_whiteboard_test.go | 8 ++------ integration/dkg/node.go | 1 - 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/integration/dkg/dkg_emulator_suite.go b/integration/dkg/dkg_emulator_suite.go index 6360137f81d..a7e99821bd7 100644 --- a/integration/dkg/dkg_emulator_suite.go +++ b/integration/dkg/dkg_emulator_suite.go @@ -502,7 +502,6 @@ func (s *EmulatorSuite) initEngines(node *node, ids flow.IdentityList) { node.GenericNode = core node.messagingEngine = messagingEngine node.dkgState = dkgState - node.safeBeaconKeys = badger.NewSafeBeaconPrivateKeys(dkgState) node.reactorEngine = reactorEngine } diff --git a/integration/dkg/dkg_emulator_test.go b/integration/dkg/dkg_emulator_test.go index 4e61ee37127..3905652729c 100644 --- a/integration/dkg/dkg_emulator_test.go +++ b/integration/dkg/dkg_emulator_test.go @@ -136,9 +136,9 @@ func (s *EmulatorSuite) runTest(goodNodes int, emulatorProblems bool) { signatures := []crypto.Signature{} indices := []int{} for i, n := range nodes { - // TODO: to replace with safeBeaconKeys - beaconKey, err := n.dkgState.RetrieveMyBeaconPrivateKey(nextEpochSetup.Counter) + beaconKey, safe, err := n.dkgState.RetrieveMyBeaconPrivateKey(nextEpochSetup.Counter) require.NoError(s.T(), err) + require.True(s.T(), safe) signature, err := beaconKey.Sign(sigData, hasher) require.NoError(s.T(), err) diff --git a/integration/dkg/dkg_whiteboard_test.go b/integration/dkg/dkg_whiteboard_test.go index c708bcfe11e..a81da29315a 100644 --- a/integration/dkg/dkg_whiteboard_test.go +++ b/integration/dkg/dkg_whiteboard_test.go @@ -165,13 +165,10 @@ func createNode( // reactorEngine consumes the EpochSetupPhaseStarted event core.ProtocolEvents.AddConsumer(reactorEngine) - safeBeaconKeys := badger.NewSafeBeaconPrivateKeys(dkgState) - node := node{ t: t, GenericNode: core, dkgState: dkgState, - safeBeaconKeys: safeBeaconKeys, messagingEngine: messagingEngine, reactorEngine: reactorEngine, } @@ -298,10 +295,9 @@ func TestWithWhiteboard(t *testing.T) { signatures := []crypto.Signature{} indices := []int{} for i, n := range nodes { - - // TODO: to replace with safeBeaconKeys - beaconKey, err := n.dkgState.RetrieveMyBeaconPrivateKey(nextEpochSetup.Counter) + beaconKey, safe, err := n.dkgState.RetrieveMyBeaconPrivateKey(nextEpochSetup.Counter) require.NoError(t, err) + require.True(t, safe) signature, err := beaconKey.Sign(sigData, hasher) require.NoError(t, err) diff --git a/integration/dkg/node.go b/integration/dkg/node.go index cbea2b7f44a..eb1620d32da 100644 --- a/integration/dkg/node.go +++ b/integration/dkg/node.go @@ -36,7 +36,6 @@ type node struct { account *nodeAccount dkgContractClient *DKGClientWrapper dkgState storage.DKGState - safeBeaconKeys storage.SafeBeaconKeys messagingEngine *dkg.MessagingEngine reactorEngine *dkg.ReactorEngine } From 62d399d42657830d11caa05c876548433354ef69 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 2 Dec 2024 19:10:20 +0200 Subject: [PATCH 26/34] Linted --- cmd/util/cmd/common/node_info.go | 6 +++--- engine/common/grpc/forwarder/forwarder.go | 2 +- .../tests/access/cohort3/execution_data_pruning_test.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/util/cmd/common/node_info.go b/cmd/util/cmd/common/node_info.go index 061741d0955..0b5093954f5 100644 --- a/cmd/util/cmd/common/node_info.go +++ b/cmd/util/cmd/common/node_info.go @@ -41,16 +41,16 @@ func ReadFullPartnerNodeInfos(log zerolog.Logger, partnerWeightsPath, partnerNod } err = ValidateNetworkPubKey(partner.NetworkPubKey) if err != nil { - return nil, fmt.Errorf(fmt.Sprintf("invalid network public key: %s", partner.NetworkPubKey)) + return nil, fmt.Errorf("invalid network public key: %s", partner.NetworkPubKey) } err = ValidateStakingPubKey(partner.StakingPubKey) if err != nil { - return nil, fmt.Errorf(fmt.Sprintf("invalid staking public key: %s", partner.StakingPubKey)) + return nil, fmt.Errorf("invalid staking public key: %s", partner.StakingPubKey) } weight := weights[partner.NodeID] if valid := ValidateWeight(weight); !valid { - return nil, fmt.Errorf(fmt.Sprintf("invalid partner weight: %d", weight)) + return nil, fmt.Errorf("invalid partner weight: %d", weight) } if weight != flow.DefaultInitialWeight { diff --git a/engine/common/grpc/forwarder/forwarder.go b/engine/common/grpc/forwarder/forwarder.go index a0af264b55a..3b9b44d269e 100644 --- a/engine/common/grpc/forwarder/forwarder.go +++ b/engine/common/grpc/forwarder/forwarder.go @@ -75,7 +75,7 @@ func (f *Forwarder) reconnectingClient(i int) error { // FaultTolerantClient implements an upstream connection that reconnects on errors // a reasonable amount of time. func (f *Forwarder) FaultTolerantClient() (access.AccessAPIClient, io.Closer, error) { - if f.upstream == nil || len(f.upstream) == 0 { + if len(f.upstream) == 0 { return nil, nil, status.Errorf(codes.Unimplemented, "method not implemented") } diff --git a/integration/tests/access/cohort3/execution_data_pruning_test.go b/integration/tests/access/cohort3/execution_data_pruning_test.go index 312ee60347c..4c117c68da6 100644 --- a/integration/tests/access/cohort3/execution_data_pruning_test.go +++ b/integration/tests/access/cohort3/execution_data_pruning_test.go @@ -85,7 +85,7 @@ func (s *ExecutionDataPruningSuite) SetupTest() { testnet.WithAdditionalFlagf("--event-query-mode=local-only"), testnet.WithAdditionalFlagf("--execution-data-height-range-target=%d", s.heightRangeTarget), testnet.WithAdditionalFlagf("--execution-data-height-range-threshold=%d", s.threshold), - testnet.WithAdditionalFlagf(fmt.Sprintf("--execution-data-pruning-interval=%s", s.pruningInterval)), + testnet.WithAdditionalFlagf("--execution-data-pruning-interval=%s", s.pruningInterval), ) consensusConfigs := []func(config *testnet.NodeConfig){ From bea9c1a3fcfcbc811cbec0166314467f49c189bf Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Mon, 2 Dec 2024 22:23:47 +0200 Subject: [PATCH 27/34] Fixed broken integration tests for DKG --- integration/dkg/dkg_emulator_test.go | 3 +-- integration/dkg/dkg_whiteboard_test.go | 3 +-- integration/dkg/node.go | 4 ---- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/integration/dkg/dkg_emulator_test.go b/integration/dkg/dkg_emulator_test.go index 3905652729c..1ffe9d8334f 100644 --- a/integration/dkg/dkg_emulator_test.go +++ b/integration/dkg/dkg_emulator_test.go @@ -136,9 +136,8 @@ func (s *EmulatorSuite) runTest(goodNodes int, emulatorProblems bool) { signatures := []crypto.Signature{} indices := []int{} for i, n := range nodes { - beaconKey, safe, err := n.dkgState.RetrieveMyBeaconPrivateKey(nextEpochSetup.Counter) + beaconKey, err := n.dkgState.UnsafeRetrieveMyBeaconPrivateKey(nextEpochSetup.Counter) require.NoError(s.T(), err) - require.True(s.T(), safe) signature, err := beaconKey.Sign(sigData, hasher) require.NoError(s.T(), err) diff --git a/integration/dkg/dkg_whiteboard_test.go b/integration/dkg/dkg_whiteboard_test.go index a81da29315a..d82211771bf 100644 --- a/integration/dkg/dkg_whiteboard_test.go +++ b/integration/dkg/dkg_whiteboard_test.go @@ -295,9 +295,8 @@ func TestWithWhiteboard(t *testing.T) { signatures := []crypto.Signature{} indices := []int{} for i, n := range nodes { - beaconKey, safe, err := n.dkgState.RetrieveMyBeaconPrivateKey(nextEpochSetup.Counter) + beaconKey, err := n.dkgState.UnsafeRetrieveMyBeaconPrivateKey(nextEpochSetup.Counter) require.NoError(t, err) - require.True(t, safe) signature, err := beaconKey.Sign(sigData, hasher) require.NoError(t, err) diff --git a/integration/dkg/node.go b/integration/dkg/node.go index eb1620d32da..2734edd40f5 100644 --- a/integration/dkg/node.go +++ b/integration/dkg/node.go @@ -4,8 +4,6 @@ import ( "crypto" "testing" - "github.com/stretchr/testify/require" - sdk "github.com/onflow/flow-go-sdk" sdkcrypto "github.com/onflow/flow-go-sdk/crypto" "github.com/onflow/flow-go/engine/consensus/dkg" @@ -53,8 +51,6 @@ func (n *node) Ready() <-chan struct{} { } func (n *node) Done() <-chan struct{} { - require.NoError(n.t, n.PublicDB.Close()) - require.NoError(n.t, n.SecretsDB.Close()) return util.AllDone(n.messagingEngine, n.reactorEngine) } From 0b9f7325371fef1ffa76d50eb8f733a9ccc60ba0 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 3 Dec 2024 16:46:44 +0200 Subject: [PATCH 28/34] Fixed initialization of beacon private key state machine --- cmd/consensus/main.go | 78 ++++++++++++++++++++++--------------------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/cmd/consensus/main.go b/cmd/consensus/main.go index c546a80ea3d..4b7ca00664f 100644 --- a/cmd/consensus/main.go +++ b/cmd/consensus/main.go @@ -65,7 +65,6 @@ import ( "github.com/onflow/flow-go/state/protocol/blocktimer" "github.com/onflow/flow-go/state/protocol/events/gadgets" protocol_state "github.com/onflow/flow-go/state/protocol/protocol_state/state" - "github.com/onflow/flow-go/storage" bstorage "github.com/onflow/flow-go/storage/badger" "github.com/onflow/flow-go/utils/io" ) @@ -104,31 +103,31 @@ func main() { insecureAccessAPI bool accessNodeIDS []string - err error - mutableState protocol.ParticipantState - beaconPrivateKey *encodable.RandomBeaconPrivKey - guarantees mempool.Guarantees - receipts mempool.ExecutionTree - seals mempool.IncorporatedResultSeals - pendingReceipts mempool.PendingReceipts - receiptRequester *requester.Engine - syncCore *chainsync.Core - comp *compliance.Engine - hot module.HotStuff - conMetrics module.ConsensusMetrics - machineAccountMetrics module.MachineAccountMetrics - mainMetrics module.HotstuffMetrics - receiptValidator module.ReceiptValidator - chunkAssigner *chmodule.ChunkAssigner - followerDistributor *pubsub.FollowerDistributor - dkgBrokerTunnel *dkgmodule.BrokerTunnel - blockTimer protocol.BlockTimer - proposalDurProvider hotstuff.ProposalDurationProvider - committee *committees.Consensus - epochLookup *epochs.EpochLookup - hotstuffModules *consensus.HotstuffModules - dkgState *bstorage.RecoverablePrivateBeaconKeyStateMachine - getSealingConfigs module.SealingConfigsGetter + err error + mutableState protocol.ParticipantState + beaconPrivateKey *encodable.RandomBeaconPrivKey + guarantees mempool.Guarantees + receipts mempool.ExecutionTree + seals mempool.IncorporatedResultSeals + pendingReceipts mempool.PendingReceipts + receiptRequester *requester.Engine + syncCore *chainsync.Core + comp *compliance.Engine + hot module.HotStuff + conMetrics module.ConsensusMetrics + machineAccountMetrics module.MachineAccountMetrics + mainMetrics module.HotstuffMetrics + receiptValidator module.ReceiptValidator + chunkAssigner *chmodule.ChunkAssigner + followerDistributor *pubsub.FollowerDistributor + dkgBrokerTunnel *dkgmodule.BrokerTunnel + blockTimer protocol.BlockTimer + proposalDurProvider hotstuff.ProposalDurationProvider + committee *committees.Consensus + epochLookup *epochs.EpochLookup + hotstuffModules *consensus.HotstuffModules + myBeaconKeyStateMachine *bstorage.RecoverablePrivateBeaconKeyStateMachine + getSealingConfigs module.SealingConfigsGetter ) var deprecatedFlagBlockRateDelay time.Duration @@ -213,7 +212,7 @@ func main() { return nil }). Module("dkg state", func(node *cmd.NodeConfig) error { - dkgState, err = bstorage.NewRecoverableRandomBeaconStateMachine(node.Metrics.Cache, node.SecretsDB) + myBeaconKeyStateMachine, err = bstorage.NewRecoverableRandomBeaconStateMachine(node.Metrics.Cache, node.SecretsDB) return err }). Module("updatable sealing config", func(node *cmd.NodeConfig) error { @@ -339,21 +338,24 @@ func main() { myBeaconPublicKeyShare) } - // store my beacon key for the first epoch post-spork - err = dkgState.InsertMyBeaconPrivateKey(epochCounter, beaconPrivateKey.PrivateKey) - if err != nil && !errors.Is(err, storage.ErrAlreadyExists) { - return err + started, err := myBeaconKeyStateMachine.GetDKGStarted(epochCounter) + if err != nil { + return fmt.Errorf("could not get DKG started flag for root epoch %d: %w", epochCounter, err) } - // mark the root DKG as successful, so it is considered safe to use the key - err = dkgState.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) - if err != nil && !errors.Is(err, storage.ErrAlreadyExists) { - return err + + // perform this only if state machine is in initial state + if !started { + // store my beacon key for the first epoch post-spork + err = myBeaconKeyStateMachine.UpsertMyBeaconPrivateKey(epochCounter, beaconPrivateKey.PrivateKey) + if err != nil { + return fmt.Errorf("could not upsert my beacon private key for root epoch %d: %w", epochCounter, err) + } } return nil }). Module("my beacon key epoch recovery", func(node *cmd.NodeConfig) error { - myBeaconKeyRecovery, err := dkgmodule.NewBeaconKeyRecovery(node.Logger, node.Me, node.State, dkgState) + myBeaconKeyRecovery, err := dkgmodule.NewBeaconKeyRecovery(node.Logger, node.Me, node.State, myBeaconKeyStateMachine) if err != nil { return fmt.Errorf("could not initialize my beacon key epoch recovery: %w", err) } @@ -576,7 +578,7 @@ func main() { // wrap Main consensus committee with metrics wrappedCommittee := committees.NewMetricsWrapper(committee, mainMetrics) // wrapper for measuring time spent determining consensus committee relations - beaconKeyStore := hotsignature.NewEpochAwareRandomBeaconKeyStore(epochLookup, dkgState) + beaconKeyStore := hotsignature.NewEpochAwareRandomBeaconKeyStore(epochLookup, myBeaconKeyStateMachine) // initialize the combined signer for hotstuff var signer hotstuff.Signer @@ -916,7 +918,7 @@ func main() { node.Logger, node.Me, node.State, - dkgState, + myBeaconKeyStateMachine, dkgmodule.NewControllerFactory( node.Logger, node.Me, From 5a98f7630babb0395527a5e329c513213801e966 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 3 Dec 2024 19:36:48 +0200 Subject: [PATCH 29/34] Updated godoc. Added specific sentinel for error handling --- model/flow/dkg.go | 29 +++++++++++++--------- storage/badger/dkg_state.go | 21 +++++++++++----- storage/dkg.go | 49 +++++++++++++++++++++++++++++-------- 3 files changed, 71 insertions(+), 28 deletions(-) diff --git a/model/flow/dkg.go b/model/flow/dkg.go index d5a38f0f2dc..05a33a5626e 100644 --- a/model/flow/dkg.go +++ b/model/flow/dkg.go @@ -8,17 +8,22 @@ const ( // Conceptually, this is the 'initial' state of a finite state machine before any transitions. DKGStateUninitialized DKGState = iota // DKGStateStarted - the DKG process has been started. This state is set when the node enters the [flow.EpochPhaseSetup] - // phase and starts the DKG process, which will on the happy path result in generating a random beacon key. + // phase and starts the DKG process, which will on the happy path result in generating a Random Beacon key. DKGStateStarted // DKGStateCompleted - the DKG process has been locally completed by this node. This state is set when the node successfully - // completes the DKG process and has generated a random beacon key. - // ATTENTION: This state does not imply that there is a safe random beacon key available for the next epoch. Only after + // completes the DKG process and has generated a Random Beacon key. + // ATTENTION: This state does not imply that there is a safe Random Beacon key available for the next epoch. Only after // the node enters [flow.EpochPhaseCommitted] and the [flow.EpochCommit] service event has been finalized, we can be sure - // that our beacon key share is part of the Random Beacon Committee for the next epoch. + // that our beacon key share is part of the Random Beacon Committee for the next epoch, in this case the state will be [flow.RandomBeaconKeyCommitted]. DKGStateCompleted - // RandomBeaconKeyCommitted - + // RandomBeaconKeyCommitted - the Random Beacon key has been committed. This state is set when the node has observed an [flow.EpochCommit] + // which contains the public key share that matches the private key share that the node has obtained. + // A node can obtain a key share by successfully completing the DKG process or by manually injecting a key share obtained + // by other means (e.g. key recovery). + // Despite the key origin this is a terminal state which defines a safe Random Beacon key for the next epoch and allow node + // to participate in the Random Beacon protocol. RandomBeaconKeyCommitted - // DKGStateFailure - the underlying DKG library reported an error. + // DKGStateFailure - DKG process has failed, this state indicates that we have left the happy path. DKGStateFailure ) @@ -45,7 +50,7 @@ func (state DKGState) String() string { // - The values in DKGIndexMap must form the set {0, 1, …, n-1}, as required by the low level cryptography // module (convention simplifying the implementation). // -// Flow's random beacon utilizes a threshold signature scheme run by the committee 𝒟. +// Flow's Random Beacon utilizes a threshold signature scheme run by the committee 𝒟. // In the formal cryptographic protocol for a threshold signature with n parties, the // individual participants are identified by n public distinct non-negative integers, or simply indices. // These public indices are agreed upon by all participants and are used by the low-level @@ -57,7 +62,7 @@ func (state DKGState) String() string { // the set {0, 1, ..., n-1}. // // On the protocol level, only consensus nodes (identified by their nodeIDs) are allowed to contribute -// random beacon signature shares. Hence, the protocol level needs to map nodeIDs to the indices when +// Random Beacon signature shares. Hence, the protocol level needs to map nodeIDs to the indices when // calling into the lower-level cryptographic primitives. // // CAUTION: It is important to cleanly differentiate between the consensus committee 𝒞, the DKG committee 𝒟 @@ -69,11 +74,11 @@ func (state DKGState) String() string { // - The DKG committee 𝒟 is the set of parties that were authorized to participate in the DKG (happy path; or // eligible to receive a private key share from an alternative source on the fallback path). Mathematically, // the DKGIndexMap is a bijective function DKGIndexMap: 𝒟 ↦ {0,1,…,n-1}. -// - Only consensus nodes are allowed to contribute to the random beacon. Informally, we define ℛ as the +// - Only consensus nodes are allowed to contribute to the Random Beacon. Informally, we define ℛ as the // as the subset of the consensus committee (ℛ ⊆ 𝒞), which _successfully_ completed the DKG (hence ℛ ⊆ 𝒟). // Specifically, r ∈ ℛ iff and only if r has a private Random Beacon key share matching the respective public // key share in the `EpochCommit` event. In other words, consensus nodes are in ℛ iff and only if they are able -// to submit valid random beacon votes. Based on this definition we note that ℛ ⊆ (𝒟 ∩ 𝒞). +// to submit valid Random Beacon votes. Based on this definition we note that ℛ ⊆ (𝒟 ∩ 𝒞). // // The protocol explicitly ALLOWS additional parties outside the current epoch's consensus committee to participate. // In particular, there can be a key-value pair (d,i) ∈ DKGIndexMap, such that the nodeID d is *not* a consensus @@ -87,9 +92,9 @@ func (state DKGState) String() string { // // Nevertheless, there is an important liveness constraint: the committee ℛ should be a large number of nodes. // Specifically, an honest supermajority of consensus nodes must contain enough successful DKG participants -// (about |𝒟|/2 + 1) to produce a valid group signature for the random beacon at each block [1, 3]. +// (about |𝒟|/2 + 1) to produce a valid group signature for the Random Beacon at each block [1, 3]. // Therefore, we have the approximate lower bound |ℛ| ≳ n/2 + 1 = |𝒟|/2 + 1 = len(DKGIndexMap)/2 + 1. -// Operating close to this lower bound would require that every random beacon key-holder ϱ ∈ ℛ remaining in the consensus committee is honest +// Operating close to this lower bound would require that every Random Beacon key-holder ϱ ∈ ℛ remaining in the consensus committee is honest // (incl. quickly responsive) *all the time*. Such a reliability assumption is unsuited for decentralized production networks. // To reject configurations that are vulnerable to liveness failures, the protocol uses the threshold `t_safety` // (heuristic, see [2]), which is implemented on the smart contract level. diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index f47db15d979..15607b83e1a 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -44,6 +44,7 @@ type RecoverablePrivateBeaconKeyStateMachine struct { var _ storage.EpochRecoveryMyBeaconKey = (*RecoverablePrivateBeaconKeyStateMachine)(nil) // NewRecoverableRandomBeaconStateMachine returns the RecoverablePrivateBeaconKeyStateMachine implementation backed by Badger DB. +// No errors are expected during normal operations. func NewRecoverableRandomBeaconStateMachine(collector module.CacheMetrics, db *badger.DB) (*RecoverablePrivateBeaconKeyStateMachine, error) { err := operation.EnsureSecretDB(db) if err != nil { @@ -78,7 +79,10 @@ func NewRecoverableRandomBeaconStateMachine(collector module.CacheMetrics, db *b // // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys -// to guarantee only keys safe for signing are returned +// to guarantee only keys safe for signing are returned. +// Error returns: +// - [storage.ErrAlreadyExists] - if there is already a key stored for given epoch. +// - [storage.InvalidTransitionRandomBeaconStateMachineError] - if the requested state transition is invalid. func (ds *RecoverablePrivateBeaconKeyStateMachine) InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { if key == nil { return fmt.Errorf("will not store nil beacon key") @@ -98,7 +102,8 @@ func (ds *RecoverablePrivateBeaconKeyStateMachine) InsertMyBeaconPrivateKey(epoc // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned -// Error returns: storage.ErrNotFound +// Error returns: +// - [storage.ErrNotFound] - if there is no key stored for given epoch. func (ds *RecoverablePrivateBeaconKeyStateMachine) UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) { tx := ds.db.NewTransaction(false) defer tx.Discard() @@ -122,11 +127,14 @@ func (ds *RecoverablePrivateBeaconKeyStateMachine) GetDKGStarted(epochCounter ui // state directly from [flow.DKGStateStarted], even if such transition is valid. The reason for this is that some states require additional // data to be processed by the state machine before the transition can be made. For such cases there are dedicated methods that should be used, ex. // InsertMyBeaconPrivateKey and UpsertMyBeaconPrivateKey, which allow to store the needed data and perform the transition in one atomic operation. -// No errors are expected during normal operations. +// Error returns: +// - [storage.InvalidTransitionRandomBeaconStateMachineError] - if the requested state transition is invalid. func (ds *RecoverablePrivateBeaconKeyStateMachine) SetDKGState(epochCounter uint64, newState flow.DKGState) error { return operation.RetryOnConflictTx(ds.db, transaction.Update, ds.processStateTransition(epochCounter, newState)) } +// Error returns: +// - storage.InvalidTransitionRandomBeaconStateMachineError - if the requested state transition is invalid func (ds *RecoverablePrivateBeaconKeyStateMachine) processStateTransition(epochCounter uint64, newState flow.DKGState) func(*transaction.Tx) error { return func(tx *transaction.Tx) error { var currentState flow.DKGState @@ -141,7 +149,7 @@ func (ds *RecoverablePrivateBeaconKeyStateMachine) processStateTransition(epochC allowedStates := allowedStateTransitions[currentState] if slices.Index(allowedStates, newState) < 0 { - return fmt.Errorf("invalid state transition from %s to %s", currentState, newState) + return storage.NewInvalidTransitionRandomBeaconStateMachine(currentState, newState) } // ensure invariant holds and we still have a valid private key stored @@ -158,7 +166,8 @@ func (ds *RecoverablePrivateBeaconKeyStateMachine) processStateTransition(epochC // GetDKGState retrieves the current state of the state machine for the given epoch. // If an error is returned, the state is undefined meaning that state machine is in initial state -// Error returns: storage.ErrNotFound. +// Error returns: +// - [storage.ErrNotFound] - if there is no state stored for given epoch, meaning the state machine is in initial state. func (ds *RecoverablePrivateBeaconKeyStateMachine) GetDKGState(epochCounter uint64) (flow.DKGState, error) { var currentState flow.DKGState err := ds.db.Update(operation.RetrieveDKGStateForEpoch(epochCounter, ¤tState)) @@ -172,7 +181,7 @@ func (ds *RecoverablePrivateBeaconKeyStateMachine) GetDKGState(epochCounter uint // - (key, true, nil) if the key is present and confirmed valid // - (nil, false, nil) if the key has been marked invalid or unavailable // -> no beacon key will ever be available for the epoch in this case -// - (nil, false, storage.ErrNotFound) if the DKG has not ended +// - (nil, false, [storage.ErrNotFound]) if the DKG has not ended // - (nil, false, error) for any unexpected exception func (ds *RecoverablePrivateBeaconKeyStateMachine) RetrieveMyBeaconPrivateKey(epochCounter uint64) (key crypto.PrivateKey, safe bool, err error) { err = ds.db.View(func(txn *badger.Txn) error { diff --git a/storage/dkg.go b/storage/dkg.go index 9aea4c61665..fcc19c03372 100644 --- a/storage/dkg.go +++ b/storage/dkg.go @@ -1,6 +1,8 @@ package storage import ( + "errors" + "fmt" "github.com/onflow/crypto" "github.com/onflow/flow-go/model/flow" @@ -16,7 +18,7 @@ type SafeBeaconKeys interface { // - (key, true, nil) if the key is present and confirmed valid // - (nil, false, nil) if the key has been marked invalid or unavailable // -> no beacon key will ever be available for the epoch in this case - // - (nil, false, storage.ErrNotFound) if the DKG has not ended + // - (nil, false, [storage.ErrNotFound]) if the DKG has not ended // - (nil, false, error) for any unexpected exception RetrieveMyBeaconPrivateKey(epochCounter uint64) (key crypto.PrivateKey, safe bool, err error) } @@ -27,7 +29,8 @@ type DKGStateReader interface { // GetDKGState retrieves the current state of the state machine for the given epoch. // If an error is returned, the state is undefined meaning that state machine is in initial state - // Error returns: storage.ErrNotFound. + // Error returns: + // - [storage.ErrNotFound] - if there is no state stored for given epoch, meaning the state machine is in initial state. GetDKGState(epochCounter uint64) (flow.DKGState, error) // GetDKGStarted checks whether the DKG has been started for the given epoch. @@ -39,7 +42,8 @@ type DKGStateReader interface { // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned - // Error returns: storage.ErrNotFound. + // Error returns: + // - [storage.ErrNotFound] - if there is no key stored for given epoch. UnsafeRetrieveMyBeaconPrivateKey(epochCounter uint64) (crypto.PrivateKey, error) } @@ -55,7 +59,8 @@ type DKGState interface { // state directly from [flow.DKGStateStarted], even if such transition is valid. The reason for this is that some states require additional // data to be processed by the state machine before the transition can be made. For such cases there are dedicated methods that should be used, ex. // InsertMyBeaconPrivateKey and UpsertMyBeaconPrivateKey, which allow to store the needed data and perform the transition in one atomic operation. - // No errors are expected during normal operations. + // Error returns: + // - [storage.InvalidTransitionRandomBeaconStateMachineError] - if the requested state transition is invalid. SetDKGState(epochCounter uint64, newState flow.DKGState) error // InsertMyBeaconPrivateKey stores the random beacon private key for an epoch. @@ -63,7 +68,9 @@ type DKGState interface { // CAUTION: these keys are stored before they are validated against the // canonical key vector and may not be valid for use in signing. Use SafeBeaconKeys // to guarantee only keys safe for signing are returned - // Error returns: storage.ErrAlreadyExists + // Error returns: + // - [storage.ErrAlreadyExists] - if there is already a key stored for given epoch. + // - [storage.InvalidTransitionRandomBeaconStateMachineError] - if the requested state transition is invalid. InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error } @@ -71,15 +78,37 @@ type DKGState interface { // This interface is used *ONLY* in the epoch recovery process and only by the consensus participants. // Each consensus participant takes part in the DKG, after finishing the DKG protocol each replica obtains a random beacon // private key which is stored in the database along with DKG state which will be equal to [flow.RandomBeaconKeyCommitted]. -// If for any reason DKG fails, then the private key will be nil and DKG end state will be equal to flow.DKGEndStateDKGFailure. -// It's not a problem by itself, but when the epoch recovery takes place, we need to query last valid beacon private key for -// the current replica and set it for recovered epoch, otherwise replicas won't be able to vote for blocks in the recovered epoch. +// If for any reason DKG fails, then the private key will be nil and DKG end state will be equal to [flow.DKGStateFailure]. +// This module allows to overwrite the random beacon private key in case of EFM recovery or other configuration issues. type EpochRecoveryMyBeaconKey interface { DKGStateReader // UpsertMyBeaconPrivateKey overwrites the random beacon private key for the epoch that recovers the protocol from - // epoch fallback mode. Effectively, this function overwrites whatever might be available in the database with - // given private key for current consensus participant. + // Epoch Fallback Mode. Effectively, this function overwrites whatever might be available in the database with + // the given private key and sets the [flow.DKGState] to [flow.RandomBeaconKeyCommitted]. // No errors are expected during normal operations. UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error } + +// InvalidTransitionRandomBeaconStateMachineError is a sentinel error that is returned in case an invalid state transition is attempted. +type InvalidTransitionRandomBeaconStateMachineError struct { + From flow.DKGState + To flow.DKGState +} + +func (e InvalidTransitionRandomBeaconStateMachineError) Error() string { + return fmt.Sprintf("invalid state transition from %s to %s", e.From.String(), e.To.String()) +} + +func IsInvalidTransitionRandomBeaconStateMachineError(err error) bool { + var e InvalidTransitionRandomBeaconStateMachineError + return errors.As(err, &e) +} + +// NewInvalidTransitionRandomBeaconStateMachine constructs a new InvalidTransitionRandomBeaconStateMachineError error. +func NewInvalidTransitionRandomBeaconStateMachine(from, to flow.DKGState) error { + return InvalidTransitionRandomBeaconStateMachineError{ + From: from, + To: to, + } +} From b63974277a544d24f808dad98ddf22de70e770e6 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 3 Dec 2024 19:49:43 +0200 Subject: [PATCH 30/34] Updated assertions with the account for sentinel errors --- storage/badger/dkg_state.go | 10 ++++---- storage/badger/dkg_state_test.go | 40 +++++++++++++++++++++++++------- storage/dkg.go | 32 ++++++++++++++++--------- 3 files changed, 58 insertions(+), 24 deletions(-) diff --git a/storage/badger/dkg_state.go b/storage/badger/dkg_state.go index 15607b83e1a..a44d6fc23f5 100644 --- a/storage/badger/dkg_state.go +++ b/storage/badger/dkg_state.go @@ -82,7 +82,7 @@ func NewRecoverableRandomBeaconStateMachine(collector module.CacheMetrics, db *b // to guarantee only keys safe for signing are returned. // Error returns: // - [storage.ErrAlreadyExists] - if there is already a key stored for given epoch. -// - [storage.InvalidTransitionRandomBeaconStateMachineError] - if the requested state transition is invalid. +// - [storage.InvalidTransitionRandomBeaconStateMachineErr] - if the requested state transition is invalid. func (ds *RecoverablePrivateBeaconKeyStateMachine) InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error { if key == nil { return fmt.Errorf("will not store nil beacon key") @@ -128,13 +128,13 @@ func (ds *RecoverablePrivateBeaconKeyStateMachine) GetDKGStarted(epochCounter ui // data to be processed by the state machine before the transition can be made. For such cases there are dedicated methods that should be used, ex. // InsertMyBeaconPrivateKey and UpsertMyBeaconPrivateKey, which allow to store the needed data and perform the transition in one atomic operation. // Error returns: -// - [storage.InvalidTransitionRandomBeaconStateMachineError] - if the requested state transition is invalid. +// - [storage.InvalidTransitionRandomBeaconStateMachineErr] - if the requested state transition is invalid. func (ds *RecoverablePrivateBeaconKeyStateMachine) SetDKGState(epochCounter uint64, newState flow.DKGState) error { return operation.RetryOnConflictTx(ds.db, transaction.Update, ds.processStateTransition(epochCounter, newState)) } // Error returns: -// - storage.InvalidTransitionRandomBeaconStateMachineError - if the requested state transition is invalid +// - storage.InvalidTransitionRandomBeaconStateMachineErr - if the requested state transition is invalid func (ds *RecoverablePrivateBeaconKeyStateMachine) processStateTransition(epochCounter uint64, newState flow.DKGState) func(*transaction.Tx) error { return func(tx *transaction.Tx) error { var currentState flow.DKGState @@ -149,14 +149,14 @@ func (ds *RecoverablePrivateBeaconKeyStateMachine) processStateTransition(epochC allowedStates := allowedStateTransitions[currentState] if slices.Index(allowedStates, newState) < 0 { - return storage.NewInvalidTransitionRandomBeaconStateMachine(currentState, newState) + return storage.NewInvalidTransitionRandomBeaconStateMachineErr(currentState, newState) } // ensure invariant holds and we still have a valid private key stored if newState == flow.RandomBeaconKeyCommitted || newState == flow.DKGStateCompleted { _, err = ds.keyCache.Get(epochCounter)(tx.DBTxn) if err != nil { - return fmt.Errorf("cannot transition to %s without a valid random beacon key: %w", newState, err) + return storage.NewInvalidTransitionRandomBeaconStateMachineErrf(currentState, newState, "cannot transition without a valid random beacon key: %w", err) } } diff --git a/storage/badger/dkg_state_test.go b/storage/badger/dkg_state_test.go index 7ab7a1b9b28..f294a2090a3 100644 --- a/storage/badger/dkg_state_test.go +++ b/storage/badger/dkg_state_test.go @@ -22,7 +22,10 @@ func TestDKGState_UninitializedState(t *testing.T) { store, err := NewRecoverableRandomBeaconStateMachine(metrics, db) require.NoError(t, err) - epochCounter := rand.Uint64() + setupState := func() uint64 { + return rand.Uint64() + } + epochCounter := setupState() started, err := store.GetDKGStarted(epochCounter) require.NoError(t, err) @@ -42,35 +45,38 @@ func TestDKGState_UninitializedState(t *testing.T) { require.Nil(t, pk) t.Run("-> flow.DKGStateUninitialized, not allowed", func(t *testing.T) { - epochCounter++ - err = store.SetDKGState(epochCounter, flow.DKGStateUninitialized) + err = store.SetDKGState(setupState(), flow.DKGStateUninitialized) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateStarted, should be allowed", func(t *testing.T) { epochCounter++ - err = store.SetDKGState(epochCounter, flow.DKGStateStarted) + err = store.SetDKGState(setupState(), flow.DKGStateStarted) require.NoError(t, err) }) t.Run("-> flow.DKGStateFailure, should be allowed", func(t *testing.T) { epochCounter++ - err = store.SetDKGState(epochCounter, flow.DKGStateFailure) + err = store.SetDKGState(setupState(), flow.DKGStateFailure) require.NoError(t, err) }) t.Run("-> flow.DKGStateCompleted, not allowed", func(t *testing.T) { - epochCounter++ + epochCounter := setupState() err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) require.Error(t, err, "should not be able to enter completed state without starting") + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) err = store.SetDKGState(epochCounter, flow.DKGStateCompleted) require.Error(t, err, "should not be able to enter completed state without starting") + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.RandomBeaconKeyCommitted, should be allowed", func(t *testing.T) { - epochCounter++ + epochCounter := setupState() err = store.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) require.Error(t, err, "should not be able to set DKG state to recovered, only using dedicated interface") + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) err = store.UpsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) require.NoError(t, err) }) @@ -114,11 +120,13 @@ func TestDKGState_StartedState(t *testing.T) { t.Run("-> flow.DKGStateUninitialized, not allowed", func(t *testing.T) { err = store.SetDKGState(setupState(), flow.DKGStateUninitialized) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateStarted, not allowed", func(t *testing.T) { err = store.SetDKGState(setupState(), flow.DKGStateStarted) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateFailure, should be allowed", func(t *testing.T) { @@ -130,6 +138,7 @@ func TestDKGState_StartedState(t *testing.T) { epochCounter := setupState() err = store.SetDKGState(epochCounter, flow.DKGStateCompleted) require.Error(t, err, "should not be able to enter completed state without providing a private key") + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) require.NoError(t, err) }) @@ -138,6 +147,7 @@ func TestDKGState_StartedState(t *testing.T) { epochCounter := setupState() err = store.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) require.Error(t, err, "should not be able to set DKG state to recovered, only using dedicated interface") + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) err = store.UpsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) require.NoError(t, err) }) @@ -183,11 +193,13 @@ func TestDKGState_CompletedState(t *testing.T) { t.Run("-> flow.DKGStateUninitialized, not allowed", func(t *testing.T) { err = store.SetDKGState(setupState(), flow.DKGStateUninitialized) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateStarted, not allowed", func(t *testing.T) { err = store.SetDKGState(setupState(), flow.DKGStateStarted) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateFailure, should be allowed", func(t *testing.T) { @@ -199,8 +211,10 @@ func TestDKGState_CompletedState(t *testing.T) { epochCounter := setupState() err = store.SetDKGState(epochCounter, flow.DKGStateCompleted) require.Error(t, err, "already in this state") + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) require.Error(t, err, "already inserted private key") + require.ErrorIs(t, err, storage.ErrAlreadyExists) }) t.Run("-> flow.RandomBeaconKeyCommitted, should be allowed", func(t *testing.T) { @@ -254,30 +268,36 @@ func TestDKGState_FailureState(t *testing.T) { t.Run("-> flow.DKGStateUninitialized, not allowed", func(t *testing.T) { err = store.SetDKGState(setupState(), flow.DKGStateUninitialized) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateStarted, not allowed", func(t *testing.T) { err = store.SetDKGState(setupState(), flow.DKGStateStarted) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateFailure, not allowed", func(t *testing.T) { err = store.SetDKGState(setupState(), flow.DKGStateFailure) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateCompleted, not allowed", func(t *testing.T) { epochCounter := setupState() err = store.SetDKGState(epochCounter, flow.DKGStateCompleted) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.RandomBeaconKeyCommitted, should be allowed", func(t *testing.T) { epochCounter := setupState() err = store.SetDKGState(epochCounter, flow.RandomBeaconKeyCommitted) require.Error(t, err, "should not be able to set DKG state to recovered, only using dedicated interface") + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) expectedKey := unittest.RandomBeaconPriv() err = store.UpsertMyBeaconPrivateKey(epochCounter, expectedKey) require.NoError(t, err) @@ -326,24 +346,28 @@ func TestDKGState_RandomBeaconKeyCommittedState(t *testing.T) { t.Run("-> flow.DKGStateUninitialized, not allowed", func(t *testing.T) { err = store.SetDKGState(setupState(), flow.DKGStateUninitialized) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateStarted, not allowed", func(t *testing.T) { err = store.SetDKGState(setupState(), flow.DKGStateStarted) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateFailure, not allowed", func(t *testing.T) { err = store.SetDKGState(setupState(), flow.DKGStateFailure) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) }) t.Run("-> flow.DKGStateCompleted, not allowed", func(t *testing.T) { epochCounter := setupState() err = store.SetDKGState(epochCounter, flow.DKGStateCompleted) require.Error(t, err) + require.True(t, storage.IsInvalidTransitionRandomBeaconStateMachineErr(err)) err = store.InsertMyBeaconPrivateKey(epochCounter, unittest.RandomBeaconPriv()) - require.Error(t, err) + require.ErrorIs(t, err, storage.ErrAlreadyExists) }) t.Run("-> flow.RandomBeaconKeyCommitted, allowed", func(t *testing.T) { diff --git a/storage/dkg.go b/storage/dkg.go index fcc19c03372..acdffbc39d2 100644 --- a/storage/dkg.go +++ b/storage/dkg.go @@ -60,7 +60,7 @@ type DKGState interface { // data to be processed by the state machine before the transition can be made. For such cases there are dedicated methods that should be used, ex. // InsertMyBeaconPrivateKey and UpsertMyBeaconPrivateKey, which allow to store the needed data and perform the transition in one atomic operation. // Error returns: - // - [storage.InvalidTransitionRandomBeaconStateMachineError] - if the requested state transition is invalid. + // - [storage.InvalidTransitionRandomBeaconStateMachineErr] - if the requested state transition is invalid. SetDKGState(epochCounter uint64, newState flow.DKGState) error // InsertMyBeaconPrivateKey stores the random beacon private key for an epoch. @@ -70,7 +70,7 @@ type DKGState interface { // to guarantee only keys safe for signing are returned // Error returns: // - [storage.ErrAlreadyExists] - if there is already a key stored for given epoch. - // - [storage.InvalidTransitionRandomBeaconStateMachineError] - if the requested state transition is invalid. + // - [storage.InvalidTransitionRandomBeaconStateMachineErr] - if the requested state transition is invalid. InsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error } @@ -90,25 +90,35 @@ type EpochRecoveryMyBeaconKey interface { UpsertMyBeaconPrivateKey(epochCounter uint64, key crypto.PrivateKey) error } -// InvalidTransitionRandomBeaconStateMachineError is a sentinel error that is returned in case an invalid state transition is attempted. -type InvalidTransitionRandomBeaconStateMachineError struct { +// InvalidTransitionRandomBeaconStateMachineErr is a sentinel error that is returned in case an invalid state transition is attempted. +type InvalidTransitionRandomBeaconStateMachineErr struct { + err error From flow.DKGState To flow.DKGState } -func (e InvalidTransitionRandomBeaconStateMachineError) Error() string { - return fmt.Sprintf("invalid state transition from %s to %s", e.From.String(), e.To.String()) +func (e InvalidTransitionRandomBeaconStateMachineErr) Error() string { + return fmt.Sprintf("invalid state transition from %s to %s: %w", e.From.String(), e.To.String(), e.err.Error()) } -func IsInvalidTransitionRandomBeaconStateMachineError(err error) bool { - var e InvalidTransitionRandomBeaconStateMachineError +func IsInvalidTransitionRandomBeaconStateMachineErr(err error) bool { + var e InvalidTransitionRandomBeaconStateMachineErr return errors.As(err, &e) } -// NewInvalidTransitionRandomBeaconStateMachine constructs a new InvalidTransitionRandomBeaconStateMachineError error. -func NewInvalidTransitionRandomBeaconStateMachine(from, to flow.DKGState) error { - return InvalidTransitionRandomBeaconStateMachineError{ +// NewInvalidTransitionRandomBeaconStateMachineErr constructs a new InvalidTransitionRandomBeaconStateMachineErr error. +func NewInvalidTransitionRandomBeaconStateMachineErr(from, to flow.DKGState) error { + return InvalidTransitionRandomBeaconStateMachineErr{ From: from, To: to, } } + +// NewInvalidTransitionRandomBeaconStateMachineErrf constructs a new InvalidTransitionRandomBeaconStateMachineErr error with a formatted message. +func NewInvalidTransitionRandomBeaconStateMachineErrf(from, to flow.DKGState, msg string, args ...any) error { + return InvalidTransitionRandomBeaconStateMachineErr{ + From: from, + To: to, + err: fmt.Errorf(msg, args...), + } +} From be18c575fe5bf1b7b40cea74ffcbe6779764a465 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 3 Dec 2024 19:51:27 +0200 Subject: [PATCH 31/34] Updated logging --- module/dkg/recovery.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/dkg/recovery.go b/module/dkg/recovery.go index 93bc93262d5..2ad456a9fad 100644 --- a/module/dkg/recovery.go +++ b/module/dkg/recovery.go @@ -167,7 +167,7 @@ func (b *BeaconKeyRecovery) recoverMyBeaconPrivateKey(final protocol.Snapshot) e if err != nil { return fmt.Errorf("could not overwrite my beacon private key for the next epoch: %w", err) } - log.Info().Msgf("succesfully recovered my beacon private key for the next epoch") + log.Warn().Msgf("succesfully recovered my beacon private key for the next epoch") } else { log.Debug().Msgf("my beacon key is not part of the next epoch DKG") } From d0df4fba36ef34cd201e02a1e458393b25a9a123 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 3 Dec 2024 19:54:38 +0200 Subject: [PATCH 32/34] Linted --- storage/dkg.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/storage/dkg.go b/storage/dkg.go index acdffbc39d2..db7fe977e9e 100644 --- a/storage/dkg.go +++ b/storage/dkg.go @@ -3,6 +3,7 @@ package storage import ( "errors" "fmt" + "github.com/onflow/crypto" "github.com/onflow/flow-go/model/flow" @@ -98,7 +99,7 @@ type InvalidTransitionRandomBeaconStateMachineErr struct { } func (e InvalidTransitionRandomBeaconStateMachineErr) Error() string { - return fmt.Sprintf("invalid state transition from %s to %s: %w", e.From.String(), e.To.String(), e.err.Error()) + return fmt.Sprintf("invalid state transition from %s to %s: %s", e.From.String(), e.To.String(), e.err.Error()) } func IsInvalidTransitionRandomBeaconStateMachineErr(err error) bool { From 0f98cf77ac6bef6d9d734a479c547ea70e0410f4 Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Tue, 3 Dec 2024 21:06:12 +0200 Subject: [PATCH 33/34] Fixed invalid exit logic in DKG reactor engine --- engine/consensus/dkg/reactor_engine.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/engine/consensus/dkg/reactor_engine.go b/engine/consensus/dkg/reactor_engine.go index 958bf97498d..c60c815bce4 100644 --- a/engine/consensus/dkg/reactor_engine.go +++ b/engine/consensus/dkg/reactor_engine.go @@ -271,9 +271,13 @@ func (e *ReactorEngine) handleEpochCommittedPhaseStarted(currentEpochCounter uin // This can happen if the DKG failed locally, if we failed to generate // a local private beacon key, or if we crashed while performing this // check previously. - endState, err := e.dkgState.GetDKGState(nextEpochCounter) - if err == nil { - log.Warn().Msgf("checking beacon key consistency: exiting because dkg end state was already set: %s", endState.String()) + currentState, err := e.dkgState.GetDKGState(nextEpochCounter) + if err != nil { + log.Fatal().Err(err).Msg("failed to get dkg state, by this point it should have been set") + return + } + if currentState != flow.DKGStateCompleted { + log.Warn().Msgf("checking beacon key consistency: exiting because dkg didn't reach completed state: %s", currentState.String()) return } From 6ea64d6e6aa33047416cdb95f13fb6c4305d9b3b Mon Sep 17 00:00:00 2001 From: Yurii Oleksyshyn Date: Wed, 4 Dec 2024 11:24:14 +0200 Subject: [PATCH 34/34] Fixed broken test --- engine/consensus/dkg/reactor_engine_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/engine/consensus/dkg/reactor_engine_test.go b/engine/consensus/dkg/reactor_engine_test.go index b59e5f6cc7e..cdf4a853616 100644 --- a/engine/consensus/dkg/reactor_engine_test.go +++ b/engine/consensus/dkg/reactor_engine_test.go @@ -290,7 +290,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) NextEpochCounter() uint64 { func (suite *ReactorEngineSuite_CommittedPhase) SetupTest() { suite.epochCounter = rand.Uint64() - suite.DKGState = flow.DKGStateUninitialized + suite.DKGState = flow.DKGStateCompleted // we start with the completed state since we are going to test the transition to committed suite.me = new(module.Local) id := unittest.IdentifierFixture() @@ -312,7 +312,7 @@ func (suite *ReactorEngineSuite_CommittedPhase) SetupTest() { ) suite.dkgState.On("SetDKGState", suite.NextEpochCounter(), mock.Anything). Run(func(args mock.Arguments) { - assert.Equal(suite.T(), flow.DKGStateUninitialized, suite.DKGState) // must be unset + assert.Equal(suite.T(), flow.DKGStateCompleted, suite.DKGState) // must be equal to the initial state of the test endState := args[1].(flow.DKGState) suite.DKGState = endState }).