From dcf84795d8a0b0ecbf1b34aa3bf9475bdd5c0eff Mon Sep 17 00:00:00 2001 From: Jonathan Weiss Date: Thu, 15 Sep 2022 17:52:11 +0300 Subject: [PATCH 1/7] fix: panic in GetStateProofSecretsForRound --- data/account/participationRegistry.go | 6 ++++++ data/accountManager.go | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/data/account/participationRegistry.go b/data/account/participationRegistry.go index e36f224516..183e871096 100644 --- a/data/account/participationRegistry.go +++ b/data/account/participationRegistry.go @@ -220,6 +220,9 @@ var ErrNoKeyForID = errors.New("no valid key found for the participationID") // ErrSecretNotFound is used when attempting to lookup secrets for a particular round. var ErrSecretNotFound = errors.New("the participation ID did not have secrets for the requested round") +// ErrStateProofVerifierIsNil states that no state proof field was found. +var ErrStateProofVerifierIsNil = errors.New("record contains no StateProofVerifier") + // ParticipationRegistry contain all functions for interacting with the Participation Registry. type ParticipationRegistry interface { // Insert adds a record to storage and computes the ParticipationID @@ -767,6 +770,9 @@ func (db *participationDB) GetStateProofSecretsForRound(id ParticipationID, roun if err != nil { return StateProofSecretsForRound{}, err } + if partRecord.StateProof == nil { + return StateProofSecretsForRound{}, ErrStateProofVerifierIsNil + } var result StateProofSecretsForRound result.ParticipationRecord = partRecord.ParticipationRecord diff --git a/data/accountManager.go b/data/accountManager.go index d44091f806..aa5064e093 100644 --- a/data/accountManager.go +++ b/data/accountManager.go @@ -79,10 +79,10 @@ func (manager *AccountManager) Keys(rnd basics.Round) (out []account.Participati // StateProofKeys returns a list of Participation accounts, and their stateproof secrets func (manager *AccountManager) StateProofKeys(rnd basics.Round) (out []account.StateProofSecretsForRound) { for _, part := range manager.registry.GetAll() { - if part.OverlapsInterval(rnd, rnd) { + if part.StateProof != nil && part.OverlapsInterval(rnd, rnd) { partRndSecrets, err := manager.registry.GetStateProofSecretsForRound(part.ParticipationID, rnd) if err != nil { - manager.log.Errorf("error while loading round secrets from participation registry: %w", err) + manager.log.Errorf("error while loading round secrets from participation registry: %v", err) continue } out = append(out, partRndSecrets) From 67d68b958a67d875f442d735a6fef3cf607e5c26 Mon Sep 17 00:00:00 2001 From: Jonathan Weiss Date: Thu, 15 Sep 2022 18:03:19 +0300 Subject: [PATCH 2/7] added unit-test to replicate the issue --- data/account/participationRegistry_test.go | 33 ++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/data/account/participationRegistry_test.go b/data/account/participationRegistry_test.go index 8e02ff4e43..5a4beafe1e 100644 --- a/data/account/participationRegistry_test.go +++ b/data/account/participationRegistry_test.go @@ -27,10 +27,9 @@ import ( "os" "path/filepath" "strconv" - "sync/atomic" - "strings" "sync" + "sync/atomic" "testing" "time" @@ -958,6 +957,36 @@ func TestAddStateProofKeys(t *testing.T) { } } +func TestGetRoundSecretsWhithout(t *testing.T) { + partitiontest.PartitionTest(t) + a := assert.New(t) + registry, dbfile := getRegistry(t) + defer registryCloseTest(t, registry, dbfile) + + access, err := db.MakeAccessor("stateprooftest", false, true) + if err != nil { + panic(err) + } + root, err := GenerateRoot(access) + p, err := FillDBWithParticipationKeys(access, root.Address(), 0, basics.Round(stateProofIntervalForTests*2), 3) + access.Close() + a.NoError(err) + + // Install a key for testing + id, err := registry.Insert(p.Participation) + a.NoError(err) + + // ensuring that GetStateProof will receive from cache a participationRecord without StateProof field. + prt := registry.cache[id] + prt.StateProof = nil + registry.cache[id] = prt + + a.NoError(registry.Flush(defaultTimeout)) + + _, err = registry.GetStateProofSecretsForRound(id, basics.Round(stateProofIntervalForTests)-1) + a.ErrorIs(err, ErrStateProofVerifierIsNil) +} + func TestSecretNotFound(t *testing.T) { partitiontest.PartitionTest(t) a := require.New(t) From feceeccc9db4dca26afb1da3db75131b44fa7cc1 Mon Sep 17 00:00:00 2001 From: Jonathan Weiss Date: Thu, 15 Sep 2022 18:49:52 +0300 Subject: [PATCH 3/7] added unit test to account manager that catches the issue without the fix --- data/accountManager_test.go | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/data/accountManager_test.go b/data/accountManager_test.go index 9d464cba17..5db36908d1 100644 --- a/data/accountManager_test.go +++ b/data/accountManager_test.go @@ -17,6 +17,7 @@ package data import ( + "bytes" "fmt" "os" "path/filepath" @@ -248,3 +249,46 @@ func TestAccountManagerOverlappingStateProofKeys(t *testing.T) { res = acctManager.StateProofKeys(basics.Round(merklesignature.KeyLifetimeDefault * 3)) a.Equal(1, len(res)) } + +func TestAccountManagerOverlappingStateProofKeys1(t *testing.T) { + partitiontest.PartitionTest(t) + a := assert.New(t) + + registry, dbName := getRegistryImpl(t, false, true) + defer registryCloseTest(t, registry, dbName) + + log := logging.TestingLog(t) + log.SetLevel(logging.Error) + logbuffer := bytes.NewBuffer(nil) + log.SetOutput(logbuffer) + + acctManager := MakeAccountManager(log, registry) + databaseFiles := make([]string, 0) + defer func() { + for _, fileName := range databaseFiles { + os.Remove(fileName) + os.Remove(fileName + "-shm") + os.Remove(fileName + "-wal") + os.Remove(fileName + "-journal") + } + }() + + // Generate 2 participations under the same account + store, err := db.MakeAccessor("stateprooftest", false, true) + a.NoError(err) + root, err := account.GenerateRoot(store) + a.NoError(err) + part1, err := account.FillDBWithParticipationKeys(store, root.Address(), 0, basics.Round(merklesignature.KeyLifetimeDefault*2), 3) + a.NoError(err) + store.Close() + + part1.StateProofSecrets = nil + _, err = registry.Insert(part1.Participation) + a.NoError(err) + + logbuffer.Reset() + acctManager.StateProofKeys(1) + lg := logbuffer.String() + a.False(strings.Contains(lg, account.ErrStateProofVerifierIsNil.Error())) + a.False(strings.Contains(lg, "level=error"), "expected no error in log:", lg) +} From 24387ec2f4142f986349e17ca26814046b2181ef Mon Sep 17 00:00:00 2001 From: Jonathan Weiss Date: Thu, 15 Sep 2022 19:16:21 +0300 Subject: [PATCH 4/7] fix: test names didnt make sense --- data/account/participationRegistry_test.go | 2 +- data/accountManager_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/data/account/participationRegistry_test.go b/data/account/participationRegistry_test.go index 5a4beafe1e..9c27908eb4 100644 --- a/data/account/participationRegistry_test.go +++ b/data/account/participationRegistry_test.go @@ -957,7 +957,7 @@ func TestAddStateProofKeys(t *testing.T) { } } -func TestGetRoundSecretsWhithout(t *testing.T) { +func TestGetRoundSecretsWithNilStateProofVerifier(t *testing.T) { partitiontest.PartitionTest(t) a := assert.New(t) registry, dbfile := getRegistry(t) diff --git a/data/accountManager_test.go b/data/accountManager_test.go index 5db36908d1..cd20c796a0 100644 --- a/data/accountManager_test.go +++ b/data/accountManager_test.go @@ -250,7 +250,7 @@ func TestAccountManagerOverlappingStateProofKeys(t *testing.T) { a.Equal(1, len(res)) } -func TestAccountManagerOverlappingStateProofKeys1(t *testing.T) { +func TestGetStateProofKeysDontLogErrorOnNilStateProof(t *testing.T) { partitiontest.PartitionTest(t) a := assert.New(t) From cc76ee64f260be553bc290b5d8edd3a483665e8d Mon Sep 17 00:00:00 2001 From: Jonathan Weiss Date: Mon, 19 Sep 2022 18:36:53 +0300 Subject: [PATCH 5/7] rename error --- data/account/participationRegistry.go | 6 +++--- data/account/participationRegistry_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/data/account/participationRegistry.go b/data/account/participationRegistry.go index 183e871096..4cdc5f60e6 100644 --- a/data/account/participationRegistry.go +++ b/data/account/participationRegistry.go @@ -220,8 +220,8 @@ var ErrNoKeyForID = errors.New("no valid key found for the participationID") // ErrSecretNotFound is used when attempting to lookup secrets for a particular round. var ErrSecretNotFound = errors.New("the participation ID did not have secrets for the requested round") -// ErrStateProofVerifierIsNil states that no state proof field was found. -var ErrStateProofVerifierIsNil = errors.New("record contains no StateProofVerifier") +// ErrStateProofVerifierNotFound states that no state proof field was found. +var ErrStateProofVerifierNotFound = errors.New("record contains no StateProofVerifier") // ParticipationRegistry contain all functions for interacting with the Participation Registry. type ParticipationRegistry interface { @@ -771,7 +771,7 @@ func (db *participationDB) GetStateProofSecretsForRound(id ParticipationID, roun return StateProofSecretsForRound{}, err } if partRecord.StateProof == nil { - return StateProofSecretsForRound{}, ErrStateProofVerifierIsNil + return StateProofSecretsForRound{}, ErrStateProofVerifierNotFound } var result StateProofSecretsForRound diff --git a/data/account/participationRegistry_test.go b/data/account/participationRegistry_test.go index 9c27908eb4..286edf117b 100644 --- a/data/account/participationRegistry_test.go +++ b/data/account/participationRegistry_test.go @@ -984,7 +984,7 @@ func TestGetRoundSecretsWithNilStateProofVerifier(t *testing.T) { a.NoError(registry.Flush(defaultTimeout)) _, err = registry.GetStateProofSecretsForRound(id, basics.Round(stateProofIntervalForTests)-1) - a.ErrorIs(err, ErrStateProofVerifierIsNil) + a.ErrorIs(err, ErrStateProofVerifierNotFound) } func TestSecretNotFound(t *testing.T) { From 0d9dcad5d540078cacd0c47682c2bfc6501f87a2 Mon Sep 17 00:00:00 2001 From: Jonathan Weiss Date: Mon, 19 Sep 2022 18:43:18 +0300 Subject: [PATCH 6/7] added ID to error to improve debugging capabilities --- data/account/participationRegistry.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/data/account/participationRegistry.go b/data/account/participationRegistry.go index 4cdc5f60e6..e1c82a8924 100644 --- a/data/account/participationRegistry.go +++ b/data/account/participationRegistry.go @@ -771,7 +771,8 @@ func (db *participationDB) GetStateProofSecretsForRound(id ParticipationID, roun return StateProofSecretsForRound{}, err } if partRecord.StateProof == nil { - return StateProofSecretsForRound{}, ErrStateProofVerifierNotFound + return StateProofSecretsForRound{}, + fmt.Errorf("%w: for participation ID %v", ErrStateProofVerifierNotFound, id) } var result StateProofSecretsForRound From 1c7004a9da6aadf0840c960c2e753b5c9489dd2d Mon Sep 17 00:00:00 2001 From: algoidan Date: Mon, 19 Sep 2022 13:15:24 -0400 Subject: [PATCH 7/7] fix error ref --- data/accountManager_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data/accountManager_test.go b/data/accountManager_test.go index cd20c796a0..1fcfe56bf7 100644 --- a/data/accountManager_test.go +++ b/data/accountManager_test.go @@ -289,6 +289,6 @@ func TestGetStateProofKeysDontLogErrorOnNilStateProof(t *testing.T) { logbuffer.Reset() acctManager.StateProofKeys(1) lg := logbuffer.String() - a.False(strings.Contains(lg, account.ErrStateProofVerifierIsNil.Error())) + a.False(strings.Contains(lg, account.ErrStateProofVerifierNotFound.Error())) a.False(strings.Contains(lg, "level=error"), "expected no error in log:", lg) }