From bef9995697176bad2496744897b3e2322f94ec2b Mon Sep 17 00:00:00 2001 From: nan01ab Date: Mon, 23 Sep 2024 22:34:44 +0800 Subject: [PATCH 1/6] fix: concurrency conflict in MemPool.TryRemoveUnVerified --- src/Neo/Ledger/MemoryPool.cs | 18 ++++++++++++++++-- tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs | 19 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 8f9437641b..d96c46d242 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -444,6 +444,20 @@ private void RemoveConflictsOfVerified(PoolItem item) [MethodImpl(MethodImplOptions.AggressiveInlining)] internal bool TryRemoveUnVerified(UInt256 hash, [MaybeNullWhen(false)] out PoolItem? item) + { + _txRwLock.EnterWriteLock(); + try + { + return TryRemoveUnVerifiedLocked(hash, out item); + } + finally + { + _txRwLock.ExitWriteLock(); + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private bool TryRemoveUnVerifiedLocked(UInt256 hash, [MaybeNullWhen(false)] out PoolItem? item) { if (!_unverifiedTransactions.TryGetValue(hash, out item)) return false; @@ -481,7 +495,7 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) // No need to modify VerificationContext as it will be reset afterwards. foreach (Transaction tx in block.Transactions) { - if (!TryRemoveVerified(tx.Hash, out _)) TryRemoveUnVerified(tx.Hash, out _); + if (!TryRemoveVerified(tx.Hash, out _)) TryRemoveUnVerifiedLocked(tx.Hash, out _); var conflictingSigners = tx.Signers.Select(s => s.Account); foreach (var h in tx.GetAttributes().Select(a => a.Hash)) { @@ -509,7 +523,7 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) } foreach (var h in stale) { - if (!TryRemoveVerified(h, out _)) TryRemoveUnVerified(h, out _); + if (!TryRemoveVerified(h, out _)) TryRemoveUnVerifiedLocked(h, out _); } // Add all the previously verified transactions back to the unverified transactions and clear mempool conflicts list. diff --git a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs index ca4e08341e..3226015f18 100644 --- a/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs +++ b/tests/Neo.UnitTests/Ledger/UT_MemoryPool.cs @@ -747,6 +747,25 @@ public void TestUpdatePoolForBlockPersisted() _unit.VerifiedCount.Should().Be(0); } + [TestMethod] + public void TestTryRemoveUnVerified() + { + AddTransactions(32); + _unit.SortedTxCount.Should().Be(32); + + var txs = _unit.GetSortedVerifiedTransactions().ToArray(); + _unit.InvalidateVerifiedTransactions(); + + _unit.SortedTxCount.Should().Be(0); + + foreach (var tx in txs) + { + _unit.TryRemoveUnVerified(tx.Hash, out _); + } + + _unit.UnVerifiedCount.Should().Be(0); + } + public static StorageKey CreateStorageKey(int id, byte prefix, byte[] key = null) { byte[] buffer = GC.AllocateUninitializedArray(sizeof(byte) + (key?.Length ?? 0)); From 252d3f921a1ba64180edd5abe5324f61b7e186c7 Mon Sep 17 00:00:00 2001 From: Shargon Date: Tue, 24 Sep 2024 03:08:18 -0700 Subject: [PATCH 2/6] Remove method --- src/Neo/Ledger/MemoryPool.cs | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index d96c46d242..c2c91d5ef0 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -448,7 +448,11 @@ internal bool TryRemoveUnVerified(UInt256 hash, [MaybeNullWhen(false)] out PoolI _txRwLock.EnterWriteLock(); try { - return TryRemoveUnVerifiedLocked(hash, out item); + if (!_unverifiedTransactions.TryGetValue(hash, out item)) + return false; + + _unverifiedTransactions.Remove(hash); + _unverifiedSortedTransactions.Remove(item); } finally { @@ -456,17 +460,6 @@ internal bool TryRemoveUnVerified(UInt256 hash, [MaybeNullWhen(false)] out PoolI } } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private bool TryRemoveUnVerifiedLocked(UInt256 hash, [MaybeNullWhen(false)] out PoolItem? item) - { - if (!_unverifiedTransactions.TryGetValue(hash, out item)) - return false; - - _unverifiedTransactions.Remove(hash); - _unverifiedSortedTransactions.Remove(item); - return true; - } - [MethodImpl(MethodImplOptions.AggressiveInlining)] internal void InvalidateVerifiedTransactions() { From bb3f1dae0eebc437ed62a858e23e40b402027a18 Mon Sep 17 00:00:00 2001 From: Shargon Date: Tue, 24 Sep 2024 03:09:06 -0700 Subject: [PATCH 3/6] Update src/Neo/Ledger/MemoryPool.cs --- src/Neo/Ledger/MemoryPool.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index c2c91d5ef0..cdfd7bc579 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -453,6 +453,7 @@ internal bool TryRemoveUnVerified(UInt256 hash, [MaybeNullWhen(false)] out PoolI _unverifiedTransactions.Remove(hash); _unverifiedSortedTransactions.Remove(item); + return true; } finally { From a6eb672404a75750992b16cd4600dc736acf6187 Mon Sep 17 00:00:00 2001 From: Shargon Date: Tue, 24 Sep 2024 03:09:42 -0700 Subject: [PATCH 4/6] clean --- src/Neo/Ledger/MemoryPool.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index cdfd7bc579..70b86586d3 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -453,7 +453,7 @@ internal bool TryRemoveUnVerified(UInt256 hash, [MaybeNullWhen(false)] out PoolI _unverifiedTransactions.Remove(hash); _unverifiedSortedTransactions.Remove(item); - return true; + return true; } finally { From 7f8d87d660964c46263dfb281e9bb643f109b8a3 Mon Sep 17 00:00:00 2001 From: Shargon Date: Tue, 24 Sep 2024 03:10:26 -0700 Subject: [PATCH 5/6] Apply suggestions from code review --- src/Neo/Ledger/MemoryPool.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 70b86586d3..5d9df989fc 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -489,7 +489,7 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) // No need to modify VerificationContext as it will be reset afterwards. foreach (Transaction tx in block.Transactions) { - if (!TryRemoveVerified(tx.Hash, out _)) TryRemoveUnVerifiedLocked(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().Select(a => a.Hash)) { @@ -517,7 +517,7 @@ internal void UpdatePoolForBlockPersisted(Block block, DataCache snapshot) } foreach (var h in stale) { - if (!TryRemoveVerified(h, out _)) TryRemoveUnVerifiedLocked(h, out _); + if (!TryRemoveVerified(h, out _)) TryRemoveUnVerified(h, out _); } // Add all the previously verified transactions back to the unverified transactions and clear mempool conflicts list. From ba297f2f54fbfd271ac9b6794a770c1b629931e7 Mon Sep 17 00:00:00 2001 From: nan01ab Date: Tue, 24 Sep 2024 21:03:02 +0800 Subject: [PATCH 6/6] reformat --- src/Neo/Ledger/MemoryPool.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Neo/Ledger/MemoryPool.cs b/src/Neo/Ledger/MemoryPool.cs index 5d9df989fc..badef9d3a0 100644 --- a/src/Neo/Ledger/MemoryPool.cs +++ b/src/Neo/Ledger/MemoryPool.cs @@ -450,7 +450,7 @@ internal bool TryRemoveUnVerified(UInt256 hash, [MaybeNullWhen(false)] out PoolI { if (!_unverifiedTransactions.TryGetValue(hash, out item)) return false; - + _unverifiedTransactions.Remove(hash); _unverifiedSortedTransactions.Remove(item); return true;