fix(dot/sync): verify justification before importing blocks

dot/interfaces.go

@@ -27,7 +27,7 @@ type ServiceRegisterer interface {
 
 // BlockJustificationVerifier has a verification method for block justifications.
 type BlockJustificationVerifier interface {
-	VerifyBlockJustification(common.Hash, []byte) error
+	VerifyBlockJustification(common.Hash, []byte) (round uint64, setID uint64, err error)
 }
 
 // Telemetry is the telemetry client to send telemetry messages.

dot/network/message.go

@@ -388,7 +388,7 @@ func NewAscendingBlockRequests(startNumber, targetNumber uint, requestedData byt
 	numRequests := diff / MaxBlocksInResponse
 	// we should check if the diff is in the maxResponseSize bounds
 	// otherwise we should increase the numRequests by one, take this
-	// example, we want to sync from 0 to 259, the diff is 259
+	// example, we want to sync from 1 to 259, the diff is 259
 	// then the num of requests is 2 (uint(259)/uint(128)) however two requests will
 	// retrieve only 256 blocks (each request can retrieve a max of 128 blocks), so we should
 	// create one more request to retrieve those missing blocks, 3 in this example.

dot/sync/chain_sync.go

@@ -23,7 +23,6 @@ import (
 	"github.com/ChainSafe/gossamer/dot/telemetry"
 	"github.com/ChainSafe/gossamer/dot/types"
 	"github.com/ChainSafe/gossamer/internal/database"
-	"github.com/ChainSafe/gossamer/lib/blocktree"
 	"github.com/ChainSafe/gossamer/lib/common"
 	"github.com/ChainSafe/gossamer/lib/common/variadic"
 	"github.com/ChainSafe/gossamer/lib/trie"
@@ -795,94 +794,51 @@ func (cs *chainSync) handleReadyBlock(bd *types.BlockData) error {
 // returns the index of the last BlockData it handled on success,
 // or the index of the block data that errored on failure.
 func (cs *chainSync) processBlockData(blockData types.BlockData) error {
-	headerInState, err := cs.blockState.HasHeader(blockData.Hash)
-	if err != nil {
-		return fmt.Errorf("checking if block state has header: %w", err)
-	}
-
-	bodyInState, err := cs.blockState.HasBlockBody(blockData.Hash)
-	if err != nil {
-		return fmt.Errorf("checking if block state has body: %w", err)
-	}
-
 	// while in bootstrap mode we don't need to broadcast block announcements
 	announceImportedBlock := cs.getSyncMode() == tip
-	if headerInState && bodyInState {
-		err = cs.processBlockDataWithStateHeaderAndBody(blockData, announceImportedBlock)
-		if err != nil {
-			return fmt.Errorf("processing block data with header and "+
-				"body in block state: %w", err)
-		}
-		return nil
+	var blockDataJustification []byte
+	if blockData.Justification != nil {
+		blockDataJustification = *blockData.Justification
 	}
 
 	if blockData.Header != nil {
+		round, setID, err := cs.verifyJustification(blockData.Header.Hash(), blockDataJustification)
+		if err != nil {
+			return err
+		}
+
 		if blockData.Body != nil {
 			err = cs.processBlockDataWithHeaderAndBody(blockData, announceImportedBlock)
 			if err != nil {
 				return fmt.Errorf("processing block data with header and body: %w", err)
 			}
 		}
 
-		if blockData.Justification != nil && len(*blockData.Justification) > 0 {
-			logger.Infof("handling justification for block %s (#%d)", blockData.Hash.Short(), blockData.Number())
-			err = cs.handleJustification(blockData.Header, *blockData.Justification)
-			if err != nil {
-				return fmt.Errorf("handling justification: %w", err)
-			}
+		err = cs.finalizeAndSetJustification(
+			blockData.Header,
+			round, setID,
+			blockDataJustification)
+		if err != nil {
+			return fmt.Errorf("while setting justification: %w", err)
 		}
 	}
 
-	err = cs.blockState.CompareAndSetBlockData(&blockData)
+	err := cs.blockState.CompareAndSetBlockData(&blockData)
 	if err != nil {
 		return fmt.Errorf("comparing and setting block data: %w", err)
 	}
 
 	return nil
 }
 
-func (cs *chainSync) processBlockDataWithStateHeaderAndBody(blockData types.BlockData,
-	announceImportedBlock bool) (err error) {
-	// TODO: fix this; sometimes when the node shuts down the "best block" isn't stored properly,
-	// so when the node restarts it has blocks higher than what it thinks is the best, causing it not to sync
-	// if we update the node to only store finalised blocks in the database, this should be fixed and the entire
-	// code block can be removed (#1784)
-	block, err := cs.blockState.GetBlockByHash(blockData.Hash)
-	if err != nil {
-		return fmt.Errorf("getting block by hash: %w", err)
-	}
-
-	err = cs.blockState.AddBlockToBlockTree(block)
-	if errors.Is(err, blocktree.ErrBlockExists) {
-		logger.Debugf(
-			"block number %d with hash %s already exists in block tree, skipping it.",
-			block.Header.Number, blockData.Hash)
-		return nil
-	} else if err != nil {
-		return fmt.Errorf("adding block to blocktree: %w", err)
-	}
-
-	if blockData.Justification != nil && len(*blockData.Justification) > 0 {
-		err = cs.handleJustification(&block.Header, *blockData.Justification)
-		if err != nil {
-			return fmt.Errorf("handling justification: %w", err)
-		}
-	}
-
-	// TODO: this is probably unnecessary, since the state is already in the database
-	// however, this case shouldn't be hit often, since it's only hit if the node state
-	// is rewinded or if the node shuts down unexpectedly (#1784)
-	state, err := cs.storageState.TrieState(&block.Header.StateRoot)
-	if err != nil {
-		return fmt.Errorf("loading trie state: %w", err)
-	}
-
-	err = cs.blockImportHandler.HandleBlockImport(block, state, announceImportedBlock)
-	if err != nil {
-		return fmt.Errorf("handling block import: %w", err)
+func (cs *chainSync) verifyJustification(headerHash common.Hash, justification []byte) (
+	round uint64, setID uint64, err error) {
+	if len(justification) > 0 {
+		round, setID, err = cs.finalityGadget.VerifyBlockJustification(headerHash, justification)
+		return round, setID, err
 	}
 
-	return nil
+	return 0, 0, nil
 }
 
 func (cs *chainSync) processBlockDataWithHeaderAndBody(blockData types.BlockData,
@@ -918,21 +874,27 @@ func (cs *chainSync) handleBody(body *types.Body) {
 	blockSizeGauge.Set(float64(acc))
 }
 
-func (cs *chainSync) handleJustification(header *types.Header, justification []byte) (err error) {
-	logger.Debugf("handling justification for block %d...", header.Number)
+func (cs *chainSync) finalizeAndSetJustification(header *types.Header,
+	round, setID uint64, justification []byte) (err error) {
+	if len(justification) > 0 {
+		err = cs.blockState.SetFinalisedHash(header.Hash(), round, setID)
+		if err != nil {
+			return fmt.Errorf("setting finalised hash: %w", err)
+		}
 
-	headerHash := header.Hash()
-	err = cs.finalityGadget.VerifyBlockJustification(headerHash, justification)
-	if err != nil {
-		return fmt.Errorf("verifying block number %d justification: %w", header.Number, err)
-	}
+		logger.Debugf(
+			"finalised block with hash #%d (%s), round %d and set id %d",
+			header.Number, header.Hash(), round, setID)
 
-	err = cs.blockState.SetJustification(headerHash, justification)
-	if err != nil {
-		return fmt.Errorf("setting justification for block number %d: %w", header.Number, err)
+		err = cs.blockState.SetJustification(header.Hash(), justification)
+		if err != nil {
+			return fmt.Errorf("setting justification for block number %d: %w",
+				header.Number, err)
+		}
+
+		logger.Infof("🔨 finalised block number #%d (%s)", header.Number, header.Hash())
 	}
 
-	logger.Infof("🔨 finalised block number %d with hash %s", header.Number, headerHash)
 	return nil
 }
 

dot/sync/chain_sync_test.go

@@ -1288,8 +1288,6 @@ func ensureSuccessfulBlockImportFlow(t *testing.T, parentHeader *types.Header,
 	t.Helper()
 
 	for idx, blockData := range blocksReceived {
-		mockBlockState.EXPECT().HasHeader(blockData.Header.Hash()).Return(false, nil)
-		mockBlockState.EXPECT().HasBlockBody(blockData.Header.Hash()).Return(false, nil)
 		mockBabeVerifier.EXPECT().VerifyBlock(blockData.Header).Return(nil)
 
 		var previousHeader *types.Header
@@ -1676,3 +1674,93 @@ func TestChainSync_getHighestBlock(t *testing.T) {
 		})
 	}
 }
+
+func TestChainSync_BootstrapSync_SuccessfulSync_WithInvalidJusticationBlock(t *testing.T) {
+	t.Parallel()
+
+	ctrl := gomock.NewController(t)
+	mockBlockState := NewMockBlockState(ctrl)
+	mockBlockState.EXPECT().GetFinalisedNotifierChannel().Return(make(chan *types.FinalisationInfo))
+	mockedGenesisHeader := types.NewHeader(common.NewHash([]byte{0}), trie.EmptyHash,
+		trie.EmptyHash, 0, types.NewDigest())
+
+	mockNetwork := NewMockNetwork(ctrl)
+	mockRequestMaker := NewMockRequestMaker(ctrl)
+
+	mockBabeVerifier := NewMockBabeVerifier(ctrl)
+	mockStorageState := NewMockStorageState(ctrl)
+	mockImportHandler := NewMockBlockImportHandler(ctrl)
+	mockTelemetry := NewMockTelemetry(ctrl)
+	mockFinalityGadget := NewMockFinalityGadget(ctrl)
+
+	// this test expects two workers responding each request with 128 blocks which means
+	// we should import 256 blocks in total
+	blockResponse := createSuccesfullBlockResponse(t, mockedGenesisHeader.Hash(), 1, 129)
+	const announceBlock = false
+
+	invalidJustificationBlock := blockResponse.BlockData[90]
+	invalidJustification := &[]byte{0x01, 0x01, 0x01, 0x02}
+	invalidJustificationBlock.Justification = invalidJustification
+
+	// here we split the whole set in two parts each one will be the "response" for each peer
+	worker1Response := &network.BlockResponseMessage{
+		BlockData: blockResponse.BlockData[:128],
+	}
+
+	// the first peer will respond the from the block 1 to 128 so the ensureBlockImportFlow
+	// will setup the expectations starting from the genesis header until block 128
+	ensureSuccessfulBlockImportFlow(t, mockedGenesisHeader, worker1Response.BlockData[:90], mockBlockState,
+		mockBabeVerifier, mockStorageState, mockImportHandler, mockTelemetry, announceBlock)
+
+	errVerifyBlockJustification := errors.New("VerifyBlockJustification mock error")
+	mockFinalityGadget.EXPECT().
+		VerifyBlockJustification(
+			invalidJustificationBlock.Header.Hash(),
+			*invalidJustification).
+		Return(uint64(0), uint64(0), errVerifyBlockJustification)
+
+	// we use gomock.Any since I cannot guarantee which peer picks which request
+	// but the first call to DoBlockRequest will return the first set and the second
+	// call will return the second set
+	mockRequestMaker.EXPECT().
+		Do(gomock.Any(), gomock.Any(), &network.BlockResponseMessage{}).
+		DoAndReturn(func(peerID, _, response any) any {
+			responsePtr := response.(*network.BlockResponseMessage)
+			*responsePtr = *worker1Response
+
+			fmt.Println("mocked request maker")
+			return nil
+		})
+
+	// setup a chain sync which holds in its peer view map
+	// 3 peers, each one announce block 129 as its best block number.
+	// We start this test with genesis block being our best block, so
+	// we're far behind by 128 blocks, we should execute a bootstrap
+	// sync request those blocks
+	const blocksAhead = 128
+	cs := setupChainSyncToBootstrapMode(t, blocksAhead,
+		mockBlockState, mockNetwork, mockRequestMaker, mockBabeVerifier,
+		mockStorageState, mockImportHandler, mockTelemetry)
+
+	cs.finalityGadget = mockFinalityGadget
+
+	target, err := cs.getTarget()
+	require.NoError(t, err)
+	require.Equal(t, uint(blocksAhead), target)
+
+	// include a new worker in the worker pool set, this worker
+	// should be an available peer that will receive a block request
+	// the worker pool executes the workers management
+	cs.workerPool.fromBlockAnnounce(peer.ID("alice"))
+	//cs.workerPool.fromBlockAnnounce(peer.ID("bob"))
+
+	err = cs.requestMaxBlocksFrom(mockedGenesisHeader)
+	require.ErrorIs(t, err, errVerifyBlockJustification)
+
+	err = cs.workerPool.stop()
+	require.NoError(t, err)
+
+	// peer should be not in the worker pool
+	// peer should be in the ignore list
+	require.Len(t, cs.workerPool.workers, 1)
+}

dot/sync/interfaces.go

@@ -20,7 +20,6 @@ type BlockState interface {
 	BestBlockHeader() (*types.Header, error)
 	BestBlockNumber() (number uint, err error)
 	CompareAndSetBlockData(bd *types.BlockData) error
-	HasBlockBody(hash common.Hash) (bool, error)
 	GetBlockBody(common.Hash) (*types.Body, error)
 	GetHeader(common.Hash) (*types.Header, error)
 	HasHeader(hash common.Hash) (bool, error)
@@ -40,6 +39,7 @@ type BlockState interface {
 	GetHeaderByNumber(num uint) (*types.Header, error)
 	GetAllBlocksAtNumber(num uint) ([]common.Hash, error)
 	IsDescendantOf(parent, child common.Hash) (bool, error)
+	SetFinalisedHash(common.Hash, uint64, uint64) error
 }
 
 // StorageState is the interface for the storage state
@@ -60,7 +60,7 @@ type BabeVerifier interface {
 
 // FinalityGadget implements justification verification functionality
 type FinalityGadget interface {
-	VerifyBlockJustification(common.Hash, []byte) error
+	VerifyBlockJustification(common.Hash, []byte) (round uint64, setID uint64, err error)
 }
 
 // BlockImportHandler is the interface for the handler of newly imported blocks

dot/sync/syncer_integration_test.go

@@ -111,9 +111,10 @@ func newTestSyncer(t *testing.T) *Service {
 	cfg.LogLvl = log.Trace
 	mockFinalityGadget := NewMockFinalityGadget(ctrl)
 	mockFinalityGadget.EXPECT().VerifyBlockJustification(gomock.AssignableToTypeOf(common.Hash{}),
-		gomock.AssignableToTypeOf([]byte{})).DoAndReturn(func(hash common.Hash, justification []byte) error {
-		return nil
-	}).AnyTimes()
+		gomock.AssignableToTypeOf([]byte{})).DoAndReturn(
+		func(hash common.Hash, justification []byte) (uint64, uint64, error) {
+			return 1, 1, nil
+		}).AnyTimes()
 
 	cfg.FinalityGadget = mockFinalityGadget
 	cfg.Network = NewMockNetwork(ctrl)