fix(dot/sync): Revert 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) (round uint64, setID uint64, err error)
+	VerifyBlockJustification(common.Hash, []byte) error
 }
 
 // Telemetry is the telemetry client to send telemetry messages.

dot/state/block.go

@@ -492,21 +492,6 @@ func (bs *BlockState) AddBlockWithArrivalTime(block *types.Block, arrivalTime ti
 	return nil
 }
 
-// AddBlockToBlockTree adds the given block to the blocktree. It does not write it to the database.
-// TODO: remove this func and usage from sync (after sync refactor?)
-func (bs *BlockState) AddBlockToBlockTree(block *types.Block) error {
-	bs.Lock()
-	defer bs.Unlock()
-
-	arrivalTime, err := bs.GetArrivalTime(block.Header.Hash())
-	if err != nil {
-		arrivalTime = time.Now()
-	}
-
-	bs.unfinalisedBlocks.store(block)
-	return bs.bt.AddBlock(&block.Header, arrivalTime)
-}
-
 // GetAllBlocksAtNumber returns all unfinalised blocks with the given number
 func (bs *BlockState) GetAllBlocksAtNumber(num uint) ([]common.Hash, error) {
 	header, err := bs.GetHeaderByNumber(num)

dot/state/block_test.go

@@ -635,26 +635,6 @@ func TestAddBlock_WithReOrg(t *testing.T) {
 	require.Equal(t, header3a.Hash(), block3hash)
 }
 
-func TestAddBlockToBlockTree(t *testing.T) {
-	bs := newTestBlockState(t, newTriesEmpty())
-
-	header := &types.Header{
-		Number:     1,
-		Digest:     createPrimaryBABEDigest(t),
-		ParentHash: testGenesisHeader.Hash(),
-	}
-
-	err := bs.setArrivalTime(header.Hash(), time.Now())
-	require.NoError(t, err)
-
-	err = bs.AddBlockToBlockTree(&types.Block{
-		Header: *header,
-		Body:   types.Body{},
-	})
-	require.NoError(t, err)
-	require.Equal(t, bs.BestBlockHash(), header.Hash())
-}
-
 func TestNumberIsFinalised(t *testing.T) {
 	tries := newTriesEmpty()
 

dot/sync/chain_sync.go

@@ -816,33 +816,25 @@ func (cs *chainSync) handleReadyBlock(bd *types.BlockData, origin blockOrigin) e
 // processBlockData processes the BlockData from a BlockResponse and
 // returns the index of the last BlockData it handled on success,
 // or the index of the block data that errored on failure.
+// TODO: https://github.com/ChainSafe/gossamer/issues/3468
 func (cs *chainSync) processBlockData(blockData types.BlockData, origin blockOrigin) error {
 	// while in bootstrap mode we don't need to broadcast block announcements
 	announceImportedBlock := cs.getSyncMode() == tip
-	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, origin, announceImportedBlock)
+			err := cs.processBlockDataWithHeaderAndBody(blockData, origin, announceImportedBlock)
 			if err != nil {
 				return fmt.Errorf("processing block data with header and body: %w", err)
 			}
 		}
 
-		err = cs.finalizeAndSetJustification(
-			blockData.Header,
-			round, setID,
-			blockDataJustification)
-		if err != nil {
-			return fmt.Errorf("while setting justification: %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)
+			}
 		}
 	}
 
@@ -854,16 +846,6 @@ func (cs *chainSync) processBlockData(blockData types.BlockData, origin blockOri
 	return nil
 }
 
-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 0, 0, nil
-}
-
 func (cs *chainSync) processBlockDataWithHeaderAndBody(blockData types.BlockData,
 	origin blockOrigin, announceImportedBlock bool) (err error) {
 
@@ -900,27 +882,21 @@ func (cs *chainSync) handleBody(body *types.Body) {
 	blockSizeGauge.Set(float64(acc))
 }
 
-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)
-		}
-
-		logger.Debugf(
-			"finalised block with hash #%d (%s), round %d and set id %d",
-			header.Number, header.Hash(), round, setID)
+func (cs *chainSync) handleJustification(header *types.Header, justification []byte) (err error) {
+	logger.Debugf("handling justification for block %d...", header.Number)
 
-		err = cs.blockState.SetJustification(header.Hash(), justification)
-		if err != nil {
-			return fmt.Errorf("setting justification for block number %d: %w",
-				header.Number, 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.Infof("🔨 finalised block number #%d (%s)", header.Number, header.Hash())
+	err = cs.blockState.SetJustification(headerHash, justification)
+	if err != nil {
+		return fmt.Errorf("setting justification for block number %d: %w", header.Number, err)
 	}
 
+	logger.Infof("🔨 finalised block number %d with hash %s", header.Number, headerHash)
 	return nil
 }
 

dot/sync/chain_sync_test.go

@@ -1676,8 +1676,9 @@ func TestChainSync_getHighestBlock(t *testing.T) {
 		})
 	}
 }
-
 func TestChainSync_BootstrapSync_SuccessfulSync_WithInvalidJusticationBlock(t *testing.T) {
+	// TODO: https://github.com/ChainSafe/gossamer/issues/3468
+	t.Skip()
 	t.Parallel()
 
 	ctrl := gomock.NewController(t)

dot/sync/interfaces.go

@@ -29,7 +29,6 @@ type BlockState interface {
 	GetMessageQueue(common.Hash) ([]byte, error)
 	GetJustification(common.Hash) ([]byte, error)
 	SetJustification(hash common.Hash, data []byte) error
-	AddBlockToBlockTree(block *types.Block) error
 	GetHashByNumber(blockNumber uint) (common.Hash, error)
 	GetBlockByHash(common.Hash) (*types.Block, error)
 	GetRuntime(blockHash common.Hash) (runtime runtime.Instance, err error)
@@ -39,7 +38,6 @@ 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 +58,7 @@ type BabeVerifier interface {
 
 // FinalityGadget implements justification verification functionality
 type FinalityGadget interface {
-	VerifyBlockJustification(common.Hash, []byte) (round uint64, setID uint64, err error)
+	VerifyBlockJustification(common.Hash, []byte) error
 }
 
 // BlockImportHandler is the interface for the handler of newly imported blocks

dot/sync/syncer_integration_test.go

@@ -111,10 +111,9 @@ 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) (uint64, uint64, error) {
-			return 1, 1, nil
-		}).AnyTimes()
+		gomock.AssignableToTypeOf([]byte{})).DoAndReturn(func(hash common.Hash, justification []byte) error {
+		return nil
+	}).AnyTimes()
 
 	cfg.FinalityGadget = mockFinalityGadget
 	cfg.Network = NewMockNetwork(ctrl)