From 821c9b4525eaf1cd01e2e50380148ca04acce548 Mon Sep 17 00:00:00 2001 From: Oleg Kovalov Date: Mon, 11 Nov 2024 11:01:36 +0100 Subject: [PATCH] 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 9d9d6965..4179f6a0 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -139,13 +139,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() @@ -178,12 +177,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 { @@ -213,13 +218,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() @@ -252,12 +256,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 { @@ -266,13 +275,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 } } @@ -286,13 +295,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() @@ -325,6 +333,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) @@ -349,8 +360,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 {