From 7ded03ca7cdd89b74dc31155ad96798c0c153d97 Mon Sep 17 00:00:00 2001 From: Steve <1848680+misko9@users.noreply.github.com> Date: Mon, 11 Dec 2023 23:14:45 -0700 Subject: [PATCH 1/2] Reverse where we start checking for a non-expired nonce, oldest -> newest (#234) * Reverse where we start checking for a non-expired nonce, oldest -> newest * Improve TestNonceCacheExpiration * lint fix * Fix unit test * Add a little more breathing room for unit test --- signer/cosigner_nonce_cache.go | 22 +++++++++------- signer/cosigner_nonce_cache_test.go | 41 ++++++++++++++++++----------- 2 files changed, 38 insertions(+), 25 deletions(-) diff --git a/signer/cosigner_nonce_cache.go b/signer/cosigner_nonce_cache.go index bc934289..a1b276a9 100644 --- a/signer/cosigner_nonce_cache.go +++ b/signer/cosigner_nonce_cache.go @@ -374,21 +374,23 @@ CheckNoncesLoop: func (cnc *CosignerNonceCache) PruneNonces() int { cnc.cache.mu.Lock() defer cnc.cache.mu.Unlock() - nonExpiredIndex := len(cnc.cache.cache) - 1 - for i := len(cnc.cache.cache) - 1; i >= 0; i-- { + nonExpiredIndex := -1 + for i := 0; i < len(cnc.cache.cache); i++ { if time.Now().Before(cnc.cache.cache[i].Expiration) { nonExpiredIndex = i break } - if i == 0 { - deleteCount := len(cnc.cache.cache) - cnc.cache.cache = nil - return deleteCount - } } - deleteCount := len(cnc.cache.cache) - nonExpiredIndex - 1 - if nonExpiredIndex != len(cnc.cache.cache)-1 { - cnc.cache.cache = cnc.cache.cache[:nonExpiredIndex+1] + + var deleteCount int + if nonExpiredIndex == -1 { + // No non-expired nonces, delete everything + deleteCount = len(cnc.cache.cache) + cnc.cache.cache = nil + } else { + // Prune everything up to the non-expired nonce + deleteCount = nonExpiredIndex + cnc.cache.cache = cnc.cache.cache[nonExpiredIndex:] } return deleteCount } diff --git a/signer/cosigner_nonce_cache_test.go b/signer/cosigner_nonce_cache_test.go index 756f5960..efa9a201 100644 --- a/signer/cosigner_nonce_cache_test.go +++ b/signer/cosigner_nonce_cache_test.go @@ -168,8 +168,8 @@ func TestNonceCacheDemand(t *testing.T) { count, pruned := mp.Result() - require.Greater(t, count, 0) - require.Equal(t, 0, pruned) + require.Greater(t, count, 0, "count of pruning calls must be greater than 0") + require.Equal(t, 0, pruned, "no nonces should have been pruned") } func TestNonceCacheExpiration(t *testing.T) { @@ -181,13 +181,16 @@ func TestNonceCacheExpiration(t *testing.T) { mp := &mockPruner{} + noncesExpiration := 1000 * time.Millisecond + getNoncesInterval := noncesExpiration / 5 + getNoncesTimeout := 10 * time.Millisecond nonceCache := NewCosignerNonceCache( cometlog.NewTMLogger(cometlog.NewSyncWriter(os.Stdout)), cosigners, &MockLeader{id: 1, leader: &ThresholdValidator{myCosigner: lcs[0]}}, - 250*time.Millisecond, - 10*time.Millisecond, - 500*time.Millisecond, + getNoncesInterval, + getNoncesTimeout, + noncesExpiration, 2, mp, ) @@ -196,27 +199,35 @@ func TestNonceCacheExpiration(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) - const loadN = 500 - + const loadN = 100 + // Load first set of 100 nonces nonceCache.LoadN(ctx, loadN) go nonceCache.Start(ctx) - time.Sleep(1 * time.Second) + // Sleep for 1/2 nonceExpiration, no nonces should have expired yet + time.Sleep(noncesExpiration / 2) + + // Load second set of 100 nonces + nonceCache.LoadN(ctx, loadN) + + // Wait for first set of nonces to expire + wait for the interval to have run + time.Sleep((noncesExpiration / 2) + getNoncesInterval) count, pruned := mp.Result() - // we should have pruned at least three times after - // waiting for a second with a reconcile interval of 250ms - require.GreaterOrEqual(t, count, 3) + // we should have pruned at least 5 times after + // waiting for 1200ms with a reconcile interval of 200ms + require.GreaterOrEqual(t, count, 5) - // we should have pruned at least the number of nonces we loaded and knew would expire - require.GreaterOrEqual(t, pruned, loadN) + // we should have pruned only the first set of nonces + // The second set of nonces should not have expired yet and we should not have load any more + require.Equal(t, pruned, loadN) cancel() - // the cache should be empty or 1 since no nonces are being consumed. - require.LessOrEqual(t, nonceCache.cache.Size(), 1) + // the cache should be 100 (loadN) as the second set should not have expired. + require.LessOrEqual(t, nonceCache.cache.Size(), loadN) } func TestNonceCacheDemandSlow(t *testing.T) { From 60760f27ff9267167c71babe728ca8efc1ea7658 Mon Sep 17 00:00:00 2001 From: Andrew Gouin Date: Mon, 11 Dec 2023 23:26:12 -0700 Subject: [PATCH 2/2] add unit tests for prune (#235) fix test --- signer/cosigner_nonce_cache.go | 53 ++++----- signer/cosigner_nonce_cache_test.go | 162 +++++++++++++++++++++++++++- 2 files changed, 185 insertions(+), 30 deletions(-) diff --git a/signer/cosigner_nonce_cache.go b/signer/cosigner_nonce_cache.go index a1b276a9..f9b34b89 100644 --- a/signer/cosigner_nonce_cache.go +++ b/signer/cosigner_nonce_cache.go @@ -32,7 +32,7 @@ type CosignerNonceCache struct { threshold uint8 - cache NonceCache + cache *NonceCache pruner NonceCachePruner @@ -112,6 +112,30 @@ func (nc *NonceCache) Delete(index int) { nc.cache = append(nc.cache[:index], nc.cache[index+1:]...) } +func (nc *NonceCache) PruneNonces() int { + nc.mu.Lock() + defer nc.mu.Unlock() + nonExpiredIndex := -1 + for i := 0; i < len(nc.cache); i++ { + if time.Now().Before(nc.cache[i].Expiration) { + nonExpiredIndex = i + break + } + } + + var deleteCount int + if nonExpiredIndex == -1 { + // No non-expired nonces, delete everything + deleteCount = len(nc.cache) + nc.cache = nil + } else { + // Prune everything up to the non-expired nonce + deleteCount = nonExpiredIndex + nc.cache = nc.cache[nonExpiredIndex:] + } + return deleteCount +} + type CosignerNoncesRel struct { Cosigner Cosigner Nonces CosignerNonces @@ -152,13 +176,14 @@ func NewCosignerNonceCache( nonceExpiration: nonceExpiration, threshold: threshold, pruner: pruner, + cache: new(NonceCache), // buffer up to 1000 empty events so that we don't ever block empty: make(chan struct{}, 1000), movingAverage: newMovingAverage(4 * getNoncesInterval), // weighted average over 4 intervals } // the only time pruner is expected to be non-nil is during tests, otherwise we use the cache logic. if pruner == nil { - cnc.pruner = cnc + cnc.pruner = cnc.cache } return cnc @@ -371,30 +396,6 @@ CheckNoncesLoop: return nil, fmt.Errorf("no nonces found involving cosigners %+v", cosignerInts) } -func (cnc *CosignerNonceCache) PruneNonces() int { - cnc.cache.mu.Lock() - defer cnc.cache.mu.Unlock() - nonExpiredIndex := -1 - for i := 0; i < len(cnc.cache.cache); i++ { - if time.Now().Before(cnc.cache.cache[i].Expiration) { - nonExpiredIndex = i - break - } - } - - var deleteCount int - if nonExpiredIndex == -1 { - // No non-expired nonces, delete everything - deleteCount = len(cnc.cache.cache) - cnc.cache.cache = nil - } else { - // Prune everything up to the non-expired nonce - deleteCount = nonExpiredIndex - cnc.cache.cache = cnc.cache.cache[nonExpiredIndex:] - } - return deleteCount -} - func (cnc *CosignerNonceCache) ClearNonces(cosigner Cosigner) { cnc.cache.mu.Lock() defer cnc.cache.mu.Unlock() diff --git a/signer/cosigner_nonce_cache_test.go b/signer/cosigner_nonce_cache_test.go index efa9a201..ea65696a 100644 --- a/signer/cosigner_nonce_cache_test.go +++ b/signer/cosigner_nonce_cache_test.go @@ -64,6 +64,7 @@ func TestClearNonces(t *testing.T) { cnc := CosignerNonceCache{ threshold: 2, + cache: new(NonceCache), } for i := 0; i < 10; i++ { @@ -102,14 +103,14 @@ func TestClearNonces(t *testing.T) { } type mockPruner struct { - cnc *CosignerNonceCache + cache *NonceCache count int pruned int mu sync.Mutex } func (mp *mockPruner) PruneNonces() int { - pruned := mp.cnc.PruneNonces() + pruned := mp.cache.PruneNonces() mp.mu.Lock() defer mp.mu.Unlock() mp.count++ @@ -143,7 +144,7 @@ func TestNonceCacheDemand(t *testing.T) { mp, ) - mp.cnc = nonceCache + mp.cache = nonceCache.cache ctx, cancel := context.WithCancel(context.Background()) @@ -195,7 +196,7 @@ func TestNonceCacheExpiration(t *testing.T) { mp, ) - mp.cnc = nonceCache + mp.cache = nonceCache.cache ctx, cancel := context.WithCancel(context.Background()) @@ -230,6 +231,159 @@ func TestNonceCacheExpiration(t *testing.T) { require.LessOrEqual(t, nonceCache.cache.Size(), loadN) } +func TestNonceCachePrune(t *testing.T) { + type testCase struct { + name string + nonces []*CachedNonce + expected []*CachedNonce + } + + now := time.Now() + + testCases := []testCase{ + { + name: "no nonces", + nonces: nil, + expected: nil, + }, + { + name: "no expired nonces", + nonces: []*CachedNonce{ + { + UUID: uuid.MustParse("d6ef381f-6234-432d-b204-d8957fe60360"), + Expiration: now.Add(1 * time.Second), + }, + { + UUID: uuid.MustParse("cdc3673d-7946-459a-b458-cbbde0eecd04"), + Expiration: now.Add(2 * time.Second), + }, + { + UUID: uuid.MustParse("38c6a201-0b8b-46eb-ab69-c7b2716d408e"), + Expiration: now.Add(3 * time.Second), + }, + { + UUID: uuid.MustParse("5caf5ab2-d460-430f-87fa-8ed2983ae8fb"), + Expiration: now.Add(4 * time.Second), + }, + }, + expected: []*CachedNonce{ + { + UUID: uuid.MustParse("d6ef381f-6234-432d-b204-d8957fe60360"), + Expiration: now.Add(1 * time.Second), + }, + { + UUID: uuid.MustParse("cdc3673d-7946-459a-b458-cbbde0eecd04"), + Expiration: now.Add(2 * time.Second), + }, + { + UUID: uuid.MustParse("38c6a201-0b8b-46eb-ab69-c7b2716d408e"), + Expiration: now.Add(3 * time.Second), + }, + { + UUID: uuid.MustParse("5caf5ab2-d460-430f-87fa-8ed2983ae8fb"), + Expiration: now.Add(4 * time.Second), + }, + }, + }, + { + name: "first nonce is expired", + nonces: []*CachedNonce{ + { + UUID: uuid.MustParse("d6ef381f-6234-432d-b204-d8957fe60360"), + Expiration: now.Add(-1 * time.Second), + }, + { + UUID: uuid.MustParse("cdc3673d-7946-459a-b458-cbbde0eecd04"), + Expiration: now.Add(2 * time.Second), + }, + { + UUID: uuid.MustParse("38c6a201-0b8b-46eb-ab69-c7b2716d408e"), + Expiration: now.Add(3 * time.Second), + }, + { + UUID: uuid.MustParse("5caf5ab2-d460-430f-87fa-8ed2983ae8fb"), + Expiration: now.Add(4 * time.Second), + }, + }, + expected: []*CachedNonce{ + { + UUID: uuid.MustParse("cdc3673d-7946-459a-b458-cbbde0eecd04"), + Expiration: now.Add(2 * time.Second), + }, + { + UUID: uuid.MustParse("38c6a201-0b8b-46eb-ab69-c7b2716d408e"), + Expiration: now.Add(3 * time.Second), + }, + { + UUID: uuid.MustParse("5caf5ab2-d460-430f-87fa-8ed2983ae8fb"), + Expiration: now.Add(4 * time.Second), + }, + }, + }, + { + name: "all but last nonce expired", + nonces: []*CachedNonce{ + { + UUID: uuid.MustParse("d6ef381f-6234-432d-b204-d8957fe60360"), + Expiration: now.Add(-1 * time.Second), + }, + { + UUID: uuid.MustParse("cdc3673d-7946-459a-b458-cbbde0eecd04"), + Expiration: now.Add(-1 * time.Second), + }, + { + UUID: uuid.MustParse("38c6a201-0b8b-46eb-ab69-c7b2716d408e"), + Expiration: now.Add(-1 * time.Second), + }, + { + UUID: uuid.MustParse("5caf5ab2-d460-430f-87fa-8ed2983ae8fb"), + Expiration: now.Add(4 * time.Second), + }, + }, + expected: []*CachedNonce{ + { + UUID: uuid.MustParse("5caf5ab2-d460-430f-87fa-8ed2983ae8fb"), + Expiration: now.Add(4 * time.Second), + }, + }, + }, + { + name: "all nonces expired", + nonces: []*CachedNonce{ + { + UUID: uuid.MustParse("d6ef381f-6234-432d-b204-d8957fe60360"), + Expiration: now.Add(-1 * time.Second), + }, + { + UUID: uuid.MustParse("cdc3673d-7946-459a-b458-cbbde0eecd04"), + Expiration: now.Add(-1 * time.Second), + }, + { + UUID: uuid.MustParse("38c6a201-0b8b-46eb-ab69-c7b2716d408e"), + Expiration: now.Add(-1 * time.Second), + }, + { + UUID: uuid.MustParse("5caf5ab2-d460-430f-87fa-8ed2983ae8fb"), + Expiration: now.Add(-1 * time.Second), + }, + }, + expected: nil, + }, + } + + for _, tc := range testCases { + nc := NonceCache{ + cache: tc.nonces, + } + + pruned := nc.PruneNonces() + + require.Equal(t, len(tc.nonces)-len(tc.expected), pruned, tc.name) + + require.Equal(t, tc.expected, nc.cache, tc.name) + } +} + func TestNonceCacheDemandSlow(t *testing.T) { lcs, _ := getTestLocalCosigners(t, 2, 3) cosigners := make([]Cosigner, len(lcs))