Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: MemPool null checks #3367

Merged
merged 4 commits into from
Jul 1, 2024
Merged

Fix: MemPool null checks #3367

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b791c28
Fix null error
shargon Jun 28, 2024
7f98106
Add [MaybeNullWhen(false)]
shargon Jun 29, 2024
dd3ca6f
Update MemoryPool.cs
shargon Jul 1, 2024
bbe93a7
Merge branch 'master' into fix-mem-pool-null
shargon Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 25 additions & 19 deletions src/Neo/Ledger/MemoryPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@
// Redistribution and use in source and binary forms with or without
// modifications are permitted.

#nullable enable
shargon marked this conversation as resolved.
Show resolved Hide resolved
using Akka.Util.Internal;
using Neo.Network.P2P;
using Neo.Network.P2P.Payloads;
using Neo.Persistence;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;
Expand All @@ -27,8 +29,8 @@ namespace Neo.Ledger
/// </summary>
public class MemoryPool : IReadOnlyCollection<Transaction>
{
public event EventHandler<Transaction> TransactionAdded;
public event EventHandler<TransactionRemovedEventArgs> TransactionRemoved;
public event EventHandler<Transaction>? TransactionAdded;
public event EventHandler<TransactionRemovedEventArgs>? TransactionRemoved;

// Allow a reverified transaction to be rebroadcast if it has been this many block times since last broadcast.
private const int BlocksTillRebroadcast = 10;
Expand Down Expand Up @@ -157,15 +159,15 @@ public bool ContainsKey(UInt256 hash)
/// <param name="hash">The hash of the <see cref="Transaction"/> to get.</param>
/// <param name="tx">When this method returns, contains the <see cref="Transaction"/> associated with the specified hash, if the hash is found; otherwise, <see langword="null"/>.</param>
/// <returns><see langword="true"/> if the <see cref="MemoryPool"/> contains a <see cref="Transaction"/> with the specified hash; otherwise, <see langword="false"/>.</returns>
public bool TryGetValue(UInt256 hash, out Transaction tx)
public bool TryGetValue(UInt256 hash, [MaybeNullWhen(false)] out Transaction? tx)
{
_txRwLock.EnterReadLock();
try
{
bool ret = _unsortedTransactions.TryGetValue(hash, out PoolItem item)
|| _unverifiedTransactions.TryGetValue(hash, out item);
tx = ret ? item.Tx : null;
return ret;
_ = _unsortedTransactions.TryGetValue(hash, out var item)
|| _unverifiedTransactions.TryGetValue(hash, out item);
tx = item?.Tx;
return tx != null;
}
finally
{
Expand Down Expand Up @@ -247,13 +249,13 @@ public IEnumerable<Transaction> GetSortedVerifiedTransactions()
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static PoolItem GetLowestFeeTransaction(SortedSet<PoolItem> verifiedTxSorted,
SortedSet<PoolItem> unverifiedTxSorted, out SortedSet<PoolItem> sortedPool)
private static PoolItem? GetLowestFeeTransaction(SortedSet<PoolItem> verifiedTxSorted,
SortedSet<PoolItem> unverifiedTxSorted, out SortedSet<PoolItem>? sortedPool)
{
PoolItem minItem = unverifiedTxSorted.Min;
var minItem = unverifiedTxSorted.Min;
sortedPool = minItem != null ? unverifiedTxSorted : null;

PoolItem verifiedMin = verifiedTxSorted.Min;
var verifiedMin = verifiedTxSorted.Min;
if (verifiedMin == null) return minItem;

if (minItem != null && verifiedMin.CompareTo(minItem) >= 0)
Expand All @@ -265,7 +267,7 @@ private static PoolItem GetLowestFeeTransaction(SortedSet<PoolItem> verifiedTxSo
return minItem;
}

private PoolItem GetLowestFeeTransaction(out Dictionary<UInt256, PoolItem> unsortedTxPool, out SortedSet<PoolItem> sortedPool)
private PoolItem? GetLowestFeeTransaction(out Dictionary<UInt256, PoolItem> unsortedTxPool, out SortedSet<PoolItem>? sortedPool)
{
sortedPool = null;

Expand All @@ -286,7 +288,10 @@ internal bool CanTransactionFitInPool(Transaction tx)
{
if (Count < Capacity) return true;

return GetLowestFeeTransaction(out _, out _).CompareTo(tx) <= 0;
var item = GetLowestFeeTransaction(out _, out _);
if (item == null) return false;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can fail if it's null


return item.CompareTo(tx) <= 0;
}

internal VerifyResult TryAdd(Transaction tx, DataCache snapshot)
Expand All @@ -295,7 +300,7 @@ internal VerifyResult TryAdd(Transaction tx, DataCache snapshot)

if (_unsortedTransactions.ContainsKey(tx.Hash)) return VerifyResult.AlreadyInPool;

List<Transaction> removedTransactions = null;
List<Transaction>? removedTransactions = null;
_txRwLock.EnterWriteLock();
try
{
Expand Down Expand Up @@ -368,7 +373,7 @@ private bool CheckConflicts(Transaction tx, out List<PoolItem> conflictsList)
// 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 (_unsortedTransactions.TryGetValue(hash, out var unsortedTx))
{
if (!tx.Signers.Select(p => p.Account).Intersect(unsortedTx.Tx.Signers.Select(p => p.Account)).Any()) return false;
conflictsFeeSum += unsortedTx.Tx.NetworkFee;
Expand All @@ -390,7 +395,8 @@ private List<Transaction> RemoveOverCapacity()
List<Transaction> removedTransactions = new();
do
{
PoolItem minItem = GetLowestFeeTransaction(out var unsortedPool, out var sortedPool);
var minItem = GetLowestFeeTransaction(out var unsortedPool, out var sortedPool);
if (minItem == null || sortedPool == null) break;
Copy link
Member Author

@shargon shargon Jun 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can fail if it's null, because lime 258 and 261

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the closing } from line 397. Looks like your missing one

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can fail if it's null, because lime 258 and 261

It doesn't matter which ever is true 1st. Doesn't matter even if both are null.


unsortedPool.Remove(minItem.Tx.Hash);
sortedPool.Remove(minItem);
Expand All @@ -407,7 +413,7 @@ private List<Transaction> RemoveOverCapacity()
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private bool TryRemoveVerified(UInt256 hash, out PoolItem item)
private bool TryRemoveVerified(UInt256 hash, [MaybeNullWhen(false)] out PoolItem? item)
{
if (!_unsortedTransactions.TryGetValue(hash, out item))
return false;
Expand All @@ -425,7 +431,7 @@ 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))
if (_conflicts.TryGetValue(h, out var conflicts))
{
conflicts.Remove(item.Tx.Hash);
if (conflicts.Count() == 0)
Expand All @@ -437,7 +443,7 @@ private void RemoveConflictsOfVerified(PoolItem item)
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal bool TryRemoveUnVerified(UInt256 hash, out PoolItem item)
internal bool TryRemoveUnVerified(UInt256 hash, [MaybeNullWhen(false)] out PoolItem? item)
{
if (!_unverifiedTransactions.TryGetValue(hash, out item))
return false;
Expand Down
Loading