*: move NVB and Conflicts attributes out of extensions

Signed-off-by: Anna Shaleva <[email protected]>

docs/node-configuration.md

@@ -354,7 +354,7 @@ protocol-related settings described in the table below.
 | MemPoolSize | `int` | `50000` | Size of the node's memory pool where transactions are stored before they are added to block. |
 | NativeActivations | `map[string][]uint32` | ContractManagement: [0]<br>StdLib: [0]<br>CryptoLib: [0]<br>LedgerContract: [0]<br>NeoToken: [0]<br>GasToken: [0]<br>PolicyContract: [0]<br>RoleManagement: [0]<br>OracleContract: [0] | The list of histories of native contracts updates. Each list item shod be presented as a known native contract name with the corresponding list of chain's heights. The contract is not active until chain reaches the first height value specified in the list. | `Notary` is supported. |
 | P2PNotaryRequestPayloadPoolSize | `int` | `1000` | Size of the node's P2P Notary request payloads memory pool where P2P Notary requests are stored before main or fallback transaction is completed and added to the chain.<br>This option is valid only if `P2PSigExtensions` are enabled. | Not supported by the C# node, thus may affect heterogeneous networks functionality. |
-| P2PSigExtensions | `bool` | `false` | Enables following additional Notary service related logic:<br>• Transaction attributes `NotValidBefore`, `Conflicts` and `NotaryAssisted`<br>• Network payload of the `P2PNotaryRequest` type<br>• Native `Notary` contract<br>• Notary node module | Not supported by the C# node, thus may affect heterogeneous networks functionality. |
+| P2PSigExtensions | `bool` | `false` | Enables following additional Notary service related logic:<br>• Transaction attribute `NotaryAssisted`<br>• Network payload of the `P2PNotaryRequest` type<br>• Native `Notary` contract<br>• Notary node module | Not supported by the C# node, thus may affect heterogeneous networks functionality. |
 | P2PStateExchangeExtensions | `bool` | `false` | Enables the following P2P MPT state data exchange logic: <br>• `StateSyncInterval` protocol setting <br>• P2P commands `GetMPTDataCMD` and `MPTDataCMD` | Not supported by the C# node, thus may affect heterogeneous networks functionality. Can be supported either on MPT-complete node (`KeepOnlyLatestState`=`false`) or on light GC-enabled node (`RemoveUntraceableBlocks=true`) in which case `KeepOnlyLatestState` setting doesn't change the behavior, an appropriate set of MPTs is always stored (see `RemoveUntraceableBlocks`). |
 | RemoveUntraceableBlocks | `bool`| `false` | Denotes whether old blocks should be removed from cache and database. If enabled, then only the last `MaxTraceableBlocks` are stored and accessible to smart contracts. Old MPT data is also deleted in accordance with `GarbageCollectionPeriod` setting. If enabled along with `P2PStateExchangeExtensions`, then old blocks and MPT states will be removed up to the second latest state synchronisation point (see `StateSyncInterval`). | This setting is deprecated in favor of the same setting in the ApplicationConfiguration and will be removed in future node versions. If both settings are used, setting any of them to true enables the function. |
 | ReservedAttributes | `bool` | `false` | Allows to have reserved attributes range for experimental or private purposes. |

pkg/core/blockchain.go

@@ -2561,9 +2561,6 @@ func (bc *Blockchain) verifyTxAttributes(d *dao.Simple, tx *transaction.Transact
 				return fmt.Errorf("%w: oracle tx has insufficient gas", ErrInvalidAttribute)
 			}
 		case transaction.NotValidBeforeT:
-			if !bc.config.P2PSigExtensions {
-				return fmt.Errorf("%w: NotValidBefore attribute was found, but P2PSigExtensions are disabled", ErrInvalidAttribute)
-			}
 			nvb := tx.Attributes[i].Value.(*transaction.NotValidBefore).Height
 			curHeight := bc.BlockHeight()
 			if isPartialTx {
@@ -2583,9 +2580,6 @@ func (bc *Blockchain) verifyTxAttributes(d *dao.Simple, tx *transaction.Transact
 				}
 			}
 		case transaction.ConflictsT:
-			if !bc.config.P2PSigExtensions {
-				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) {
 				return fmt.Errorf("%w: conflicting transaction %s is already on chain", ErrInvalidAttribute, conflicts.Hash.StringLE())

pkg/core/blockchain_neotest_test.go

@@ -1535,16 +1535,15 @@ func TestBlockchain_VerifyTx(t *testing.T) {
 				}}
 				return tx
 			}
-			t.Run("Disabled", func(t *testing.T) {
+			t.Run("Disabled", func(t *testing.T) { // check that NVB attribute is not an extension anymore.
 				bcBad, validatorBad, committeeBad := chain.NewMultiWithCustomConfig(t, func(c *config.Blockchain) {
 					c.P2PSigExtensions = false
 					c.ReservedAttributes = false
 				})
 				eBad := neotest.NewExecutor(t, bcBad, validatorBad, committeeBad)
 				tx := getNVBTx(eBad, bcBad.BlockHeight())
 				err := bcBad.VerifyTx(tx)
-				require.Error(t, err)
-				require.True(t, strings.Contains(err.Error(), "invalid attribute: NotValidBefore attribute was found, but P2PSigExtensions are disabled"))
+				require.NoError(t, err)
 			})
 			t.Run("Enabled", func(t *testing.T) {
 				t.Run("NotYetValid", func(t *testing.T) {
@@ -1621,16 +1620,15 @@ func TestBlockchain_VerifyTx(t *testing.T) {
 				}}
 				return tx
 			}
-			t.Run("disabled", func(t *testing.T) {
+			t.Run("disabled", func(t *testing.T) { // check that Conflicts attribute is not an extension anymore.
 				bcBad, validatorBad, committeeBad := chain.NewMultiWithCustomConfig(t, func(c *config.Blockchain) {
 					c.P2PSigExtensions = false
 					c.ReservedAttributes = false
 				})
 				eBad := neotest.NewExecutor(t, bcBad, validatorBad, committeeBad)
 				tx := getConflictsTx(eBad, util.Uint256{1, 2, 3})
 				err := bcBad.VerifyTx(tx)
-				require.Error(t, err)
-				require.True(t, strings.Contains(err.Error(), "invalid attribute: Conflicts attribute was found, but P2PSigExtensions are disabled"))
+				require.NoError(t, err)
 			})
 			t.Run("enabled", func(t *testing.T) {
 				t.Run("dummy on-chain conflict", func(t *testing.T) {

pkg/core/dao/dao.go

@@ -740,12 +740,10 @@ func (dao *Simple) DeleteBlock(h util.Uint256) error {
 	for _, tx := range b.Transactions {
 		copy(key[1:], tx.Hash().BytesBE())
 		dao.Store.Delete(key)
-		if dao.Version.P2PSigExtensions {
-			for _, attr := range tx.GetAttributes(transaction.ConflictsT) {
-				hash := attr.Value.(*transaction.Conflicts).Hash
-				copy(key[1:], hash.BytesBE())
-				dao.Store.Delete(key)
-			}
+		for _, attr := range tx.GetAttributes(transaction.ConflictsT) {
+			hash := attr.Value.(*transaction.Conflicts).Hash
+			copy(key[1:], hash.BytesBE())
+			dao.Store.Delete(key)
 		}
 	}
 
@@ -804,20 +802,18 @@ func (dao *Simple) StoreAsTransaction(tx *transaction.Transaction, index uint32,
 		return buf.Err
 	}
 	dao.Store.Put(key, buf.Bytes())
-	if dao.Version.P2PSigExtensions {
-		var value []byte
-		for _, attr := range tx.GetAttributes(transaction.ConflictsT) {
-			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()
-			}
-			dao.Store.Put(key, value)
+	var value []byte
+	for _, attr := range tx.GetAttributes(transaction.ConflictsT) {
+		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()
 		}
+		dao.Store.Put(key, value)
 	}
 	return nil
 }

pkg/core/mempool/feer.go

@@ -11,5 +11,4 @@ type Feer interface {
 	FeePerByte() int64
 	GetUtilityTokenBalance(util.Uint160) *big.Int
 	BlockHeight() uint32
-	P2PSigExtensionsEnabled() bool
 }

pkg/core/mempool/mem_pool.go

@@ -145,16 +145,14 @@ func (mp *Pool) HasConflicts(t *transaction.Transaction, fee Feer) bool {
 	if mp.containsKey(t.Hash()) {
 		return true
 	}
-	if fee.P2PSigExtensionsEnabled() {
-		// do not check sender's signature and fee
-		if _, ok := mp.conflicts[t.Hash()]; ok {
+	// do not check sender's signature and fee
+	if _, ok := mp.conflicts[t.Hash()]; ok {
+		return true
+	}
+	for _, attr := range t.GetAttributes(transaction.ConflictsT) {
+		if mp.containsKey(attr.Value.(*transaction.Conflicts).Hash) {
 			return true
 		}
-		for _, attr := range t.GetAttributes(transaction.ConflictsT) {
-			if mp.containsKey(attr.Value.(*transaction.Conflicts).Hash) {
-				return true
-			}
-		}
 	}
 	return false
 }
@@ -229,11 +227,9 @@ func (mp *Pool) Add(t *transaction.Transaction, fee Feer, data ...any) error {
 		mp.oracleResp[id] = t.Hash()
 	}
 
-	if fee.P2PSigExtensionsEnabled() {
-		// Remove conflicting transactions.
-		for _, conflictingTx := range conflictsToBeRemoved {
-			mp.removeInternal(conflictingTx.Hash(), fee)
-		}
+	// Remove conflicting transactions.
+	for _, conflictingTx := range conflictsToBeRemoved {
+		mp.removeInternal(conflictingTx.Hash(), fee)
 	}
 	// Insert into a sorted array (from max to min, that could also be done
 	// using sort.Sort(sort.Reverse()), but it incurs more overhead. Notice
@@ -255,9 +251,7 @@ func (mp *Pool) Add(t *transaction.Transaction, fee Feer, data ...any) error {
 		// Ditch the last one.
 		unlucky := mp.verifiedTxes[len(mp.verifiedTxes)-1]
 		delete(mp.verifiedMap, unlucky.txn.Hash())
-		if fee.P2PSigExtensionsEnabled() {
-			mp.removeConflictsOf(unlucky.txn)
-		}
+		mp.removeConflictsOf(unlucky.txn)
 		if attrs := unlucky.txn.GetAttributes(transaction.OracleResponseT); len(attrs) != 0 {
 			delete(mp.oracleResp, attrs[0].Value.(*transaction.OracleResponse).ID)
 		}
@@ -277,12 +271,10 @@ func (mp *Pool) Add(t *transaction.Transaction, fee Feer, data ...any) error {
 		mp.verifiedTxes[n] = pItem
 	}
 	mp.verifiedMap[t.Hash()] = t
-	if fee.P2PSigExtensionsEnabled() {
-		// Add conflicting hashes to the mp.conflicts list.
-		for _, attr := range t.GetAttributes(transaction.ConflictsT) {
-			hash := attr.Value.(*transaction.Conflicts).Hash
-			mp.conflicts[hash] = append(mp.conflicts[hash], t.Hash())
-		}
+	// Add conflicting hashes to the mp.conflicts list.
+	for _, attr := range t.GetAttributes(transaction.ConflictsT) {
+		hash := attr.Value.(*transaction.Conflicts).Hash
+		mp.conflicts[hash] = append(mp.conflicts[hash], t.Hash())
 	}
 	// we already checked balance in checkTxConflicts, so don't need to check again
 	mp.tryAddSendersFee(pItem.txn, fee, false)
@@ -330,10 +322,8 @@ func (mp *Pool) removeInternal(hash util.Uint256, feer Feer) {
 		senderFee := mp.fees[payer]
 		senderFee.feeSum.SubUint64(&senderFee.feeSum, uint64(tx.SystemFee+tx.NetworkFee))
 		mp.fees[payer] = senderFee
-		if feer.P2PSigExtensionsEnabled() {
-			// remove all conflicting hashes from mp.conflicts list
-			mp.removeConflictsOf(tx)
-		}
+		// remove all conflicting hashes from mp.conflicts list
+		mp.removeConflictsOf(tx)
 		if attrs := tx.GetAttributes(transaction.OracleResponseT); len(attrs) != 0 {
 			delete(mp.oracleResp, attrs[0].Value.(*transaction.OracleResponse).ID)
 		}
@@ -360,21 +350,17 @@ func (mp *Pool) RemoveStale(isOK func(*transaction.Transaction) bool, feer Feer)
 	// because items are iterated one-by-one in increasing order.
 	newVerifiedTxes := mp.verifiedTxes[:0]
 	mp.fees = make(map[util.Uint160]utilityBalanceAndFees) // it'd be nice to reuse existing map, but we can't easily clear it
-	if feer.P2PSigExtensionsEnabled() {
-		mp.conflicts = make(map[util.Uint256][]util.Uint256)
-	}
+	mp.conflicts = make(map[util.Uint256][]util.Uint256)
 	height := feer.BlockHeight()
 	var (
 		staleItems []item
 	)
 	for _, itm := range mp.verifiedTxes {
 		if isOK(itm.txn) && mp.checkPolicy(itm.txn, policyChanged) && mp.tryAddSendersFee(itm.txn, feer, true) {
 			newVerifiedTxes = append(newVerifiedTxes, itm)
-			if feer.P2PSigExtensionsEnabled() {
-				for _, attr := range itm.txn.GetAttributes(transaction.ConflictsT) {
-					hash := attr.Value.(*transaction.Conflicts).Hash
-					mp.conflicts[hash] = append(mp.conflicts[hash], itm.txn.Hash())
-				}
+			for _, attr := range itm.txn.GetAttributes(transaction.ConflictsT) {
+				hash := attr.Value.(*transaction.Conflicts).Hash
+				mp.conflicts[hash] = append(mp.conflicts[hash], itm.txn.Hash())
 			}
 			if mp.resendThreshold != 0 {
 				// item is resent at resendThreshold, 2*resendThreshold, 4*resendThreshold ...
@@ -524,42 +510,38 @@ func (mp *Pool) checkTxConflicts(tx *transaction.Transaction, fee Feer) ([]*tran
 		conflictsToBeRemoved []*transaction.Transaction
 		conflictingFee       int64
 	)
-	if fee.P2PSigExtensionsEnabled() {
-		// Step 1: check if `tx` was in attributes of mempooled transactions.
-		if conflictingHashes, ok := mp.conflicts[tx.Hash()]; ok {
-			for _, hash := range conflictingHashes {
-				existingTx := mp.verifiedMap[hash]
-				if existingTx.HasSigner(payer) {
-					conflictingFee += existingTx.NetworkFee
-				}
-				conflictsToBeRemoved = append(conflictsToBeRemoved, existingTx)
+	// Step 1: check if `tx` was in attributes of mempooled transactions.
+	if conflictingHashes, ok := mp.conflicts[tx.Hash()]; ok {
+		for _, hash := range conflictingHashes {
+			existingTx := mp.verifiedMap[hash]
+			if existingTx.HasSigner(payer) {
+				conflictingFee += existingTx.NetworkFee
 			}
-		}
-		// 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
-			}
-			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())
-			}
-			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)
+	}
+	// 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
 		}
-		// Step 3: take into account sender's conflicting transactions before balance check.
-		expectedSenderFee = actualSenderFee
-		for _, conflictingTx := range conflictsToBeRemoved {
-			if conflictingTx.Signers[mp.payerIndex].Account.Equals(payer) {
-				expectedSenderFee.feeSum.SubUint64(&expectedSenderFee.feeSum, uint64(conflictingTx.SystemFee+conflictingTx.NetworkFee))
-			}
+		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())
+		}
+		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)
+	}
+	// Step 3: take into account sender's conflicting transactions before balance check.
+	expectedSenderFee = actualSenderFee
+	for _, conflictingTx := range conflictsToBeRemoved {
+		if conflictingTx.Signers[mp.payerIndex].Account.Equals(payer) {
+			expectedSenderFee.feeSum.SubUint64(&expectedSenderFee.feeSum, uint64(conflictingTx.SystemFee+conflictingTx.NetworkFee))
 		}
-	} else {
-		expectedSenderFee = actualSenderFee
 	}
 	_, err := checkBalance(tx, expectedSenderFee)
 	return conflictsToBeRemoved, err

pkg/core/mempool/mem_pool_test.go

@@ -40,10 +40,6 @@ func (fs *FeerStub) GetUtilityTokenBalance(uint160 util.Uint160) *big.Int {
 	return big.NewInt(fs.balance)
 }
 
-func (fs *FeerStub) P2PSigExtensionsEnabled() bool {
-	return fs.p2pSigExt
-}
-
 func testMemPoolAddRemoveWithFeer(t *testing.T, fs Feer) {
 	mp := New(10, 0, false, nil)
 	tx := transaction.New([]byte{byte(opcode.PUSH1)}, 0)

pkg/network/notary_feer.go

@@ -26,11 +26,6 @@ func (f NotaryFeer) BlockHeight() uint32 {
 	return f.bc.BlockHeight()
 }
 
-// P2PSigExtensionsEnabled implements mempool.Feer interface.
-func (f NotaryFeer) P2PSigExtensionsEnabled() bool {
-	return f.bc.P2PSigExtensionsEnabled()
-}
-
 // NewNotaryFeer returns new NotaryFeer instance.
 func NewNotaryFeer(bc Ledger) NotaryFeer {
 	return NotaryFeer{

pkg/network/server_test.go

@@ -1001,7 +1001,6 @@ type feerStub struct {
 func (f feerStub) FeePerByte() int64                            { return 1 }
 func (f feerStub) GetUtilityTokenBalance(util.Uint160) *big.Int { return big.NewInt(100000000) }
 func (f feerStub) BlockHeight() uint32                          { return f.blockHeight }
-func (f feerStub) P2PSigExtensionsEnabled() bool                { return false }
 func (f feerStub) GetBaseExecFee() int64                        { return interop.DefaultBaseExecFee }
 
 func TestMemPool(t *testing.T) {

pkg/services/rpcsrv/server.go

@@ -98,6 +98,7 @@ type (
 		GetValidators() ([]*keys.PublicKey, error)
 		HeaderHeight() uint32
 		InitVerificationContext(ic *interop.Context, hash util.Uint160, witness *transaction.Witness) error
+		P2PSigExtensionsEnabled() bool
 		SubscribeForBlocks(ch chan *block.Block)
 		SubscribeForExecutions(ch chan *state.AppExecResult)
 		SubscribeForNotifications(ch chan *state.ContainedNotificationEvent)

pkg/services/rpcsrv/server_helper_test.go

@@ -173,10 +173,6 @@ func (fs *FeerStub) GetUtilityTokenBalance(acc util.Uint160) *big.Int {
 	return big.NewInt(1000000 * native.GASFactor)
 }
 
-func (fs FeerStub) P2PSigExtensionsEnabled() bool {
-	return false
-}
-
 func (fs FeerStub) GetBaseExecFee() int64 {
 	return interop.DefaultBaseExecFee
 }