Limit invalid tx size - #1090
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master-n3 #1090 +/- ##
=============================================
+ Coverage 50.94% 50.96% +0.01%
=============================================
Files 284 284
Lines 16658 16663 +5
Branches 2137 2137
=============================================
+ Hits 8487 8492 +5
Misses 7605 7605
Partials 566 566 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
roman-khimov
left a comment
roman-khimov
left a comment
There was a problem hiding this comment.
Otherwise the limit is OK.
| @@ -221,10 +221,10 @@ private void OnChangeViewReceived(ExtensiblePayload payload, ChangeView message) | |||
| foreach (UInt256 hash in message.RejectedHashes) | |||
There was a problem hiding this comment.
One of the problems of this mechanism in general is that this set is completely disconnected from the proposal. Given that MemoryPoolMaxTransactions is usually less than ushort.MaxValue malicious node can subvert the mechanism completely by sending a full set of random hashes that will wipe out any existing ones.
My suggestion is to revert #984. Another option could be filtering hashes through PrepareRequest set of hashes if it's received and ignoring it completely if it's not.
|
Lgtm @shargon , |
Merge this to fix the issue, and discuss the revert later? |
|
This is still relevant:
We need to either fix it by filtering through the proposal or just drop the mechanism completely. I'm ok both ways. |
| hashset.Value.Add(pubkey); | ||
| else | ||
| context.InvalidTransactions.Add(hash, [pubkey]); | ||
| context.InvalidTransactions.Add(new ConsensusContext.UnvalidTxCacheItem(hash, new HashSet<ECPoint> { pubkey })); |
There was a problem hiding this comment.
[bug] Every hash in message.RejectedHashes is inserted into InvalidTransactions with no check that it is in the mempool or the current PrepareRequest. ChangeView.Deserialize accepts up to ushort.MaxValue hashes, which is larger than the new capacity (MemoryPoolMaxTransactions, default 50_000). FIFOCache evicts the oldest key on Add once full (Cache.AddInternal), and FIFO OnAccess is a no-op, so later spam stays and earlier real votes disappear. After eviction, EnsureMaxBlockLimitation (TryGet + hashset.Value.Count > F) no longer skips that tx, so the primary can include it again. Honest backups still reject it (AddTransaction → RequestChangeView), producing repeated view changes — the stall #984 was meant to stop. One Byzantine validator can do this with a single TxInvalid/TxRejectedByPolicy ChangeView (NewViewNumber only has to be higher than that validator's last CV). Safety (commit/prepare of an invalid tx) is unchanged; the bounded cache is what makes wipe-by-eviction possible (the old Dictionary grew instead of dropping votes). LRU would not save a 50k+ flood of new keys either.
Suggestion: Do not cache hashes that are not in the mempool and/or the current proposal (ignore RejectedHashes entirely if no PrepareRequest is in hand). Also cap accepted RejectedHashes length to what honest nodes send (one hash per CV). Capacity can stay at MemoryPoolMaxTransactions once admission is filtered, because MemPool_TransactionRemoved already Removes pool hashes.
|
|
||
| Assert.AreSame(changeViewPayload, context.ChangeViewPayloads[2]); | ||
| CollectionAssert.Contains(context.InvalidTransactions[rejectedHash].ToArray(), context.Validators[2]); | ||
| CollectionAssert.Contains(context.InvalidTransactions[rejectedHash].Value.ToArray(), context.Validators[2]); |
There was a problem hiding this comment.
[suggestion] The only test update is InvalidTransactions[rejectedHash].Value after one TxInvalid ChangeView. Nothing fills the cache to MemoryPoolMaxTransactions, asserts TryGet false after FIFO wrap, or drives EnsureMaxBlockLimitation to skip a tx once Value.Count > F. The skip path in ConsensusContext.MakePayload.cs:87-88 is therefore untested, as is the eviction behavior this PR exists to add.
Suggestion: Add a unit test that constructs InvalidCache (or a ConsensusContext) with a tiny capacity, inserts capacity+1 distinct hashes, and checks the oldest is gone and a later TryGet/EnsureMaxBlockLimitation still skips a hash with > F votes. A second case should show a spam flood of unknown hashes evicting a still-in-mempool invalid tx if admission stays unfiltered.
|
|
||
| public sealed partial class ConsensusContext : IDisposable, ISerializable | ||
| { | ||
| public record UnvalidTxCacheItem(UInt256 Key, HashSet<ECPoint> Value); |
There was a problem hiding this comment.
[nit] UnvalidTxCacheItem is a typo for Invalid. The record and InvalidCache are also public nested types on ConsensusContext, which leaks a cache implementation detail from a type that is already a large public surface.
Suggestion: Rename to InvalidTxCacheItem. Make both nested types private (or file-scoped) if callers only need InvalidTransactions lookup/add/remove.
There was a problem hiding this comment.
[nit]
UnvalidTxCacheItemis a typo for Invalid. The record andInvalidCacheare also public nested types onConsensusContext, which leaks a cache implementation detail from a type that is already a large public surface.Suggestion: Rename to
InvalidTxCacheItem. Make both nested typesprivate(or file-scoped) if callers only needInvalidTransactionslookup/add/remove.
@copilot fix it
4 participants
This pull request refactors how invalid transactions are tracked in the consensus process by introducing a custom FIFO cache and updating all related usages. The main goal is to improve memory management and provide a more structured way to handle invalid transactions. The changes affect both the core consensus logic and related tests.
Invalid transaction tracking improvements:
Dictionary<UInt256, HashSet<ECPoint>>previously used forInvalidTransactionswith a custom FIFO cache class (InvalidCache) that holdsUnvalidTxCacheItemrecords, improving memory management and eviction of old invalid entries. [1] [2] [3]InvalidTransactionsto work with the new cache structure, including accessing the.Valueproperty ofUnvalidTxCacheItemand adding new items in the correct format. [1] [2] [3]Codebase maintenance:
using Neo.IO.Caching;directive to support the new cache implementation.