Skip to content

Commit

Permalink
Payloads: implement Conflicts attribute (#2818)
Browse files Browse the repository at this point in the history 
* Payloads: implement Conflicts attribute

Closes #1991.

* Update src/Neo/Ledger/MemoryPool.cs

* Fix conflicting tx fees accounting

Fix the comment #2818 (comment).

* Reformat code and improve variables naming

Fix Vitor's comments.

* Make comments more clear and adjust variables naming

Fix Owen's feedback.

* Fix Conflicts attribute verification

Consider the case when transaction B has the hash of transaction A
in the Conflicts attribute and transaction B is on-chain. Let the
transaction C has the hash of transaction A in the Conflicts attribute.
Then transaction C still should be able to pass verification and should
be accepted to the subsequent block, because transaction A isn't on chain
and transaction A will never be accepted.

Thanks to Owen for testing, this case is described at the
#2818 (review).
The expected behaviour in the described case is that TXID-D and TXID-E will
be successfully accepted to the mempool and to chain, because the conflicting
TXID-A is not on chain (and it's OK that the hash of TXID-A is on-chain as
the conflicting hash).

* Fix formatting, comments and add a testcase for conflicting mempool txs

* Add one more testcase for conflicting transactions

Testcase provided by Vitor in #2818 (comment).

* Implement economic adjustments for Conflicts attribute

Transaction that conflicts with mempooled transactions have to pay
larger network fee than the sum of all conflicting transactions in
the pool that have the same sender as the newcomer.

Port the nspcc-dev/neo-go#3031.

* Remove Trimmed value

* Check signers of on-chained conflict during new tx verification

Fix the problem described in
#2818 (review).

During new transaction verification if there's an on-chain conflicting
transaction, we should check the signers of this conflicting transaction.
If the signers contain payer 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 in this case.

* Properly handle duplicating Conflicts hashes from the persisted transactions

* Store signers of all conflicting on-chain transactions with the same Conflicts attribute

Store not only signers of the latest conflicting on-chain transaction, but
signers of all conflicting transactions with the same attribute.

* MemoryPool: consider signers intersection on Conflicts check

This patch should be a part of fd1748d, but
was accidentally skipped. This patch matches exactly the NeoGo behaviour that
was added in nspcc-dev/neo-go#3061 and that was
discussed earlier in #2818 (comment).

Fix the issue described in #2818 (comment).

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

* MemoryPool: add test for on-chain conflict

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

* Clean using

* Refactor Malicious_OnChain_Conflict test

Move it to the UT_Blockchain.cs file and refactor it a bit to make it
actually pass as expected.

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

* Use Distinct to filter out duplicating conflicting on-chain signers

Co-authored-by: Shargon <[email protected]>

* Use HashSet as a container of conflicting hashes in the mempool

Fix #2818 (comment).

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

---------

Signed-off-by: Anna Shaleva <[email protected]>
Co-authored-by: Shargon <[email protected]>
Co-authored-by: Vitor Nazário Coelho <[email protected]>
Co-authored-by: Jimmy <[email protected]>
  • Loading branch information
@AnnaShaleva @shargon
@vncoelho @Jim8y
4 people authored Sep 4, 2023
1 parent 5210728 commit 6398801
Show file tree
Hide file tree
Showing 18 changed files with 807 additions and 51 deletions.
8 changes: 6 additions & 2 deletions src/Neo/Ledger/Blockchain.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@ private void OnFillMemoryPool(IEnumerable<Transaction> transactions)
{
if (NativeContract.Ledger.ContainsTransaction(snapshot, tx.Hash))
continue;
if (NativeContract.Ledger.ContainsConflictHash(snapshot, tx.Hash, tx.Signers.Select(s => s.Account)))
continue;
// First remove the tx if it is unverified in the pool.
system.MemPool.TryRemoveUnVerified(tx.Hash, out _);
// Add to the memory pool
Expand Down Expand Up @@ -337,6 +339,7 @@ private VerifyResult OnNewExtensiblePayload(ExtensiblePayload payload)
private VerifyResult OnNewTransaction(Transaction transaction)
{
if (system.ContainsTransaction(transaction.Hash)) return VerifyResult.AlreadyExists;
if (system.ContainsConflictHash(transaction.Hash, transaction.Signers.Select(s => s.Account))) return VerifyResult.HasConflicts;
return system.MemPool.TryAdd(transaction, system.StoreView);
}

Expand Down Expand Up @@ -391,8 +394,9 @@ private void OnTransaction(Transaction tx)
{
if (system.ContainsTransaction(tx.Hash))
SendRelayResult(tx, VerifyResult.AlreadyExists);
else
system.TxRouter.Forward(new TransactionRouter.Preverify(tx, true));
else if (system.ContainsConflictHash(tx.Hash, tx.Signers.Select(s => s.Account)))
SendRelayResult(tx, VerifyResult.HasConflicts);
else system.TxRouter.Forward(new TransactionRouter.Preverify(tx, true));
}

private void Persist(Block block)
Expand Down
169 changes: 155 additions & 14 deletions src/Neo/Ledger/MemoryPool.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ public class MemoryPool : IReadOnlyCollection<Transaction>
/// </summary>
private readonly Dictionary<UInt256, PoolItem> _unsortedTransactions = new();
/// <summary>
/// Store transaction hashes that conflict with verified mempooled transactions.
/// </summary>
private readonly Dictionary<UInt256, HashSet<UInt256>> _conflicts = new();
/// <summary>
/// Stores the verified sorted transactions currently in the pool.
/// </summary>
private readonly SortedSet<PoolItem> _sortedTransactions = new();
Expand Down Expand Up @@ -275,7 +279,8 @@ private PoolItem GetLowestFeeTransaction(out Dictionary<UInt256, PoolItem> unsor
}
}

// Note: this must only be called from a single thread (the Blockchain actor)
// Note: this must only be called from a single thread (the Blockchain actor) and
// doesn't take into account conflicting transactions.
internal bool CanTransactionFitInPool(Transaction tx)
{
if (Count < Capacity) return true;
Expand All @@ -293,23 +298,39 @@ internal VerifyResult TryAdd(Transaction tx, DataCache snapshot)
_txRwLock.EnterWriteLock();
try
{
VerifyResult result = tx.VerifyStateDependent(_system.Settings, snapshot, VerificationContext);
if (!CheckConflicts(tx, out List<PoolItem> conflictsToBeRemoved)) return VerifyResult.HasConflicts;
VerifyResult result = tx.VerifyStateDependent(_system.Settings, snapshot, VerificationContext, conflictsToBeRemoved.Select(c => c.Tx));
if (result != VerifyResult.Succeed) return result;

_unsortedTransactions.Add(tx.Hash, poolItem);
VerificationContext.AddTransaction(tx);
_sortedTransactions.Add(poolItem);
foreach (var conflict in conflictsToBeRemoved)
{
if (TryRemoveVerified(conflict.Tx.Hash, out var _))
VerificationContext.RemoveTransaction(conflict.Tx);
}
removedTransactions = conflictsToBeRemoved.Select(itm => itm.Tx).ToList();
foreach (var attr in tx.GetAttributes<Conflicts>())
{
if (!_conflicts.TryGetValue(attr.Hash, out var pooled))
{
pooled = new HashSet<UInt256>();
}
pooled.Add(tx.Hash);
_conflicts.AddOrSet(attr.Hash, pooled);
}

if (Count > Capacity)
removedTransactions = RemoveOverCapacity();
removedTransactions.AddRange(RemoveOverCapacity());
}
finally
{
_txRwLock.ExitWriteLock();
}

TransactionAdded?.Invoke(this, poolItem.Tx);
if (removedTransactions != null)
if (removedTransactions.Count() > 0)
TransactionRemoved?.Invoke(this, new()
{
Transactions = removedTransactions,
Expand All @@ -320,6 +341,49 @@ internal VerifyResult TryAdd(Transaction tx, DataCache snapshot)
return VerifyResult.Succeed;
}

/// <summary>
/// Checks whether there is no mismatch in Conflicts attributes between the current transaction
/// and mempooled unsorted transactions. If true, then these unsorted transactions will be added
/// into conflictsList.
/// </summary>
/// <param name="tx">The <see cref="Transaction"/>current transaction needs to be checked.</param>
/// <param name="conflictsList">The list of conflicting verified transactions that should be removed from the pool if tx fits the pool.</param>
/// <returns>True if transaction fits the pool, otherwise false.</returns>
private bool CheckConflicts(Transaction tx, out List<PoolItem> conflictsList)
{
conflictsList = new();
long conflictsFeeSum = 0;
// Step 1: check if `tx` was in Conflicts attributes of unsorted transactions.
if (_conflicts.TryGetValue(tx.Hash, out var conflicting))
{
foreach (var hash in conflicting)
{
var unsortedTx = _unsortedTransactions[hash];
if (unsortedTx.Tx.Signers.Select(s => s.Account).Contains(tx.Sender))
conflictsFeeSum += unsortedTx.Tx.NetworkFee;
conflictsList.Add(unsortedTx);
}
}
// Step 2: check if unsorted transactions were in `tx`'s Conflicts attributes.
foreach (var hash in tx.GetAttributes<Conflicts>().Select(p => p.Hash))
{
if (_unsortedTransactions.TryGetValue(hash, out PoolItem unsortedTx))
{
if (!tx.Signers.Select(p => p.Account).Intersect(unsortedTx.Tx.Signers.Select(p => p.Account)).Any()) return false;
conflictsFeeSum += unsortedTx.Tx.NetworkFee;
conflictsList.Add(unsortedTx);
}
}
// Network fee of tx have to be larger than the sum of conflicting txs network fees.
if (conflictsFeeSum != 0 && conflictsFeeSum >= tx.NetworkFee)
return false;

// Step 3: take into account sender's conflicting transactions while balance check,
// this will be done in VerifyStateDependant.

return true;
}

private List<Transaction> RemoveOverCapacity()
{
List<Transaction> removedTransactions = new();
Expand All @@ -332,7 +396,10 @@ private List<Transaction> RemoveOverCapacity()
removedTransactions.Add(minItem.Tx);

if (ReferenceEquals(sortedPool, _sortedTransactions))
{
RemoveConflictsOfVerified(minItem);
VerificationContext.RemoveTransaction(minItem.Tx);
}
} while (Count > Capacity);

return removedTransactions;
Expand All @@ -347,9 +414,27 @@ private bool TryRemoveVerified(UInt256 hash, out PoolItem item)
_unsortedTransactions.Remove(hash);
_sortedTransactions.Remove(item);

RemoveConflictsOfVerified(item);

return true;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void RemoveConflictsOfVerified(PoolItem item)
{
foreach (var h in item.Tx.GetAttributes<Conflicts>().Select(attr => attr.Hash))
{
if (_conflicts.TryGetValue(h, out HashSet<UInt256> conflicts))
{
conflicts.Remove(item.Tx.Hash);
if (conflicts.Count() == 0)
{
_conflicts.Remove(h);
}
}
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal bool TryRemoveUnVerified(UInt256 hash, out PoolItem item)
{
Expand All @@ -374,28 +459,67 @@ internal void InvalidateVerifiedTransactions()
_unsortedTransactions.Clear();
VerificationContext = new TransactionVerificationContext();
_sortedTransactions.Clear();
_conflicts.Clear();
}

// Note: this must only be called from a single thread (the Blockchain actor)
internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot)
{
var conflictingItems = new List<Transaction>();
_txRwLock.EnterWriteLock();
try
{
Dictionary<UInt256, List<UInt160>> conflicts = new Dictionary<UInt256, List<UInt160>>();
// First remove the transactions verified in the block.
// No need to modify VerificationContext as it will be reset afterwards.
foreach (Transaction tx in block.Transactions)
{
if (TryRemoveVerified(tx.Hash, out _)) continue;
TryRemoveUnVerified(tx.Hash, out _);
if (!TryRemoveVerified(tx.Hash, out _)) TryRemoveUnVerified(tx.Hash, out _);
var conflictingSigners = tx.Signers.Select(s => s.Account);
foreach (var h in tx.GetAttributes<Conflicts>().Select(a => a.Hash))
{
if (conflicts.TryGetValue(h, out var signersList))
{
signersList.AddRange(conflictingSigners);
continue;
}
signersList = conflictingSigners.ToList();
conflicts.Add(h, signersList);
}
}

// Add all the previously verified transactions back to the unverified transactions
// Then remove the transactions conflicting with the accepted ones.
// No need to modify VerificationContext as it will be reset afterwards.
var persisted = block.Transactions.Select(t => t.Hash);
var stale = new List<UInt256>();
foreach (var item in _sortedTransactions)
{
if ((conflicts.TryGetValue(item.Tx.Hash, out var signersList) && signersList.Intersect(item.Tx.Signers.Select(s => s.Account)).Any()) || item.Tx.GetAttributes<Conflicts>().Select(a => a.Hash).Intersect(persisted).Any())
{
stale.Add(item.Tx.Hash);
conflictingItems.Add(item.Tx);
}
}
foreach (var h in stale)
{
if (!TryRemoveVerified(h, out _)) TryRemoveUnVerified(h, out _);
}

// Add all the previously verified transactions back to the unverified transactions and clear mempool conflicts list.
InvalidateVerifiedTransactions();
}
finally
{
_txRwLock.ExitWriteLock();
}
if (conflictingItems.Count() > 0)
{
TransactionRemoved?.Invoke(this, new()
{
Transactions = conflictingItems.ToArray(),
Reason = TransactionRemovalReason.Conflict,
});
}

// If we know about headers of future blocks, no point in verifying transactions from the unverified tx pool
// until we get caught up.
Expand Down Expand Up @@ -431,10 +555,31 @@ private int ReverifyTransactions(SortedSet<PoolItem> verifiedSortedTxPool,
// Since unverifiedSortedTxPool is ordered in an ascending manner, we take from the end.
foreach (PoolItem item in unverifiedSortedTxPool.Reverse().Take(count))
{
if (item.Tx.VerifyStateDependent(_system.Settings, snapshot, VerificationContext) == VerifyResult.Succeed)
if (CheckConflicts(item.Tx, out List<PoolItem> conflictsToBeRemoved) &&
item.Tx.VerifyStateDependent(_system.Settings, snapshot, VerificationContext, conflictsToBeRemoved.Select(c => c.Tx)) == VerifyResult.Succeed)
{
reverifiedItems.Add(item);
VerificationContext.AddTransaction(item.Tx);
if (_unsortedTransactions.TryAdd(item.Tx.Hash, item))
{
verifiedSortedTxPool.Add(item);
foreach (var attr in item.Tx.GetAttributes<Conflicts>())
{
if (!_conflicts.TryGetValue(attr.Hash, out var pooled))
{
pooled = new HashSet<UInt256>();
}
pooled.Add(item.Tx.Hash);
_conflicts.AddOrSet(attr.Hash, pooled);
}
VerificationContext.AddTransaction(item.Tx);
foreach (var conflict in conflictsToBeRemoved)
{
if (TryRemoveVerified(conflict.Tx.Hash, out var _))
VerificationContext.RemoveTransaction(conflict.Tx);
invalidItems.Add(conflict);
}

}
}
else // Transaction no longer valid -- it will be removed from unverifiedTxPool.
invalidItems.Add(item);
Expand All @@ -450,18 +595,14 @@ private int ReverifyTransactions(SortedSet<PoolItem> verifiedSortedTxPool,
var rebroadcastCutOffTime = TimeProvider.Current.UtcNow.AddMilliseconds(-_system.Settings.MillisecondsPerBlock * blocksTillRebroadcast);
foreach (PoolItem item in reverifiedItems)
{
if (_unsortedTransactions.TryAdd(item.Tx.Hash, item))
if (_unsortedTransactions.ContainsKey(item.Tx.Hash))
{
verifiedSortedTxPool.Add(item);

if (item.LastBroadcastTimestamp < rebroadcastCutOffTime)
{
_system.LocalNode.Tell(new LocalNode.RelayDirectly { Inventory = item.Tx }, _system.Blockchain);
item.LastBroadcastTimestamp = TimeProvider.Current.UtcNow;
}
}
else
VerificationContext.RemoveTransaction(item.Tx);

_unverifiedTransactions.Remove(item.Tx.Hash);
unverifiedSortedTxPool.Remove(item);
Expand Down
9 changes: 7 additions & 2 deletions src/Neo/Ledger/TransactionRemovalReason.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,18 @@ namespace Neo.Ledger
public enum TransactionRemovalReason : byte
{
/// <summary>
/// The transaction was ejected since it was the lowest priority transaction and the memory pool capacity was exceeded.
/// The transaction was rejected since it was the lowest priority transaction and the memory pool capacity was exceeded.
/// </summary>
CapacityExceeded,

/// <summary>
/// The transaction was ejected due to failing re-validation after a block was persisted.
/// The transaction was rejected due to failing re-validation after a block was persisted.
/// </summary>
NoLongerValid,

/// <summary>
/// The transaction was rejected due to conflict with higher priority transactions with Conflicts attribute.
/// </summary>
Conflict,
}
}
10 changes: 7 additions & 3 deletions src/Neo/Ledger/TransactionVerificationContext.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Neo.Persistence;
using Neo.SmartContract.Native;
using System.Collections.Generic;
using System.Linq;
using System.Numerics;

namespace Neo.Ledger
Expand Down Expand Up @@ -50,15 +51,18 @@ public void AddTransaction(Transaction tx)
/// Determine whether the specified <see cref="Transaction"/> conflicts with other transactions.
/// </summary>
/// <param name="tx">The specified <see cref="Transaction"/>.</param>
/// <param name="conflictingTxs">The list of <see cref="Transaction"/> that conflicts with the specified one and are to be removed from the pool.</param>
/// <param name="snapshot">The snapshot used to verify the <see cref="Transaction"/>.</param>
/// <returns><see langword="true"/> if the <see cref="Transaction"/> passes the check; otherwise, <see langword="false"/>.</returns>
public bool CheckTransaction(Transaction tx, DataCache snapshot)
public bool CheckTransaction(Transaction tx, IEnumerable<Transaction> conflictingTxs, DataCache snapshot)
{
BigInteger balance = NativeContract.GAS.BalanceOf(snapshot, tx.Sender);
senderFee.TryGetValue(tx.Sender, out var totalSenderFeeFromPool);

BigInteger fee = tx.SystemFee + tx.NetworkFee + totalSenderFeeFromPool;
if (balance < fee) return false;
BigInteger expectedFee = tx.SystemFee + tx.NetworkFee + totalSenderFeeFromPool;
foreach (var conflictTx in conflictingTxs.Where(c => c.Sender.Equals(tx.Sender)))
expectedFee -= (conflictTx.NetworkFee + conflictTx.SystemFee);
if (balance < expectedFee) return false;

var oracle = tx.GetAttribute<OracleResponse>();
if (oracle != null && oracleResponses.ContainsKey(oracle.Id))
Expand Down
5 changes: 5 additions & 0 deletions src/Neo/Ledger/VerifyResult.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ public enum VerifyResult : byte
/// </summary>
PolicyFail,

/// <summary>
/// Indicates that the <see cref="Transaction"/> failed to verify because it conflicts with on-chain or mempooled transactions.
/// </summary>
HasConflicts,

/// <summary>
/// Indicates that the <see cref="IInventory"/> failed to verify due to other reasons.
/// </summary>
Expand Down
11 changes: 11 additions & 0 deletions src/Neo/NeoSystem.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -278,5 +278,16 @@ public bool ContainsTransaction(UInt256 hash)
if (MemPool.ContainsKey(hash)) return true;
return NativeContract.Ledger.ContainsTransaction(StoreView, hash);
}

/// <summary>
/// Determines whether the specified transaction conflicts with some on-chain transaction.
/// </summary>
/// <param name="hash">The hash of the transaction</param>
/// <param name="signers">The list of signer accounts of the transaction</param>
/// <returns><see langword="true"/> if the transaction conflicts with on-chain transaction; otherwise, <see langword="false"/>.</returns>
public bool ContainsConflictHash(UInt256 hash, IEnumerable<UInt160> signers)
{
return NativeContract.Ledger.ContainsConflictHash(StoreView, hash, signers);
}
}
}
Loading

0 comments on commit 6398801

Please sign in to comment.