Validate commitment bridge transactions for finalized blocks (#1599)

blockchain/blockchain.go

@@ -91,7 +91,7 @@ type Verifier interface {
 	VerifyHeader(header *types.Header) error
 	ProcessHeaders(headers []*types.Header) error
 	GetBlockCreator(header *types.Header) (types.Address, error)
-	PreCommitState(header *types.Header, txn *state.Transition) error
+	PreCommitState(block *types.Block, txn *state.Transition) error
 }
 
 type Executor interface {
@@ -689,12 +689,7 @@ func (b *Blockchain) verifyBlock(block *types.Block) ([]*types.Receipt, error) {
 	}
 
 	// Make sure the block body data is valid
-	receipts, err := b.verifyBlockBody(block)
-	if err != nil {
-		return nil, err
-	}
-
-	return receipts, nil
+	return b.verifyBlockBody(block)
 }
 
 // verifyBlockParent makes sure that the child block is in line
@@ -836,7 +831,7 @@ func (b *Blockchain) executeBlockTransactions(block *types.Block) (*BlockResult,
 		return nil, err
 	}
 
-	if err := b.consensus.PreCommitState(header, txn); err != nil {
+	if err := b.consensus.PreCommitState(block, txn); err != nil {
 		return nil, err
 	}
 

blockchain/testing.go

@@ -225,7 +225,7 @@ func NewMockBlockchain(
 type verifyHeaderDelegate func(*types.Header) error
 type processHeadersDelegate func([]*types.Header) error
 type getBlockCreatorDelegate func(*types.Header) (types.Address, error)
-type preStateCommitDelegate func(*types.Header, *state.Transition) error
+type preStateCommitDelegate func(*types.Block, *state.Transition) error
 
 type MockVerifier struct {
 	verifyHeaderFn    verifyHeaderDelegate
@@ -270,9 +270,9 @@ func (m *MockVerifier) HookGetBlockCreator(fn getBlockCreatorDelegate) {
 	m.getBlockCreatorFn = fn
 }
 
-func (m *MockVerifier) PreCommitState(header *types.Header, txn *state.Transition) error {
+func (m *MockVerifier) PreCommitState(block *types.Block, txn *state.Transition) error {
 	if m.preStateCommitFn != nil {
-		return m.preStateCommitFn(header, txn)
+		return m.preStateCommitFn(block, txn)
 	}
 
 	return nil

consensus/consensus.go

@@ -29,7 +29,7 @@ type Consensus interface {
 	GetBlockCreator(header *types.Header) (types.Address, error)
 
 	// PreCommitState a hook to be called before finalizing state transition on inserting block
-	PreCommitState(header *types.Header, txn *state.Transition) error
+	PreCommitState(block *types.Block, txn *state.Transition) error
 
 	// GetSyncProgression retrieves the current sync progression, if any
 	GetSyncProgression() *progress.Progression

consensus/dev/dev.go

@@ -235,7 +235,7 @@ func (d *Dev) GetBlockCreator(header *types.Header) (types.Address, error) {
 }
 
 // PreCommitState a hook to be called before finalizing state transition on inserting block
-func (d *Dev) PreCommitState(_header *types.Header, _txn *state.Transition) error {
+func (d *Dev) PreCommitState(_ *types.Block, _ *state.Transition) error {
 	return nil
 }
 

consensus/dummy/dummy.go

@@ -61,7 +61,7 @@ func (d *Dummy) GetBlockCreator(header *types.Header) (types.Address, error) {
 }
 
 // PreCommitState a hook to be called before finalizing state transition on inserting block
-func (d *Dummy) PreCommitState(_header *types.Header, _txn *state.Transition) error {
+func (d *Dummy) PreCommitState(_ *types.Block, _ *state.Transition) error {
 	return nil
 }
 

consensus/ibft/consensus_backend.go

@@ -215,7 +215,9 @@ func (i *backendIBFT) buildBlock(parent *types.Header) (*types.Block, error) {
 		transition,
 	)
 
-	if err := i.PreCommitState(header, transition); err != nil {
+	// provide dummy block instance to the PreCommitState
+	// (for the IBFT consensus, it is correct to have just a header, as only it is used)
+	if err := i.PreCommitState(&types.Block{Header: header}, transition); err != nil {
 		return nil, err
 	}
 

consensus/ibft/ibft.go

@@ -490,10 +490,10 @@ func (i *backendIBFT) GetBlockCreator(header *types.Header) (types.Address, erro
 }
 
 // PreCommitState a hook to be called before finalizing state transition on inserting block
-func (i *backendIBFT) PreCommitState(header *types.Header, txn *state.Transition) error {
-	hooks := i.forkManager.GetHooks(header.Number)
+func (i *backendIBFT) PreCommitState(block *types.Block, txn *state.Transition) error {
+	hooks := i.forkManager.GetHooks(block.Number())
 
-	return hooks.PreCommitState(header, txn)
+	return hooks.PreCommitState(block.Header, txn)
 }
 
 // GetEpoch returns the current epoch

consensus/polybft/blockchain_wrapper.go

@@ -79,11 +79,7 @@ func (p *blockchainWrapper) CurrentHeader() *types.Header {
 
 // CommitBlock commits a block to the chain
 func (p *blockchainWrapper) CommitBlock(block *types.FullBlock) error {
-	if err := p.blockchain.WriteFullBlock(block, consensusSource); err != nil {
-		return err
-	}
-
-	return nil
+	return p.blockchain.WriteFullBlock(block, consensusSource)
 }
 
 // ProcessBlock builds a final block from given 'block' on top of 'parent'

consensus/polybft/consensus_runtime.go

@@ -167,7 +167,7 @@ func (c *consensusRuntime) close() {
 func (c *consensusRuntime) initStateSyncManager(logger hcf.Logger) error {
 	if c.IsBridgeEnabled() {
 		stateSenderAddr := c.config.PolyBFTConfig.Bridge.StateSenderAddr
-		stateSyncManager, err := newStateSyncManager(
+		stateSyncManager := newStateSyncManager(
 			logger.Named("state-sync-manager"),
 			c.config.State,
 			&stateSyncConfig{
@@ -182,10 +182,6 @@ func (c *consensusRuntime) initStateSyncManager(logger hcf.Logger) error {
 			},
 		)
 
-		if err != nil {
-			return err
-		}
-
 		c.stateSyncManager = stateSyncManager
 	} else {
 		c.stateSyncManager = &dummyStateSyncManager{}

consensus/polybft/fsm.go

@@ -419,46 +419,26 @@ func (f *fsm) VerifyStateTransactions(transactions []*types.Transaction) error {
 			continue
 		}
 
-		decodedStateTx, err := decodeStateTransaction(tx.Input) // used to be Data
+		decodedStateTx, err := decodeStateTransaction(tx.Input)
 		if err != nil {
 			return fmt.Errorf("unknown state transaction: tx = %v, err = %w", tx.Hash, err)
 		}
 
 		switch stateTxData := decodedStateTx.(type) {
 		case *CommitmentMessageSigned:
 			if !f.isEndOfSprint {
-				return fmt.Errorf("found commitment tx in block which should not contain it: tx = %v", tx.Hash)
+				return fmt.Errorf("found commitment tx in block which should not contain it (tx hash=%s)", tx.Hash)
 			}
 
 			if commitmentTxExists {
-				return fmt.Errorf("only one commitment tx is allowed per block: %v", tx.Hash)
+				return fmt.Errorf("only one commitment tx is allowed per block (tx hash=%s)", tx.Hash)
 			}
 
 			commitmentTxExists = true
 
-			signers, err := f.validators.Accounts().GetFilteredValidators(stateTxData.AggSignature.Bitmap)
-			if err != nil {
-				return fmt.Errorf("error for state transaction while retrieving signers: tx = %v, error = %w", tx.Hash, err)
-			}
-
-			if !f.validators.HasQuorum(signers.GetAddressesAsSet()) {
-				return fmt.Errorf("quorum size not reached for state tx: %v", tx.Hash)
-			}
-
-			aggs, err := bls.UnmarshalSignature(stateTxData.AggSignature.AggregatedSignature)
-			if err != nil {
-				return fmt.Errorf("error for state transaction while unmarshaling signature: tx = %v, error = %w", tx.Hash, err)
-			}
-
-			hash, err := stateTxData.Hash()
-			if err != nil {
+			if err = verifyBridgeCommitmentTx(tx.Hash, stateTxData, f.validators); err != nil {
 				return err
 			}
-
-			verified := aggs.VerifyAggregated(signers.GetBlsKeys(), hash.Bytes(), bls.DomainStateReceiver)
-			if !verified {
-				return fmt.Errorf("invalid signature for tx = %v", tx.Hash)
-			}
 		case *contractsapi.CommitEpochValidatorSetFn:
 			if commitEpochTxExists {
 				// if we already validated commit epoch tx,
@@ -635,6 +615,37 @@ func (f *fsm) verifyDistributeRewardsTx(distributeRewardsTx *types.Transaction)
 	return errDistributeRewardsTxNotExpected
 }
 
+// verifyBridgeCommitmentTx validates bridge commitment transaction
+func verifyBridgeCommitmentTx(txHash types.Hash,
+	commitment *CommitmentMessageSigned,
+	validators validator.ValidatorSet) error {
+	signers, err := validators.Accounts().GetFilteredValidators(commitment.AggSignature.Bitmap)
+	if err != nil {
+		return fmt.Errorf("failed to retrieve signers for state tx (%s): %w", txHash, err)
+	}
+
+	if !validators.HasQuorum(signers.GetAddressesAsSet()) {
+		return fmt.Errorf("quorum size not reached for state tx (%s)", txHash)
+	}
+
+	commitmentHash, err := commitment.Hash()
+	if err != nil {
+		return err
+	}
+
+	signature, err := bls.UnmarshalSignature(commitment.AggSignature.AggregatedSignature)
+	if err != nil {
+		return fmt.Errorf("error for state tx (%s) while unmarshaling signature: %w", txHash, err)
+	}
+
+	verified := signature.VerifyAggregated(signers.GetBlsKeys(), commitmentHash.Bytes(), bls.DomainStateReceiver)
+	if !verified {
+		return fmt.Errorf("invalid signature for state tx (%s)", txHash)
+	}
+
+	return nil
+}
+
 func validateHeaderFields(parent *types.Header, header *types.Header, blockTimeDrift uint64) error {
 	// header extra data must be higher or equal to ExtraVanity = 32 in order to be compliant with Ethereum blocks
 	if len(header.ExtraData) < ExtraVanity {

consensus/polybft/fsm_test.go

@@ -1362,8 +1362,7 @@ func TestFSM_VerifyStateTransaction_InvalidSignature(t *testing.T) {
 	txns = append(txns,
 		createStateTransactionWithData(contracts.StateReceiverContract, inputData))
 
-	err = f.VerifyStateTransactions(txns)
-	require.ErrorContains(t, err, "invalid signature for tx")
+	require.ErrorContains(t, f.VerifyStateTransactions(txns), "invalid signature for state tx")
 }
 
 func TestFSM_VerifyStateTransaction_TwoCommitmentMessages(t *testing.T) {

consensus/polybft/polybft.go

@@ -656,8 +656,41 @@ func (p *Polybft) GetBlockCreator(h *types.Header) (types.Address, error) {
 }
 
 // PreCommitState a hook to be called before finalizing state transition on inserting block
-func (p *Polybft) PreCommitState(_ *types.Header, _ *state.Transition) error {
-	// Not required
+func (p *Polybft) PreCommitState(block *types.Block, _ *state.Transition) error {
+	commitmentTxExists := false
+
+	validators, err := p.GetValidators(block.Number()-1, nil)
+	if err != nil {
+		return err
+	}
+
+	// validate commitment state transactions
+	for _, tx := range block.Transactions {
+		if tx.Type != types.StateTx {
+			continue
+		}
+
+		decodedStateTx, err := decodeStateTransaction(tx.Input)
+		if err != nil {
+			return fmt.Errorf("unknown state transaction: tx=%v, error: %w", tx.Hash, err)
+		}
+
+		if signedCommitment, ok := decodedStateTx.(*CommitmentMessageSigned); ok {
+			if commitmentTxExists {
+				return fmt.Errorf("only one commitment state tx is allowed per block: %v", tx.Hash)
+			}
+
+			commitmentTxExists = true
+
+			if err := verifyBridgeCommitmentTx(
+				tx.Hash,
+				signedCommitment,
+				validator.NewValidatorSet(validators, p.logger)); err != nil {
+				return err
+			}
+		}
+	}
+
 	return nil
 }
 

consensus/polybft/state_sync_manager.go

@@ -90,15 +90,13 @@ type topic interface {
 }
 
 // newStateSyncManager creates a new instance of state sync manager
-func newStateSyncManager(logger hclog.Logger, state *State, config *stateSyncConfig) (*stateSyncManager, error) {
-	s := &stateSyncManager{
+func newStateSyncManager(logger hclog.Logger, state *State, config *stateSyncConfig) *stateSyncManager {
+	return &stateSyncManager{
 		logger:  logger,
 		state:   state,
 		config:  config,
 		closeCh: make(chan struct{}),
 	}
-
-	return s, nil
 }
 
 // Init subscribes to bridge topics (getting votes) and start the event tracker routine

consensus/polybft/state_sync_manager_test.go

@@ -33,7 +33,7 @@ func newTestStateSyncManager(t *testing.T, key *validator.TestValidator) *stateS
 
 	topic := &mockTopic{}
 
-	s, err := newStateSyncManager(hclog.NewNullLogger(), state,
+	s := newStateSyncManager(hclog.NewNullLogger(), state,
 		&stateSyncConfig{
 			stateSenderAddr:   types.Address{},
 			jsonrpcAddr:       "",
@@ -43,8 +43,6 @@ func newTestStateSyncManager(t *testing.T, key *validator.TestValidator) *stateS
 			maxCommitmentSize: maxCommitmentSize,
 		})
 
-	require.NoError(t, err)
-
 	t.Cleanup(func() {
 		os.RemoveAll(tmpDir)
 	})