Merge pull request #3437 from nspcc-dev/fix-conflicts

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))
 			}
 		}
 	}

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))
+}

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.

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)
 	})
 }
 

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 {