eth: fix multiple checkpoint fetch logic (#465)

* fix: handle checkpoint ordering

* create checkpoint verifier, add unit tests

* fix: remove mutex

* address comment for default checkpoint verifier

Co-authored-by: Evgeny Danienko <6655321@bk.ru>

eth/backend.go

@@ -695,7 +695,9 @@ func (s *Ethereum) handleWhitelistCheckpoint(ctx context.Context, first bool) er
 		return ErrBorConsensusWithoutHeimdall
 	}
 
-	blockNums, blockHashes, err := ethHandler.fetchWhitelistCheckpoints(ctx, bor, first)
+	// Create a new checkpoint verifier
+	verifier := newCheckpointVerifier(nil)
+	blockNums, blockHashes, err := ethHandler.fetchWhitelistCheckpoints(ctx, bor, verifier, first)
 	// If the array is empty, we're bound to receive an error. Non-nill error and non-empty array
 	// means that array has partial elements and it failed for some block. We'll add those partial
 	// elements anyway.

eth/bor_checkpoint_verifier.go

@@ -0,0 +1,60 @@
+package eth
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/ethereum/go-ethereum/common/hexutil"
+	"github.com/ethereum/go-ethereum/consensus/bor/heimdall/checkpoint"
+	"github.com/ethereum/go-ethereum/log"
+	"github.com/ethereum/go-ethereum/rpc"
+)
+
+type checkpointVerifier struct {
+	verify func(ctx context.Context, handler *ethHandler, checkpoint *checkpoint.Checkpoint) (string, error)
+}
+
+func newCheckpointVerifier(verifyFn func(ctx context.Context, handler *ethHandler, checkpoint *checkpoint.Checkpoint) (string, error)) *checkpointVerifier {
+	if verifyFn != nil {
+		return &checkpointVerifier{verifyFn}
+	}
+
+	verifyFn = func(ctx context.Context, handler *ethHandler, checkpoint *checkpoint.Checkpoint) (string, error) {
+		var (
+			startBlock = checkpoint.StartBlock.Uint64()
+			endBlock   = checkpoint.EndBlock.Uint64()
+		)
+
+		// check if we have the checkpoint blocks
+		head := handler.ethAPI.BlockNumber()
+		if head < hexutil.Uint64(endBlock) {
+			log.Debug("Head block behind checkpoint block", "head", head, "checkpoint end block", endBlock)
+			return "", errMissingCheckpoint
+		}
+
+		// verify the root hash of checkpoint
+		roothash, err := handler.ethAPI.GetRootHash(ctx, startBlock, endBlock)
+		if err != nil {
+			log.Debug("Failed to get root hash of checkpoint while whitelisting", "err", err)
+			return "", errRootHash
+		}
+
+		if roothash != checkpoint.RootHash.String()[2:] {
+			log.Warn("Checkpoint root hash mismatch while whitelisting", "expected", checkpoint.RootHash.String()[2:], "got", roothash)
+			return "", errCheckpointRootHashMismatch
+		}
+
+		// fetch the end checkpoint block hash
+		block, err := handler.ethAPI.GetBlockByNumber(ctx, rpc.BlockNumber(endBlock), false)
+		if err != nil {
+			log.Debug("Failed to get end block hash of checkpoint while whitelisting", "err", err)
+			return "", errEndBlock
+		}
+
+		hash := fmt.Sprintf("%v", block["hash"])
+
+		return hash, nil
+	}
+
+	return &checkpointVerifier{verifyFn}
+}

eth/handler_bor.go

@@ -3,13 +3,10 @@ package eth
 import (
 	"context"
 	"errors"
-	"fmt"
 
 	"github.com/ethereum/go-ethereum/common"
-	"github.com/ethereum/go-ethereum/common/hexutil"
 	"github.com/ethereum/go-ethereum/consensus/bor"
 	"github.com/ethereum/go-ethereum/log"
-	"github.com/ethereum/go-ethereum/rpc"
 )
 
 var (
@@ -43,7 +40,7 @@ var (
 
 // fetchWhitelistCheckpoints fetches the latest checkpoint/s from it's local heimdall
 // and verifies the data against bor data.
-func (h *ethHandler) fetchWhitelistCheckpoints(ctx context.Context, bor *bor.Bor, first bool) ([]uint64, []common.Hash, error) {
+func (h *ethHandler) fetchWhitelistCheckpoints(ctx context.Context, bor *bor.Bor, checkpointVerifier *checkpointVerifier, first bool) ([]uint64, []common.Hash, error) {
 	// Create an array for block number and block hashes
 	//nolint:prealloc
 	var (
@@ -62,56 +59,44 @@ func (h *ethHandler) fetchWhitelistCheckpoints(ctx context.Context, bor *bor.Bor
 		return blockNums, blockHashes, errNoCheckpoint
 	}
 
-	// If we're in the first iteration, we'll fetch last 10 checkpoints, else only the latest one
-	iterations := 1
-	if first {
-		iterations = 10
+	var (
+		start int64
+		end   int64
+	)
+
+	// Prepare the checkpoint range to fetch
+	if count <= 10 {
+		start = 1
+	} else {
+		start = count - 10 + 1 // 10 is the max number of checkpoints to fetch
 	}
 
-	for i := 0; i < iterations; i++ {
-		// If we don't have any checkpoints in heimdall, break
-		if count == 0 {
-			break
-		}
+	end = count
 
-		// fetch `count` indexed checkpoint from heimdall
-		checkpoint, err := bor.HeimdallClient.FetchCheckpoint(ctx, count)
+	// If we're in not in the first iteration, only fetch the latest checkpoint
+	if !first {
+		start = count
+	}
+
+	for i := start; i <= end; i++ {
+		// fetch `i` indexed checkpoint from heimdall
+		checkpoint, err := bor.HeimdallClient.FetchCheckpoint(ctx, i)
 		if err != nil {
 			log.Debug("Failed to fetch latest checkpoint for whitelisting", "err", err)
 			return blockNums, blockHashes, errCheckpoint
 		}
 
-		// check if we have the checkpoint blocks
-		head := h.ethAPI.BlockNumber()
-		if head < hexutil.Uint64(checkpoint.EndBlock.Uint64()) {
-			log.Debug("Head block behind checkpoint block", "head", head, "checkpoint end block", checkpoint.EndBlock)
-			return blockNums, blockHashes, errMissingCheckpoint
-		}
+		// Verify if the checkpoint fetched can be added to the local whitelist entry or not
+		// If verified, it returns the hash of the end block of the checkpoint. If not,
+		// it will return appropriate error.
 
-		// verify the root hash of checkpoint
-		roothash, err := h.ethAPI.GetRootHash(ctx, checkpoint.StartBlock.Uint64(), checkpoint.EndBlock.Uint64())
+		hash, err := checkpointVerifier.verify(ctx, h, checkpoint)
 		if err != nil {
-			log.Debug("Failed to get root hash of checkpoint while whitelisting", "err", err)
-			return blockNums, blockHashes, errRootHash
+			return blockNums, blockHashes, err
 		}
 
-		if roothash != checkpoint.RootHash.String()[2:] {
-			log.Warn("Checkpoint root hash mismatch while whitelisting", "expected", checkpoint.RootHash.String()[2:], "got", roothash)
-			return blockNums, blockHashes, errCheckpointRootHashMismatch
-		}
-
-		// fetch the end checkpoint block hash
-		block, err := h.ethAPI.GetBlockByNumber(ctx, rpc.BlockNumber(checkpoint.EndBlock.Uint64()), false)
-		if err != nil {
-			log.Debug("Failed to get end block hash of checkpoint while whitelisting", "err", err)
-			return blockNums, blockHashes, errEndBlock
-		}
-
-		hash := fmt.Sprintf("%v", block["hash"])
-
 		blockNums = append(blockNums, checkpoint.EndBlock.Uint64())
 		blockHashes = append(blockHashes, common.HexToHash(hash))
-		count--
 	}
 
 	return blockNums, blockHashes, nil

eth/handler_bor_test.go

@@ -0,0 +1,134 @@
+package eth
+
+import (
+	"context"
+	"math/big"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/ethereum/go-ethereum/consensus/bor"
+	"github.com/ethereum/go-ethereum/consensus/bor/clerk"
+	"github.com/ethereum/go-ethereum/consensus/bor/heimdall/checkpoint"
+	"github.com/ethereum/go-ethereum/consensus/bor/heimdall/span"
+)
+
+type mockHeimdall struct {
+	fetchCheckpoint      func(ctx context.Context, number int64) (*checkpoint.Checkpoint, error)
+	fetchCheckpointCount func(ctx context.Context) (int64, error)
+}
+
+func (m *mockHeimdall) StateSyncEvents(ctx context.Context, fromID uint64, to int64) ([]*clerk.EventRecordWithTime, error) {
+	return nil, nil
+}
+func (m *mockHeimdall) Span(ctx context.Context, spanID uint64) (*span.HeimdallSpan, error) {
+	//nolint:nilnil
+	return nil, nil
+}
+func (m *mockHeimdall) FetchCheckpoint(ctx context.Context, number int64) (*checkpoint.Checkpoint, error) {
+	return m.fetchCheckpoint(ctx, number)
+}
+func (m *mockHeimdall) FetchCheckpointCount(ctx context.Context) (int64, error) {
+	return m.fetchCheckpointCount(ctx)
+}
+func (m *mockHeimdall) Close() {}
+
+func TestFetchWhitelistCheckpoints(t *testing.T) {
+	t.Parallel()
+
+	// create an empty ethHandler
+	handler := &ethHandler{}
+
+	// create a mock checkpoint verification function and use it to create a verifier
+	verify := func(ctx context.Context, handler *ethHandler, checkpoint *checkpoint.Checkpoint) (string, error) {
+		return "", nil
+	}
+
+	verifier := newCheckpointVerifier(verify)
+
+	// Create a mock heimdall instance and use it for creating a bor instance
+	var heimdall mockHeimdall
+
+	bor := &bor.Bor{HeimdallClient: &heimdall}
+
+	// create 20 mock checkpoints
+	checkpoints := createMockCheckpoints(20)
+
+	// create a mock fetch checkpoint function
+	heimdall.fetchCheckpoint = func(_ context.Context, number int64) (*checkpoint.Checkpoint, error) {
+		return checkpoints[number-1], nil // we're sure that number won't exceed 20
+	}
+
+	// create a background context
+	ctx := context.Background()
+
+	testCases := []struct {
+		name        string
+		first       bool
+		count       int64
+		length      int
+		start       uint64
+		end         uint64
+		fetchErr    error
+		expectedErr error
+	}{
+		{"fail to fetch checkpoint count", false, 0, 0, 0, 0, errCheckpointCount, errCheckpointCount},
+		{"no checkpoints available", false, 0, 0, 0, 0, nil, errNoCheckpoint},
+		{"fetch multiple checkpoints (count < 10)", true, 6, 6, 0, 6, nil, nil},
+		{"fetch multiple checkpoints (count = 10)", true, 10, 10, 0, 10, nil, nil},
+		{"fetch multiple checkpoints (count > 10)", true, 16, 10, 6, 16, nil, nil},
+		{"fetch single checkpoint", false, 18, 1, 17, 18, nil, nil},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			heimdall.fetchCheckpointCount = getMockFetchCheckpointFn(tc.count, tc.fetchErr)
+			blockNums, blockHashes, err := handler.fetchWhitelistCheckpoints(ctx, bor, verifier, tc.first)
+
+			// Check if we have expected result
+			require.Equal(t, tc.expectedErr, err)
+			require.Equal(t, tc.length, len(blockNums))
+			require.Equal(t, tc.length, len(blockHashes))
+			validateBlockNumber(t, blockNums, checkpoints[tc.start:tc.end])
+		})
+	}
+}
+
+func validateBlockNumber(t *testing.T, blockNums []uint64, checkpoints []*checkpoint.Checkpoint) {
+	t.Helper()
+
+	for i, blockNum := range blockNums {
+		require.Equal(t, blockNum, checkpoints[i].EndBlock.Uint64(), "expect block number in array to match with checkpoint")
+	}
+}
+
+func getMockFetchCheckpointFn(number int64, err error) func(ctx context.Context) (int64, error) {
+	return func(_ context.Context) (int64, error) {
+		return number, err
+	}
+}
+
+func createMockCheckpoints(count int) []*checkpoint.Checkpoint {
+	var (
+		checkpoints []*checkpoint.Checkpoint = make([]*checkpoint.Checkpoint, count)
+		startBlock  int64                    = 257 // any number can be used
+	)
+
+	for i := 0; i < count; i++ {
+		checkpoints[i] = &checkpoint.Checkpoint{
+			Proposer:   common.Address{},
+			StartBlock: big.NewInt(startBlock),
+			EndBlock:   big.NewInt(startBlock + 255),
+			RootHash:   common.Hash{},
+			BorChainID: "137",
+			Timestamp:  uint64(time.Now().Unix()),
+		}
+		startBlock += 256
+	}
+
+	return checkpoints
+}