From 4257bdfa26914c81c047704ebe5fc3c010df59f7 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Tue, 24 Sep 2024 16:03:20 +0200 Subject: [PATCH 01/20] feat(sync): bifurcation for syncTarget --- sync/sync_head.go | 75 ++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 64 insertions(+), 11 deletions(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index c74347b7..f5d82181 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -3,6 +3,7 @@ package sync import ( "context" "errors" + "fmt" "time" "github.com/celestiaorg/go-header" @@ -173,22 +174,74 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { } var verErr *header.VerifyError - if errors.As(err, &verErr) && !verErr.SoftFailure { - logF := log.Warnw - if errors.Is(err, header.ErrKnownHeader) { - logF = log.Debugw - } - logF("invalid network header", - "height_of_invalid", newHead.Height(), - "hash_of_invalid", newHead.Hash(), - "height_of_subjective", sbjHead.Height(), - "hash_of_subjective", sbjHead.Hash(), - "reason", verErr.Reason) + if !errors.As(err, &verErr) { + return false, nil + } + + if verErr.SoftFailure { + err := s.verifySkipping(ctx, sbjHead.Height(), newHead) + return false, err + } + + logF := log.Warnw + if errors.Is(err, header.ErrKnownHeader) { + logF = log.Debugw } + logF("invalid network header", + "height_of_invalid", newHead.Height(), + "hash_of_invalid", newHead.Hash(), + "height_of_subjective", sbjHead.Height(), + "hash_of_subjective", sbjHead.Hash(), + "reason", verErr.Reason, + ) return verErr.SoftFailure, err } +/* +Subjective head is 500, network head is 1000. +Header at height 1000 does not have sufficient validator set overlap, +so the client downloads height 750 (which does have enough sufficient overlap), +verifies it against 500 and advances the subjective head to 750. + +Client tries to apply height 1000 against 750 and if there is sufficient overlap, +it applies 1000 as the subjective head. +If not, it downloads the halfway point and retries the process. +*/ +func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHeight uint64, networkHeader H) error { + diff := networkHeader.Height() - subjHeight + if diff <= 0 { + panic(fmt.Sprintf("implementation bug: diff is %d", diff)) + } + + for diff > 0 { + diff = diff / 2 + subjHeight += diff + + subjHeader, err := s.getter.GetByHeight(ctx, subjHeight) + if err != nil { + return err + } + + if err := header.Verify(subjHeader, networkHeader); err == nil { + return nil + } + } + return &NewValidatorSetCantBeTrustedError{ + NetHeadHeight: networkHeader.Height(), + NetHeadHash: networkHeader.Hash(), + } +} + +type NewValidatorSetCantBeTrustedError struct { + NetHeadHeight uint64 + NetHeadHash []byte +} + +func (e *NewValidatorSetCantBeTrustedError) Error() string { + return fmt.Sprintf("sync: new validator set cant be trusted: head %d, attempted %s", e.NetHeadHeight, e.NetHeadHash) +} + // isExpired checks if header is expired against trusting period. func isExpired[H header.Header[H]](header H, period time.Duration) bool { expirationTime := header.Time().Add(period) From a260e8d19b3ad515f0b7cea1d88063d7ae7cdc6b Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Fri, 4 Oct 2024 13:27:34 +0200 Subject: [PATCH 02/20] fix --- sync/sync_head.go | 6 ++++++ sync/sync_head_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/sync/sync_head.go b/sync/sync_head.go index f5d82181..cec9a485 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -223,6 +223,12 @@ func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHeight uint64, netwo return err } + _, _ = s.subjectiveHead(ctx) + + if err := header.Verify(subjHeader, networkHeader); err == nil { + return nil + } + if err := header.Verify(subjHeader, networkHeader); err == nil { return nil } diff --git a/sync/sync_head_test.go b/sync/sync_head_test.go index cc60e481..5a291be0 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -94,6 +94,50 @@ func TestSyncer_HeadWithTrustedHead(t *testing.T) { require.True(t, wrappedGetter.withTrustedHead) } +func TestSyncer_HeadWithNotEnoughValidators(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) + t.Cleanup(cancel) + + suite := headertest.NewTestSuite(t) + head := suite.Head() + + localStore := newTestStore(t, ctx, head) + remoteStore := newTestStore(t, ctx, head) + + err := remoteStore.Append(ctx, suite.GenDummyHeaders(100)...) + require.NoError(t, err) + + // create a wrappedGetter to track exchange interactions + wrappedGetter := newWrappedGetter(local.NewExchange(remoteStore)) + + syncer, err := NewSyncer( + wrappedGetter, + localStore, + headertest.NewDummySubscriber(), + WithBlockTime(time.Nanosecond), + WithRecencyThreshold(time.Nanosecond), // forces a request for a new sync target + // ensures that syncer's store contains a subjective head that is within + // the unbonding period so that the syncer can use a header from the network + // as a sync target + WithTrustingPeriod(time.Hour), + ) + require.NoError(t, err) + + // start the syncer which triggers a Head request that will + // load the syncer's subjective head from the store, and request + // a new sync target from the network rather than from trusted peers + err = syncer.Start(ctx) + require.NoError(t, err) + t.Cleanup(func() { + err = syncer.Stop(ctx) + require.NoError(t, err) + }) + + // ensure the syncer really requested Head from the network + // rather than from trusted peers + require.True(t, wrappedGetter.withTrustedHead) +} + type wrappedGetter struct { ex header.Exchange[*headertest.DummyHeader] From c628f3c916c63d32b0f833d074e57422f3d8854f Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Wed, 9 Oct 2024 15:01:18 +0200 Subject: [PATCH 03/20] new algo --- sync/sync_head.go | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index cec9a485..691a1cfc 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -179,7 +179,7 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { } if verErr.SoftFailure { - err := s.verifySkipping(ctx, sbjHead.Height(), newHead) + err := s.verifySkipping(ctx, sbjHead, newHead) return false, err } @@ -208,31 +208,42 @@ Client tries to apply height 1000 against 750 and if there is sufficient overlap it applies 1000 as the subjective head. If not, it downloads the halfway point and retries the process. */ -func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHeight uint64, networkHeader H) error { +func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader H) error { + subjHeight := subjHead.Height() + diff := networkHeader.Height() - subjHeight if diff <= 0 { panic(fmt.Sprintf("implementation bug: diff is %d", diff)) } - for diff > 0 { - diff = diff / 2 - subjHeight += diff + for diff > 1 { + candidateHeight := subjHeight + diff/2 - subjHeader, err := s.getter.GetByHeight(ctx, subjHeight) + candidateHeader, err := s.getter.GetByHeight(ctx, candidateHeight) if err != nil { return err } - _, _ = s.subjectiveHead(ctx) - - if err := header.Verify(subjHeader, networkHeader); err == nil { - return nil + if err := header.Verify(subjHead, candidateHeader); err != nil { + // candidate failed, go deeper in 1st half. + diff = diff / 2 + continue } - if err := header.Verify(subjHeader, networkHeader); err == nil { + // candidate was validated properly, update subjHead. + subjHead = candidateHeader + // TODO: s.setSubjectiveHead(ctx, subjHead) + + if err := header.Verify(subjHead, networkHeader); err == nil { + // network head validate properly, return success. return nil } + + // new subjHead failed, go deeper in 2nd half. + subjHeight = subjHead.Height() + diff = networkHeader.Height() - subjHeight } + return &NewValidatorSetCantBeTrustedError{ NetHeadHeight: networkHeader.Height(), NetHeadHash: networkHeader.Hash(), From 2fb0ef9ee146084fb99f040c193f0a7136fa1fff Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Mon, 14 Oct 2024 11:58:08 +0200 Subject: [PATCH 04/20] add tests --- headertest/dummy_header.go | 6 ++ sync/sync_head.go | 2 +- sync/sync_head_test.go | 143 ++++++++++++++++++++++++++++++++++++- 3 files changed, 148 insertions(+), 3 deletions(-) diff --git a/headertest/dummy_header.go b/headertest/dummy_header.go index e8480dde..108c4bc3 100644 --- a/headertest/dummy_header.go +++ b/headertest/dummy_header.go @@ -30,6 +30,9 @@ type DummyHeader struct { // SoftFailure allows for testing scenarios where a header would fail // verification with SoftFailure set to true SoftFailure bool + + // VerifyFn can be used to change header.Verify behaviour per header. + VerifyFn func(hdr *DummyHeader) error `json:"-"` } func RandDummyHeader(t *testing.T) *DummyHeader { @@ -100,6 +103,9 @@ func (d *DummyHeader) IsExpired(period time.Duration) bool { } func (d *DummyHeader) Verify(hdr *DummyHeader) error { + if d.VerifyFn != nil { + return d.VerifyFn(hdr) + } if hdr.VerifyFailure { return &header.VerifyError{Reason: ErrDummyVerify, SoftFailure: hdr.SoftFailure} } diff --git a/sync/sync_head.go b/sync/sync_head.go index 691a1cfc..50bcf6d4 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -256,7 +256,7 @@ type NewValidatorSetCantBeTrustedError struct { } func (e *NewValidatorSetCantBeTrustedError) Error() string { - return fmt.Sprintf("sync: new validator set cant be trusted: head %d, attempted %s", e.NetHeadHeight, e.NetHeadHash) + return fmt.Sprintf("sync: new validator set cant be trusted: head %d, attempted %x", e.NetHeadHeight, e.NetHeadHash) } // isExpired checks if header is expired against trusting period. diff --git a/sync/sync_head_test.go b/sync/sync_head_test.go index 5a291be0..1e268e0e 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -138,6 +138,146 @@ func TestSyncer_HeadWithNotEnoughValidators(t *testing.T) { require.True(t, wrappedGetter.withTrustedHead) } +func TestSyncer_verifySkipping(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) + t.Cleanup(cancel) + + const total = 1000 + const badHeaderHeight = total + 1 + + suite := headertest.NewTestSuite(t) + head := suite.Head() + + localStore := newTestStore(t, ctx, head) + remoteStore := newTestStore(t, ctx, head) + + // create a wrappedGetter to track exchange interactions + wrappedGetter := newWrappedGetter(local.NewExchange(remoteStore)) + + syncer, err := NewSyncer( + wrappedGetter, + localStore, + headertest.NewDummySubscriber(), + WithBlockTime(time.Nanosecond), + WithRecencyThreshold(time.Nanosecond), // forces a request for a new sync target + // ensures that syncer's store contains a subjective head that is within + // the unbonding period so that the syncer can use a header from the network + // as a sync target + WithTrustingPeriod(time.Hour), + ) + require.NoError(t, err) + + // start the syncer which triggers a Head request that will + // load the syncer's subjective head from the store, and request + // a new sync target from the network rather than from trusted peers + err = syncer.Start(ctx) + require.NoError(t, err) + t.Cleanup(func() { + err = syncer.Stop(ctx) + require.NoError(t, err) + }) + + t.Run("success (with bad candidates)", func(t *testing.T) { + const iters = 4 + + headers := suite.GenDummyHeaders(total) + err = remoteStore.Append(ctx, headers...) + require.NoError(t, err) + + var verifyCounter atomic.Int32 + for i := range total { + headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { + if i >= 501 { + return nil + } + + verifyCounter.Add(1) + if verifyCounter.Load() <= iters { + return &header.VerifyError{ + Reason: headertest.ErrDummyVerify, + SoftFailure: hdr.SoftFailure, + } + } + return nil + } + } + + headers[total-1].VerifyFailure = true + headers[total-1].SoftFailure = true + + subjHead, err := syncer.subjectiveHead(ctx) + require.NoError(t, err) + + err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) + require.NoError(t, err) + }) + + t.Run("success", func(t *testing.T) { + const iters = 4 + + headers := suite.GenDummyHeaders(total) + err = remoteStore.Append(ctx, headers...) + require.NoError(t, err) + + var verifyCounter atomic.Int32 + for i := range total { + headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { + if hdr.Height() != badHeaderHeight { + return nil + } + + verifyCounter.Add(1) + if verifyCounter.Load() >= iters { + return nil + } + + return &header.VerifyError{ + Reason: headertest.ErrDummyVerify, + SoftFailure: hdr.SoftFailure, + } + } + } + + headers[total-1].VerifyFailure = true + headers[total-1].SoftFailure = true + + subjHead, err := syncer.subjectiveHead(ctx) + require.NoError(t, err) + + err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) + require.NoError(t, err) + }) + + t.Run("cannot verify", func(t *testing.T) { + headers := suite.GenDummyHeaders(total) + err = remoteStore.Append(ctx, headers...) + require.NoError(t, err) + + for i := range total { + headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { + if hdr.Height() != badHeaderHeight { + return nil + } + + return &header.VerifyError{ + Reason: headertest.ErrDummyVerify, + SoftFailure: hdr.SoftFailure, + } + } + } + + headers[total-1].VerifyFailure = true + headers[total-1].SoftFailure = true + + subjHead, err := syncer.subjectiveHead(ctx) + require.NoError(t, err) + + err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) + var verErr *NewValidatorSetCantBeTrustedError + assert.ErrorAs(t, err, &verErr) + }) +} + type wrappedGetter struct { ex header.Exchange[*headertest.DummyHeader] @@ -170,8 +310,7 @@ func (t *wrappedGetter) Get(ctx context.Context, hash header.Hash) (*headertest. } func (t *wrappedGetter) GetByHeight(ctx context.Context, u uint64) (*headertest.DummyHeader, error) { - // TODO implement me - panic("implement me") + return t.ex.GetByHeight(ctx, u) } func (t *wrappedGetter) GetRangeByHeight( From 35f6731ca93814f8254050223053e378bc269317 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Mon, 14 Oct 2024 12:40:36 +0200 Subject: [PATCH 05/20] fix returned error and split tests --- sync/sync_head.go | 36 ++++--- sync/sync_head_test.go | 206 ++++++++++++++++++++++++++++------------- 2 files changed, 157 insertions(+), 85 deletions(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index 50bcf6d4..f808f6c6 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -174,28 +174,26 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { } var verErr *header.VerifyError - if !errors.As(err, &verErr) { - return false, nil - } + if errors.As(err, &verErr) { + if verErr.SoftFailure { + err := s.verifySkipping(ctx, sbjHead, newHead) + var errValSet *NewValidatorSetCantBeTrustedError + return errors.As(err, &errValSet), err + } - if verErr.SoftFailure { - err := s.verifySkipping(ctx, sbjHead, newHead) - return false, err + logF := log.Warnw + if errors.Is(err, header.ErrKnownHeader) { + logF = log.Debugw + } + logF("invalid network header", + "height_of_invalid", newHead.Height(), + "hash_of_invalid", newHead.Hash(), + "height_of_subjective", sbjHead.Height(), + "hash_of_subjective", sbjHead.Hash(), + "reason", verErr.Reason) } - logF := log.Warnw - if errors.Is(err, header.ErrKnownHeader) { - logF = log.Debugw - } - logF("invalid network header", - "height_of_invalid", newHead.Height(), - "hash_of_invalid", newHead.Hash(), - "height_of_subjective", sbjHead.Height(), - "hash_of_subjective", sbjHead.Hash(), - "reason", verErr.Reason, - ) - - return verErr.SoftFailure, err + return false, err } /* diff --git a/sync/sync_head_test.go b/sync/sync_head_test.go index 1e268e0e..73749261 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -138,7 +138,7 @@ func TestSyncer_HeadWithNotEnoughValidators(t *testing.T) { require.True(t, wrappedGetter.withTrustedHead) } -func TestSyncer_verifySkipping(t *testing.T) { +func TestSyncer_verifySkippingSuccess(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) @@ -177,105 +177,179 @@ func TestSyncer_verifySkipping(t *testing.T) { require.NoError(t, err) }) - t.Run("success (with bad candidates)", func(t *testing.T) { - const iters = 4 + const iters = 4 - headers := suite.GenDummyHeaders(total) - err = remoteStore.Append(ctx, headers...) - require.NoError(t, err) + headers := suite.GenDummyHeaders(total) + err = remoteStore.Append(ctx, headers...) + require.NoError(t, err) - var verifyCounter atomic.Int32 - for i := range total { - headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { - if i >= 501 { - return nil - } + var verifyCounter atomic.Int32 + for i := range total { + headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { + if hdr.Height() != badHeaderHeight { + return nil + } - verifyCounter.Add(1) - if verifyCounter.Load() <= iters { - return &header.VerifyError{ - Reason: headertest.ErrDummyVerify, - SoftFailure: hdr.SoftFailure, - } - } + verifyCounter.Add(1) + if verifyCounter.Load() >= iters { return nil } + + return &header.VerifyError{ + Reason: headertest.ErrDummyVerify, + SoftFailure: hdr.SoftFailure, + } } + } - headers[total-1].VerifyFailure = true - headers[total-1].SoftFailure = true + headers[total-1].VerifyFailure = true + headers[total-1].SoftFailure = true - subjHead, err := syncer.subjectiveHead(ctx) - require.NoError(t, err) + subjHead, err := syncer.subjectiveHead(ctx) + require.NoError(t, err) - err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) - require.NoError(t, err) - }) + err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) + require.NoError(t, err) +} + +func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) + t.Cleanup(cancel) + + const total = 1000 + const badHeaderHeight = total + 1 - t.Run("success", func(t *testing.T) { - const iters = 4 + suite := headertest.NewTestSuite(t) + head := suite.Head() - headers := suite.GenDummyHeaders(total) - err = remoteStore.Append(ctx, headers...) + localStore := newTestStore(t, ctx, head) + remoteStore := newTestStore(t, ctx, head) + + // create a wrappedGetter to track exchange interactions + wrappedGetter := newWrappedGetter(local.NewExchange(remoteStore)) + + syncer, err := NewSyncer( + wrappedGetter, + localStore, + headertest.NewDummySubscriber(), + WithBlockTime(time.Nanosecond), + WithRecencyThreshold(time.Nanosecond), // forces a request for a new sync target + // ensures that syncer's store contains a subjective head that is within + // the unbonding period so that the syncer can use a header from the network + // as a sync target + WithTrustingPeriod(time.Hour), + ) + require.NoError(t, err) + + // start the syncer which triggers a Head request that will + // load the syncer's subjective head from the store, and request + // a new sync target from the network rather than from trusted peers + err = syncer.Start(ctx) + require.NoError(t, err) + t.Cleanup(func() { + err = syncer.Stop(ctx) require.NoError(t, err) + }) - var verifyCounter atomic.Int32 - for i := range total { - headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { - if hdr.Height() != badHeaderHeight { - return nil - } + const iters = 4 - verifyCounter.Add(1) - if verifyCounter.Load() >= iters { - return nil - } + headers := suite.GenDummyHeaders(total) + err = remoteStore.Append(ctx, headers...) + require.NoError(t, err) + + var verifyCounter atomic.Int32 + for i := range total { + headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { + if i >= 501 { + return nil + } + verifyCounter.Add(1) + if verifyCounter.Load() <= iters { return &header.VerifyError{ Reason: headertest.ErrDummyVerify, SoftFailure: hdr.SoftFailure, } } + return nil } + } - headers[total-1].VerifyFailure = true - headers[total-1].SoftFailure = true + headers[total-1].VerifyFailure = true + headers[total-1].SoftFailure = true - subjHead, err := syncer.subjectiveHead(ctx) - require.NoError(t, err) + subjHead, err := syncer.subjectiveHead(ctx) + require.NoError(t, err) + + err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) + require.NoError(t, err) +} + +func TestSyncer_verifySkippingCannotVerify(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) + t.Cleanup(cancel) + + const total = 1000 + const badHeaderHeight = total + 1 + + suite := headertest.NewTestSuite(t) + head := suite.Head() + + localStore := newTestStore(t, ctx, head) + remoteStore := newTestStore(t, ctx, head) + + // create a wrappedGetter to track exchange interactions + wrappedGetter := newWrappedGetter(local.NewExchange(remoteStore)) - err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) + syncer, err := NewSyncer( + wrappedGetter, + localStore, + headertest.NewDummySubscriber(), + WithBlockTime(time.Nanosecond), + WithRecencyThreshold(time.Nanosecond), // forces a request for a new sync target + // ensures that syncer's store contains a subjective head that is within + // the unbonding period so that the syncer can use a header from the network + // as a sync target + WithTrustingPeriod(time.Hour), + ) + require.NoError(t, err) + + // start the syncer which triggers a Head request that will + // load the syncer's subjective head from the store, and request + // a new sync target from the network rather than from trusted peers + err = syncer.Start(ctx) + require.NoError(t, err) + t.Cleanup(func() { + err = syncer.Stop(ctx) require.NoError(t, err) }) - t.Run("cannot verify", func(t *testing.T) { - headers := suite.GenDummyHeaders(total) - err = remoteStore.Append(ctx, headers...) - require.NoError(t, err) + headers := suite.GenDummyHeaders(total) + err = remoteStore.Append(ctx, headers...) + require.NoError(t, err) - for i := range total { - headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { - if hdr.Height() != badHeaderHeight { - return nil - } + for i := range total { + headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { + if hdr.Height() != badHeaderHeight { + return nil + } - return &header.VerifyError{ - Reason: headertest.ErrDummyVerify, - SoftFailure: hdr.SoftFailure, - } + return &header.VerifyError{ + Reason: headertest.ErrDummyVerify, + SoftFailure: hdr.SoftFailure, } } + } - headers[total-1].VerifyFailure = true - headers[total-1].SoftFailure = true + headers[total-1].VerifyFailure = true + headers[total-1].SoftFailure = true - subjHead, err := syncer.subjectiveHead(ctx) - require.NoError(t, err) + subjHead, err := syncer.subjectiveHead(ctx) + require.NoError(t, err) - err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) - var verErr *NewValidatorSetCantBeTrustedError - assert.ErrorAs(t, err, &verErr) - }) + err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) + var verErr *NewValidatorSetCantBeTrustedError + assert.ErrorAs(t, err, &verErr, "%T", err) } type wrappedGetter struct { From 85937b700658e6eae3ba3671a888bc0bcc503b02 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Mon, 14 Oct 2024 13:08:40 +0200 Subject: [PATCH 06/20] remove old comment --- header.go | 3 ++- p2p/server_test.go | 2 +- sync/sync_head.go | 20 ++++++++++---------- 3 files changed, 13 insertions(+), 12 deletions(-) diff --git a/header.go b/header.go index 010ab2ca..75cfa4d0 100644 --- a/header.go +++ b/header.go @@ -12,7 +12,8 @@ type Header[H any] interface { // New creates new instance of a header. // It exists to overcome limitation of Go's type system. // See: - // https://go.googlesource.com/proposal/+/refs/heads/master/design/43651-type-parameters.md#pointer-method-example + // + //https://go.googlesource.com/proposal/+/refs/heads/master/design/43651-type-parameters.md#pointer-method-example New() H // IsZero reports whether Header is a zero value of it's concrete type. IsZero() bool diff --git a/p2p/server_test.go b/p2p/server_test.go index 1e896b2e..77496d92 100644 --- a/p2p/server_test.go +++ b/p2p/server_test.go @@ -176,7 +176,7 @@ func (timeoutStore[H]) Append(ctx context.Context, _ ...H) error { return ctx.Err() } -func (timeoutStore[H]) GetRange(ctx context.Context, _ uint64, _ uint64) ([]H, error) { +func (timeoutStore[H]) GetRange(ctx context.Context, _, _ uint64) ([]H, error) { <-ctx.Done() return nil, ctx.Err() } diff --git a/sync/sync_head.go b/sync/sync_head.go index f808f6c6..552d5b62 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -196,16 +196,16 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { return false, err } -/* -Subjective head is 500, network head is 1000. -Header at height 1000 does not have sufficient validator set overlap, -so the client downloads height 750 (which does have enough sufficient overlap), -verifies it against 500 and advances the subjective head to 750. - -Client tries to apply height 1000 against 750 and if there is sufficient overlap, -it applies 1000 as the subjective head. -If not, it downloads the halfway point and retries the process. -*/ +// verifySkipping will try to find such headers in range (subjHead, networkHeader) +// that can be verified by subjHead, literally: +// +// header.Verify(subjHead, candidate) +// +// and also such headers can verify `networkHeader`, literally +// +// header.Verify(candidate, networkHeader) +// +// When such candidates cannot be found [NewValidatorSetCantBeTrustedError] will be returned. func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader H) error { subjHeight := subjHead.Height() From ba6e8b17c69bea78750fd76b54caff5796757cf8 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Tue, 5 Nov 2024 15:10:54 +0100 Subject: [PATCH 07/20] review suggestions --- sync/sync_head.go | 21 +++++++++++---------- sync/sync_head_test.go | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index 552d5b62..1fdacfef 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -196,14 +196,11 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { return false, err } -// verifySkipping will try to find such headers in range (subjHead, networkHeader) -// that can be verified by subjHead, literally: -// -// header.Verify(subjHead, candidate) -// -// and also such headers can verify `networkHeader`, literally -// -// header.Verify(candidate, networkHeader) +// verifySkipping performs a bifurcated header verification process such that +// it tries to find a header (or several headers if necessary) +// between the networkHead and the subjectiveHead such that non-adjacent +// (or in the worst case adjacent) verification passes and the networkHead +// can be verified as a valid sync target against the syncer's subjectiveHead. // // When such candidates cannot be found [NewValidatorSetCantBeTrustedError] will be returned. func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader H) error { @@ -211,7 +208,11 @@ func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader diff := networkHeader.Height() - subjHeight if diff <= 0 { - panic(fmt.Sprintf("implementation bug: diff is %d", diff)) + panic(fmt.Sprintf("implementation bug: diff %d, subjective height %d (%X), network height %d (%X)", + diff, + subjHeight, subjHead.Hash(), + networkHeader.Height(), networkHeader.Hash(), + )) } for diff > 1 { @@ -230,7 +231,7 @@ func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader // candidate was validated properly, update subjHead. subjHead = candidateHeader - // TODO: s.setSubjectiveHead(ctx, subjHead) + s.setSubjectiveHead(ctx, subjHead) if err := header.Verify(subjHead, networkHeader); err == nil { // network head validate properly, return success. diff --git a/sync/sync_head_test.go b/sync/sync_head_test.go index 73749261..8defd97b 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -349,7 +349,7 @@ func TestSyncer_verifySkippingCannotVerify(t *testing.T) { err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) var verErr *NewValidatorSetCantBeTrustedError - assert.ErrorAs(t, err, &verErr, "%T", err) + assert.ErrorIs(t, err, verErr, "%T", err) } type wrappedGetter struct { From de56e36a420e8b9a627ba4ebe20a2a2c2f15cd52 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Mon, 11 Nov 2024 11:01:36 +0100 Subject: [PATCH 08/20] review suggestions --- sync/metrics.go | 18 +++++++++++++++++ sync/sync_head.go | 18 ++++------------- sync/sync_head_test.go | 44 ++++++++++++++++++++++++++---------------- 3 files changed, 49 insertions(+), 31 deletions(-) diff --git a/sync/metrics.go b/sync/metrics.go index 590bd5ba..2269a539 100644 --- a/sync/metrics.go +++ b/sync/metrics.go @@ -22,6 +22,7 @@ type metrics struct { trustedPeersOutOfSync metric.Int64Counter outdatedHeader metric.Int64Counter subjectiveInit metric.Int64Counter + failedAgainstSubjHead metric.Int64Counter subjectiveHead atomic.Int64 @@ -71,6 +72,16 @@ func newMetrics() (*metrics, error) { return nil, err } + failedAgainstSubjHead, err := meter.Int64Counter( + "hdr_sync_subj_validation_failed", + metric.WithDescription( + "tracks how many times validation against subjective head failed", + ), + ) + if err != nil { + return nil, err + } + subjectiveHead, err := meter.Int64ObservableGauge( "hdr_sync_subjective_head_gauge", metric.WithDescription("subjective head height"), @@ -112,6 +123,7 @@ func newMetrics() (*metrics, error) { trustedPeersOutOfSync: trustedPeersOutOfSync, outdatedHeader: outdatedHeader, subjectiveInit: subjectiveInit, + failedAgainstSubjHead: failedAgainstSubjHead, syncLoopDurationHist: syncLoopDurationHist, syncLoopRunningInst: syncLoopRunningInst, requestRangeTimeHist: requestRangeTimeHist, @@ -186,6 +198,12 @@ func (m *metrics) newSubjectiveHead(ctx context.Context, height uint64, timestam }) } +func (m *metrics) failedValidationAgainstSubjHead(ctx context.Context) { + m.observe(ctx, func(ctx context.Context) { + m.failedAgainstSubjHead.Add(ctx, 1) + }) +} + func (m *metrics) rangeRequestStart() { if m == nil { return diff --git a/sync/sync_head.go b/sync/sync_head.go index 1fdacfef..41a661d3 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -177,8 +177,7 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { if errors.As(err, &verErr) { if verErr.SoftFailure { err := s.verifySkipping(ctx, sbjHead, newHead) - var errValSet *NewValidatorSetCantBeTrustedError - return errors.As(err, &errValSet), err + return err != nil, err } logF := log.Warnw @@ -243,19 +242,10 @@ func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader diff = networkHeader.Height() - subjHeight } - return &NewValidatorSetCantBeTrustedError{ - NetHeadHeight: networkHeader.Height(), - NetHeadHash: networkHeader.Hash(), - } -} - -type NewValidatorSetCantBeTrustedError struct { - NetHeadHeight uint64 - NetHeadHash []byte -} + s.metrics.failedValidationAgainstSubjHead(ctx) + log.Warnw("sync: header validation against subjHead", "height", networkHeader.Height(), "hash", networkHeader.Hash().String()) -func (e *NewValidatorSetCantBeTrustedError) Error() string { - return fmt.Sprintf("sync: new validator set cant be trusted: head %d, attempted %x", e.NetHeadHeight, e.NetHeadHash) + return fmt.Errorf("sync: header validation against subjHead height:%d hash:%x", networkHeader.Height(), networkHeader.Hash().String()) } // isExpired checks if header is expired against trusting period. diff --git a/sync/sync_head_test.go b/sync/sync_head_test.go index 8defd97b..0657b1f4 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -138,13 +138,12 @@ func TestSyncer_HeadWithNotEnoughValidators(t *testing.T) { require.True(t, wrappedGetter.withTrustedHead) } +// Test will simulate a case with upto `iters` failures before we will get to +// the header that can be verified against subjectiveHead. func TestSyncer_verifySkippingSuccess(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) - const total = 1000 - const badHeaderHeight = total + 1 - suite := headertest.NewTestSuite(t) head := suite.Head() @@ -177,12 +176,18 @@ func TestSyncer_verifySkippingSuccess(t *testing.T) { require.NoError(t, err) }) + // when + const total = 1000 + const badHeaderHeight = total + 1 // make the last header bad const iters = 4 headers := suite.GenDummyHeaders(total) err = remoteStore.Append(ctx, headers...) require.NoError(t, err) + // configure header verification method is such way + // that the first [iters] verification will fail + // but all other will be ok. var verifyCounter atomic.Int32 for i := range total { headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { @@ -212,13 +217,12 @@ func TestSyncer_verifySkippingSuccess(t *testing.T) { require.NoError(t, err) } +// Test will simulate a case with upto `iters` failures before we will get to +// the header that can be verified against subjectiveHead. func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) - const total = 1000 - const badHeaderHeight = total + 1 - suite := headertest.NewTestSuite(t) head := suite.Head() @@ -251,12 +255,17 @@ func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) { require.NoError(t, err) }) + const total = 1000 + const badHeaderHeight = total + 1 const iters = 4 headers := suite.GenDummyHeaders(total) err = remoteStore.Append(ctx, headers...) require.NoError(t, err) + // configure header verification method is such way + // that the first [iters] verification will fail + // but all other will be ok. var verifyCounter atomic.Int32 for i := range total { headers[i].VerifyFn = func(hdr *headertest.DummyHeader) error { @@ -265,13 +274,13 @@ func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) { } verifyCounter.Add(1) - if verifyCounter.Load() <= iters { - return &header.VerifyError{ - Reason: headertest.ErrDummyVerify, - SoftFailure: hdr.SoftFailure, - } + if verifyCounter.Load() > iters { + return nil + } + return &header.VerifyError{ + Reason: headertest.ErrDummyVerify, + SoftFailure: hdr.SoftFailure, } - return nil } } @@ -285,13 +294,12 @@ func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) { require.NoError(t, err) } +// Test will simulate a case when no headers can be verified against subjectiveHead. +// As a result the [NewValidatorSetCantBeTrustedError] error will be returned. func TestSyncer_verifySkippingCannotVerify(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) - const total = 1000 - const badHeaderHeight = total + 1 - suite := headertest.NewTestSuite(t) head := suite.Head() @@ -324,6 +332,9 @@ func TestSyncer_verifySkippingCannotVerify(t *testing.T) { require.NoError(t, err) }) + const total = 1000 + const badHeaderHeight = total + 1 + headers := suite.GenDummyHeaders(total) err = remoteStore.Append(ctx, headers...) require.NoError(t, err) @@ -348,8 +359,7 @@ func TestSyncer_verifySkippingCannotVerify(t *testing.T) { require.NoError(t, err) err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) - var verErr *NewValidatorSetCantBeTrustedError - assert.ErrorIs(t, err, verErr, "%T", err) + assert.Error(t, err) } type wrappedGetter struct { From 475bd5536f12f557d91f893a4040332267336bc5 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Wed, 13 Nov 2024 11:15:52 +0100 Subject: [PATCH 09/20] fix panic msg --- sync/sync_head.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index 41a661d3..b2fc399f 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -207,8 +207,7 @@ func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader diff := networkHeader.Height() - subjHeight if diff <= 0 { - panic(fmt.Sprintf("implementation bug: diff %d, subjective height %d (%X), network height %d (%X)", - diff, + panic(fmt.Sprintf("implementation bug:\n subjective head height %d, hash %X,\n network head height %d, hash %X", subjHeight, subjHead.Hash(), networkHeader.Height(), networkHeader.Hash(), )) From a24a1ea3ea5f260469d79a9398987f97b954d34a Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Fri, 22 Nov 2024 12:39:42 +0100 Subject: [PATCH 10/20] remove a test --- sync/sync_head_test.go | 44 ------------------------------------------ 1 file changed, 44 deletions(-) diff --git a/sync/sync_head_test.go b/sync/sync_head_test.go index 0657b1f4..65eeb69a 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -94,50 +94,6 @@ func TestSyncer_HeadWithTrustedHead(t *testing.T) { require.True(t, wrappedGetter.withTrustedHead) } -func TestSyncer_HeadWithNotEnoughValidators(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) - t.Cleanup(cancel) - - suite := headertest.NewTestSuite(t) - head := suite.Head() - - localStore := newTestStore(t, ctx, head) - remoteStore := newTestStore(t, ctx, head) - - err := remoteStore.Append(ctx, suite.GenDummyHeaders(100)...) - require.NoError(t, err) - - // create a wrappedGetter to track exchange interactions - wrappedGetter := newWrappedGetter(local.NewExchange(remoteStore)) - - syncer, err := NewSyncer( - wrappedGetter, - localStore, - headertest.NewDummySubscriber(), - WithBlockTime(time.Nanosecond), - WithRecencyThreshold(time.Nanosecond), // forces a request for a new sync target - // ensures that syncer's store contains a subjective head that is within - // the unbonding period so that the syncer can use a header from the network - // as a sync target - WithTrustingPeriod(time.Hour), - ) - require.NoError(t, err) - - // start the syncer which triggers a Head request that will - // load the syncer's subjective head from the store, and request - // a new sync target from the network rather than from trusted peers - err = syncer.Start(ctx) - require.NoError(t, err) - t.Cleanup(func() { - err = syncer.Stop(ctx) - require.NoError(t, err) - }) - - // ensure the syncer really requested Head from the network - // rather than from trusted peers - require.True(t, wrappedGetter.withTrustedHead) -} - // Test will simulate a case with upto `iters` failures before we will get to // the header that can be verified against subjectiveHead. func TestSyncer_verifySkippingSuccess(t *testing.T) { From 2a91b0c5496fb2eb8e13bd16ef84afccbc525905 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Wed, 4 Dec 2024 12:16:10 +0100 Subject: [PATCH 11/20] add metric attributes --- sync/metrics.go | 9 +++++++-- sync/sync_head.go | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/sync/metrics.go b/sync/metrics.go index 2269a539..f60103b1 100644 --- a/sync/metrics.go +++ b/sync/metrics.go @@ -198,9 +198,14 @@ func (m *metrics) newSubjectiveHead(ctx context.Context, height uint64, timestam }) } -func (m *metrics) failedValidationAgainstSubjHead(ctx context.Context) { +func (m *metrics) failedValidationAgainstSubjHead(ctx context.Context, height int64, hash string) { m.observe(ctx, func(ctx context.Context) { - m.failedAgainstSubjHead.Add(ctx, 1) + m.failedAgainstSubjHead.Add(ctx, 1, + metric.WithAttributes( + attribute.Int64("height", height), + attribute.String("hash", hash), + ), + ) }) } diff --git a/sync/sync_head.go b/sync/sync_head.go index b2fc399f..50df75d6 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -241,7 +241,7 @@ func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader diff = networkHeader.Height() - subjHeight } - s.metrics.failedValidationAgainstSubjHead(ctx) + s.metrics.failedValidationAgainstSubjHead(ctx, int64(networkHeader.Height()), networkHeader.Hash().String()) log.Warnw("sync: header validation against subjHead", "height", networkHeader.Height(), "hash", networkHeader.Hash().String()) return fmt.Errorf("sync: header validation against subjHead height:%d hash:%x", networkHeader.Height(), networkHeader.Hash().String()) From afefc7fa1cf729a8162f77bb938a372c8257f68a Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Wed, 4 Dec 2024 14:38:50 +0100 Subject: [PATCH 12/20] review suggestions --- sync/metrics.go | 14 +++++++------- sync/sync_head.go | 36 +++++++++++++++--------------------- sync/sync_head_test.go | 12 ++++++------ 3 files changed, 28 insertions(+), 34 deletions(-) diff --git a/sync/metrics.go b/sync/metrics.go index f60103b1..ceb43766 100644 --- a/sync/metrics.go +++ b/sync/metrics.go @@ -22,7 +22,7 @@ type metrics struct { trustedPeersOutOfSync metric.Int64Counter outdatedHeader metric.Int64Counter subjectiveInit metric.Int64Counter - failedAgainstSubjHead metric.Int64Counter + failedBifurcations metric.Int64Counter subjectiveHead atomic.Int64 @@ -72,10 +72,10 @@ func newMetrics() (*metrics, error) { return nil, err } - failedAgainstSubjHead, err := meter.Int64Counter( - "hdr_sync_subj_validation_failed", + failedBifurcations, err := meter.Int64Counter( + "hdr_failed_bifurcations_total", metric.WithDescription( - "tracks how many times validation against subjective head failed", + "tracks how many times bifurcation failed against subjective head", ), ) if err != nil { @@ -123,7 +123,7 @@ func newMetrics() (*metrics, error) { trustedPeersOutOfSync: trustedPeersOutOfSync, outdatedHeader: outdatedHeader, subjectiveInit: subjectiveInit, - failedAgainstSubjHead: failedAgainstSubjHead, + failedBifurcations: failedBifurcations, syncLoopDurationHist: syncLoopDurationHist, syncLoopRunningInst: syncLoopRunningInst, requestRangeTimeHist: requestRangeTimeHist, @@ -198,9 +198,9 @@ func (m *metrics) newSubjectiveHead(ctx context.Context, height uint64, timestam }) } -func (m *metrics) failedValidationAgainstSubjHead(ctx context.Context, height int64, hash string) { +func (m *metrics) failedBifurcation(ctx context.Context, height int64, hash string) { m.observe(ctx, func(ctx context.Context) { - m.failedAgainstSubjHead.Add(ctx, 1, + m.failedBifurcations.Add(ctx, 1, metric.WithAttributes( attribute.Int64("height", height), attribute.String("hash", hash), diff --git a/sync/sync_head.go b/sync/sync_head.go index 50df75d6..791bc799 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -176,7 +176,7 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { var verErr *header.VerifyError if errors.As(err, &verErr) { if verErr.SoftFailure { - err := s.verifySkipping(ctx, sbjHead, newHead) + err := s.verifyBifurcating(ctx, sbjHead, newHead) return err != nil, err } @@ -195,23 +195,17 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { return false, err } -// verifySkipping performs a bifurcated header verification process such that -// it tries to find a header (or several headers if necessary) -// between the networkHead and the subjectiveHead such that non-adjacent -// (or in the worst case adjacent) verification passes and the networkHead -// can be verified as a valid sync target against the syncer's subjectiveHead. -// -// When such candidates cannot be found [NewValidatorSetCantBeTrustedError] will be returned. -func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader H) error { +// verifyBifurcating verifies networkHead against subjHead via the interim headers when direct +// verification is impossible. +// It tries to find a header (or several headers if necessary) between the networkHead and +// the subjectiveHead such that non-adjacent (or in the worst case adjacent) verification +// passes and the networkHead can be verified as a valid sync target against the syncer's +// subjectiveHead. +// A non-nil error is returned when networkHead can't be verified. +func (s *Syncer[H]) verifyBifurcating(ctx context.Context, subjHead, networkHead H) error { subjHeight := subjHead.Height() - diff := networkHeader.Height() - subjHeight - if diff <= 0 { - panic(fmt.Sprintf("implementation bug:\n subjective head height %d, hash %X,\n network head height %d, hash %X", - subjHeight, subjHead.Hash(), - networkHeader.Height(), networkHeader.Hash(), - )) - } + diff := networkHead.Height() - subjHeight for diff > 1 { candidateHeight := subjHeight + diff/2 @@ -231,20 +225,20 @@ func (s *Syncer[H]) verifySkipping(ctx context.Context, subjHead, networkHeader subjHead = candidateHeader s.setSubjectiveHead(ctx, subjHead) - if err := header.Verify(subjHead, networkHeader); err == nil { + if err := header.Verify(subjHead, networkHead); err == nil { // network head validate properly, return success. return nil } // new subjHead failed, go deeper in 2nd half. subjHeight = subjHead.Height() - diff = networkHeader.Height() - subjHeight + diff = networkHead.Height() - subjHeight } - s.metrics.failedValidationAgainstSubjHead(ctx, int64(networkHeader.Height()), networkHeader.Hash().String()) - log.Warnw("sync: header validation against subjHead", "height", networkHeader.Height(), "hash", networkHeader.Hash().String()) + s.metrics.failedBifurcation(ctx, int64(networkHead.Height()), networkHead.Hash().String()) + log.Warnw("sync: header validation against subjHead", "height", networkHead.Height(), "hash", networkHead.Hash().String()) - return fmt.Errorf("sync: header validation against subjHead height:%d hash:%x", networkHeader.Height(), networkHeader.Hash().String()) + return fmt.Errorf("sync: header validation against subjHead height:%d hash:%x", networkHead.Height(), networkHead.Hash().String()) } // isExpired checks if header is expired against trusting period. diff --git a/sync/sync_head_test.go b/sync/sync_head_test.go index 65eeb69a..5ed8db98 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -96,7 +96,7 @@ func TestSyncer_HeadWithTrustedHead(t *testing.T) { // Test will simulate a case with upto `iters` failures before we will get to // the header that can be verified against subjectiveHead. -func TestSyncer_verifySkippingSuccess(t *testing.T) { +func TestSyncer_verifyBifurcatingSuccess(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) @@ -169,13 +169,13 @@ func TestSyncer_verifySkippingSuccess(t *testing.T) { subjHead, err := syncer.subjectiveHead(ctx) require.NoError(t, err) - err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) + err = syncer.verifyBifurcating(ctx, subjHead, headers[total-1]) require.NoError(t, err) } // Test will simulate a case with upto `iters` failures before we will get to // the header that can be verified against subjectiveHead. -func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) { +func TestSyncer_verifyBifurcatingSuccessWithBadCandidates(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) @@ -246,13 +246,13 @@ func TestSyncer_verifySkippingSuccessWithBadCandidates(t *testing.T) { subjHead, err := syncer.subjectiveHead(ctx) require.NoError(t, err) - err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) + err = syncer.verifyBifurcating(ctx, subjHead, headers[total-1]) require.NoError(t, err) } // Test will simulate a case when no headers can be verified against subjectiveHead. // As a result the [NewValidatorSetCantBeTrustedError] error will be returned. -func TestSyncer_verifySkippingCannotVerify(t *testing.T) { +func TestSyncer_verifyBifurcatingCannotVerify(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) @@ -314,7 +314,7 @@ func TestSyncer_verifySkippingCannotVerify(t *testing.T) { subjHead, err := syncer.subjectiveHead(ctx) require.NoError(t, err) - err = syncer.verifySkipping(ctx, subjHead, headers[total-1]) + err = syncer.verifyBifurcating(ctx, subjHead, headers[total-1]) assert.Error(t, err) } From fb0b4c26ff7fb3ca168a4f469cb92a192fe482c1 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Thu, 5 Dec 2024 10:56:13 +0100 Subject: [PATCH 13/20] review suggestion --- sync/sync_head.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index 791bc799..c50cc6c4 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -160,7 +160,7 @@ func (s *Syncer[H]) incomingNetworkHead(ctx context.Context, head H) error { } // verify verifies given network head candidate. -func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { +func (s *Syncer[H]) verify(ctx context.Context, newHead H) (isSoft bool, _ error) { sbjHead, err := s.subjectiveHead(ctx) if err != nil { log.Errorw("getting subjective head during validation", "err", err) @@ -203,6 +203,8 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { // subjectiveHead. // A non-nil error is returned when networkHead can't be verified. func (s *Syncer[H]) verifyBifurcating(ctx context.Context, subjHead, networkHead H) error { + log.Warnw("sync: header bifurcation started", "height", networkHead.Height(), "hash", networkHead.Hash().String()) + subjHeight := subjHead.Height() diff := networkHead.Height() - subjHeight @@ -238,7 +240,10 @@ func (s *Syncer[H]) verifyBifurcating(ctx context.Context, subjHead, networkHead s.metrics.failedBifurcation(ctx, int64(networkHead.Height()), networkHead.Hash().String()) log.Warnw("sync: header validation against subjHead", "height", networkHead.Height(), "hash", networkHead.Hash().String()) - return fmt.Errorf("sync: header validation against subjHead height:%d hash:%x", networkHead.Height(), networkHead.Hash().String()) + return &header.VerifyError{ + Reason: fmt.Errorf("sync: header validation against subjHead height:%d hash:%x", networkHead.Height(), networkHead.Hash().String()), + SoftFailure: false, + } } // isExpired checks if header is expired against trusting period. From 16bff7dd294dd01474768fe7b1bef8c449e17168 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Thu, 5 Dec 2024 17:01:13 +0100 Subject: [PATCH 14/20] more review suggestions --- sync/sync_head.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index c50cc6c4..e56a9c54 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -203,7 +203,7 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (isSoft bool, _ error // subjectiveHead. // A non-nil error is returned when networkHead can't be verified. func (s *Syncer[H]) verifyBifurcating(ctx context.Context, subjHead, networkHead H) error { - log.Warnw("sync: header bifurcation started", "height", networkHead.Height(), "hash", networkHead.Hash().String()) + log.Warnw("header bifurcation started", "height", networkHead.Height(), "hash", networkHead.Hash().String()) subjHeight := subjHead.Height() @@ -218,6 +218,11 @@ func (s *Syncer[H]) verifyBifurcating(ctx context.Context, subjHead, networkHead } if err := header.Verify(subjHead, candidateHeader); err != nil { + var verErr *header.VerifyError + if errors.As(err, &verErr) && !verErr.SoftFailure { + return err + } + // candidate failed, go deeper in 1st half. diff = diff / 2 continue @@ -238,7 +243,7 @@ func (s *Syncer[H]) verifyBifurcating(ctx context.Context, subjHead, networkHead } s.metrics.failedBifurcation(ctx, int64(networkHead.Height()), networkHead.Hash().String()) - log.Warnw("sync: header validation against subjHead", "height", networkHead.Height(), "hash", networkHead.Hash().String()) + log.Warnw("header validation against subjHead", "height", networkHead.Height(), "hash", networkHead.Hash().String()) return &header.VerifyError{ Reason: fmt.Errorf("sync: header validation against subjHead height:%d hash:%x", networkHead.Height(), networkHead.Hash().String()), From 5fdacb9d0a0d60dd6774efaa11ab976f25d3ff94 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Thu, 5 Dec 2024 17:02:07 +0100 Subject: [PATCH 15/20] upd log --- sync/sync_head.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index e56a9c54..99de4cf0 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -243,7 +243,7 @@ func (s *Syncer[H]) verifyBifurcating(ctx context.Context, subjHead, networkHead } s.metrics.failedBifurcation(ctx, int64(networkHead.Height()), networkHead.Hash().String()) - log.Warnw("header validation against subjHead", "height", networkHead.Height(), "hash", networkHead.Hash().String()) + log.Warnw("header bifurcation failed", "height", networkHead.Height(), "hash", networkHead.Hash().String()) return &header.VerifyError{ Reason: fmt.Errorf("sync: header validation against subjHead height:%d hash:%x", networkHead.Height(), networkHead.Hash().String()), From 5cc3dbf1f301986de8242511fa7ff563e33296ae Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Mon, 6 Jan 2025 14:56:48 +0100 Subject: [PATCH 16/20] upd error and logging after a review --- sync/sync_head.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index 99de4cf0..f2308adb 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -243,10 +243,10 @@ func (s *Syncer[H]) verifyBifurcating(ctx context.Context, subjHead, networkHead } s.metrics.failedBifurcation(ctx, int64(networkHead.Height()), networkHead.Hash().String()) - log.Warnw("header bifurcation failed", "height", networkHead.Height(), "hash", networkHead.Hash().String()) + log.Errorw("header bifurcation failed", "height", networkHead.Height(), "hash", networkHead.Hash().String()) return &header.VerifyError{ - Reason: fmt.Errorf("sync: header validation against subjHead height:%d hash:%x", networkHead.Height(), networkHead.Hash().String()), + Reason: fmt.Errorf("sync: header validation against subjHead height:%d hash:%s", networkHead.Height(), networkHead.Hash().String()), SoftFailure: false, } } From f0ee886fde637e4e55e78b89c4c5ab169b8e285c Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Mon, 6 Jan 2025 15:41:09 +0100 Subject: [PATCH 17/20] update doc-comment --- sync/sync_head.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index f2308adb..53bfbf70 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -160,7 +160,8 @@ func (s *Syncer[H]) incomingNetworkHead(ctx context.Context, head H) error { } // verify verifies given network head candidate. -func (s *Syncer[H]) verify(ctx context.Context, newHead H) (isSoft bool, _ error) { +// bool reports whether the returned error is a soft error. +func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { sbjHead, err := s.subjectiveHead(ctx) if err != nil { log.Errorw("getting subjective head during validation", "err", err) From 64e7be555c12e10695c4cdb556b09f14670c8245 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Tue, 21 Jan 2025 16:24:56 +0100 Subject: [PATCH 18/20] fix after merge --- sync/sync_head_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sync/sync_head_test.go b/sync/sync_head_test.go index 5ed8db98..f27b50a4 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -358,6 +358,5 @@ func (t *wrappedGetter) GetRangeByHeight( from *headertest.DummyHeader, to uint64, ) ([]*headertest.DummyHeader, error) { - // TODO implement me - panic("implement me") + return t.ex.GetRangeByHeight(ctx, from, to) } From 671a7dfb1be489521ed26e8a23bf84ede333bf63 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Wed, 22 Jan 2025 14:49:17 +0100 Subject: [PATCH 19/20] do not set subjHead in bifurcation due to adj check --- sync/sync_head.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sync/sync_head.go b/sync/sync_head.go index 53bfbf70..c15405e2 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -192,8 +192,7 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { "hash_of_subjective", sbjHead.Hash(), "reason", verErr.Reason) } - - return false, err + return verErr.SoftFailure, err } // verifyBifurcating verifies networkHead against subjHead via the interim headers when direct @@ -231,7 +230,6 @@ func (s *Syncer[H]) verifyBifurcating(ctx context.Context, subjHead, networkHead // candidate was validated properly, update subjHead. subjHead = candidateHeader - s.setSubjectiveHead(ctx, subjHead) if err := header.Verify(subjHead, networkHead); err == nil { // network head validate properly, return success. From eb13b856d1f317d8ea5f89f6d4aa75ffae0f39a3 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Wed, 22 Jan 2025 15:19:12 +0100 Subject: [PATCH 20/20] fix nl --- sync/sync_head.go | 1 + 1 file changed, 1 insertion(+) diff --git a/sync/sync_head.go b/sync/sync_head.go index c15405e2..46f0d5cc 100644 --- a/sync/sync_head.go +++ b/sync/sync_head.go @@ -192,6 +192,7 @@ func (s *Syncer[H]) verify(ctx context.Context, newHead H) (bool, error) { "hash_of_subjective", sbjHead.Hash(), "reason", verErr.Reason) } + return verErr.SoftFailure, err }