core: check signers of on-chained conflict during new tx verification

During new transaction verification if there's an on-chain conflicting
transaction, we should check the signers of this conflicting transaction.
If the signers intersect with signers of the incoming transaction, then
the conflict is treated as valid and verification for new incoming
transaction should fail. Otherwise, the conflict is treated as the
malicious attack attempt and will not be taken into account;
verification for the new incoming transaction should continue.

This commint implements the scheme described at
neo-project/neo#2818 (comment),
thanks to @shargon for digging.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

pkg/core/blockchain.go

@@ -45,7 +45,7 @@ import (
 
 // Tuning parameters.
 const (
-	version = "0.2.8"
+	version = "0.2.9"
 
 	defaultInitialGAS                      = 52000000_00000000
 	defaultGCPeriod                        = 10000
@@ -2477,7 +2477,11 @@ func (bc *Blockchain) verifyAndPoolTx(t *transaction.Transaction, pool *mempool.
 		return fmt.Errorf("%w: net fee is %v, need %v", ErrTxSmallNetworkFee, t.NetworkFee, needNetworkFee)
 	}
 	// check that current tx wasn't included in the conflicts attributes of some other transaction which is already in the chain
-	if err := bc.dao.HasTransaction(t.Hash()); err != nil {
+	var signers = make(map[util.Uint160]struct{}, len(t.Signers))
+	for _, s := range t.Signers {
+		signers[s.Account] = struct{}{}
+	}
+	if err := bc.dao.HasTransaction(t.Hash(), signers); err != nil {
 		switch {
 		case errors.Is(err, dao.ErrAlreadyExists):
 			return fmt.Errorf("blockchain: %w", ErrAlreadyExists)
@@ -2578,7 +2582,9 @@ func (bc *Blockchain) verifyTxAttributes(d *dao.Simple, tx *transaction.Transact
 				return fmt.Errorf("%w: Conflicts attribute was found, but P2PSigExtensions are disabled", ErrInvalidAttribute)
 			}
 			conflicts := tx.Attributes[i].Value.(*transaction.Conflicts)
-			if err := bc.dao.HasTransaction(conflicts.Hash); errors.Is(err, dao.ErrAlreadyExists) {
+			// Only fully-qualified dao.ErrAlreadyExists error bothers us here, thus, we
+			// can safely omit the payer argument to HasTransaction call to improve performance a bit.
+			if err := bc.dao.HasTransaction(conflicts.Hash, nil); errors.Is(err, dao.ErrAlreadyExists) {
 				return fmt.Errorf("%w: conflicting transaction %s is already on chain", ErrInvalidAttribute, conflicts.Hash.StringLE())
 			}
 		case transaction.NotaryAssistedT:
@@ -2611,7 +2617,11 @@ func (bc *Blockchain) IsTxStillRelevant(t *transaction.Transaction, txpool *memp
 		return false
 	}
 	if txpool == nil {
-		if bc.dao.HasTransaction(t.Hash()) != nil {
+		var signers = make(map[util.Uint160]struct{}, len(t.Signers))
+		for _, s := range t.Signers {
+			signers[s.Account] = struct{}{}
+		}
+		if bc.dao.HasTransaction(t.Hash(), signers) != nil {
 			return false
 		}
 	} else if txpool.HasConflicts(t, bc) {

pkg/core/blockchain_neotest_test.go

@@ -1634,28 +1634,60 @@ func TestBlockchain_VerifyTx(t *testing.T) {
 			})
 			t.Run("enabled", func(t *testing.T) {
 				t.Run("dummy on-chain conflict", func(t *testing.T) {
-					tx := newTestTx(t, h, testScript)
-					require.NoError(t, accs[0].SignTx(netmode.UnitTestNet, tx))
-					conflicting := transaction.New([]byte{byte(opcode.RET)}, 1000_0000)
-					conflicting.ValidUntilBlock = bc.BlockHeight() + 1
-					conflicting.Signers = []transaction.Signer{
-						{
-							Account: validator.ScriptHash(),
-							Scopes:  transaction.CalledByEntry,
-						},
-					}
-					conflicting.Attributes = []transaction.Attribute{
-						{
-							Type: transaction.ConflictsT,
-							Value: &transaction.Conflicts{
-								Hash: tx.Hash(),
+					t.Run("on-chain conflict signed by malicious party", func(t *testing.T) {
+						tx := newTestTx(t, h, testScript)
+						require.NoError(t, accs[0].SignTx(netmode.UnitTestNet, tx))
+						conflicting := transaction.New([]byte{byte(opcode.RET)}, 1000_0000)
+						conflicting.ValidUntilBlock = bc.BlockHeight() + 1
+						conflicting.Signers = []transaction.Signer{
+							{
+								Account: validator.ScriptHash(),
+								Scopes:  transaction.CalledByEntry,
 							},
-						},
-					}
-					conflicting.NetworkFee = 1000_0000
-					require.NoError(t, validator.SignTx(netmode.UnitTestNet, conflicting))
-					e.AddNewBlock(t, conflicting)
-					require.ErrorIs(t, bc.VerifyTx(tx), core.ErrHasConflicts)
+						}
+						conflicting.Attributes = []transaction.Attribute{
+							{
+								Type: transaction.ConflictsT,
+								Value: &transaction.Conflicts{
+									Hash: tx.Hash(),
+								},
+							},
+						}
+						conflicting.NetworkFee = 1000_0000
+						require.NoError(t, validator.SignTx(netmode.UnitTestNet, conflicting))
+						e.AddNewBlock(t, conflicting)
+						// We expect `tx` to pass verification, because on-chained `conflicting` doesn't have
+						// `tx`'s payer in the signers list, thus, `confclicting` should be considered as
+						// malicious conflict.
+						require.NoError(t, bc.VerifyTx(tx))
+					})
+					t.Run("on-chain conflict signed by valid sender", func(t *testing.T) {
+						tx := newTestTx(t, h, testScript)
+						tx.Signers = []transaction.Signer{{Account: validator.ScriptHash()}}
+						require.NoError(t, validator.SignTx(netmode.UnitTestNet, tx))
+						conflicting := transaction.New([]byte{byte(opcode.RET)}, 1000_0000)
+						conflicting.ValidUntilBlock = bc.BlockHeight() + 1
+						conflicting.Signers = []transaction.Signer{
+							{
+								Account: validator.ScriptHash(),
+								Scopes:  transaction.CalledByEntry,
+							},
+						}
+						conflicting.Attributes = []transaction.Attribute{
+							{
+								Type: transaction.ConflictsT,
+								Value: &transaction.Conflicts{
+									Hash: tx.Hash(),
+								},
+							},
+						}
+						conflicting.NetworkFee = 1000_0000
+						require.NoError(t, validator.SignTx(netmode.UnitTestNet, conflicting))
+						e.AddNewBlock(t, conflicting)
+						// We expect `tx` to fail verification, because on-chained `conflicting` has
+						// `tx`'s payer as a signer.
+						require.ErrorIs(t, bc.VerifyTx(tx), core.ErrHasConflicts)
+					})
 				})
 				t.Run("attribute on-chain conflict", func(t *testing.T) {
 					tx := neoValidatorsInvoker.Invoke(t, stackitem.NewBool(true), "transfer", neoOwner, neoOwner, 1, nil)

pkg/core/dao/dao.go

@@ -684,8 +684,11 @@ func (dao *Simple) StoreHeaderHashes(hashes []util.Uint256, height uint32) error
 
 // HasTransaction returns nil if the given store does not contain the given
 // Transaction hash. It returns an error in case the transaction is in chain
-// or in the list of conflicting transactions.
-func (dao *Simple) HasTransaction(hash util.Uint256) error {
+// or in the list of conflicting transactions. If non-zero signers are specified,
+// then additional check against the conflicting transaction signers intersection
+// is held. Do not omit signers in case if it's important to check the validity
+// of a supposedly conflicting on-chain transaction.
+func (dao *Simple) HasTransaction(hash util.Uint256, signers map[util.Uint160]struct{}) error {
 	key := dao.makeExecutableKey(hash)
 	bytes, err := dao.Store.Get(key)
 	if err != nil {
@@ -695,10 +698,35 @@ func (dao *Simple) HasTransaction(hash util.Uint256) error {
 	if len(bytes) < 6 {
 		return nil
 	}
-	if bytes[5] == transaction.DummyVersion {
+	if bytes[5] != transaction.DummyVersion {
+		return ErrAlreadyExists
+	}
+	if len(signers) == 0 {
 		return ErrHasConflicts
 	}
-	return ErrAlreadyExists
+
+	var conflictTxSigners [][]util.Uint160
+	br := io.NewBinReaderFromBuf(bytes[6:])
+	for {
+		var ss []util.Uint160
+		br.ReadArray(&ss)
+		if br.Err != nil {
+			if errors.Is(br.Err, iocore.EOF) {
+				break
+			}
+			return fmt.Errorf("failed to decode conflict record: %w", err)
+		}
+		conflictTxSigners = append(conflictTxSigners, ss)
+	}
+
+	for _, ss := range conflictTxSigners {
+		for _, s := range ss {
+			if _, ok := signers[s]; ok {
+				return ErrHasConflicts
+			}
+		}
+	}
+	return nil
 }
 
 // StoreAsBlock stores given block as DataBlock. It can reuse given buffer for
@@ -805,18 +833,53 @@ func (dao *Simple) StoreAsTransaction(tx *transaction.Transaction, index uint32,
 	}
 	dao.Store.Put(key, buf.Bytes())
 	if dao.Version.P2PSigExtensions {
-		var value []byte
+		var (
+			valuePrefix []byte
+			oldSigners  []byte
+			newSigners  []byte
+		)
 		for _, attr := range tx.GetAttributes(transaction.ConflictsT) {
 			hash := attr.Value.(*transaction.Conflicts).Hash
 			copy(key[1:], hash.BytesBE())
-			if value == nil {
+
+			old, err := dao.Store.Get(key)
+			if err != nil && !errors.Is(err, storage.ErrKeyNotFound) {
+				return fmt.Errorf("failed to retrieve previous conflict record for %s: %w", hash.StringLE(), err)
+			}
+			if err == nil {
+				if len(old) <= 6 { // storage.ExecTransaction + U32LE index + transaction.DummyVersion
+					return fmt.Errorf("invalid conflict record format of length %d", len(old))
+				}
+				valuePrefix = old[:6]
+				oldSigners = old[6:]
+			}
+			if valuePrefix == nil {
 				buf.Reset()
 				buf.WriteB(storage.ExecTransaction)
 				buf.WriteU32LE(index)
 				buf.BinWriter.WriteB(transaction.DummyVersion)
-				value = buf.Bytes()
+				b := buf.Bytes()
+				valuePrefix = make([]byte, len(b))
+				copy(valuePrefix, b)
+			}
+			if newSigners == nil {
+				buf.Reset()
+				signerHashes := make([]util.Uint160, len(tx.Signers))
+				for i := range signerHashes {
+					signerHashes[i] = tx.Signers[i].Account
+				}
+				buf.WriteArray(signerHashes)
+				b := buf.Bytes()
+				newSigners = make([]byte, len(b))
+				copy(newSigners, b)
 			}
+			// QUESTION: we may consider writing signers as an array of arrays instead of raw of arrays, but
+			// currently we don't have any demand for this, and it allows to save one byte per conflict record
+			// and avoid array length changes in the first byte on new signers list appending.
+			// We can even store raw of UInt160 instead of raw of arrays of UInt160, do we need this?
+			value := append(valuePrefix, append(oldSigners, newSigners...)...)
 			dao.Store.Put(key, value)
+			oldSigners = nil
 		}
 	}
 	return nil

pkg/core/dao/dao_test.go

@@ -186,8 +186,8 @@ func TestStoreAsTransaction(t *testing.T) {
 		}
 		err := dao.StoreAsTransaction(tx, 0, aer)
 		require.NoError(t, err)
-		err = dao.HasTransaction(hash)
-		require.NotNil(t, err)
+		err = dao.HasTransaction(hash, nil)
+		require.ErrorIs(t, err, ErrAlreadyExists)
 		gotAppExecResult, err := dao.GetAppExecResults(hash, trigger.All)
 		require.NoError(t, err)
 		require.Equal(t, 1, len(gotAppExecResult))
@@ -197,34 +197,84 @@ func TestStoreAsTransaction(t *testing.T) {
 	t.Run("P2PSigExtensions on", func(t *testing.T) {
 		dao := NewSimple(storage.NewMemoryStore(), false, true)
 		conflictsH := util.Uint256{1, 2, 3}
-		tx := transaction.New([]byte{byte(opcode.PUSH1)}, 1)
-		tx.Signers = append(tx.Signers, transaction.Signer{})
-		tx.Scripts = append(tx.Scripts, transaction.Witness{})
-		tx.Attributes = []transaction.Attribute{
+		signer1 := util.Uint160{1, 2, 3}
+		signer2 := util.Uint160{4, 5, 6}
+		signer3 := util.Uint160{7, 8, 9}
+		signerMalicious := util.Uint160{10, 11, 12}
+		tx1 := transaction.New([]byte{byte(opcode.PUSH1)}, 1)
+		tx1.Signers = append(tx1.Signers, transaction.Signer{Account: signer1}, transaction.Signer{Account: signer2})
+		tx1.Scripts = append(tx1.Scripts, transaction.Witness{}, transaction.Witness{})
+		tx1.Attributes = []transaction.Attribute{
 			{
 				Type:  transaction.ConflictsT,
 				Value: &transaction.Conflicts{Hash: conflictsH},
 			},
 		}
-		hash := tx.Hash()
-		aer := &state.AppExecResult{
-			Container: hash,
+		hash1 := tx1.Hash()
+		tx2 := transaction.New([]byte{byte(opcode.PUSH1)}, 1)
+		tx2.Signers = append(tx2.Signers, transaction.Signer{Account: signer3})
+		tx2.Scripts = append(tx2.Scripts, transaction.Witness{})
+		tx2.Attributes = []transaction.Attribute{
+			{
+				Type:  transaction.ConflictsT,
+				Value: &transaction.Conflicts{Hash: conflictsH},
+			},
+		}
+		hash2 := tx2.Hash()
+		aer1 := &state.AppExecResult{
+			Container: hash1,
 			Execution: state.Execution{
 				Trigger: trigger.Application,
 				Events:  []state.NotificationEvent{},
 				Stack:   []stackitem.Item{},
 			},
 		}
-		err := dao.StoreAsTransaction(tx, 0, aer)
+		err := dao.StoreAsTransaction(tx1, 0, aer1)
+		require.NoError(t, err)
+		aer2 := &state.AppExecResult{
+			Container: hash2,
+			Execution: state.Execution{
+				Trigger: trigger.Application,
+				Events:  []state.NotificationEvent{},
+				Stack:   []stackitem.Item{},
+			},
+		}
+		err = dao.StoreAsTransaction(tx2, 0, aer2)
 		require.NoError(t, err)
-		err = dao.HasTransaction(hash)
+		err = dao.HasTransaction(hash1, nil)
+		require.ErrorIs(t, err, ErrAlreadyExists)
+		err = dao.HasTransaction(hash2, nil)
 		require.ErrorIs(t, err, ErrAlreadyExists)
-		err = dao.HasTransaction(conflictsH)
+
+		// Conflicts: unimportant payer.
+		err = dao.HasTransaction(conflictsH, nil)
 		require.ErrorIs(t, err, ErrHasConflicts)
-		gotAppExecResult, err := dao.GetAppExecResults(hash, trigger.All)
+
+		// Conflicts: payer is important, conflict isn't malicious, test signer #1.
+		err = dao.HasTransaction(conflictsH, map[util.Uint160]struct{}{signer1: {}})
+		require.ErrorIs(t, err, ErrHasConflicts)
+
+		// Conflicts: payer is important, conflict isn't malicious, test signer #2.
+		err = dao.HasTransaction(conflictsH, map[util.Uint160]struct{}{signer2: {}})
+		require.ErrorIs(t, err, ErrHasConflicts)
+
+		// Conflicts: payer is important, conflict isn't malicious, test signer #3.
+		err = dao.HasTransaction(conflictsH, map[util.Uint160]struct{}{signer3: {}})
+		require.ErrorIs(t, err, ErrHasConflicts)
+
+		// Conflicts: payer is important, conflict is malicious.
+		err = dao.HasTransaction(conflictsH, map[util.Uint160]struct{}{signerMalicious: {}})
+		require.NoError(t, err)
+
+		gotAppExecResult, err := dao.GetAppExecResults(hash1, trigger.All)
 		require.NoError(t, err)
 		require.Equal(t, 1, len(gotAppExecResult))
-		require.Equal(t, *aer, gotAppExecResult[0])
+		require.Equal(t, *aer1, gotAppExecResult[0])
+
+		gotAppExecResult, err = dao.GetAppExecResults(hash2, trigger.All)
+		require.NoError(t, err)
+		require.Equal(t, 1, len(gotAppExecResult))
+		require.Equal(t, *aer2, gotAppExecResult[0])
 	})
 }
 

pkg/core/mempool/mem_pool.go

@@ -542,8 +542,19 @@ func (mp *Pool) checkTxConflicts(tx *transaction.Transaction, fee Feer) ([]*tran
 			if !ok {
 				continue
 			}
-			if !tx.HasSigner(existingTx.Signers[mp.payerIndex].Account) {
-				return nil, fmt.Errorf("%w: not signed by the sender of conflicting transaction %s", ErrConflictsAttribute, existingTx.Hash().StringBE())
+			signers := make(map[util.Uint160]struct{}, len(existingTx.Signers))
+			for _, s := range existingTx.Signers {
+				signers[s.Account] = struct{}{}
+			}
+			var signerOK bool
+			for _, s := range tx.Signers {
+				if _, ok := signers[s.Account]; ok {
+					signerOK = true
+					break
+				}
+			}
+			if !signerOK {
+				return nil, fmt.Errorf("%w: not signed by a signer of conflicting transaction %s", ErrConflictsAttribute, existingTx.Hash().StringBE())
 			}
 			conflictingFee += existingTx.NetworkFee
 			conflictsToBeRemoved = append(conflictsToBeRemoved, existingTx)