Skip to content

Limit invalid tx size #1090

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

Open
shargon wants to merge 2 commits into
base: master-n3
Choose a base branch
from
Open

Limit invalid tx size #1090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66c5ee3
Limit invalid tx size
shargon Jul 18, 2026
0cb5aa2
Merge branch 'master-n3' into limit-invalid-tx-size
shargon Aug 26, 2026
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
4 changes: 2 additions & 2 deletions plugins/DBFTPlugin/Consensus/ConsensusContext.MakePayload.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,8 @@ internal void EnsureMaxBlockLimitation(Transaction[] txs)
// Iterate transaction until reach the size or maximum system fee
foreach (Transaction tx in txs)
{
if (InvalidTransactions.TryGetValue(tx.Hash, out var hashset))
if (hashset.Count > F) continue;
if (InvalidTransactions.TryGet(tx.Hash, out var hashset))
if (hashset.Value.Count > F) continue;

// Check if maximum block size has been already exceeded with the current selected set
blockSize += tx.Size;
Expand Down
13 changes: 12 additions & 1 deletion plugins/DBFTPlugin/Consensus/ConsensusContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Neo.Cryptography.ECC;
using Neo.Extensions;
using Neo.IO;
using Neo.IO.Caching;
using Neo.Ledger;
using Neo.Network.P2P.Payloads;
using Neo.Persistence;
Expand All @@ -26,6 +27,15 @@ namespace Neo.Plugins.DBFTPlugin.Consensus;

public sealed partial class ConsensusContext : IDisposable, ISerializable
{
public record UnvalidTxCacheItem(UInt256 Key, HashSet<ECPoint> Value);

@cschuchardt88 cschuchardt88 Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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.

@shargon shargon Aug 26, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

[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.

@copilot fix it

public class InvalidCache(int maxCapacity) : FIFOCache<UInt256, UnvalidTxCacheItem>(maxCapacity)
{
protected override UInt256 GetKeyForItem(UnvalidTxCacheItem item)
{
return item.Key;
}
}

/// <summary>
/// Key for saving consensus state.
/// </summary>
Expand All @@ -50,7 +60,7 @@ public sealed partial class ConsensusContext : IDisposable, ISerializable
/// Store all verified unsorted transactions' senders' fee currently in the consensus context.
/// </summary>
public TransactionVerificationContext VerificationContext = new();
public Dictionary<UInt256, HashSet<ECPoint>> InvalidTransactions = new();
public InvalidCache InvalidTransactions;

public StoreCache Snapshot { get; private set; }
private ECPoint _myPublicKey;
Expand Down Expand Up @@ -115,6 +125,7 @@ public ConsensusContext(NeoSystem neoSystem, DbftSettings settings, ISigner sign
_signer = signer;
this.neoSystem = neoSystem;
dbftSettings = settings;
InvalidTransactions = new InvalidCache(neoSystem.Settings.MemoryPoolMaxTransactions);

if (dbftSettings.IgnoreRecoveryLogs == false)
{
Expand Down
6 changes: 3 additions & 3 deletions plugins/DBFTPlugin/Consensus/ConsensusService.OnMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,10 +221,10 @@ private void OnChangeViewReceived(ExtensiblePayload payload, ChangeView message)
foreach (UInt256 hash in message.RejectedHashes)

@roman-khimov roman-khimov Jul 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

{
ECPoint pubkey = context.Validators[message.ValidatorIndex];
if (context.InvalidTransactions.TryGetValue(hash, out var hashset))
hashset.Add(pubkey);
if (context.InvalidTransactions.TryGet(hash, out var hashset))
hashset.Value.Add(pubkey);
else
context.InvalidTransactions.Add(hash, [pubkey]);
context.InvalidTransactions.Add(new ConsensusContext.UnvalidTxCacheItem(hash, new HashSet<ECPoint> { pubkey }));

@cschuchardt88 cschuchardt88 Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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.

}
break;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/Neo.Plugins.DBFTPlugin.Tests/UT_ConsensusService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ public void TestConsensusServiceRoutesConsensusMessagesIntoContextState()
InvokeConsensusMethod(actor, "OnConsensusPayload", changeViewPayload);

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]);

@cschuchardt88 cschuchardt88 Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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.


context.TransactionHashes = null;
var commit = new Commit
Expand Down
Loading