From f8e4fb4072624bc18cfb774efeaa9eeace2fc68c Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Fri, 7 Jul 2023 18:28:17 +0200 Subject: [PATCH 1/3] test(sync): Implement sync test for recovering from bad sync target --- headertest/dummy_header.go | 7 ++++ sync/sync_test.go | 83 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/headertest/dummy_header.go b/headertest/dummy_header.go index f4ec3507..52d7e342 100644 --- a/headertest/dummy_header.go +++ b/headertest/dummy_header.go @@ -27,6 +27,8 @@ type DummyHeader struct { Raw hash header.Hash + + VerifyFailure bool // TODO } func RandDummyHeader(t *testing.T) *DummyHeader { @@ -39,6 +41,7 @@ func RandDummyHeader(t *testing.T) *DummyHeader { Time: time.Now().UTC(), }, nil, + false, } err := dh.rehash() if err != nil { @@ -100,6 +103,10 @@ func (d *DummyHeader) IsExpired(period time.Duration) bool { } func (d *DummyHeader) Verify(header header.Header) error { + if dummy, _ := header.(*DummyHeader); dummy.VerifyFailure { + return fmt.Errorf("bad header") // TODO + } + epsilon := 10 * time.Second if header.Time().After(time.Now().Add(epsilon)) { return fmt.Errorf("header Time too far in the future") diff --git a/sync/sync_test.go b/sync/sync_test.go index d67d7caa..bd5afb78 100644 --- a/sync/sync_test.go +++ b/sync/sync_test.go @@ -285,6 +285,89 @@ func TestSyncerIncomingDuplicate(t *testing.T) { require.NoError(t, err) } +// TestSync_InvalidSyncTarget tests the possible case that an incoming +// header passes non-adjacent verification and is set as the sync target +// but is actually invalid once it is processed via VerifyAdjacent during sync +func TestSync_InvalidSyncTarget(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) + t.Cleanup(cancel) + + suite := headertest.NewTestSuite(t) + head := suite.Head() + + remoteStore := headertest.NewStore[*headertest.DummyHeader](t, suite, 100) + localStore := store.NewTestStore(ctx, t, head) + + syncer, err := NewSyncer[*headertest.DummyHeader]( + local.NewExchange[*headertest.DummyHeader](remoteStore), + localStore, + headertest.NewDummySubscriber(), + WithTrustingPeriod(time.Second), + WithBlockTime(time.Nanosecond), + ) + require.NoError(t, err) + + headers := suite.GenDummyHeaders(300) + // malform the remote store's head so that it can serve + // the syncer a "bad" sync target that passes initial validation, + // but not verification. + maliciousHeader := headers[299] + maliciousHeader.VerifyFailure = true + err = remoteStore.Append(ctx, headers...) + require.NoError(t, err) + + // TODO + h, err := syncer.Head(ctx) + require.NoError(t, err) + require.Equal(t, maliciousHeader.Height(), h.Height()) + + err = syncer.Start(ctx) + require.NoError(t, err) + + time.Sleep(time.Millisecond * 100) // TODO flakey? + + shortCtx, cancel := context.WithTimeout(ctx, time.Millisecond*200) + err = syncer.SyncWait(shortCtx) + require.ErrorIs(t, err, context.DeadlineExceeded) + cancel() + + // TODO + require.Equal(t, uint64(maliciousHeader.Height()), syncer.State().ToHeight) + + h, err = localStore.Head(ctx) + require.NoError(t, err) + require.Equal(t, maliciousHeader.Height()-1, h.Height()) + + // TODO + remoteStore.Headers[maliciousHeader.Height()].VerifyFailure = false + + // generate more headers and trigger sync again + err = remoteStore.Append(ctx, suite.GenDummyHeaders(100)...) + require.NoError(t, err) + + // pretend new header is received from network + expectedHead, err := remoteStore.Head(ctx) + require.NoError(t, err) + syncer.incomingNetworkHead(ctx, expectedHead) + err = syncer.SyncWait(ctx) + require.NoError(t, err) + + // ensure that maliciousHeader height was re-requested and a good one was + // found + rerequested, err := localStore.GetByHeight(ctx, uint64(maliciousHeader.Height())) + require.NoError(t, err) + require.False(t, rerequested.VerifyFailure) + + gotHead, err := localStore.Head(ctx) + require.NoError(t, err) + + syncHead, err := syncer.Head(ctx) + require.NoError(t, err) + + require.Equal(t, expectedHead.Height()-1, gotHead.Height()) + require.Equal(t, expectedHead.Height(), syncHead.Height()) +} + type delayedGetter[H header.Header] struct { header.Getter[H] } From 591b03760c492189503de6635db5e0ca8f90a1f2 Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Tue, 11 Jul 2023 10:51:25 +0200 Subject: [PATCH 2/3] doc(sync): add more clarifying comments to test --- headertest/dummy_header.go | 6 +++-- sync/sync_test.go | 51 ++++++++++++++++++++++---------------- 2 files changed, 34 insertions(+), 23 deletions(-) diff --git a/headertest/dummy_header.go b/headertest/dummy_header.go index 52d7e342..a6576e93 100644 --- a/headertest/dummy_header.go +++ b/headertest/dummy_header.go @@ -28,7 +28,9 @@ type DummyHeader struct { hash header.Hash - VerifyFailure bool // TODO + // VerifyFailure allows for testing scenarios where a header would fail + // verification. When set to true, it forces a failure. + VerifyFailure bool } func RandDummyHeader(t *testing.T) *DummyHeader { @@ -104,7 +106,7 @@ func (d *DummyHeader) IsExpired(period time.Duration) bool { func (d *DummyHeader) Verify(header header.Header) error { if dummy, _ := header.(*DummyHeader); dummy.VerifyFailure { - return fmt.Errorf("bad header") // TODO + return fmt.Errorf("header at height %d failed verification", header.Height()) } epsilon := 10 * time.Second diff --git a/sync/sync_test.go b/sync/sync_test.go index bd5afb78..3b677244 100644 --- a/sync/sync_test.go +++ b/sync/sync_test.go @@ -285,9 +285,11 @@ func TestSyncerIncomingDuplicate(t *testing.T) { require.NoError(t, err) } -// TestSync_InvalidSyncTarget tests the possible case that an incoming -// header passes non-adjacent verification and is set as the sync target -// but is actually invalid once it is processed via VerifyAdjacent during sync +// TestSync_InvalidSyncTarget tests the possible case that a sync target +// passes non-adjacent verification but is actually invalid once it is processed +// via VerifyAdjacent during sync. The expected behaviour is that the syncer would +// discard the invalid sync target and listen for a new sync target from headersub +// and sync the valid chain. func TestSync_InvalidSyncTarget(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) @@ -295,18 +297,21 @@ func TestSync_InvalidSyncTarget(t *testing.T) { suite := headertest.NewTestSuite(t) head := suite.Head() - remoteStore := headertest.NewStore[*headertest.DummyHeader](t, suite, 100) + // create a local store which is initialised at genesis height localStore := store.NewTestStore(ctx, t, head) + // create a peer which is already on height 100 + remoteStore := headertest.NewStore[*headertest.DummyHeader](t, suite, 100) syncer, err := NewSyncer[*headertest.DummyHeader]( local.NewExchange[*headertest.DummyHeader](remoteStore), localStore, headertest.NewDummySubscriber(), WithTrustingPeriod(time.Second), - WithBlockTime(time.Nanosecond), + WithBlockTime(time.Nanosecond), // force syncer to request more recent sync target ) require.NoError(t, err) + // generate 300 more headers headers := suite.GenDummyHeaders(300) // malform the remote store's head so that it can serve // the syncer a "bad" sync target that passes initial validation, @@ -316,39 +321,43 @@ func TestSync_InvalidSyncTarget(t *testing.T) { err = remoteStore.Append(ctx, headers...) require.NoError(t, err) - // TODO - h, err := syncer.Head(ctx) - require.NoError(t, err) - require.Equal(t, maliciousHeader.Height(), h.Height()) - + // start syncer so that it immediately requests the bad + // sync target from the remote peer via Head request err = syncer.Start(ctx) require.NoError(t, err) - time.Sleep(time.Millisecond * 100) // TODO flakey? + // give syncer some time to register the sync job before + // we wait for it to "finish syncing" + time.Sleep(time.Millisecond * 100) + // expect syncer to not be able to finish the sync + // job as the bad sync target cannot be applied shortCtx, cancel := context.WithTimeout(ctx, time.Millisecond*200) err = syncer.SyncWait(shortCtx) require.ErrorIs(t, err, context.DeadlineExceeded) cancel() - - // TODO + // ensure that syncer still expects to sync to the bad sync target's + // height require.Equal(t, uint64(maliciousHeader.Height()), syncer.State().ToHeight) - - h, err = localStore.Head(ctx) + // ensure syncer could only sync up to one header below the bad sync target + h, err := localStore.Head(ctx) require.NoError(t, err) require.Equal(t, maliciousHeader.Height()-1, h.Height()) - // TODO + // manually change bad sync target to a good header in remote peer + // store so it can re-serve it to syncer once it re-requests the height remoteStore.Headers[maliciousHeader.Height()].VerifyFailure = false - // generate more headers and trigger sync again + // generate more headers err = remoteStore.Append(ctx, suite.GenDummyHeaders(100)...) require.NoError(t, err) - // pretend new header is received from network + // pretend new header is received from network to trigger + // a new sync job to a good sync target expectedHead, err := remoteStore.Head(ctx) require.NoError(t, err) syncer.incomingNetworkHead(ctx, expectedHead) + // wait for syncer to finish err = syncer.SyncWait(ctx) require.NoError(t, err) @@ -358,13 +367,13 @@ func TestSync_InvalidSyncTarget(t *testing.T) { require.NoError(t, err) require.False(t, rerequested.VerifyFailure) - gotHead, err := localStore.Head(ctx) + // check store head and syncer head + storeHead, err := localStore.Head(ctx) require.NoError(t, err) - syncHead, err := syncer.Head(ctx) require.NoError(t, err) - require.Equal(t, expectedHead.Height()-1, gotHead.Height()) + require.Equal(t, expectedHead.Height()-1, storeHead.Height()) // head might not be applied to store yet require.Equal(t, expectedHead.Height(), syncHead.Height()) } From e8eb8fca604b69e20829b49876612661dd3f036e Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Wed, 12 Jul 2023 10:52:23 +0200 Subject: [PATCH 3/3] test(sync): Add another time.Sleep (unfortunately) to reduce flake --- sync/sync_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/sync/sync_test.go b/sync/sync_test.go index 3b677244..06f486c3 100644 --- a/sync/sync_test.go +++ b/sync/sync_test.go @@ -357,7 +357,10 @@ func TestSync_InvalidSyncTarget(t *testing.T) { expectedHead, err := remoteStore.Head(ctx) require.NoError(t, err) syncer.incomingNetworkHead(ctx, expectedHead) - // wait for syncer to finish + + // wait for syncer to finish (give it a bit of time to register + // new job with new sync target) + time.Sleep(100 * time.Millisecond) err = syncer.SyncWait(ctx) require.NoError(t, err) @@ -373,8 +376,8 @@ func TestSync_InvalidSyncTarget(t *testing.T) { syncHead, err := syncer.Head(ctx) require.NoError(t, err) - require.Equal(t, expectedHead.Height()-1, storeHead.Height()) // head might not be applied to store yet require.Equal(t, expectedHead.Height(), syncHead.Height()) + require.Equal(t, expectedHead.Height(), storeHead.Height()) } type delayedGetter[H header.Header] struct {