Skip to content

Fix: MemPool null checks #3367

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

Merged
merged 4 commits into from
Jul 1, 2024
Merged

Fix: MemPool null checks #3367

Changes from 2 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
40 changes: 23 additions & 17 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
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,14 +159,14 @@ 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)
var ret = _unsortedTransactions.TryGetValue(hash, out var item)
|| _unverifiedTransactions.TryGetValue(hash, out item);
tx = ret ? item.Tx : null;
tx = ret ? item!.Tx : null;
Copy link
Contributor

@Jim8y Jim8y Jul 1, 2024

Choose a reason for hiding this comment

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

Isn't this the same as itms?.tx?

Copy link
Member

@cschuchardt88 cschuchardt88 Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree should be item?.tx

tx = ret ? item?.Tx;

Copy link
Member Author

@shargon shargon Jul 1, 2024

Choose a reason for hiding this comment

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

It will be != null always that ret is true

Copy link
Member

@cschuchardt88 cschuchardt88 Jul 1, 2024

Choose a reason for hiding this comment

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

I meant tx = item?.tx

Copy link
Member

@cschuchardt88 cschuchardt88 Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From looking it again. It will always have the result of _unverifiedTransactions.TryGetValue(hash, out item). Unless it false depending on the TryGetValue if it sets null or not for item.

So this is a bug in MemoryPool that needs to be addressed.

Copy link
Contributor

@Jim8y Jim8y Jul 1, 2024

Choose a reason for hiding this comment

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

It will be != null always that ret is true

It should be equal to item?.tx, otherwise there are problems in TryGetValue

return ret;
}
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

@shargon shargon Jun 28, 2024

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

@cschuchardt88 cschuchardt88 Jun 30, 2024

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

@cschuchardt88 cschuchardt88 Jun 30, 2024

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