diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index f23a6deadc..e22d737b3d 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,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) { diff --git a/pkg/core/blockchain_neotest_test.go b/pkg/core/blockchain_neotest_test.go index 69463f5c6d..906d68d004 100644 --- a/pkg/core/blockchain_neotest_test.go +++ b/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) diff --git a/pkg/core/dao/dao.go b/pkg/core/dao/dao.go index ae3862eee0..7580b7073f 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 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 diff --git a/pkg/core/dao/dao_test.go b/pkg/core/dao/dao_test.go index c9eea92b8f..628d84cf77 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) + 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]) }) } diff --git a/pkg/core/mempool/mem_pool.go b/pkg/core/mempool/mem_pool.go index 10512f9460..4c9e84d8e6 100644 --- a/pkg/core/mempool/mem_pool.go +++ b/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)