From 7c332a4c9bc9058301b6dbf2eb6dddfde4ae8a12 Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Mon, 9 Oct 2023 11:59:26 +0200 Subject: [PATCH 1/6] refactor!: Merge GetVerifiedRange into GetRangeByHeight and remove redundant endpoint --- headertest/store.go | 24 +++++++++------- interface.go | 10 +++---- local/exchange.go | 11 ++------ p2p/exchange.go | 26 ++++------------- p2p/exchange_test.go | 60 +++++++++++++++++++++++++--------------- p2p/server.go | 4 +-- p2p/server_test.go | 21 ++++++++++++++ p2p/session.go | 2 +- store/store.go | 46 ++++++++++++++++-------------- store/store_test.go | 14 +++++----- sync/sync.go | 9 +++--- sync/sync_getter_test.go | 6 +--- sync/sync_head_test.go | 11 ++++---- sync/sync_test.go | 4 +-- 14 files changed, 132 insertions(+), 116 deletions(-) diff --git a/headertest/store.go b/headertest/store.go index 414ce154..7a663360 100644 --- a/headertest/store.go +++ b/headertest/store.go @@ -64,8 +64,20 @@ func (m *Store[H]) GetByHeight(ctx context.Context, height uint64) (H, error) { return m.Headers[height], nil } -func (m *Store[H]) GetRangeByHeight(ctx context.Context, from, to uint64) ([]H, error) { - headers := make([]H, to-from) +func (m *Store[H]) GetRange(ctx context.Context, from, to uint64) ([]H, error) { + return m.getRangeByHeight(ctx, from, to) +} + +// GetRangeByHeight returns headers in range [from; to). +func (m *Store[H]) GetRangeByHeight(ctx context.Context, fromHead H, to uint64) ([]H, error) { + from := fromHead.Height() + 1 + return m.getRangeByHeight(ctx, from, to) +} + +func (m *Store[H]) getRangeByHeight(ctx context.Context, from, to uint64) ([]H, error) { + amount := to - from + headers := make([]H, amount) + // As the requested range is [from; to), // check that (to-1) height in request is less than // the biggest header height in store. @@ -79,14 +91,6 @@ func (m *Store[H]) GetRangeByHeight(ctx context.Context, from, to uint64) ([]H, return headers, nil } -func (m *Store[H]) GetVerifiedRange( - ctx context.Context, - h H, - to uint64, -) ([]H, error) { - return m.GetRangeByHeight(ctx, uint64(h.Height())+1, to) -} - func (m *Store[H]) Has(context.Context, header.Hash) (bool, error) { return false, nil } diff --git a/interface.go b/interface.go index 15dc8fa2..730674e8 100644 --- a/interface.go +++ b/interface.go @@ -93,6 +93,9 @@ type Store[H Header[H]] interface { // It returns the amount of successfully applied headers, // so caller can understand what given header was invalid, if any. Append(context.Context, ...H) error + + // GetRange returns the range [from:to). + GetRange(context.Context, uint64, uint64) ([]H, error) } // Getter contains the behavior necessary for a component to retrieve @@ -106,12 +109,9 @@ type Getter[H Header[H]] interface { // GetByHeight returns the Header corresponding to the given block height. GetByHeight(context.Context, uint64) (H, error) - // GetRangeByHeight returns the given range of Headers. - GetRangeByHeight(ctx context.Context, from, amount uint64) ([]H, error) - - // GetVerifiedRange requests the header range from the provided Header and + // GetRangeByHeight requests the header range from the provided Header and // verifies that the returned headers are adjacent to each other. - GetVerifiedRange(ctx context.Context, from H, amount uint64) ([]H, error) + GetRangeByHeight(ctx context.Context, from H, to uint64) ([]H, error) } // Head contains the behavior necessary for a component to retrieve diff --git a/local/exchange.go b/local/exchange.go index c3d58488..fdc6081a 100644 --- a/local/exchange.go +++ b/local/exchange.go @@ -34,16 +34,9 @@ func (l *Exchange[H]) GetByHeight(ctx context.Context, height uint64) (H, error) return l.store.GetByHeight(ctx, height) } -func (l *Exchange[H]) GetRangeByHeight(ctx context.Context, origin, amount uint64) ([]H, error) { - if amount == 0 { - return nil, nil - } - return l.store.GetRangeByHeight(ctx, origin, origin+amount) -} - -func (l *Exchange[H]) GetVerifiedRange(ctx context.Context, from H, amount uint64, +func (l *Exchange[H]) GetRangeByHeight(ctx context.Context, from H, to uint64, ) ([]H, error) { - return l.store.GetVerifiedRange(ctx, from, uint64(from.Height())+amount+1) + return l.store.GetRangeByHeight(ctx, from, to) } func (l *Exchange[H]) Get(ctx context.Context, hash header.Hash) (H, error) { diff --git a/p2p/exchange.go b/p2p/exchange.go index 7198c316..3164556d 100644 --- a/p2p/exchange.go +++ b/p2p/exchange.go @@ -213,36 +213,20 @@ func (ex *Exchange[H]) GetByHeight(ctx context.Context, height uint64) (H, error return headers[0], nil } -// GetRangeByHeight performs a request for the given range of Headers -// to the network. Note that the Headers must be verified thereafter. -func (ex *Exchange[H]) GetRangeByHeight(ctx context.Context, from, amount uint64) ([]H, error) { - if amount == 0 { - return make([]H, 0), nil - } - if amount > header.MaxRangeRequestSize { - return nil, header.ErrHeadersLimitExceeded - } - session := newSession[H](ex.ctx, ex.host, ex.peerTracker, ex.protocolID, ex.Params.RangeRequestTimeout) - defer session.close() - return session.getRangeByHeight(ctx, from, amount, ex.Params.MaxHeadersPerRangeRequest) -} - -// GetVerifiedRange performs a request for the given range of Headers to the network and +// GetRangeByHeight performs a request for the given range of Headers to the network and // ensures that returned headers are correct against the passed one. -func (ex *Exchange[H]) GetVerifiedRange( +func (ex *Exchange[H]) GetRangeByHeight( ctx context.Context, from H, - amount uint64, + to uint64, ) ([]H, error) { - if amount == 0 { - return make([]H, 0), nil - } session := newSession[H]( ex.ctx, ex.host, ex.peerTracker, ex.protocolID, ex.Params.RangeRequestTimeout, withValidation(from), ) defer session.close() // we request the next header height that we don't have: `fromHead`+1 - return session.getRangeByHeight(ctx, uint64(from.Height())+1, amount, ex.Params.MaxHeadersPerRangeRequest) + amount := to - (from.Height() + 1) + return session.getRangeByHeight(ctx, from.Height()+1, amount, ex.Params.MaxHeadersPerRangeRequest) } // Get performs a request for the Header by the given hash corresponding diff --git a/p2p/exchange_test.go b/p2p/exchange_test.go index 62311d47..b15aa542 100644 --- a/p2p/exchange_test.go +++ b/p2p/exchange_test.go @@ -167,10 +167,17 @@ func TestExchange_RequestHeader(t *testing.T) { } func TestExchange_RequestHeaders(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + hosts := createMocknet(t, 2) exchg, store := createP2PExAndServer(t, hosts[0], hosts[1]) + + gen, err := store.GetByHeight(ctx, 1) + require.NoError(t, err) + // perform expected request - gotHeaders, err := exchg.GetRangeByHeight(context.Background(), 1, 5) + gotHeaders, err := exchg.GetRangeByHeight(ctx, gen, 5) require.NoError(t, err) for _, got := range gotHeaders { assert.Equal(t, store.Headers[got.Height()].Height(), got.Height()) @@ -183,19 +190,19 @@ func TestExchange_RequestVerifiedHeaders(t *testing.T) { exchg, store := createP2PExAndServer(t, hosts[0], hosts[1]) // perform expected request h := store.Headers[1] - _, err := exchg.GetVerifiedRange(context.Background(), h, 3) + _, err := exchg.GetRangeByHeight(context.Background(), h, 3) require.NoError(t, err) } func TestExchange_RequestVerifiedHeadersFails(t *testing.T) { hosts := createMocknet(t, 2) exchg, store := createP2PExAndServer(t, hosts[0], hosts[1]) - store.Headers[2] = store.Headers[3] + store.Headers[3].VerifyFailure = true // force a verification failure on the 3rd header // perform expected request h := store.Headers[1] ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*500) t.Cleanup(cancel) - _, err := exchg.GetVerifiedRange(ctx, h, 3) + _, err := exchg.GetRangeByHeight(ctx, h, 4) // requests range (1:4) assert.Error(t, err) // ensure that peer was added to the blacklist @@ -209,7 +216,7 @@ func TestExchange_RequestVerifiedHeadersFails(t *testing.T) { func TestExchange_RequestFullRangeHeaders(t *testing.T) { // create mocknet with 5 peers hosts := createMocknet(t, 5) - store := headertest.NewStore[*headertest.DummyHeader](t, headertest.NewTestSuite(t), int(header.MaxRangeRequestSize)) + store := headertest.NewStore[*headertest.DummyHeader](t, headertest.NewTestSuite(t), int(header.MaxRangeRequestSize)+1) connGater, err := conngater.NewBasicConnectionGater(sync.MutexWrap(datastore.NewMapDatastore())) require.NoError(t, err) @@ -238,20 +245,15 @@ func TestExchange_RequestFullRangeHeaders(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) t.Cleanup(cancel) - // request headers from 1 to totalAmount(80) - headers, err := exchange.GetRangeByHeight(ctx, 1, header.MaxRangeRequestSize) + + gen, err := store.GetByHeight(ctx, 1) require.NoError(t, err) - require.Len(t, headers, int(header.MaxRangeRequestSize)) -} -// TestExchange_RequestHeadersLimitExceeded tests that the Exchange instance will return -// header.ErrHeadersLimitExceeded if the requested range will be move than MaxRangeRequestSize. -func TestExchange_RequestHeadersLimitExceeded(t *testing.T) { - hosts := createMocknet(t, 2) - exchg, _ := createP2PExAndServer(t, hosts[0], hosts[1]) - _, err := exchg.GetRangeByHeight(context.Background(), 1, 600) - require.Error(t, err) - require.ErrorAs(t, err, &header.ErrHeadersLimitExceeded) + // request the max amount of headers + to := gen.Height() + header.MaxRangeRequestSize + 1 // add 1 to account for `from` being inclusive + headers, err := exchange.GetRangeByHeight(ctx, gen, to) + require.NoError(t, err) + assert.Len(t, headers, int(header.MaxRangeRequestSize)) } // TestExchange_RequestHeadersFromAnotherPeer tests that the Exchange instance will request range @@ -259,7 +261,7 @@ func TestExchange_RequestHeadersLimitExceeded(t *testing.T) { func TestExchange_RequestHeadersFromAnotherPeer(t *testing.T) { hosts := createMocknet(t, 3) // create client + server(it does not have needed headers) - exchg, _ := createP2PExAndServer(t, hosts[0], hosts[1]) + exchg, store := createP2PExAndServer(t, hosts[0], hosts[1]) // create one more server(with more headers in the store) serverSideEx, err := NewExchangeServer[*headertest.DummyHeader]( hosts[2], headertest.NewStore[*headertest.DummyHeader](t, headertest.NewTestSuite(t), 10), @@ -273,7 +275,11 @@ func TestExchange_RequestHeadersFromAnotherPeer(t *testing.T) { exchg.peerTracker.peerLk.Lock() exchg.peerTracker.trackedPeers[hosts[2].ID()] = &peerStat{peerID: hosts[2].ID(), peerScore: 20} exchg.peerTracker.peerLk.Unlock() - _, err = exchg.GetRangeByHeight(context.Background(), 5, 3) + + h, err := store.GetByHeight(context.Background(), 5) + require.NoError(t, err) + + _, err = exchg.GetRangeByHeight(context.Background(), h, 8) require.NoError(t, err) // ensure that peerScore for the second peer is changed newPeerScore := exchg.peerTracker.trackedPeers[hosts[2].ID()].score() @@ -472,7 +478,7 @@ func TestExchange_RequestHeadersFromAnotherPeerWhenTimeout(t *testing.T) { dial(swarm1, swarm2) // create client + server(it does not have needed headers) - exchg, _ := createP2PExAndServer(t, host0, host1) + exchg, store := createP2PExAndServer(t, host0, host1) exchg.Params.RangeRequestTimeout = time.Millisecond * 100 // create one more server(with more headers in the store) serverSideEx, err := NewExchangeServer[*headertest.DummyHeader]( @@ -490,7 +496,11 @@ func TestExchange_RequestHeadersFromAnotherPeerWhenTimeout(t *testing.T) { exchg.peerTracker.peerLk.Lock() exchg.peerTracker.trackedPeers[host2.ID()] = &peerStat{peerID: host2.ID(), peerScore: 200} exchg.peerTracker.peerLk.Unlock() - _, err = exchg.GetRangeByHeight(context.Background(), 1, 3) + + gen, err := store.GetByHeight(context.Background(), 1) + require.NoError(t, err) + + _, err = exchg.GetRangeByHeight(context.Background(), gen, 3) require.NoError(t, err) newPeerScore := exchg.peerTracker.trackedPeers[host1.ID()].score() assert.NotEqual(t, newPeerScore, prevScore) @@ -500,7 +510,7 @@ func TestExchange_RequestHeadersFromAnotherPeerWhenTimeout(t *testing.T) { // from server, Exchange will re-request remaining headers from another peer func TestExchange_RequestPartialRange(t *testing.T) { hosts := createMocknet(t, 3) - exchg, _ := createP2PExAndServer(t, hosts[0], hosts[1]) + exchg, store := createP2PExAndServer(t, hosts[0], hosts[1]) // create one more server(with more headers in the store) serverSideEx, err := NewExchangeServer[*headertest.DummyHeader]( @@ -518,7 +528,11 @@ func TestExchange_RequestPartialRange(t *testing.T) { // reducing peerScore of the second server, so our exchange will request host[1] first. exchg.peerTracker.trackedPeers[hosts[2].ID()] = &peerStat{peerID: hosts[2].ID(), peerScore: 50} exchg.peerTracker.peerLk.Unlock() - h, err := exchg.GetRangeByHeight(ctx, 1, 8) + + gen, err := store.GetByHeight(context.Background(), 1) + require.NoError(t, err) + + h, err := exchg.GetRangeByHeight(ctx, gen, 8) require.NotNil(t, h) require.NoError(t, err) diff --git a/p2p/server.go b/p2p/server.go index b304d2ce..a08c1ac6 100644 --- a/p2p/server.go +++ b/p2p/server.go @@ -210,7 +210,7 @@ func (serv *ExchangeServer[H]) handleRequest(from, to uint64) ([]H, error) { } // might be a case when store hasn't synced yet to the requested range - if uint64(head.Height()) < from { + if head.Height() < from { span.SetStatus(codes.Error, header.ErrNotFound.Error()) log.Debugw("server: requested headers not stored", "from", from, @@ -229,7 +229,7 @@ func (serv *ExchangeServer[H]) handleRequest(from, to uint64) ([]H, error) { to = uint64(head.Height()) + 1 } - headersByRange, err := serv.store.GetRangeByHeight(ctx, from, to) + headersByRange, err := serv.store.GetRange(ctx, from, to) if err != nil { span.SetStatus(codes.Error, err.Error()) if errors.Is(err, context.DeadlineExceeded) { diff --git a/p2p/server_test.go b/p2p/server_test.go index eab735a0..b8e558a8 100644 --- a/p2p/server_test.go +++ b/p2p/server_test.go @@ -7,6 +7,7 @@ import ( "github.com/ipfs/go-datastore" "github.com/stretchr/testify/require" + "github.com/celestiaorg/go-header" "github.com/celestiaorg/go-header/headertest" "github.com/celestiaorg/go-header/store" ) @@ -30,3 +31,23 @@ func TestExchangeServer_handleRequestTimeout(t *testing.T) { _, err = server.handleRequest(1, 200) require.Error(t, err) } + +func TestExchangeServer_errorsOnLargeRequest(t *testing.T) { + peer := createMocknet(t, 1) + s, err := store.NewStore[*headertest.DummyHeader](datastore.NewMapDatastore()) + require.NoError(t, err) + server, err := NewExchangeServer[*headertest.DummyHeader]( + peer[0], + s, + WithNetworkID[ServerParameters](networkID), + ) + require.NoError(t, err) + err = server.Start(context.Background()) + require.NoError(t, err) + t.Cleanup(func() { + server.Stop(context.Background()) //nolint:errcheck + }) + + _, err = server.handleRequest(1, header.MaxRangeRequestSize*2) + require.Error(t, err) +} diff --git a/p2p/session.go b/p2p/session.go index 2ea21e55..ac750714 100644 --- a/p2p/session.go +++ b/p2p/session.go @@ -197,7 +197,7 @@ func (s *session[H]) doRequest( responseLn := uint64(len(h)) // ensure that we received the correct amount of headers. if responseLn < req.Amount { - from := uint64(h[responseLn-1].Height()) + from := h[responseLn-1].Height() amount := req.Amount - responseLn select { diff --git a/store/store.go b/store/store.go index dd00e117..f8f88f31 100644 --- a/store/store.go +++ b/store/store.go @@ -238,7 +238,31 @@ func (s *Store[H]) GetByHeight(ctx context.Context, height uint64) (H, error) { return s.Get(ctx, hash) } -func (s *Store[H]) GetRangeByHeight(ctx context.Context, from, to uint64) ([]H, error) { +func (s *Store[H]) GetRangeByHeight( + ctx context.Context, + from H, + to uint64, +) ([]H, error) { + headers, err := s.getRangeByHeight(ctx, from.Height()+1, to) + if err != nil { + return nil, err + } + + for _, h := range headers { + err := from.Verify(h) + if err != nil { + return nil, err + } + from = h + } + return headers, nil +} + +func (s *Store[H]) GetRange(ctx context.Context, from, to uint64) ([]H, error) { + return s.getRangeByHeight(ctx, from, to) +} + +func (s *Store[H]) getRangeByHeight(ctx context.Context, from, to uint64) ([]H, error) { // as the requested range is non-inclusive in the end[from;to), we need to compare // `from` with `to-1` if from > to-1 { @@ -263,26 +287,6 @@ func (s *Store[H]) GetRangeByHeight(ctx context.Context, from, to uint64) ([]H, return headers, nil } -func (s *Store[H]) GetVerifiedRange( - ctx context.Context, - from H, - to uint64, -) ([]H, error) { - headers, err := s.GetRangeByHeight(ctx, uint64(from.Height()+1), to) - if err != nil { - return nil, err - } - - for _, h := range headers { - err := from.Verify(h) - if err != nil { - return nil, err - } - from = h - } - return headers, nil -} - func (s *Store[H]) Has(ctx context.Context, hash header.Hash) (bool, error) { if ok := s.cache.Contains(hash.String()); ok { return ok, nil diff --git a/store/store_test.go b/store/store_test.go index 4d8e3d11..5356b667 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -34,7 +34,7 @@ func TestStore(t *testing.T) { err = store.Append(ctx, in...) require.NoError(t, err) - out, err := store.GetRangeByHeight(ctx, 2, 12) + out, err := store.getRangeByHeight(ctx, 2, 12) require.NoError(t, err) for i, h := range in { assert.Equal(t, h.Hash(), out[i].Hash()) @@ -66,16 +66,16 @@ func TestStore(t *testing.T) { h, err = store.GetByHeight(ctx, 2) require.NoError(t, err) - out, err = store.GetVerifiedRange(ctx, h, 3) + out, err = store.GetRangeByHeight(ctx, h, 3) require.Error(t, err) assert.Nil(t, out) - out, err = store.GetVerifiedRange(ctx, h, 4) + out, err = store.GetRangeByHeight(ctx, h, 4) require.NoError(t, err) assert.NotNil(t, out) assert.Len(t, out, 1) - out, err = store.GetRangeByHeight(ctx, 2, 2) + out, err = store.getRangeByHeight(ctx, 2, 2) require.Error(t, err) assert.Nil(t, out) @@ -91,7 +91,7 @@ func TestStore(t *testing.T) { require.NoError(t, err) assert.Equal(t, suite.Head().Hash(), head.Hash()) - out, err = store.GetRangeByHeight(ctx, 1, 13) + out, err = store.getRangeByHeight(ctx, 1, 13) require.NoError(t, err) assert.Len(t, out, 12) @@ -121,10 +121,10 @@ func TestStorePendingCacheMiss(t *testing.T) { err = store.Append(ctx, suite.GenDummyHeaders(50)...) require.NoError(t, err) - _, err = store.GetRangeByHeight(ctx, 1, 101) + _, err = store.getRangeByHeight(ctx, 1, 101) require.NoError(t, err) - _, err = store.GetRangeByHeight(ctx, 101, 151) + _, err = store.getRangeByHeight(ctx, 101, 151) require.NoError(t, err) } diff --git a/sync/sync.go b/sync/sync.go index b0cfa705..69add020 100644 --- a/sync/sync.go +++ b/sync/sync.go @@ -237,7 +237,7 @@ func (s *Syncer[H]) doSync(ctx context.Context, fromHead, toHead H) (err error) s.state.Start = time.Now() s.stateLk.Unlock() - err = s.processHeaders(ctx, fromHead, uint64(toHead.Height())) + err = s.processHeaders(ctx, fromHead, toHead.Height()) s.stateLk.Lock() s.state.End = time.Now() @@ -272,7 +272,7 @@ func (s *Syncer[H]) processHeaders( // check if returned range is not adjacent to `fromHead` if fromHead.Height()+1 != headers[0].Height() { // if so - request missing ones - to := uint64(headers[0].Height() - 1) + to := headers[0].Height() - 1 if err = s.requestHeaders(ctx, fromHead, to); err != nil { return err } @@ -297,7 +297,7 @@ func (s *Syncer[H]) requestHeaders( fromHead H, to uint64, ) error { - amount := to - uint64(fromHead.Height()) + amount := to - fromHead.Height() // start requesting headers until amount remaining will be 0 for amount > 0 { size := header.MaxRangeRequestSize @@ -305,7 +305,8 @@ func (s *Syncer[H]) requestHeaders( size = amount } - headers, err := s.getter.GetVerifiedRange(ctx, fromHead, size) + to := fromHead.Height() + size + 1 + headers, err := s.getter.GetRangeByHeight(ctx, fromHead, to) if err != nil { return err } diff --git a/sync/sync_getter_test.go b/sync/sync_getter_test.go index 59cc5692..80fe99e6 100644 --- a/sync/sync_getter_test.go +++ b/sync/sync_getter_test.go @@ -67,10 +67,6 @@ func (f *fakeGetter[H]) GetByHeight(ctx context.Context, u uint64) (H, error) { panic("implement me") } -func (f *fakeGetter[H]) GetRangeByHeight(ctx context.Context, from, amount uint64) ([]H, error) { - panic("implement me") -} - -func (f *fakeGetter[H]) GetVerifiedRange(ctx context.Context, from H, amount uint64) ([]H, error) { +func (f *fakeGetter[H]) GetRangeByHeight(ctx context.Context, from H, amount uint64) ([]H, error) { panic("implement me") } diff --git a/sync/sync_head_test.go b/sync/sync_head_test.go index 9ef6c266..27fbbb4f 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -135,12 +135,11 @@ func (t *wrappedGetter) GetByHeight(ctx context.Context, u uint64) (*headertest. panic("implement me") } -func (t *wrappedGetter) GetRangeByHeight(ctx context.Context, from, amount uint64) ([]*headertest.DummyHeader, error) { - //TODO implement me - panic("implement me") -} - -func (t *wrappedGetter) GetVerifiedRange(ctx context.Context, from *headertest.DummyHeader, amount uint64) ([]*headertest.DummyHeader, error) { +func (t *wrappedGetter) GetRangeByHeight( + ctx context.Context, + from *headertest.DummyHeader, + amount uint64, +) ([]*headertest.DummyHeader, error) { //TODO implement me panic("implement me") } diff --git a/sync/sync_test.go b/sync/sync_test.go index 7d95e64e..07c44560 100644 --- a/sync/sync_test.go +++ b/sync/sync_test.go @@ -388,10 +388,10 @@ type delayedGetter[H header.Header[H]] struct { header.Getter[H] } -func (d *delayedGetter[H]) GetVerifiedRange(ctx context.Context, from H, amount uint64) ([]H, error) { +func (d *delayedGetter[H]) GetRangeByHeight(ctx context.Context, from H, amount uint64) ([]H, error) { select { case <-time.After(time.Millisecond * 100): - return d.Getter.GetVerifiedRange(ctx, from, amount) + return d.Getter.GetRangeByHeight(ctx, from, amount) case <-ctx.Done(): return nil, ctx.Err() } From dc9f7cbbf4d9c72e4832d8890be84da3cc45940f Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Tue, 10 Oct 2023 14:40:54 +0200 Subject: [PATCH 2/6] test(store | p2p): ensure range requested is (fromHead:to) --- p2p/exchange_test.go | 31 +++++++++++++++++-------------- store/store_test.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 14 deletions(-) diff --git a/p2p/exchange_test.go b/p2p/exchange_test.go index b15aa542..38298e0e 100644 --- a/p2p/exchange_test.go +++ b/p2p/exchange_test.go @@ -166,35 +166,36 @@ func TestExchange_RequestHeader(t *testing.T) { assert.Equal(t, store.Headers[5].Hash(), header.Hash()) } -func TestExchange_RequestHeaders(t *testing.T) { +func TestExchange_GetRangeByHeight(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) t.Cleanup(cancel) hosts := createMocknet(t, 2) exchg, store := createP2PExAndServer(t, hosts[0], hosts[1]) - gen, err := store.GetByHeight(ctx, 1) + from, err := store.GetByHeight(ctx, 1) require.NoError(t, err) + firstHeaderInRangeHeight := from.Height() + 1 + lastHeaderInRangeHeight := uint64(4) + to := lastHeaderInRangeHeight + 1 + expectedLenHeaders := to - firstHeaderInRangeHeight // expected amount + // perform expected request - gotHeaders, err := exchg.GetRangeByHeight(ctx, gen, 5) + gotHeaders, err := exchg.GetRangeByHeight(ctx, from, to) require.NoError(t, err) + + assert.Len(t, gotHeaders, int(expectedLenHeaders)) + assert.Equal(t, firstHeaderInRangeHeight, gotHeaders[0].Height()) + assert.Equal(t, lastHeaderInRangeHeight, gotHeaders[len(gotHeaders)-1].Height()) + for _, got := range gotHeaders { assert.Equal(t, store.Headers[got.Height()].Height(), got.Height()) assert.Equal(t, store.Headers[got.Height()].Hash(), got.Hash()) } } -func TestExchange_RequestVerifiedHeaders(t *testing.T) { - hosts := createMocknet(t, 2) - exchg, store := createP2PExAndServer(t, hosts[0], hosts[1]) - // perform expected request - h := store.Headers[1] - _, err := exchg.GetRangeByHeight(context.Background(), h, 3) - require.NoError(t, err) -} - -func TestExchange_RequestVerifiedHeadersFails(t *testing.T) { +func TestExchange_GetRangeByHeight_FailsVerification(t *testing.T) { hosts := createMocknet(t, 2) exchg, store := createP2PExAndServer(t, hosts[0], hosts[1]) store.Headers[3].VerifyFailure = true // force a verification failure on the 3rd header @@ -202,7 +203,9 @@ func TestExchange_RequestVerifiedHeadersFails(t *testing.T) { h := store.Headers[1] ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*500) t.Cleanup(cancel) - _, err := exchg.GetRangeByHeight(ctx, h, 4) // requests range (1:4) + + // requests range (1:4) + _, err := exchg.GetRangeByHeight(ctx, h, 4) assert.Error(t, err) // ensure that peer was added to the blacklist diff --git a/store/store_test.go b/store/store_test.go index 5356b667..f57ca16b 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -99,6 +99,43 @@ func TestStore(t *testing.T) { require.NoError(t, err) } +//TestStoreGetByHeight_ExpectedRange +func TestStoreGetByHeight_ExpectedRange(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) + t.Cleanup(cancel) + + suite := headertest.NewTestSuite(t) + + ds := sync.MutexWrap(datastore.NewMapDatastore()) + store, err := NewStoreWithHead(ctx, ds, suite.Head()) + require.NoError(t, err) + + err = store.Start(ctx) + require.NoError(t, err) + + head, err := store.Head(ctx) + require.NoError(t, err) + assert.EqualValues(t, suite.Head().Hash(), head.Hash()) + + in := suite.GenDummyHeaders(100) + err = store.Append(ctx, in...) + require.NoError(t, err) + + // request the range [4:98) || (from:to) + from := in[2] + firstHeaderInRangeHeight := from.Height() + 1 + lastHeaderInRangeHeight := uint64(98) + to := lastHeaderInRangeHeight + 1 + expectedLenHeaders := to - firstHeaderInRangeHeight // expected amount + + out, err := store.GetRangeByHeight(ctx, from, to) + require.NoError(t, err) + + assert.Len(t, out, int(expectedLenHeaders)) + assert.Equal(t, firstHeaderInRangeHeight, out[0].Height()) + assert.Equal(t, lastHeaderInRangeHeight, out[len(out)-1].Height()) +} + func TestStorePendingCacheMiss(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) From 181510e4ff8fefb5bd19b08de7089af031b2af29 Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Tue, 10 Oct 2023 14:44:06 +0200 Subject: [PATCH 3/6] test(store): GetRange test --- store/store_test.go | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/store/store_test.go b/store/store_test.go index f57ca16b..08fdbe1d 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -99,8 +99,8 @@ func TestStore(t *testing.T) { require.NoError(t, err) } -//TestStoreGetByHeight_ExpectedRange -func TestStoreGetByHeight_ExpectedRange(t *testing.T) { +// TestStore_GetRangeByHeight_ExpectedRange +func TestStore_GetRangeByHeight_ExpectedRange(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) @@ -121,7 +121,7 @@ func TestStoreGetByHeight_ExpectedRange(t *testing.T) { err = store.Append(ctx, in...) require.NoError(t, err) - // request the range [4:98) || (from:to) + // request the range [4:98] || (from:to) from := in[2] firstHeaderInRangeHeight := from.Height() + 1 lastHeaderInRangeHeight := uint64(98) @@ -136,6 +136,42 @@ func TestStoreGetByHeight_ExpectedRange(t *testing.T) { assert.Equal(t, lastHeaderInRangeHeight, out[len(out)-1].Height()) } +// TestStore_GetRange_ExpectedRange +func TestStore_GetRange_ExpectedRange(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) + t.Cleanup(cancel) + + suite := headertest.NewTestSuite(t) + + ds := sync.MutexWrap(datastore.NewMapDatastore()) + store, err := NewStoreWithHead(ctx, ds, suite.Head()) + require.NoError(t, err) + + err = store.Start(ctx) + require.NoError(t, err) + + head, err := store.Head(ctx) + require.NoError(t, err) + assert.EqualValues(t, suite.Head().Hash(), head.Hash()) + + in := suite.GenDummyHeaders(100) + err = store.Append(ctx, in...) + require.NoError(t, err) + + // request the range [4:98] + firstHeaderInRangeHeight := uint64(4) + lastHeaderInRangeHeight := uint64(98) + to := lastHeaderInRangeHeight + 1 + expectedLenHeaders := to - firstHeaderInRangeHeight // expected amount + + out, err := store.GetRange(ctx, firstHeaderInRangeHeight, to) + require.NoError(t, err) + + assert.Len(t, out, int(expectedLenHeaders)) + assert.Equal(t, firstHeaderInRangeHeight, out[0].Height()) + assert.Equal(t, lastHeaderInRangeHeight, out[len(out)-1].Height()) +} + func TestStorePendingCacheMiss(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) From 210cb9a0c1557b0f46ca82c59e03d01a91c5ebb1 Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Tue, 10 Oct 2023 14:53:08 +0200 Subject: [PATCH 4/6] test(store): table test for GetRange --- store/store_test.go | 67 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 12 deletions(-) diff --git a/store/store_test.go b/store/store_test.go index 08fdbe1d..fadbb75d 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -136,8 +136,9 @@ func TestStore_GetRangeByHeight_ExpectedRange(t *testing.T) { assert.Equal(t, lastHeaderInRangeHeight, out[len(out)-1].Height()) } -// TestStore_GetRange_ExpectedRange -func TestStore_GetRange_ExpectedRange(t *testing.T) { +// TestStore_GetRange tests possible combinations of requests and ensures that +// the store can handle them adequately (even malformed requests) +func TestStore_GetRange(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) t.Cleanup(cancel) @@ -158,18 +159,60 @@ func TestStore_GetRange_ExpectedRange(t *testing.T) { err = store.Append(ctx, in...) require.NoError(t, err) - // request the range [4:98] - firstHeaderInRangeHeight := uint64(4) - lastHeaderInRangeHeight := uint64(98) - to := lastHeaderInRangeHeight + 1 - expectedLenHeaders := to - firstHeaderInRangeHeight // expected amount + var tests = []struct { + name string + from uint64 + to uint64 + expectedError bool + }{ + { + name: "valid request for headers all contained in store", + from: 4, + to: 99, + expectedError: false, + }, + { + name: "malformed request for headers contained in store", + from: 99, + to: 4, + expectedError: true, + }, + { + name: "valid request for headers not contained in store", + from: 100, + to: 500, + expectedError: true, + }, + { + name: "valid request for headers partially contained in store (`to` is greater than chain head)", + from: 77, + to: 200, + expectedError: true, + }, + } - out, err := store.GetRange(ctx, firstHeaderInRangeHeight, to) - require.NoError(t, err) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + firstHeaderInRangeHeight := tt.from + lastHeaderInRangeHeight := tt.to - 1 + to := lastHeaderInRangeHeight + 1 + expectedLenHeaders := to - firstHeaderInRangeHeight // expected amount + + // request the range [tt.to:tt.from) + out, err := store.GetRange(ctx, firstHeaderInRangeHeight, to) + if tt.expectedError { + assert.Error(t, err) + assert.Nil(t, out) + return + } + require.NoError(t, err) + + assert.Len(t, out, int(expectedLenHeaders)) + assert.Equal(t, firstHeaderInRangeHeight, out[0].Height()) + assert.Equal(t, lastHeaderInRangeHeight, out[len(out)-1].Height()) + }) + } - assert.Len(t, out, int(expectedLenHeaders)) - assert.Equal(t, firstHeaderInRangeHeight, out[0].Height()) - assert.Equal(t, lastHeaderInRangeHeight, out[len(out)-1].Height()) } func TestStorePendingCacheMiss(t *testing.T) { From 8eb3f562f3d69f6962bac3912e141c0055c0e4de Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Wed, 11 Oct 2023 14:58:07 +0200 Subject: [PATCH 5/6] (doc): Add expected range return val in iface --- interface.go | 1 + 1 file changed, 1 insertion(+) diff --git a/interface.go b/interface.go index 730674e8..c5cc8aac 100644 --- a/interface.go +++ b/interface.go @@ -111,6 +111,7 @@ type Getter[H Header[H]] interface { // GetRangeByHeight requests the header range from the provided Header and // verifies that the returned headers are adjacent to each other. + // Expected to return the range [from.Height()+1:to). GetRangeByHeight(ctx context.Context, from H, to uint64) ([]H, error) } From 16969fd953b578e3488309609c929f1831e97d4f Mon Sep 17 00:00:00 2001 From: rene <41963722+renaynay@users.noreply.github.com> Date: Wed, 11 Oct 2023 15:08:47 +0200 Subject: [PATCH 6/6] test: last cosmetic changes --- store/store_test.go | 8 ++++---- sync/sync_getter_test.go | 2 +- sync/sync_head_test.go | 2 +- sync/sync_test.go | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/store/store_test.go b/store/store_test.go index fadbb75d..ac0fdb12 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -34,7 +34,7 @@ func TestStore(t *testing.T) { err = store.Append(ctx, in...) require.NoError(t, err) - out, err := store.getRangeByHeight(ctx, 2, 12) + out, err := store.GetRange(ctx, 2, 12) require.NoError(t, err) for i, h := range in { assert.Equal(t, h.Hash(), out[i].Hash()) @@ -75,7 +75,7 @@ func TestStore(t *testing.T) { assert.NotNil(t, out) assert.Len(t, out, 1) - out, err = store.getRangeByHeight(ctx, 2, 2) + out, err = store.GetRange(ctx, 2, 2) require.Error(t, err) assert.Nil(t, out) @@ -237,10 +237,10 @@ func TestStorePendingCacheMiss(t *testing.T) { err = store.Append(ctx, suite.GenDummyHeaders(50)...) require.NoError(t, err) - _, err = store.getRangeByHeight(ctx, 1, 101) + _, err = store.GetRange(ctx, 1, 101) require.NoError(t, err) - _, err = store.getRangeByHeight(ctx, 101, 151) + _, err = store.GetRange(ctx, 101, 151) require.NoError(t, err) } diff --git a/sync/sync_getter_test.go b/sync/sync_getter_test.go index 80fe99e6..16ddc196 100644 --- a/sync/sync_getter_test.go +++ b/sync/sync_getter_test.go @@ -67,6 +67,6 @@ func (f *fakeGetter[H]) GetByHeight(ctx context.Context, u uint64) (H, error) { panic("implement me") } -func (f *fakeGetter[H]) GetRangeByHeight(ctx context.Context, from H, amount uint64) ([]H, error) { +func (f *fakeGetter[H]) GetRangeByHeight(ctx context.Context, from H, to uint64) ([]H, error) { panic("implement me") } diff --git a/sync/sync_head_test.go b/sync/sync_head_test.go index 27fbbb4f..29b75ff4 100644 --- a/sync/sync_head_test.go +++ b/sync/sync_head_test.go @@ -138,7 +138,7 @@ func (t *wrappedGetter) GetByHeight(ctx context.Context, u uint64) (*headertest. func (t *wrappedGetter) GetRangeByHeight( ctx context.Context, from *headertest.DummyHeader, - amount uint64, + to uint64, ) ([]*headertest.DummyHeader, error) { //TODO implement me panic("implement me") diff --git a/sync/sync_test.go b/sync/sync_test.go index 07c44560..dc108cc5 100644 --- a/sync/sync_test.go +++ b/sync/sync_test.go @@ -388,10 +388,10 @@ type delayedGetter[H header.Header[H]] struct { header.Getter[H] } -func (d *delayedGetter[H]) GetRangeByHeight(ctx context.Context, from H, amount uint64) ([]H, error) { +func (d *delayedGetter[H]) GetRangeByHeight(ctx context.Context, from H, to uint64) ([]H, error) { select { case <-time.After(time.Millisecond * 100): - return d.Getter.GetRangeByHeight(ctx, from, amount) + return d.Getter.GetRangeByHeight(ctx, from, to) case <-ctx.Done(): return nil, ctx.Err() }