From 0a2be8996402a4fda0cdeae174c5288f48534633 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 14 Jul 2023 10:51:17 +0300 Subject: [PATCH 1/3] core: remove unused blockchain API `(*Blockchain).HasTransaction` is one of the oldest methods in our codebase, and currently it's completely unused. I also doubt that this method works as expected because it returns `true` if transaction in the mempool. Signed-off-by: Anna Shaleva --- pkg/core/blockchain.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index e6e659b7b..f23a6dead 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -2174,15 +2174,6 @@ func (bc *Blockchain) GetHeader(hash util.Uint256) (*block.Header, error) { return &block.Header, nil } -// HasTransaction returns true if the blockchain contains he given -// transaction hash. -func (bc *Blockchain) HasTransaction(hash util.Uint256) bool { - if bc.memPool.ContainsKey(hash) { - return true - } - return errors.Is(bc.dao.HasTransaction(hash), dao.ErrAlreadyExists) -} - // HasBlock returns true if the blockchain contains the given // block hash. func (bc *Blockchain) HasBlock(hash util.Uint256) bool { From 0d17273476c5363a0b8c7ff76854ad6571cdccc2 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 14 Jul 2023 11:40:44 +0300 Subject: [PATCH 2/3] core: fix formatted error on transaction verification Witnesses are not yet created by the moment we return this error, thus, it was always 0 as an actual number of witnesses in ErrInvalidWitnessNum. Signed-off-by: Anna Shaleva --- pkg/core/transaction/transaction.go | 2 +- pkg/vm/testdata/neo-vm | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/core/transaction/transaction.go b/pkg/core/transaction/transaction.go index 3c201d042..eba0c237d 100644 --- a/pkg/core/transaction/transaction.go +++ b/pkg/core/transaction/transaction.go @@ -193,7 +193,7 @@ func (t *Transaction) decodeBinaryNoSize(br *io.BinReader, buf []byte) { br.Err = errors.New("too many witnesses") return } else if int(nscripts) != len(t.Signers) { - br.Err = fmt.Errorf("%w: %d vs %d", ErrInvalidWitnessNum, len(t.Signers), len(t.Scripts)) + br.Err = fmt.Errorf("%w: %d vs %d", ErrInvalidWitnessNum, len(t.Signers), nscripts) return } t.Scripts = make([]Witness, nscripts) diff --git a/pkg/vm/testdata/neo-vm b/pkg/vm/testdata/neo-vm index 02f2c68e7..7e5996844 160000 --- a/pkg/vm/testdata/neo-vm +++ b/pkg/vm/testdata/neo-vm @@ -1 +1 @@ -Subproject commit 02f2c68e7ba2694aff88c143631e7acf158d378a +Subproject commit 7e5996844a90b514739f879bc9f873f9a34c9a67 From ee4b8f883bd04e5c0d615828e59506927aca5baf Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Fri, 14 Jul 2023 11:57:18 +0300 Subject: [PATCH 3/3] 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 https://github.com/neo-project/neo/pull/2818#issuecomment-1632972055, thanks to @shargon for digging. Signed-off-by: Anna Shaleva --- pkg/core/blockchain.go | 10 +- pkg/core/blockchain_neotest_test.go | 286 ++++++++++++++++++++++++++-- pkg/core/dao/dao.go | 84 ++++++-- pkg/core/dao/dao_test.go | 78 ++++++-- pkg/core/mempool/mem_pool.go | 32 +++- 5 files changed, 429 insertions(+), 61 deletions(-) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index f23a6dead..b5ca036dd 100644 --- a/pkg/core/blockchain.go +++ b/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,7 @@ 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 { + if err := bc.dao.HasTransaction(t.Hash(), t.Signers); err != nil { switch { case errors.Is(err, dao.ErrAlreadyExists): return fmt.Errorf("blockchain: %w", ErrAlreadyExists) @@ -2578,7 +2578,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 +2613,7 @@ func (bc *Blockchain) IsTxStillRelevant(t *transaction.Transaction, txpool *memp return false } if txpool == nil { - if bc.dao.HasTransaction(t.Hash()) != nil { + if bc.dao.HasTransaction(t.Hash(), t.Signers) != nil { return false } } else if txpool.HasConflicts(t, bc) { diff --git a/pkg/core/blockchain_neotest_test.go b/pkg/core/blockchain_neotest_test.go index 69463f5c6..c00f4b0e9 100644 --- a/pkg/core/blockchain_neotest_test.go +++ b/pkg/core/blockchain_neotest_test.go @@ -1634,28 +1634,272 @@ 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, `conflicting` should be considered as + // malicious conflict. + require.NoError(t, bc.VerifyTx(tx)) + }) + t.Run("multiple on-chain conflicts signed by malicious parties", func(t *testing.T) { + m1 := e.NewAccount(t) + m2 := e.NewAccount(t) + m3 := e.NewAccount(t) + good := e.NewAccount(t) + + // txGood doesn't conflict with anyone and signed by good signer. + txGood := newTestTx(t, good.ScriptHash(), testScript) + require.NoError(t, good.SignTx(netmode.UnitTestNet, txGood)) + + // txM1 conflicts with txGood and signed by two malicious signers. + txM1 := newTestTx(t, m1.ScriptHash(), testScript) + txM1.Signers = append(txM1.Signers, transaction.Signer{Account: m2.ScriptHash()}) + txM1.Attributes = []transaction.Attribute{ + { + Type: transaction.ConflictsT, + Value: &transaction.Conflicts{ + Hash: txGood.Hash(), + }, + }, + } + txM1.NetworkFee = 1_000_0000 + require.NoError(t, m1.SignTx(netmode.UnitTestNet, txM1)) + require.NoError(t, m2.SignTx(netmode.UnitTestNet, txM1)) + e.AddNewBlock(t, txM1) + + // txM2 conflicts with txGood and signed by one malicious signer. + txM2 := newTestTx(t, m3.ScriptHash(), testScript) + txM2.Attributes = []transaction.Attribute{ + { + Type: transaction.ConflictsT, + Value: &transaction.Conflicts{ + Hash: txGood.Hash(), + }, + }, + } + txM2.NetworkFee = 1_000_0000 + require.NoError(t, m3.SignTx(netmode.UnitTestNet, txM2)) + e.AddNewBlock(t, txM2) + + // We expect `tx` to pass verification, because on-chained `conflicting` doesn't have + // `tx`'s payer in the signers list, thus, `conflicting` should be considered as + // malicious conflict. + require.NoError(t, bc.VerifyTx(txGood)) + + // After that txGood can be added to the chain normally. + e.AddNewBlock(t, txGood) + + // And after that ErrAlreadyExist is expected on verification. + require.ErrorIs(t, bc.VerifyTx(txGood), core.ErrAlreadyExists) + }) + + t.Run("multiple on-chain conflicts signed by [valid+malicious] parties", func(t *testing.T) { + m1 := e.NewAccount(t) + m2 := e.NewAccount(t) + m3 := e.NewAccount(t) + good := e.NewAccount(t) + + // txGood doesn't conflict with anyone and signed by good signer. + txGood := newTestTx(t, good.ScriptHash(), testScript) + require.NoError(t, good.SignTx(netmode.UnitTestNet, txGood)) + + // txM1 conflicts with txGood and signed by one malicious and one good signers. + txM1 := newTestTx(t, m1.ScriptHash(), testScript) + txM1.Signers = append(txM1.Signers, transaction.Signer{Account: good.ScriptHash()}) + txM1.Attributes = []transaction.Attribute{ + { + Type: transaction.ConflictsT, + Value: &transaction.Conflicts{ + Hash: txGood.Hash(), + }, + }, + } + txM1.NetworkFee = 1_000_0000 + require.NoError(t, m1.SignTx(netmode.UnitTestNet, txM1)) + require.NoError(t, good.SignTx(netmode.UnitTestNet, txM1)) + e.AddNewBlock(t, txM1) + + // txM2 conflicts with txGood and signed by two malicious signers. + txM2 := newTestTx(t, m2.ScriptHash(), testScript) + txM2.Signers = append(txM2.Signers, transaction.Signer{Account: m3.ScriptHash()}) + txM2.Attributes = []transaction.Attribute{ + { + Type: transaction.ConflictsT, + Value: &transaction.Conflicts{ + Hash: txGood.Hash(), + }, + }, + } + txM2.NetworkFee = 1_000_0000 + require.NoError(t, m2.SignTx(netmode.UnitTestNet, txM2)) + require.NoError(t, m3.SignTx(netmode.UnitTestNet, txM2)) + e.AddNewBlock(t, txM2) + + // We expect `tx` to fail verification, because one of the on-chained `conflicting` + // transactions has common signers with `tx`, thus, `conflicting` should be + // considered as a valid conflict. + require.ErrorIs(t, bc.VerifyTx(txGood), core.ErrHasConflicts) + }) + + t.Run("multiple on-chain conflicts signed by [malicious+valid] parties", func(t *testing.T) { + m1 := e.NewAccount(t) + m2 := e.NewAccount(t) + m3 := e.NewAccount(t) + good := e.NewAccount(t) + + // txGood doesn't conflict with anyone and signed by good signer. + txGood := newTestTx(t, good.ScriptHash(), testScript) + require.NoError(t, good.SignTx(netmode.UnitTestNet, txGood)) + + // txM2 conflicts with txGood and signed by two malicious signers. + txM2 := newTestTx(t, m2.ScriptHash(), testScript) + txM2.Signers = append(txM2.Signers, transaction.Signer{Account: m3.ScriptHash()}) + txM2.Attributes = []transaction.Attribute{ + { + Type: transaction.ConflictsT, + Value: &transaction.Conflicts{ + Hash: txGood.Hash(), + }, + }, + } + txM2.NetworkFee = 1_000_0000 + require.NoError(t, m2.SignTx(netmode.UnitTestNet, txM2)) + require.NoError(t, m3.SignTx(netmode.UnitTestNet, txM2)) + e.AddNewBlock(t, txM2) + + // txM1 conflicts with txGood and signed by one malicious and one good signers. + txM1 := newTestTx(t, m1.ScriptHash(), testScript) + txM1.Signers = append(txM1.Signers, transaction.Signer{Account: good.ScriptHash()}) + txM1.Attributes = []transaction.Attribute{ + { + Type: transaction.ConflictsT, + Value: &transaction.Conflicts{ + Hash: txGood.Hash(), + }, + }, + } + txM1.NetworkFee = 1_000_0000 + require.NoError(t, m1.SignTx(netmode.UnitTestNet, txM1)) + require.NoError(t, good.SignTx(netmode.UnitTestNet, txM1)) + e.AddNewBlock(t, txM1) + + // We expect `tx` to fail verification, because one of the on-chained `conflicting` + // transactions has common signers with `tx`, thus, `conflicting` should be + // considered as a valid conflict. + require.ErrorIs(t, bc.VerifyTx(txGood), core.ErrHasConflicts) + }) + + t.Run("multiple on-chain conflicts signed by [valid + malicious + valid] parties", func(t *testing.T) { + m1 := e.NewAccount(t) + m2 := e.NewAccount(t) + m3 := e.NewAccount(t) + good := e.NewAccount(t) + + // txGood doesn't conflict with anyone and signed by good signer. + txGood := newTestTx(t, good.ScriptHash(), testScript) + require.NoError(t, good.SignTx(netmode.UnitTestNet, txGood)) + + // txM1 conflicts with txGood and signed by one malicious and one good signers. + txM1 := newTestTx(t, m1.ScriptHash(), testScript) + txM1.Signers = append(txM1.Signers, transaction.Signer{Account: good.ScriptHash()}) + txM1.Attributes = []transaction.Attribute{ + { + Type: transaction.ConflictsT, + Value: &transaction.Conflicts{ + Hash: txGood.Hash(), + }, + }, + } + txM1.NetworkFee = 1_000_0000 + require.NoError(t, m1.SignTx(netmode.UnitTestNet, txM1)) + require.NoError(t, good.SignTx(netmode.UnitTestNet, txM1)) + e.AddNewBlock(t, txM1) + + // txM2 conflicts with txGood and signed by two malicious signers. + txM2 := newTestTx(t, m2.ScriptHash(), testScript) + txM2.Signers = append(txM2.Signers, transaction.Signer{Account: m3.ScriptHash()}) + txM2.Attributes = []transaction.Attribute{ + { + Type: transaction.ConflictsT, + Value: &transaction.Conflicts{ + Hash: txGood.Hash(), + }, + }, + } + txM2.NetworkFee = 1_000_0000 + require.NoError(t, m2.SignTx(netmode.UnitTestNet, txM2)) + require.NoError(t, m3.SignTx(netmode.UnitTestNet, txM2)) + e.AddNewBlock(t, txM2) + + // txM3 conflicts with txGood and signed by one good and one malicious signers. + txM3 := newTestTx(t, good.ScriptHash(), testScript) + txM3.Signers = append(txM3.Signers, transaction.Signer{Account: m1.ScriptHash()}) + txM3.Attributes = []transaction.Attribute{ + { + Type: transaction.ConflictsT, + Value: &transaction.Conflicts{ + Hash: txGood.Hash(), + }, + }, + } + txM3.NetworkFee = 1_000_0000 + require.NoError(t, good.SignTx(netmode.UnitTestNet, txM3)) + require.NoError(t, m1.SignTx(netmode.UnitTestNet, txM3)) + e.AddNewBlock(t, txM3) + + // We expect `tx` to fail verification, because one of the on-chained `conflicting` + // transactions has common signers with `tx`, thus, `conflicting` should be + // considered as a valid conflict. + require.ErrorIs(t, bc.VerifyTx(txGood), core.ErrHasConflicts) + }) + + t.Run("on-chain conflict signed by single 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) diff --git a/pkg/core/dao/dao.go b/pkg/core/dao/dao.go index ae3862eee..b6092e1fc 100644 --- a/pkg/core/dao/dao.go +++ b/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 []transaction.Signer) error { key := dao.makeExecutableKey(hash) bytes, err := dao.Store.Get(key) if err != nil { @@ -695,10 +698,33 @@ 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 + + sMap := make(map[util.Uint160]struct{}, len(signers)) + for _, s := range signers { + sMap[s.Account] = struct{}{} + } + br := io.NewBinReaderFromBuf(bytes[6:]) + for { + var u util.Uint160 + u.DecodeBinary(br) + if br.Err != nil { + if errors.Is(br.Err, iocore.EOF) { + break + } + return fmt.Errorf("failed to decode conflict record: %w", err) + } + if _, ok := sMap[u]; ok { + return ErrHasConflicts + } + } + + return nil } // StoreAsBlock stores given block as DataBlock. It can reuse given buffer for @@ -805,18 +831,50 @@ func (dao *Simple) StoreAsTransaction(tx *transaction.Transaction, index uint32, } dao.Store.Put(key, buf.Bytes()) if dao.Version.P2PSigExtensions { - var value []byte - for _, attr := range tx.GetAttributes(transaction.ConflictsT) { + var ( + valuePrefix []byte + newSigners []byte + ) + attrs := tx.GetAttributes(transaction.ConflictsT) + for _, attr := range attrs { hash := attr.Value.(*transaction.Conflicts).Hash copy(key[1:], hash.BytesBE()) - if value == nil { - buf.Reset() - buf.WriteB(storage.ExecTransaction) - buf.WriteU32LE(index) - buf.BinWriter.WriteB(transaction.DummyVersion) - value = buf.Bytes() + + 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)) + } + } + buf.Reset() + buf.WriteBytes(old) + if len(old) == 0 { + if len(valuePrefix) != 0 { + buf.WriteBytes(valuePrefix) + } else { + buf.WriteB(storage.ExecTransaction) + buf.WriteU32LE(index) + buf.WriteB(transaction.DummyVersion) + } + } + newSignersOffset := buf.Len() + if len(newSigners) == 0 { + for _, s := range tx.Signers { + s.Account.EncodeBinary(buf.BinWriter) + } + } else { + buf.WriteBytes(newSigners) + } + val := buf.Bytes() + dao.Store.Put(key, val) + + if len(attrs) > 1 && len(valuePrefix) == 0 { + valuePrefix = slice.Copy(val[:6]) + newSigners = slice.Copy(val[newSignersOffset:]) } - dao.Store.Put(key, value) } } return nil diff --git a/pkg/core/dao/dao_test.go b/pkg/core/dao/dao_test.go index c9eea92b8..1e8b647cd 100644 --- a/pkg/core/dao/dao_test.go +++ b/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) - err = dao.HasTransaction(hash) + 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(hash1, nil) require.ErrorIs(t, err, ErrAlreadyExists) - err = dao.HasTransaction(conflictsH) + err = dao.HasTransaction(hash2, nil) + require.ErrorIs(t, err, ErrAlreadyExists) + + // 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, []transaction.Signer{{Account: signer1}}) + require.ErrorIs(t, err, ErrHasConflicts) + + // Conflicts: payer is important, conflict isn't malicious, test signer #2. + err = dao.HasTransaction(conflictsH, []transaction.Signer{{Account: signer2}}) + require.ErrorIs(t, err, ErrHasConflicts) + + // Conflicts: payer is important, conflict isn't malicious, test signer #3. + err = dao.HasTransaction(conflictsH, []transaction.Signer{{Account: signer3}}) + require.ErrorIs(t, err, ErrHasConflicts) + + // Conflicts: payer is important, conflict is malicious. + err = dao.HasTransaction(conflictsH, []transaction.Signer{{Account: 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]) }) } diff --git a/pkg/core/mempool/mem_pool.go b/pkg/core/mempool/mem_pool.go index 10512f946..2fbce047b 100644 --- a/pkg/core/mempool/mem_pool.go +++ b/pkg/core/mempool/mem_pool.go @@ -536,17 +536,31 @@ func (mp *Pool) checkTxConflicts(tx *transaction.Transaction, fee Feer) ([]*tran } } // Step 2: check if mempooled transactions were in `tx`'s attributes. - for _, attr := range tx.GetAttributes(transaction.ConflictsT) { - hash := attr.Value.(*transaction.Conflicts).Hash - existingTx, ok := mp.verifiedMap[hash] - if !ok { - continue + conflictsAttrs := tx.GetAttributes(transaction.ConflictsT) + if len(conflictsAttrs) != 0 { + txSigners := make(map[util.Uint160]struct{}, len(tx.Signers)) + for _, s := range tx.Signers { + txSigners[s.Account] = struct{}{} } - 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()) + for _, attr := range conflictsAttrs { + hash := attr.Value.(*transaction.Conflicts).Hash + existingTx, ok := mp.verifiedMap[hash] + if !ok { + continue + } + var signerOK bool + for _, s := range existingTx.Signers { + if _, ok := txSigners[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) } - conflictingFee += existingTx.NetworkFee - conflictsToBeRemoved = append(conflictsToBeRemoved, existingTx) } if conflictingFee != 0 && tx.NetworkFee <= conflictingFee { return nil, fmt.Errorf("%w: conflicting transactions have bigger or equal network fee: %d vs %d", ErrConflictsAttribute, tx.NetworkFee, conflictingFee)