refactor(libhead): unify VerifyAdjacent and VerifyNonAdjacent into Verify (#1777)

Co-authored-by: rene <[email protected]>

Author: Wondertan

Diff for: core/exchange.go

@@ -64,7 +64,7 @@ func (ce *Exchange) GetVerifiedRange(
 	}
 
 	for _, h := range headers {
-		err := from.VerifyAdjacent(h)
+		err := from.Verify(h)
 		if err != nil {
 			return nil, err
 		}

Diff for: core/header_test.go

@@ -8,8 +8,9 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/tendermint/tendermint/libs/rand"
 
+	"github.com/celestiaorg/celestia-node/header/headertest"
+
 	"github.com/celestiaorg/celestia-node/header"
-	"github.com/celestiaorg/celestia-node/headertest"
 )
 
 func TestMakeExtendedHeaderForEmptyBlock(t *testing.T) {

Diff for: das/daser_test.go

@@ -16,9 +16,10 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/tendermint/tendermint/types"
 
+	"github.com/celestiaorg/celestia-node/header/headertest"
+
 	"github.com/celestiaorg/celestia-node/fraud"
 	"github.com/celestiaorg/celestia-node/header"
-	"github.com/celestiaorg/celestia-node/headertest"
 	libhead "github.com/celestiaorg/celestia-node/libs/header"
 	"github.com/celestiaorg/celestia-node/share"
 	"github.com/celestiaorg/celestia-node/share/availability/full"

Diff for: fraud/testing.go

@@ -14,8 +14,9 @@ import (
 	mocknet "github.com/libp2p/go-libp2p/p2p/net/mock"
 	"github.com/stretchr/testify/require"
 
+	"github.com/celestiaorg/celestia-node/header/headertest"
+
 	"github.com/celestiaorg/celestia-node/header"
-	"github.com/celestiaorg/celestia-node/headertest"
 )
 
 type DummyService struct {

Diff for: headertest/serde_test.go renamed to header/headertest/serde_test.go

@@ -0,0 +1,96 @@
+package headertest
+
+import (
+	"strconv"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+
+	libhead "github.com/celestiaorg/celestia-node/libs/header"
+
+	tmrand "github.com/tendermint/tendermint/libs/rand"
+)
+
+func TestVerify(t *testing.T) {
+	h := NewTestSuite(t, 2).GenExtendedHeaders(3)
+	trusted, untrustedAdj, untrustedNonAdj := h[0], h[1], h[2]
+	tests := []struct {
+		prepare func() libhead.Header
+		err     bool
+	}{
+		{
+			prepare: func() libhead.Header { return untrustedAdj },
+			err:     false,
+		},
+		{
+			prepare: func() libhead.Header {
+				return untrustedNonAdj
+			},
+			err: false,
+		},
+		{
+			prepare: func() libhead.Header {
+				untrusted := *untrustedAdj
+				untrusted.ValidatorsHash = tmrand.Bytes(32)
+				return &untrusted
+			},
+			err: true,
+		},
+		{
+			prepare: func() libhead.Header {
+				untrusted := *untrustedNonAdj
+				untrusted.Commit = NewTestSuite(t, 2).Commit(RandRawHeader(t))
+				return &untrusted
+			},
+			err: true,
+		},
+		{
+			prepare: func() libhead.Header {
+				untrusted := *untrustedAdj
+				untrusted.RawHeader.LastBlockID.Hash = tmrand.Bytes(32)
+				return &untrusted
+			},
+			err: true,
+		},
+		{
+			prepare: func() libhead.Header {
+				untrustedAdj.RawHeader.Time = untrustedAdj.RawHeader.Time.Add(time.Minute)
+				return untrustedAdj
+			},
+			err: true,
+		},
+		{
+			prepare: func() libhead.Header {
+				untrustedAdj.RawHeader.Time = untrustedAdj.RawHeader.Time.Truncate(time.Hour)
+				return untrustedAdj
+			},
+			err: true,
+		},
+		{
+			prepare: func() libhead.Header {
+				untrustedAdj.RawHeader.ChainID = "toaster"
+				return untrustedAdj
+			},
+			err: true,
+		},
+		{
+			prepare: func() libhead.Header {
+				untrustedAdj.RawHeader.Height++
+				return untrustedAdj
+			},
+			err: true,
+		},
+	}
+
+	for i, test := range tests {
+		t.Run(strconv.Itoa(i), func(t *testing.T) {
+			err := trusted.Verify(test.prepare())
+			if test.err {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}

Diff for: headertest/testing.go renamed to header/headertest/testing.go

@@ -10,53 +10,47 @@ import (
 	libhead "github.com/celestiaorg/celestia-node/libs/header"
 )
 
-// TODO(@Wondertan): We should request TrustingPeriod from the network's state params or
-//  listen for network params changes to always have a topical value.
-
-// VerifyNonAdjacent validates non-adjacent untrusted header against trusted 'eh'.
-func (eh *ExtendedHeader) VerifyNonAdjacent(untrusted libhead.Header) error {
+// Verify validates given untrusted Header against trusted ExtendedHeader.
+func (eh *ExtendedHeader) Verify(untrusted libhead.Header) error {
 	untrst, ok := untrusted.(*ExtendedHeader)
 	if !ok {
-		return &libhead.VerifyError{Reason: fmt.Errorf("invalid header type: expected %T, got %T", eh, untrusted)}
-	}
-	if err := eh.verify(untrst); err != nil {
-		return &libhead.VerifyError{Reason: err}
+		// if the header of the type was given, something very wrong happens
+		panic(fmt.Sprintf("invalid header type: expected %T, got %T", eh, untrusted))
 	}
 
-	// Ensure that untrusted commit has enough of trusted commit's power.
-	err := eh.ValidatorSet.VerifyCommitLightTrusting(eh.ChainID(), untrst.Commit, light.DefaultTrustLevel)
-	if err != nil {
+	if err := eh.verify(untrst); err != nil {
 		return &libhead.VerifyError{Reason: err}
 	}
 
-	return nil
-}
+	isAdjacent := eh.Height()+1 == untrst.Height()
+	if isAdjacent {
+		// Optimized verification for adjacent headers
+		// Check the validator hashes are the same
+		if !bytes.Equal(untrst.ValidatorsHash, eh.NextValidatorsHash) {
+			return &libhead.VerifyError{
+				Reason: fmt.Errorf("expected old header next validators (%X) to match those from new header (%X)",
+					eh.NextValidatorsHash,
+					untrst.ValidatorsHash,
+				),
+			}
+		}
 
-// VerifyAdjacent validates adjacent untrusted header against trusted 'eh'.
-func (eh *ExtendedHeader) VerifyAdjacent(untrusted libhead.Header) error {
-	untrst, ok := untrusted.(*ExtendedHeader)
-	if !ok {
-		return &libhead.VerifyError{Reason: fmt.Errorf("invalid header type: expected %T, got %T", eh, untrusted)}
-	}
-	if untrst.Height() != eh.Height()+1 {
-		return &libhead.ErrNonAdjacent{
-			Head:      eh.Height(),
-			Attempted: untrst.Height(),
+		if !bytes.Equal(untrst.LastHeader(), eh.Hash()) {
+			return &libhead.VerifyError{
+				Reason: fmt.Errorf("expected new header to point to last header hash (%X), but got %X)",
+					eh.Hash(),
+					untrst.LastHeader(),
+				),
+			}
 		}
-	}
 
-	if err := eh.verify(untrst); err != nil {
-		return &libhead.VerifyError{Reason: err}
+		return nil
 	}
 
-	// Check the validator hashes are the same
-	if !bytes.Equal(untrst.ValidatorsHash, eh.NextValidatorsHash) {
-		return &libhead.VerifyError{
-			Reason: fmt.Errorf("expected old header next validators (%X) to match those from new header (%X)",
-				eh.NextValidatorsHash,
-				untrst.ValidatorsHash,
-			),
-		}
+	// Ensure that untrusted commit has enough of trusted commit's power.
+	err := eh.ValidatorSet.VerifyCommitLightTrusting(eh.ChainID(), untrst.Commit, light.DefaultTrustLevel)
+	if err != nil {
+		return &libhead.VerifyError{Reason: err}
 	}
 
 	return nil
@@ -67,13 +61,17 @@ func (eh *ExtendedHeader) VerifyAdjacent(untrusted libhead.Header) error {
 var clockDrift = 10 * time.Second
 
 // verify performs basic verification of untrusted header.
-func (eh *ExtendedHeader) verify(untrst *ExtendedHeader) error {
+func (eh *ExtendedHeader) verify(untrst libhead.Header) error {
+	if untrst.Height() <= eh.Height() {
+		return fmt.Errorf("untrusted header height(%d) <= current trusted header(%d)", untrst.Height(), eh.Height())
+	}
+
 	if untrst.ChainID() != eh.ChainID() {
-		return fmt.Errorf("new untrusted header has different chain %s, not %s", untrst.ChainID(), eh.ChainID())
+		return fmt.Errorf("untrusted header has different chain %s, not %s", untrst.ChainID(), eh.ChainID())
 	}
 
 	if !untrst.Time().After(eh.Time()) {
-		return fmt.Errorf("expected new untrusted header time %v to be after old header time %v", untrst.Time(), eh.Time())
+		return fmt.Errorf("untrusted header time(%v) must be after current trusted header(%v)", untrst.Time(), eh.Time())
 	}
 
 	now := time.Now()

Diff for: header/headertest/verify_test.go

@@ -2,7 +2,7 @@ package header
 
 import "fmt"
 
-// VerifyError is thrown on during VerifyAdjacent and VerifyNonAdjacent if verification fails.
+// VerifyError is thrown on during Verify if it fails.
 type VerifyError struct {
 	Reason error
 }

Diff for: header/verify.go

@@ -21,10 +21,8 @@ type Header interface {
 	LastHeader() Hash
 	// Time returns time when header was created.
 	Time() time.Time
-	// VerifyAdjacent validates adjacent untrusted header against trusted header.
-	VerifyAdjacent(Header) error
-	// VerifyNonAdjacent validates non-adjacent untrusted header against trusted header.
-	VerifyNonAdjacent(Header) error
+	// Verify validates given untrusted Header against trusted Header.
+	Verify(Header) error
 	// Validate performs basic validation to check for missed/incorrect fields.
 	Validate() error
 

Diff for: headertest/verify_test.go

@@ -79,7 +79,7 @@ func TestExchange_RequestVerifiedHeadersFails(t *testing.T) {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*500)
 	t.Cleanup(cancel)
 	_, err := exchg.GetVerifiedRange(ctx, h, 3)
-	require.Error(t, err)
+	assert.Error(t, err)
 
 	// ensure that peer was added to the blacklist
 	peers := exchg.peerTracker.connGater.ListBlockedPeers()

Diff for: libs/header/errors.go

@@ -3,6 +3,7 @@ package p2p
 import (
 	"context"
 	"errors"
+	"fmt"
 	"sort"
 	"time"
 
@@ -160,6 +161,7 @@ func (s *session[H]) doRequest(
 		case <-s.ctx.Done():
 		case s.reqCh <- req:
 		}
+		log.Errorw("processing response", "err", err)
 		return
 	}
 	log.Debugw("request headers from peer succeeded", "peer", stat.peerID, "amount", req.Amount)
@@ -201,23 +203,18 @@ func (s *session[H]) validate(headers []H) error {
 		return nil
 	}
 
-	// verify that the first header in range is valid against the trusted header.
-	err := s.from.VerifyNonAdjacent(headers[0])
-	if err != nil {
-		return nil
-	}
-
-	if len(headers) == 1 {
-		return nil
-	}
-
-	trusted := headers[0]
+	trusted := s.from
 	// verify that the whole range is valid.
-	for i := 1; i < len(headers); i++ {
-		err = trusted.VerifyAdjacent(headers[i])
+	for i := 0; i < len(headers); i++ {
+		err := trusted.Verify(headers[i])
 		if err != nil {
 			return err
 		}
+		if trusted.Height()+1 != headers[i].Height() {
+			// Exchange requires requested ranges to always consist of adjacent headers
+			return fmt.Errorf("peer sent valid but non-adjacent header")
+		}
+
 		trusted = headers[i]
 	}
 	return nil