diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index 3df35e004..308aa3be7 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -45,7 +45,7 @@ import ( // Tuning parameters. const ( - version = "0.2.11" + version = "0.2.12" // DefaultInitialGAS is the default amount of GAS emitted to the standby validators // multisignature account during native GAS contract initialization. @@ -1515,8 +1515,11 @@ func (bc *Blockchain) AddBlock(block *block.Block) error { } else { err = bc.verifyAndPoolTx(tx, mp, bc) } - if err != nil && bc.config.VerifyTransactions { - return fmt.Errorf("transaction %s failed to verify: %w", tx.Hash().StringLE(), err) + if err != nil { + if bc.config.VerifyTransactions { + return fmt.Errorf("transaction %s failed to verify: %w", tx.Hash().StringLE(), err) + } + bc.log.Warn(fmt.Sprintf("transaction %s failed to verify: %s", tx.Hash().StringLE(), err)) } } } diff --git a/pkg/core/blockchain_neotest_test.go b/pkg/core/blockchain_neotest_test.go index bec3fa91e..e40cc802d 100644 --- a/pkg/core/blockchain_neotest_test.go +++ b/pkg/core/blockchain_neotest_test.go @@ -2494,3 +2494,42 @@ func TestNativenames(t *testing.T) { require.Equal(t, cs.Manifest.Name, nativenames.All[i], i) } } + +// TestBlockchain_StoreAsTransaction_ExecutableConflict ensures that transaction conflicting with +// some on-chain block can be properly stored and doesn't break the database. +func TestBlockchain_StoreAsTransaction_ExecutableConflict(t *testing.T) { + bc, acc := chain.NewSingleWithCustomConfig(t, nil) + e := neotest.NewExecutor(t, bc, acc, acc) + genesisH := bc.GetHeaderHash(0) + currHeight := bc.BlockHeight() + + // Ensure AER can be retrieved for genesis block. + aer, err := bc.GetAppExecResults(genesisH, trigger.All) + require.NoError(t, err) + require.Equal(t, 2, len(aer)) + + tx := transaction.New([]byte{byte(opcode.PUSHT)}, 0) + tx.Nonce = 5 + tx.ValidUntilBlock = e.Chain.BlockHeight() + 1 + tx.Attributes = []transaction.Attribute{{Type: transaction.ConflictsT, Value: &transaction.Conflicts{Hash: genesisH}}} + e.SignTx(t, tx, -1, acc) + e.AddNewBlock(t, tx) + e.CheckHalt(t, tx.Hash(), stackitem.Make(true)) + + // Ensure original tx can be retrieved. + actual, actualHeight, err := bc.GetTransaction(tx.Hash()) + require.NoError(t, err) + require.Equal(t, currHeight+1, actualHeight) + require.Equal(t, tx, actual, tx) + + // Ensure conflict stub is not stored. This check doesn't give us 100% sure that + // there's no specific conflict record since GetTransaction doesn't return conflict records, + // but at least it allows to ensure that no transaction record is present. + _, _, err = bc.GetTransaction(genesisH) + require.ErrorIs(t, err, storage.ErrKeyNotFound) + + // Ensure AER still can be retrieved for genesis block. + aer, err = bc.GetAppExecResults(genesisH, trigger.All) + require.NoError(t, err) + require.Equal(t, 2, len(aer)) +} diff --git a/pkg/core/dao/dao.go b/pkg/core/dao/dao.go index 218be9123..579d35548 100644 --- a/pkg/core/dao/dao.go +++ b/pkg/core/dao/dao.go @@ -35,6 +35,11 @@ var ( ErrInternalDBInconsistency = errors.New("internal DB inconsistency") ) +// conflictRecordValueLen is the length of value of transaction conflict record. +// It consists of 1-byte [storage.ExecTransaction] prefix and 4-bytes block index +// in the LE form. +const conflictRecordValueLen = 1 + 4 + // Simple is memCached wrapper around DB, simple DAO implementation. type Simple struct { Version Version @@ -323,7 +328,7 @@ func (dao *Simple) GetTxExecResult(hash util.Uint256) (uint32, *transaction.Tran // decodeTxAndExecResult decodes transaction, its height and execution result from // the given executable bytes. It performs no executable prefix check. func decodeTxAndExecResult(buf []byte) (uint32, *transaction.Transaction, *state.AppExecResult, error) { - if len(buf) >= 6 && buf[5] == transaction.DummyVersion { + if len(buf) == conflictRecordValueLen { // conflict record stub. return 0, nil, nil, storage.ErrKeyNotFound } r := io.NewBinReaderFromBuf(buf) @@ -605,7 +610,7 @@ func (dao *Simple) DeleteHeaderHashes(since uint32, batchSize int) { } // GetTransaction returns Transaction and its height by the given hash -// if it exists in the store. It does not return dummy transactions. +// if it exists in the store. It does not return conflict record stubs. func (dao *Simple) GetTransaction(hash util.Uint256) (*transaction.Transaction, uint32, error) { key := dao.makeExecutableKey(hash) b, err := dao.Store.Get(key) @@ -619,7 +624,7 @@ func (dao *Simple) GetTransaction(hash util.Uint256) (*transaction.Transaction, // It may be a block. return nil, 0, storage.ErrKeyNotFound } - if len(b) == 1+4 { // storage.ExecTransaction + index + if len(b) == conflictRecordValueLen { // It's a conflict record stub. return nil, 0, storage.ErrKeyNotFound } @@ -699,10 +704,16 @@ func (dao *Simple) HasTransaction(hash util.Uint256, signers []transaction.Signe return nil } - if len(bytes) < 5 { // (storage.ExecTransaction + index) for conflict record + if len(bytes) < conflictRecordValueLen { // (storage.ExecTransaction + index) for conflict record return nil } - if len(bytes) != 5 { + if bytes[0] != storage.ExecTransaction { + // It's a block, thus no conflict. This path is needed since there's a transaction accepted on mainnet + // that conflicts with block. This transaction was declined by Go nodes, but accepted by C# nodes, and hence + // we need to adjust Go behaviour post-factum. Ref. #3427 and 0x289c235dcdab8be7426d05f0fbb5e86c619f81481ea136493fa95deee5dbb7cc. + return nil + } + if len(bytes) != conflictRecordValueLen { return ErrAlreadyExists // fully-qualified transaction } if len(signers) == 0 { @@ -778,6 +789,10 @@ func (dao *Simple) DeleteBlock(h util.Uint256) error { if err != nil { return fmt.Errorf("failed to retrieve conflict record stub for %s (height %d, conflict %s): %w", tx.Hash().StringLE(), b.Index, hash.StringLE(), err) } + // It might be a block since we allow transactions to have block hash in the Conflicts attribute. + if v[0] != storage.ExecTransaction { + continue + } index := binary.LittleEndian.Uint32(v[1:]) // We can check for `<=` here, but use equality comparison to be more precise // and do not touch earlier conflict records (if any). Their removal must be triggered @@ -838,8 +853,9 @@ func (dao *Simple) StoreAsCurrentBlock(block *block.Block) { dao.Store.Put(dao.mkKeyPrefix(storage.SYSCurrentBlock), buf.Bytes()) } -// StoreAsTransaction stores the given TX as DataTransaction. It also stores transactions -// the given tx has conflicts with as DataTransaction with dummy version. It can reuse the given +// StoreAsTransaction stores the given TX as DataTransaction. It also stores conflict records +// (hashes of transactions the given tx has conflicts with) as DataTransaction with value containing +// only five bytes: 1-byte [storage.ExecTransaction] executable prefix + 4-bytes-LE block index. It can reuse the given // buffer for the purpose of value serialization. func (dao *Simple) StoreAsTransaction(tx *transaction.Transaction, index uint32, aer *state.AppExecResult) error { key := dao.makeExecutableKey(tx.Hash()) @@ -857,12 +873,23 @@ func (dao *Simple) StoreAsTransaction(tx *transaction.Transaction, index uint32, val := buf.Bytes() dao.Store.Put(key, val) - val = val[:5] // storage.ExecTransaction (1 byte) + index (4 bytes) + val = val[:conflictRecordValueLen] // storage.ExecTransaction (1 byte) + index (4 bytes) attrs := tx.GetAttributes(transaction.ConflictsT) for _, attr := range attrs { // Conflict record stub. hash := attr.Value.(*transaction.Conflicts).Hash copy(key[1:], hash.BytesBE()) + + // A short path if there's a block with the matching hash. If it's there, then + // don't store the conflict record stub and conflict signers since it's a + // useless record, no transaction with the same hash is possible. + exec, err := dao.Store.Get(key) + if err == nil { + if len(exec) > 0 && exec[0] != storage.ExecTransaction { + continue + } + } + dao.Store.Put(key, val) // Conflicting signers. diff --git a/pkg/core/dao/dao_test.go b/pkg/core/dao/dao_test.go index 7149cdd17..9db364118 100644 --- a/pkg/core/dao/dao_test.go +++ b/pkg/core/dao/dao_test.go @@ -242,6 +242,53 @@ func TestStoreAsTransaction(t *testing.T) { } err = dao.StoreAsTransaction(tx2, blockIndex, aer2) require.NoError(t, err) + + // A special transaction that conflicts with genesis block. + genesis := &block.Block{ + Header: block.Header{ + Version: 0, + Timestamp: 123, + Nonce: 1, + Index: 0, + NextConsensus: util.Uint160{1, 2, 3}, + }, + } + genesisAer1 := &state.AppExecResult{ + Container: genesis.Hash(), + Execution: state.Execution{ + Trigger: trigger.OnPersist, + Events: []state.NotificationEvent{}, + Stack: []stackitem.Item{}, + }, + } + genesisAer2 := &state.AppExecResult{ + Container: genesis.Hash(), + Execution: state.Execution{ + Trigger: trigger.PostPersist, + Events: []state.NotificationEvent{}, + Stack: []stackitem.Item{}, + }, + } + require.NoError(t, dao.StoreAsBlock(genesis, genesisAer1, genesisAer2)) + tx3 := transaction.New([]byte{byte(opcode.PUSH1)}, 1) + tx3.Signers = append(tx3.Signers, transaction.Signer{Account: signer1}) + tx3.Scripts = append(tx3.Scripts, transaction.Witness{}) + tx3.Attributes = []transaction.Attribute{ + { + Type: transaction.ConflictsT, + Value: &transaction.Conflicts{Hash: genesis.Hash()}, + }, + } + hash3 := tx3.Hash() + aer3 := &state.AppExecResult{ + Container: hash3, + Execution: state.Execution{ + Trigger: trigger.Application, + Events: []state.NotificationEvent{}, + Stack: []stackitem.Item{}, + }, + } + err = dao.HasTransaction(hash1, nil, 0, 0) require.ErrorIs(t, err, ErrAlreadyExists) err = dao.HasTransaction(hash2, nil, 0, 0) @@ -280,6 +327,29 @@ func TestStoreAsTransaction(t *testing.T) { require.NoError(t, err) require.Equal(t, 1, len(gotAppExecResult)) require.Equal(t, *aer2, gotAppExecResult[0]) + + // Ensure block is not treated as transaction. + err = dao.HasTransaction(genesis.Hash(), nil, 0, 0) + require.NoError(t, err) + + // Store tx3 and ensure genesis executable record is not corrupted. + require.NoError(t, dao.StoreAsTransaction(tx3, 0, aer3)) + err = dao.HasTransaction(hash3, nil, 0, 0) + require.ErrorIs(t, err, ErrAlreadyExists) + actualAer, err := dao.GetAppExecResults(hash3, trigger.All) + require.NoError(t, err) + require.Equal(t, 1, len(actualAer)) + require.Equal(t, *aer3, actualAer[0]) + actualGenesisAer, err := dao.GetAppExecResults(genesis.Hash(), trigger.All) + require.NoError(t, err) + require.Equal(t, 2, len(actualGenesisAer)) + require.Equal(t, *genesisAer1, actualGenesisAer[0]) + require.Equal(t, *genesisAer2, actualGenesisAer[1]) + + // A special requirement for transactions that conflict with block: they should + // not produce conflict record stub, ref. #3427. + err = dao.HasTransaction(genesis.Hash(), nil, 0, 0) + require.NoError(t, err) }) } diff --git a/pkg/core/transaction/transaction.go b/pkg/core/transaction/transaction.go index a41c811a4..443e75efa 100644 --- a/pkg/core/transaction/transaction.go +++ b/pkg/core/transaction/transaction.go @@ -26,8 +26,6 @@ const ( // MaxAttributes is maximum number of attributes including signers that can be contained // within a transaction. It is set to be 16. MaxAttributes = 16 - // DummyVersion represents reserved transaction version for trimmed transactions. - DummyVersion = 255 ) // ErrInvalidWitnessNum returns when the number of witnesses does not match signers. @@ -408,7 +406,7 @@ var ( // isValid checks whether decoded/unmarshalled transaction has all fields valid. func (t *Transaction) isValid() error { - if t.Version > 0 && t.Version != DummyVersion { + if t.Version > 0 { return ErrInvalidVersion } if t.SystemFee < 0 {