refactor: extract VerifyRange and remove verification out of store (#268)

Removes verification out of the Store => requires local exchange to do
verification => extracts canonical VerifyRange with unit testing =>
uncovers minor edge cases with range verification

Closes #266

Author: Wondertan

headertest/verify_test.go

@@ -125,3 +125,96 @@ func TestVerify(t *testing.T) {
 		})
 	}
 }
+
+func TestVerifyRange(t *testing.T) {
+	tests := []struct {
+		name     string
+		setup    func(*DummySuite) (*DummyHeader, []*DummyHeader)
+		err      error
+		verified int // number of headers that should be verified before error
+	}{
+		{
+			name: "successful verification of all headers",
+			setup: func(suite *DummySuite) (*DummyHeader, []*DummyHeader) {
+				trusted := suite.GenDummyHeaders(1)[0]
+				untrstd := suite.GenDummyHeaders(5)
+				return trusted, untrstd
+			},
+			verified: 5,
+		},
+		{
+			name: "empty untrusted headers range",
+			setup: func(suite *DummySuite) (*DummyHeader, []*DummyHeader) {
+				trusted := suite.GenDummyHeaders(1)[0]
+				return trusted, []*DummyHeader{}
+			},
+			err: header.ErrEmptyRange,
+		},
+		{
+			name: "zero trusted header",
+			setup: func(suite *DummySuite) (*DummyHeader, []*DummyHeader) {
+				var zero *DummyHeader
+				return zero, []*DummyHeader{zero}
+			},
+			err: header.ErrZeroHeader,
+		},
+		{
+			name: "zero untrusted header",
+			setup: func(suite *DummySuite) (*DummyHeader, []*DummyHeader) {
+				var zero *DummyHeader
+				return zero, []*DummyHeader{zero}
+			},
+			err: header.ErrZeroHeader,
+		},
+		{
+			name: "verification fails in middle of range",
+			setup: func(suite *DummySuite) (*DummyHeader, []*DummyHeader) {
+				trusted := suite.GenDummyHeaders(1)[0]
+				headers := suite.GenDummyHeaders(5)
+				headers[2].VerifyFailure = true // make 3rd header fail verification
+				return trusted, headers
+			},
+			err:      ErrDummyVerify,
+			verified: 2,
+		},
+		{
+			name: "non-adjacent header range ",
+			setup: func(suite *DummySuite) (*DummyHeader, []*DummyHeader) {
+				trusted := suite.GenDummyHeaders(1)[0]
+				_ = suite.GenDummyHeaders(
+					1,
+				) // generate a header to ensure the range can be non-adjacent
+				headers := suite.GenDummyHeaders(3)
+				headers = append(headers[0:1], headers[2:]...)
+				return trusted, headers
+			},
+			err:      header.ErrNonAdjacentRange,
+			verified: 1,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			suite := NewTestSuite(t)
+			trusted, untrstd := tt.setup(suite)
+			verified, err := header.VerifyRange(trusted, untrstd)
+
+			if tt.err != nil {
+				var verErr *header.VerifyError
+				assert.ErrorAs(t, err, &verErr)
+				assert.ErrorIs(t, errors.Unwrap(verErr), tt.err)
+				assert.Len(t, verified, tt.verified)
+			} else {
+				assert.NoError(t, err)
+				assert.Len(t, verified, len(untrstd))
+				if len(untrstd) > 0 {
+					assert.Equal(t,
+						untrstd[len(untrstd)-1],
+						verified[len(verified)-1],
+						"last verified header should match last input header",
+					)
+				}
+			}
+		})
+	}
+}

local/exchange.go

@@ -34,9 +34,13 @@ 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, from H, to uint64,
-) ([]H, error) {
-	return l.store.GetRangeByHeight(ctx, from, to)
+func (l *Exchange[H]) GetRangeByHeight(ctx context.Context, from H, to uint64) ([]H, error) {
+	headers, err := l.store.GetRangeByHeight(ctx, from, to)
+	if err != nil {
+		return nil, err
+	}
+
+	return header.VerifyRange(from, headers)
 }
 
 func (l *Exchange[H]) Get(ctx context.Context, hash header.Hash) (H, error) {

p2p/session.go

@@ -281,41 +281,18 @@ func (s *session[H]) processResponses(responses []*p2p_pb.HeaderResponse) (h []H
 		return nil, err
 	}
 
-	return hdrs, s.verify(hdrs)
+	return s.verify(hdrs)
 }
 
 // verify checks that the received range of headers is adjacent and is valid against the provided
 // header.
-func (s *session[H]) verify(headers []H) error {
-	// if `s.from` is empty, then additional validation for the header`s range is not needed.
+func (s *session[H]) verify(headers []H) ([]H, error) {
+	// if `s.from` is empty, then we just trust the headers
 	if s.from.IsZero() {
-		return nil
+		return headers, nil
 	}
 
-	trusted := s.from
-	// verify that the whole range is valid and adjacent.
-	for _, untrusted := range headers {
-		err := trusted.Verify(untrusted)
-		if err != nil {
-			return err
-		}
-
-		// extra check for the adjacency should be performed only for the received range,
-		// because headers are received out of order and `s.from` can't be adjacent to them
-		if trusted.Height() != s.from.Height() {
-			if trusted.Height()+1 != untrusted.Height() {
-				// Exchange requires requested ranges to always consist of adjacent headers
-				return fmt.Errorf(
-					"peer sent valid but non-adjacent header. expected:%d, received:%d",
-					trusted.Height()+1,
-					untrusted.Height(),
-				)
-			}
-		}
-		// as `untrusted` was verified against previous trusted header, we can assume that it is valid
-		trusted = untrusted
-	}
-	return nil
+	return header.VerifyRange(s.from, headers)
 }
 
 // prepareRequests converts incoming range into separate HeaderRequest.

p2p/session_test.go

@@ -1,15 +1,9 @@
 package p2p
 
 import (
-	"context"
 	"testing"
-	"time"
 
-	"github.com/libp2p/go-libp2p/core/peer"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
-	"github.com/celestiaorg/go-header/headertest"
 )
 
 func Test_PrepareRequests(t *testing.T) {
@@ -20,39 +14,3 @@ func Test_PrepareRequests(t *testing.T) {
 	require.Equal(t, requests[0].GetOrigin(), uint64(1))
 	require.Equal(t, requests[1].GetOrigin(), uint64(6))
 }
-
-// Test_Validate ensures that headers range is adjacent and valid.
-func Test_Validate(t *testing.T) {
-	suite := headertest.NewTestSuite(t)
-	head := suite.Head()
-	ses := newSession(
-		context.Background(),
-		nil,
-		&peerTracker{trackedPeers: make(map[peer.ID]*peerStat)},
-		"", time.Second, nil,
-		withValidation(head),
-	)
-
-	headers := suite.GenDummyHeaders(5)
-	err := ses.verify(headers)
-	assert.NoError(t, err)
-}
-
-// Test_ValidateFails ensures that non-adjacent range will return an error.
-func Test_ValidateFails(t *testing.T) {
-	suite := headertest.NewTestSuite(t)
-	head := suite.Head()
-	ses := newSession(
-		context.Background(),
-		nil,
-		&peerTracker{trackedPeers: make(map[peer.ID]*peerStat)},
-		"", time.Second, nil,
-		withValidation(head),
-	)
-
-	headers := suite.GenDummyHeaders(5)
-	// break adjacency
-	headers[2] = headers[4]
-	err := ses.verify(headers)
-	assert.Error(t, err)
-}

store/store.go

@@ -304,67 +304,26 @@ func (s *Store[H]) Append(ctx context.Context, headers ...H) error {
 		return nil
 	}
 
-	var err error
-	// take current write head to verify headers against
-	var head H
-	headPtr := s.writeHead.Load()
-	if headPtr == nil {
-		head, err = s.Head(ctx)
-		if err != nil {
-			return err
-		}
-	} else {
-		head = *headPtr
-	}
-
-	// collect valid headers
-	verified := make([]H, 0, lh)
-	for i, h := range headers {
-		err = head.Verify(h)
-		if err != nil {
-			var verErr *header.VerifyError
-			if errors.As(err, &verErr) {
-				log.Errorw("invalid header",
-					"height_of_head", head.Height(),
-					"hash_of_head", head.Hash(),
-					"height_of_invalid", h.Height(),
-					"hash_of_invalid", h.Hash(),
-					"reason", verErr.Reason)
-			}
-			// if the first header is invalid, no need to go further
-			if i == 0 {
-				// and simply return
-				return err
-			}
-			// otherwise, stop the loop and apply headers appeared to be valid
-			break
-		}
-		verified = append(verified, h)
-		head = h
-	}
-
 	onWrite := func() {
-		newHead := verified[len(verified)-1]
+		newHead := headers[len(headers)-1]
 		s.writeHead.Store(&newHead)
 		log.Infow("new head", "height", newHead.Height(), "hash", newHead.Hash())
 		s.metrics.newHead(newHead.Height())
 	}
 
 	// queue headers to be written on disk
 	select {
-	case s.writes <- verified:
-		// we return an error here after writing,
-		// as there might be an invalid header in between of a given range
+	case s.writes <- headers:
 		onWrite()
-		return err
+		return nil
 	default:
 		s.metrics.writesQueueBlocked(ctx)
 	}
 	// if the writes queue is full, we block until it is not
 	select {
-	case s.writes <- verified:
+	case s.writes <- headers:
 		onWrite()
-		return err
+		return nil
 	case <-s.writesDn:
 		return errStoppedStore
 	case <-ctx.Done():

store/store_test.go

@@ -126,25 +126,6 @@ func TestStore_GetRangeByHeight_ExpectedRange(t *testing.T) {
 	assert.Equal(t, lastHeaderInRangeHeight, out[len(out)-1].Height())
 }
 
-func TestStore_Append_BadHeader(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 := NewTestStore(t, ctx, ds, suite.Head())
-
-	head, err := store.Head(ctx)
-	require.NoError(t, err)
-	assert.EqualValues(t, suite.Head().Hash(), head.Hash())
-
-	in := suite.GenDummyHeaders(10)
-	in[0].VerifyFailure = true
-	err = store.Append(ctx, in...)
-	require.Error(t, err)
-}
-
 // 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) {

verify.go

@@ -6,6 +6,45 @@ import (
 	"time"
 )
 
+// VerifyRange verifies a range of adjacent untrusted Headers against trusted following general Header checks and
+// custom user-specific checks defined in [Verify].
+//
+// In case of error, returns verified sub-range of Headers and error.
+func VerifyRange[H Header[H]](trstd H, untrstdRange []H) ([]H, error) {
+	if len(untrstdRange) == 0 {
+		return nil, &VerifyError{Reason: ErrEmptyRange}
+	}
+
+	verified := make([]H, 0, len(untrstdRange))
+	for i, untrstd := range untrstdRange {
+		err := Verify(trstd, untrstd)
+		if err != nil {
+			return verified, err
+		}
+
+		// ensure range is adjacent, as Verify allows non-adjacency
+		// NOTE: i > 0 because we allow the input trusted header to be non-adjacent
+		// so given trusted header height 100, we can have untrusted 151-155
+		if i > 0 && trstd.Height()+1 != untrstd.Height() {
+			reason := fmt.Errorf(
+				"%w; trusted: %d, expected_untrusted: %d, received_untrusted: %d",
+				ErrNonAdjacentRange,
+				trstd.Height(),
+				trstd.Height()+1,
+				untrstd.Height(),
+			)
+			// TODO(@Wondertan): Attach trusted and untrusted headers to the error
+			//  instead of manual error creation
+			return verified, &VerifyError{Reason: reason}
+		}
+
+		verified = append(verified, untrstd)
+		trstd = untrstd
+	}
+
+	return verified, nil
+}
+
 // Verify verifies untrusted Header against trusted following general Header checks and
 // custom user-specific checks defined in Header.Verify.
 //
@@ -87,11 +126,13 @@ func verify[H Header[H]](trstd, untrstd H) error {
 }
 
 var (
-	ErrZeroHeader    = errors.New("zero header")
-	ErrWrongChainID  = errors.New("wrong chain id")
-	ErrUnorderedTime = errors.New("unordered headers")
-	ErrFromFuture    = errors.New("header is from the future")
-	ErrKnownHeader   = errors.New("known header")
+	ErrZeroHeader       = errors.New("zero header")
+	ErrWrongChainID     = errors.New("wrong chain id")
+	ErrUnorderedTime    = errors.New("unordered headers")
+	ErrFromFuture       = errors.New("header is from the future")
+	ErrKnownHeader      = errors.New("known header")
+	ErrEmptyRange       = errors.New("empty header range")
+	ErrNonAdjacentRange = errors.New("non-adjacent headers range")
 )
 
 // VerifyError is thrown if a Header failed verification.