feat(xdc): reject blacklisted addresses on pool admission - #12879
Conversation
The XDC blacklist was only enforced during execution, in XdcTransactionProcessor.ValidateSender. A transaction with a blacklisted sender or recipient was therefore accepted into the pool and gossiped to peers, only to be skipped by the block producer or to invalidate a block that included it. The Go reference client checks the denylist in both places: core/txpool/validation.go on pool admission (for RPC submissions and p2p-received transactions alike) and core/state_processor.go during block processing. This adds the missing mempool-level check as a new incoming tx filter, reading activation and the address set from the spec at the current head. Since the pool takes a single custom filter, a small composite runs it alongside the existing SignTransactionFilter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @ak88's task in 8m 39s —— View job Review: reject blacklisted addresses on pool admission
The change is well-shaped and the reasoning in the PR body checks out. Two findings are worth resolving before merge; the rest are minor. 7 inline comments posted. What I verified as correct
Findings
The two mediums interact, which is why #1 is more than cosmetic
But if (accepted == AcceptTxResult.Invalid)
disconnectRequest ??= new(DisconnectReason.InvalidTxReceived, "invalid tx", ...);So in that window, node A at Independently of the boundary, #2 deserves an explicit note in the PR body: this makes "peer relayed a blacklisted tx" a disconnect condition, which is defensible (these txs are permanently invalid, and Not doneI could not build or run the tests — |
| if (blockTree.Head is null) | ||
| return AcceptTxResult.Syncing; | ||
|
|
||
| IXdcReleaseSpec spec = specProvider.GetXdcSpec(blockTree.Head.Number); |
There was a problem hiding this comment.
Medium — spec is read at the head block, not the block the tx would land in.
The sibling filter in the same composite deliberately looks one block ahead (SignTransactionFilter.cs:46):
ulong headerNumber = header.Number + 1;
IXdcReleaseSpec spec = specProvider.GetXdcSpec(headerNumber);because an admitted tx executes in head + 1, not in head. Using head.Number here means that while head.Number == BlackListHFNumber - 1 a blacklisted tx is admitted and gossiped, and then rejected at execution — exactly the case this PR sets out to close.
This interacts badly with AcceptTxResult.Invalid (see the other comment): node A at head = HF-1 admits and broadcasts the tx, node B at head = HF returns Invalid and disconnects A. Two honest nodes one block apart across the boundary drop each other. Using head.Number + 1 in both filters removes the window and makes the two filters agree on which spec the pipeline is judging against.
| IXdcReleaseSpec spec = specProvider.GetXdcSpec(blockTree.Head.Number); | |
| IXdcReleaseSpec spec = specProvider.GetXdcSpec(blockTree.Head.Number + 1); |
Note BlackListedAddressFilterTests.Accept_UsesSpecOfCurrentHead asserts the current behaviour, so it needs updating alongside.
| if (IsBlackListed(spec, tx.SenderAddress)) | ||
| return AcceptTxResult.Invalid.WithMessage("Transaction sender is blacklisted"); | ||
|
|
||
| if (IsBlackListed(spec, tx.To)) | ||
| return AcceptTxResult.Invalid.WithMessage("Transaction recipient is blacklisted"); |
There was a problem hiding this comment.
Medium — AcceptTxResult.Invalid from an incoming filter disconnects the peer.
AcceptTxResult equality is Id-based (AcceptTxResult.cs:132), so Invalid.WithMessage(...) == AcceptTxResult.Invalid. For a tx received over p2p, Eth62ProtocolHandler.PrepareAndSubmitTransaction reports the result to the flood controller, which does:
if (accepted == AcceptTxResult.Invalid)
disconnectRequest ??= new(DisconnectReason.InvalidTxReceived, "invalid tx", ...);(TxFloodController.cs:85-91 — no threshold, immediate disconnect.)
So this filter turns "peer relayed a blacklisted tx" into "drop the peer". That is arguably correct — such txs are permanently invalid under consensus, and SignTransactionFilter already returns Invalid on the same path — but it is a p2p-visible behaviour change that the PR description doesn't mention, and it affects any peer running an XDPoSChain build without the pool-level check (which will keep relaying these forever). Please either acknowledge this explicitly as intended, or pick a non-Invalid result code.
| if (blockTree.Head is null) | ||
| return AcceptTxResult.Syncing; |
There was a problem hiding this comment.
Low — head source, and a dead branch in the wired configuration.
Two smaller points:
-
Every other pool filter that needs the head takes
IChainHeadInfoProviderand readsHeadNumber(seeGasLimitTxFilter,PriorityFeeTooLowFilter, and XDC's ownXdcTxGossipPolicy.cs:15).TxPool.FilterTransactionsruns under_newHeadLockspecifically so filters see a head consistent with the pool's own state;IBlockTree.Headsidesteps that.HeadNumberalso removes this null branch entirely. -
As wired,
SignTransactionFilterruns first in the composite and already returnsSyncingfor a null head, so this branch is unreachable in production. Harmless defensively, just noting it isn't the safety net it looks like.
| private static Block HeadBlock(ulong number = 100) => | ||
| Build.A.Block.WithHeader(Build.A.XdcBlockHeader().WithNumber(number).TestObject).TestObject; |
There was a problem hiding this comment.
Low — duplicated test helper.
HeadBlock is byte-identical to SignTransactionFilterTests.cs:46-47, and the CreateFilter / Accept(filter, tx) / TxFilteringState state = default shape repeats it too. AGENTS.md asks for shared helpers when parts of tests are similar:
When only parts of tests are similar (shared setup, common assertions, recurring scenarios), factor those parts into helper methods or helper types.
A small static helper under Nethermind.Xdc.Test/Helpers covering HeadBlock (and the Accept wrapper, which both files need because ref args can't be used in an expression-bodied assert) would serve both filter test classes.
| { | ||
| public AcceptTxResult Accept(Transaction tx, ref TxFilteringState state, TxHandlingOptions txHandlingOptions) | ||
| { | ||
| foreach (IIncomingTxFilter filter in filters) | ||
| { | ||
| AcceptTxResult result = filter.Accept(tx, ref state, txHandlingOptions); | ||
| if (!result) | ||
| return result; | ||
| } | ||
|
|
||
| return AcceptTxResult.Accepted; | ||
| } |
There was a problem hiding this comment.
Low — no test, and no observability on rejection.
The logic is simple and correct (first non-accepting result wins, foreach over an array so no enumerator allocation, params array allocated once at construction), but:
- There is no test for
CompositeIncomingTxFilteritself — ordering, short-circuiting so a later filter isn't consulted after a rejection, and the rejecting filter'sAcceptTxResult(including itsWithMessagetext) being propagated verbatim rather than collapsed to a bareInvalid. That last one matters for RPC error text. - Neither the composite nor
BlackListedAddressFilterlogs or bumps a metric on rejection. MostNethermind.TxPoolfilters take anILoggerandTrace-log the discard (e.g.BalanceZeroFilter,LowNonceFilter).SignTransactionFilterdoesn't either, so this is consistent — but a blacklist drop is the kind of event an operator will want to see in a log, and the only trace today comes fromEth62ProtocolHandleron the p2p path.
| - Post-`BlackListHFNumber`, transactions with a blacklisted sender or recipient are rejected during execution, | ||
| and also on pool admission by [`BlackListedAddressFilter`](TxPool/BlackListedAddressFilter.cs) so they are | ||
| never gossiped. |
There was a problem hiding this comment.
Low — also list the new filter under ### Transaction pool.
That section (README.md:435-445) is the index of pool components — SignTransactionFilter, XdcTxGossipPolicy, XdcTxFilterPipeline, XdcTransactionComparerProvider. BlackListedAddressFilter and CompositeIncomingTxFilter belong there too; a reader looking for "what filters does the XDC pool run" won't find them under "Fees and blacklist".
| new CompositeIncomingTxFilter( | ||
| new SignTransactionFilter(ctx.Resolve<ISnapshotManager>(), ctx.Resolve<IBlockTree>(), ctx.Resolve<ISpecProvider>()), | ||
| new BlackListedAddressFilter(ctx.Resolve<IBlockTree>(), ctx.Resolve<ISpecProvider>())), |
There was a problem hiding this comment.
Low — this wiring is inert in every existing test, and is a latent trap for future ones.
WrapReleaseSpec sets BlackListedAddresses (the 0x…b1Ac70x list, XdcTestBlockchain.cs:235) but never sets IsBlackListingEnabled, so it defaults to false and the new filter short-circuits to Accepted for every tx here. The wiring is therefore never actually exercised.
The flip side: any test that calls ChangeReleaseSpec to turn blacklisting on now silently changes pool admission as well as execution. SpecialTransactionsTests already does exactly that with TestItem.AddressA — which is XdcTestBlockchain's own signer (XdcTestBlockchain.cs:170) — and with blockChain.Signer.Address. Those two tests happen to be safe because they drive XdcTransactionProcessor directly and Create(5, false) builds its blocks before ChangeReleaseSpec runs, but the next test that enables blacklisting and then submits a tx will fail in a way that points at the pool rather than at its own spec tweak.
Worth an end-to-end test that flips IsBlackListingEnabled on and asserts TxPool.SubmitTx rejects a blacklisted sender — that is the actual claim of the PR, and it would also cover the composite and this registration in one go.
Note the PR body says the branch was never built or tested locally, so CI is the first check that no existing XDC test regressed on this.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses review feedback on the XDC blacklist filter: - TxPool now takes IIncomingTxFilter[] rather than a single filter, so XDC can register both of its filters without a composite wrapper. - BlackListedAddressFilter reads the head from IChainHeadInfoProvider, like every other pool filter that needs it, and gates on the spec of head + 1 — the block the transaction would land in — matching SignTransactionFilter. This also removes the null-head branch. - The rejection no longer uses AcceptTxResult.Invalid, which makes TxFloodController disconnect the relaying peer: pool admission is judged against the local head, so two honest nodes one block apart across BlackListHFNumber would drop each other. The transaction is dropped and the peer is kept. - Log the drop at debug level, and list the filter in the README's transaction pool section. - Cover pool admission end to end through TxPool.SubmitTx with blacklisting activated, which also exercises the registration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @ak88's task in 6m 40s —— View job Re-review after the follow-up commits
All seven points from my previous pass are resolved, and the two mediums are resolved in a way that's better than what I suggested: the spec is now read at Verified
Notes1. Low — the array parameter is a DI extension point you're not using yet. Because Autofac treats 2. Low — one string allocation per rejected transaction. 3. Low — the 4. Low — test brittleness. Also, minor and pre-existing: with a non- Not verifiedSame limitation as last time: |
The rejection message was interpolated per transaction on a peer-reachable path. Bake it into two statics instead, one per role, like every other pool filter's preallocated AcceptTxResult. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @ak88's task in 5m 14s —— View job Review: reject blacklisted addresses on pool admission (3rd pass)
Nothing blocking. All prior findings are resolved; both notes from the last pass that were actionable in code (the per-rejection string allocation, and the dedicated non- What I re-verified on the current head
Findings — all Low
#4, briefly. The end-to-end test still asserts on the whole pool ( One judgement call worth recording rather than re-litigating: the comment explaining why the id is Not verifiedSame as the previous two passes: |
|
|
||
| private AcceptTxResult Reject(Transaction tx, AcceptTxResult result) | ||
| { | ||
| if (_logger.IsDebug) _logger.Debug($"Skipped adding transaction {tx.ToString(" ")}, {result}."); |
There was a problem hiding this comment.
Low — Debug diverges from the Trace convention for pool discards, on a peer-reachable path.
Every built-in incoming filter logs its discard at Trace, with this exact message shape — BalanceTooLowFilter, GapNonceFilter, GasLimitTxFilter, LowNonceFilter, MalformedTxFilter, NotSupportedTxFilter, PriorityFeeTooLowFilter. This one logs at Debug.
That matters because the trigger is remote: a blacklisted tx does not produce AcceptTxResult.Invalid, so the peer isn't disconnected for it and can keep relaying up to the flood-controller threshold (100/s). Each one writes a Debug line containing a full tx.ToString(" "). On a node running at Debug — which operators do, unlike Trace — that's a remotely-driven log amplification for no added signal, since the p2p path already traces the rejection in Eth62ProtocolHandler.
If the goal is operator visibility on blacklist hits (a reasonable goal — it's rarer and more interesting than a nonce gap), a counter is the better instrument than a per-tx line: it survives an adversary and shows up in Grafana. Trace + a metric, or Trace alone, both match the surrounding code better than Debug.
| if (_logger.IsDebug) _logger.Debug($"Skipped adding transaction {tx.ToString(" ")}, {result}."); | |
| if (_logger.IsTrace) _logger.Trace($"Skipped adding transaction {tx.ToString(" ")}, {result}."); |
|
|
||
| AcceptTxResult result = Accept(filter, tx); | ||
|
|
||
| Assert.That(result, Is.EqualTo(XdcAcceptTxResult.BlackListedSender)); |
There was a problem hiding this comment.
Low — this assertion can't fail for the reason the test name implies.
AcceptTxResult.Equals compares Id only (AcceptTxResult.cs:132), and BlackListedSender / BlackListedRecipient deliberately share BlackListedAddressId = 1000. So BlackListedSender == BlackListedRecipient is true, and line 74 passes identically in both [TestCase]s — the only assertion with teeth on the role is the ToString() check on line 75.
Same at line 125: Is.EqualTo(blackListSender ? BlackListedSender : BlackListedRecipient) is exactly equivalent to Is.EqualTo(BlackListedSender), so the end-to-end test would not notice the filter returning the wrong one of the two.
Sharing the id is the right call (it's what keeps TxFloodController on the non-Invalid branch for both, and WithMessage exists precisely to make same-id/different-message results). The fix is on the test side — assert on ToString() in both places, since the message is the only thing that actually distinguishes them:
| Assert.That(result, Is.EqualTo(XdcAcceptTxResult.BlackListedSender)); | |
| Assert.That(result.ToString(), Is.EqualTo(XdcAcceptTxResult.BlackListedSender.ToString()).Or.EqualTo(XdcAcceptTxResult.BlackListedRecipient.ToString())); |
(or simply drop line 74 and keep the Does.Contain(expectedRole) check, which already covers it.)
| IComparer<Transaction> comparer, | ||
| ITxGossipPolicy? transactionsGossipPolicy = null, | ||
| IIncomingTxFilter? incomingTxFilter = null, | ||
| IIncomingTxFilter[]? incomingTxFilters = null, |
There was a problem hiding this comment.
Low — this is a source-breaking change to a public constructor; the PR is checkboxed as non-breaking.
TxPool and this constructor are public, and IIncomingTxFilter? incomingTxFilter → IIncomingTxFilter[]? incomingTxFilters breaks any out-of-tree plugin that constructs a pool with a custom filter (the in-tree call sites — AuRa, XDC, InitializeBlockchain, the benchmark, TxPoolTests — are all updated correctly, and null/omitted still compiles, so only the "I pass one filter" shape breaks). The parameter rename also breaks named-argument callers.
Worth ticking Breaking change and giving it a line in the release notes; the migration is a one-character […]. No objection to the change itself — it's the right shape, and it's what lets XDC drop CompositeIncomingTxFilter.
Two smaller notes while here:
- The XML doc for the param is still empty. Since the array's semantics aren't obvious from the name, one line — that these are appended to the built-in post-hash filters, i.e. they run after sender recovery and can rely on
tx.SenderAddress— would earn its place. That guarantee is exactly what makesBlackListedAddressFilter's sender check safe. - Because Autofac resolves
T[]implicitly, this parameter is now a container extension point: anything registeringIIncomingTxFiltergets picked up by every DI-resolvedTxPool. Nothing in the repo does today, and the two XDC/AuRa call sites construct the pool by hand, so no behaviour change — just be aware the coupling now exists.
Changes
BlackListedAddressFilter(IIncomingTxFilter), rejecting transactions whose sender or recipient is blacklisted, gated onIsBlackListingEnabledin the spec at the current head.CompositeIncomingTxFilter, sinceTxPoolaccepts a single custom incoming filter and the new filter has to run alongside the existingSignTransactionFilter.InitializeBlockchainXdcandXdcTestBlockchain.Why
The blacklist was only enforced at execution time, in
XdcTransactionProcessor.ValidateSender. A transaction with a blacklisted sender or recipient was accepted into the pool and gossiped to peers, and was dropped only later — skipped by the block-production picker, or invalidating a received block that contained it.The Go reference client (XinFinOrg/XDPoSChain) checks the denylist in both places:
core/txpool/validation.go— pool admission, for RPC submissions and p2p-received transactions alike.core/state_processor.go— per transaction during block processing.This PR adds the missing mempool-level check; the consensus-level one already matched.
Two intentional differences from the reference remain:
number == nil). Here a null head returnsAcceptTxResult.Syncing, matching whatSignTransactionFilteralready does for a null head in the same pipeline — the address set is spec-derived, so there is no list to consult without a head.common/constants.shared.go); ours comes from the chainspec (blackListedAddressesinxdc.json/xdc-testnet.json). The mainnet/testnet chainspecs already carry the same addresses.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
BlackListedAddressFilterTestscovers sender and recipient rejection, both before and after activation, contract creation (null recipient), a null head, and that the spec is taken at the head number.Note: the build and test run were not executed in the session that produced this branch — CI is the first verification.
Documentation
Requires documentation update
Requires explanation in Release Notes
XDC nodes now reject transactions with a blacklisted sender or recipient at submission time (
eth_sendRawTransactionand p2p), instead of accepting them into the mempool and dropping them at execution.🤖 Generated with Claude Code