From 9b2f384fc180be4ee164ec56a1ca9fdacc06cdef Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Wed, 20 Sep 2023 14:06:01 +0200 Subject: [PATCH 1/2] feat(p2p): Head requests respects SoftFailure during Verify check --- headertest/dummy_header.go | 10 +++++---- p2p/exchange.go | 13 +++++++++-- p2p/exchange_test.go | 44 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 6 deletions(-) diff --git a/headertest/dummy_header.go b/headertest/dummy_header.go index 64d21f22..e8480dde 100644 --- a/headertest/dummy_header.go +++ b/headertest/dummy_header.go @@ -27,6 +27,9 @@ type DummyHeader struct { // VerifyFailure allows for testing scenarios where a header would fail // verification. When set to true, it forces a failure. VerifyFailure bool + // SoftFailure allows for testing scenarios where a header would fail + // verification with SoftFailure set to true + SoftFailure bool } func RandDummyHeader(t *testing.T) *DummyHeader { @@ -96,11 +99,10 @@ func (d *DummyHeader) IsExpired(period time.Duration) bool { return expirationTime.Before(time.Now()) } -func (d *DummyHeader) Verify(header *DummyHeader) error { - if header.VerifyFailure { - return ErrDummyVerify +func (d *DummyHeader) Verify(hdr *DummyHeader) error { + if hdr.VerifyFailure { + return &header.VerifyError{Reason: ErrDummyVerify, SoftFailure: hdr.SoftFailure} } - return nil } diff --git a/p2p/exchange.go b/p2p/exchange.go index c4dd8289..af0bf100 100644 --- a/p2p/exchange.go +++ b/p2p/exchange.go @@ -3,6 +3,7 @@ package p2p import ( "bytes" "context" + "errors" "fmt" "math/rand" "sort" @@ -155,12 +156,20 @@ func (ex *Exchange[H]) Head(ctx context.Context, opts ...header.HeadOption[H]) ( if useTrackedPeers { err = reqParams.TrustedHead.Verify(headers[0]) if err != nil { - log.Errorw("verifying head received from tracked peer", "tracked peer", from, - "height", headers[0].Height(), "err", err) + var verErr *header.VerifyError + if errors.As(err, &verErr) && verErr.SoftFailure { + log.Debugw("received head from tracked peer that soft-failed verification", + "tracked peer", from, "err", err) + headerRespCh <- headers[0] + return + } // bad head was given, block peer ex.peerTracker.blockPeer(from, fmt.Errorf("returned bad head: %w", err)) + log.Errorw("verifying head received from tracked peer", "tracked peer", from, + "height", headers[0].Height(), "err", err) headerRespCh <- zero return + } } // request ensures that the result slice will have at least one Header diff --git a/p2p/exchange_test.go b/p2p/exchange_test.go index 6b81f529..35d70d72 100644 --- a/p2p/exchange_test.go +++ b/p2p/exchange_test.go @@ -88,6 +88,50 @@ func TestExchange_RequestHead(t *testing.T) { } } +// TestExchange_RequestHead_SoftFailure tests that the exchange still processes +// a Head response that has a SoftFailure. +func TestExchange_RequestHead_SoftFailure(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + hosts := createMocknet(t, 3) + exchg, _ := createP2PExAndServer(t, hosts[0], hosts[1]) + + // create a tracked peer + suite := headertest.NewTestSuite(t) + trackedStore := headertest.NewStore[*headertest.DummyHeader](t, suite, 50) + // create a header that will SoftFail verification and append it to tracked + // peer's store + hdr := suite.GenDummyHeaders(1)[0] + hdr.VerifyFailure = true + hdr.SoftFailure = true + err := trackedStore.Append(ctx, hdr) + require.NoError(t, err) + // start the tracked peer's server + serverSideEx, err := NewExchangeServer[*headertest.DummyHeader](hosts[2], trackedStore, + WithNetworkID[ServerParameters](networkID), + ) + require.NoError(t, err) + err = serverSideEx.Start(ctx) + require.NoError(t, err) + t.Cleanup(func() { + err = serverSideEx.Stop(ctx) + require.NoError(t, err) + }) + + // get first subjective head from trusted peer to initialize the + // exchange's store + var head header.Header[*headertest.DummyHeader] + head, err = exchg.Head(ctx) + require.NoError(t, err) + + // now use that trusted head to request a new head from the exchange + // from the tracked peer + softFailHead, err := exchg.Head(ctx, header.WithTrustedHead(head)) + require.NoError(t, err) + assert.Equal(t, trackedStore.HeadHeight, softFailHead.Height()) +} + func TestExchange_RequestHead_UnresponsivePeer(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) t.Cleanup(cancel) From 19ecd5b5149129eb542be65b205f9a11e5f58237 Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Fri, 22 Sep 2023 16:07:04 +0200 Subject: [PATCH 2/2] fix(p2p): use header.Verify instead of trustedHeader.Verify(h) --- opts.go | 4 ++-- p2p/exchange.go | 4 ++-- p2p/exchange_test.go | 9 ++++----- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/opts.go b/opts.go index ce3b7cc4..10a02955 100644 --- a/opts.go +++ b/opts.go @@ -6,11 +6,11 @@ type HeadOption[H Header[H]] func(opts *HeadParams[H]) type HeadParams[H Header[H]] struct { // TrustedHead allows the caller of Head to specify a trusted header // against which the underlying implementation of Head can verify against. - TrustedHead Header[H] + TrustedHead H } // WithTrustedHead sets the TrustedHead parameter to the given header. -func WithTrustedHead[H Header[H]](verified Header[H]) func(opts *HeadParams[H]) { +func WithTrustedHead[H Header[H]](verified H) func(opts *HeadParams[H]) { return func(opts *HeadParams[H]) { opts.TrustedHead = verified } diff --git a/p2p/exchange.go b/p2p/exchange.go index af0bf100..0813e5b3 100644 --- a/p2p/exchange.go +++ b/p2p/exchange.go @@ -127,7 +127,7 @@ func (ex *Exchange[H]) Head(ctx context.Context, opts ...header.HeadOption[H]) ( // trusted peers for its Head request. If nil, trusted peers will // be used. If non-nil, Exchange will ask several peers from its network for // their Head and verify against the given trusted header. - useTrackedPeers := reqParams.TrustedHead != nil + useTrackedPeers := !reqParams.TrustedHead.IsZero() if useTrackedPeers { trackedPeers := ex.peerTracker.getPeers(maxUntrustedHeadRequests) if len(trackedPeers) > 0 { @@ -154,7 +154,7 @@ func (ex *Exchange[H]) Head(ctx context.Context, opts ...header.HeadOption[H]) ( } // if tracked (untrusted) peers were requested, verify head if useTrackedPeers { - err = reqParams.TrustedHead.Verify(headers[0]) + err = header.Verify[H](reqParams.TrustedHead, headers[0], header.DefaultHeightThreshold) if err != nil { var verErr *header.VerifyError if errors.As(err, &verErr) && verErr.SoftFailure { diff --git a/p2p/exchange_test.go b/p2p/exchange_test.go index 35d70d72..62311d47 100644 --- a/p2p/exchange_test.go +++ b/p2p/exchange_test.go @@ -52,7 +52,7 @@ func TestExchange_RequestHead(t *testing.T) { tests := []struct { requestFromTrusted bool - lastHeader header.Header[*headertest.DummyHeader] + lastHeader *headertest.DummyHeader expectedHeight uint64 expectedHash header.Hash }{ @@ -76,7 +76,7 @@ func TestExchange_RequestHead(t *testing.T) { t.Run(strconv.Itoa(i), func(t *testing.T) { var opts []header.HeadOption[*headertest.DummyHeader] if !tt.requestFromTrusted { - opts = append(opts, header.WithTrustedHead(tt.lastHeader)) + opts = append(opts, header.WithTrustedHead[*headertest.DummyHeader](tt.lastHeader)) } header, err := exchg.Head(ctx, opts...) @@ -121,13 +121,12 @@ func TestExchange_RequestHead_SoftFailure(t *testing.T) { // get first subjective head from trusted peer to initialize the // exchange's store - var head header.Header[*headertest.DummyHeader] - head, err = exchg.Head(ctx) + head, err := exchg.Head(ctx) require.NoError(t, err) // now use that trusted head to request a new head from the exchange // from the tracked peer - softFailHead, err := exchg.Head(ctx, header.WithTrustedHead(head)) + softFailHead, err := exchg.Head(ctx, header.WithTrustedHead[*headertest.DummyHeader](head)) require.NoError(t, err) assert.Equal(t, trackedStore.HeadHeight, softFailHead.Height()) }