-
Notifications
You must be signed in to change notification settings - Fork 719
feat(xdc): reject blacklisted addresses on pool admission #12879
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
ee004e3
b60b297
377551e
b3a6d8c
720189d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,128 @@ | ||||||
| // SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited | ||||||
| // SPDX-License-Identifier: LGPL-3.0-only | ||||||
|
|
||||||
| using System.Collections.Generic; | ||||||
| using System.Threading.Tasks; | ||||||
| using Nethermind.Consensus; | ||||||
| using Nethermind.Core; | ||||||
| using Nethermind.Core.Specs; | ||||||
| using Nethermind.Core.Test.Builders; | ||||||
| using Nethermind.Crypto; | ||||||
| using Nethermind.Logging; | ||||||
| using Nethermind.TxPool; | ||||||
| using Nethermind.Xdc.Spec; | ||||||
| using Nethermind.Xdc.Test.Helpers; | ||||||
| using Nethermind.Xdc.TxPool; | ||||||
| using NSubstitute; | ||||||
| using NUnit.Framework; | ||||||
|
|
||||||
| namespace Nethermind.Xdc.Test; | ||||||
|
|
||||||
| [Parallelizable(ParallelScope.All)] | ||||||
| internal class BlackListedAddressFilterTests | ||||||
| { | ||||||
| private static readonly Address BlackListed = TestItem.AddressA; | ||||||
|
|
||||||
| private static BlackListedAddressFilter CreateFilter(ulong headNumber, bool blackListingEnabled, ISpecProvider? specProvider = null) | ||||||
| { | ||||||
| IChainHeadInfoProvider chainHeadInfoProvider = Substitute.For<IChainHeadInfoProvider>(); | ||||||
| chainHeadInfoProvider.HeadNumber.Returns(headNumber); | ||||||
|
|
||||||
| IXdcReleaseSpec xdcSpec = Substitute.For<IXdcReleaseSpec>(); | ||||||
| xdcSpec.IsBlackListingEnabled.Returns(blackListingEnabled); | ||||||
| HashSet<Address> blackList = [BlackListed]; | ||||||
| xdcSpec.BlackListedAddresses.Returns(blackList); | ||||||
|
|
||||||
| specProvider ??= Substitute.For<ISpecProvider>(); | ||||||
| specProvider.GetSpec(Arg.Any<ForkActivation>()).Returns(xdcSpec); | ||||||
|
|
||||||
| return new BlackListedAddressFilter(chainHeadInfoProvider, specProvider, LimboLogs.Instance); | ||||||
| } | ||||||
|
|
||||||
| private static AcceptTxResult Accept(BlackListedAddressFilter filter, Transaction tx) | ||||||
| { | ||||||
| TxFilteringState state = default; | ||||||
| return filter.Accept(tx, ref state, TxHandlingOptions.None); | ||||||
| } | ||||||
|
|
||||||
| private static Transaction BuildTx(Address? sender, Address? to) => | ||||||
| Build.A.Transaction.WithSenderAddress(sender).WithTo(to).TestObject; | ||||||
|
|
||||||
| [TestCase(true, false, true, false, TestName = "Blacklisted sender rejected once activated")] | ||||||
| [TestCase(true, false, false, true, TestName = "Blacklisted sender allowed before activation")] | ||||||
| [TestCase(false, true, true, false, TestName = "Blacklisted recipient rejected once activated")] | ||||||
| [TestCase(false, true, false, true, TestName = "Blacklisted recipient allowed before activation")] | ||||||
| [TestCase(false, false, true, true, TestName = "Unlisted addresses accepted once activated")] | ||||||
| [TestCase(false, false, false, true, TestName = "Unlisted addresses accepted before activation")] | ||||||
| public void Accept_ChecksSenderAndRecipient(bool blackListSender, bool blackListRecipient, bool blackListingEnabled, bool expectedAccepted) | ||||||
| { | ||||||
| BlackListedAddressFilter filter = CreateFilter(headNumber: 100, blackListingEnabled); | ||||||
| Transaction tx = BuildTx(blackListSender ? BlackListed : TestItem.AddressB, blackListRecipient ? BlackListed : TestItem.AddressC); | ||||||
|
|
||||||
| Assert.That((bool)Accept(filter, tx), Is.EqualTo(expectedAccepted)); | ||||||
| } | ||||||
|
|
||||||
| [TestCase(true, "sender")] | ||||||
| [TestCase(false, "recipient")] | ||||||
| public void Accept_BlackListedAddress_ReportsRoleWithoutDisconnectingPeer(bool blackListSender, string expectedRole) | ||||||
| { | ||||||
| BlackListedAddressFilter filter = CreateFilter(headNumber: 100, blackListingEnabled: true); | ||||||
| Transaction tx = blackListSender ? BuildTx(BlackListed, TestItem.AddressC) : BuildTx(TestItem.AddressB, BlackListed); | ||||||
|
|
||||||
| AcceptTxResult result = Accept(filter, tx); | ||||||
|
|
||||||
| Assert.That(result, Is.EqualTo(XdcAcceptTxResult.BlackListedSender)); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low — this assertion can't fail for the reason the test name implies.
Same at line 125: Sharing the id is the right call (it's what keeps
Suggested change
(or simply drop line 74 and keep the |
||||||
| Assert.That(result.ToString(), Does.Contain(expectedRole)); | ||||||
| Assert.That(result, Is.Not.EqualTo(AcceptTxResult.Invalid), "Invalid makes TxFloodController disconnect the relaying peer"); | ||||||
| } | ||||||
|
|
||||||
| [Test] | ||||||
| public void Accept_ContractCreation_IsAccepted() | ||||||
| { | ||||||
| BlackListedAddressFilter filter = CreateFilter(headNumber: 100, blackListingEnabled: true); | ||||||
|
|
||||||
| Assert.That(Accept(filter, BuildTx(TestItem.AddressB, to: null)), Is.EqualTo(AcceptTxResult.Accepted)); | ||||||
| } | ||||||
|
|
||||||
| [Test] | ||||||
| public void Accept_UsesSpecOfBlockAfterHead() | ||||||
| { | ||||||
| const ulong headNumber = 1234; | ||||||
| ISpecProvider specProvider = Substitute.For<ISpecProvider>(); | ||||||
| BlackListedAddressFilter filter = CreateFilter(headNumber, blackListingEnabled: true, specProvider); | ||||||
|
|
||||||
| Accept(filter, BuildTx(TestItem.AddressB, TestItem.AddressC)); | ||||||
|
|
||||||
| specProvider.Received().GetSpec(Arg.Is<ForkActivation>(f => f.BlockNumber == headNumber + 1)); | ||||||
| } | ||||||
|
|
||||||
| [TestCase(true, true, false, TestName = "Pool rejects blacklisted sender")] | ||||||
| [TestCase(false, true, false, TestName = "Pool rejects blacklisted recipient")] | ||||||
| [TestCase(true, false, true, TestName = "Pool accepts blacklisted sender before activation")] | ||||||
| public async Task SubmitTx_BlackListedAddress_IsRejectedOnPoolAdmission(bool blackListSender, bool blackListingEnabled, bool expectedAccepted) | ||||||
| { | ||||||
| using XdcTestBlockchain chain = await XdcTestBlockchain.Create(5, false); | ||||||
| chain.ChangeReleaseSpec(spec => | ||||||
| { | ||||||
| spec.BlackListedAddresses = [blackListSender ? TestItem.AddressB : TestItem.AddressC]; | ||||||
| spec.IsBlackListingEnabled = blackListingEnabled; | ||||||
| }); | ||||||
|
|
||||||
| Transaction tx = Build.A.Transaction | ||||||
| .WithSenderAddress(TestItem.AddressB) | ||||||
| .WithTo(TestItem.AddressC) | ||||||
| .WithValue(1) | ||||||
| .WithType(TxType.Legacy) | ||||||
| .WithNonce(chain.TxPool.GetLatestPendingNonce(TestItem.AddressB)) | ||||||
| .TestObject; | ||||||
| new Signer(chain.SpecProvider.ChainId, TestItem.PrivateKeyB, NullLogManager.Instance).TrySign(tx); | ||||||
| tx.Hash = tx.CalculateHash(); | ||||||
|
|
||||||
| AcceptTxResult result = chain.TxPool.SubmitTx(tx, TxHandlingOptions.None); | ||||||
|
|
||||||
| Assert.That((bool)result, Is.EqualTo(expectedAccepted), result.ToString()); | ||||||
| if (!expectedAccepted) | ||||||
| Assert.That(result, Is.EqualTo(blackListSender ? XdcAcceptTxResult.BlackListedSender : XdcAcceptTxResult.BlackListedRecipient)); | ||||||
| Assert.That(chain.TxPool.GetPendingTransactions(), Has.Length.EqualTo(expectedAccepted ? 1 : 0)); | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,53 @@ | ||||||
| // SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited | ||||||
| // SPDX-License-Identifier: LGPL-3.0-only | ||||||
|
|
||||||
| using Nethermind.Core; | ||||||
| using Nethermind.Core.Specs; | ||||||
| using Nethermind.Logging; | ||||||
| using Nethermind.TxPool; | ||||||
| using Nethermind.TxPool.Filters; | ||||||
| using Nethermind.Xdc.Spec; | ||||||
|
|
||||||
| namespace Nethermind.Xdc.TxPool; | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Rejects transactions whose sender or recipient is blacklisted, keeping them out of the pool and out of gossip. | ||||||
| /// </summary> | ||||||
| /// <remarks> | ||||||
| /// The blacklist is a consensus rule enforced during execution by <see cref="XdcTransactionProcessor.ValidateSender"/>; | ||||||
| /// this filter is the mempool-admission counterpart, so that such transactions are dropped on submission rather than | ||||||
| /// when a block containing them is processed. Activation is read from the spec of the block the transaction would land | ||||||
| /// in, one past the current head, matching <see cref="SignTransactionFilter"/>. | ||||||
| /// </remarks> | ||||||
| internal sealed class BlackListedAddressFilter( | ||||||
| IChainHeadInfoProvider chainHeadInfoProvider, | ||||||
| ISpecProvider specProvider, | ||||||
| ILogManager logManager) : IIncomingTxFilter | ||||||
| { | ||||||
| private readonly ILogger _logger = logManager.GetClassLogger<BlackListedAddressFilter>(); | ||||||
|
|
||||||
| public AcceptTxResult Accept(Transaction tx, ref TxFilteringState state, TxHandlingOptions txHandlingOptions) | ||||||
| { | ||||||
| IXdcReleaseSpec spec = specProvider.GetXdcSpec(chainHeadInfoProvider.HeadNumber + 1); | ||||||
|
|
||||||
| if (!spec.IsBlackListingEnabled) | ||||||
| return AcceptTxResult.Accepted; | ||||||
|
|
||||||
| if (IsBlackListed(spec, tx.SenderAddress)) | ||||||
| return Reject(tx, XdcAcceptTxResult.BlackListedSender); | ||||||
|
|
||||||
| if (IsBlackListed(spec, tx.To)) | ||||||
| return Reject(tx, XdcAcceptTxResult.BlackListedRecipient); | ||||||
|
|
||||||
| return AcceptTxResult.Accepted; | ||||||
| } | ||||||
|
|
||||||
| private AcceptTxResult Reject(Transaction tx, AcceptTxResult result) | ||||||
| { | ||||||
| if (_logger.IsDebug) _logger.Debug($"Skipped adding transaction {tx.ToString(" ")}, {result}."); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Low — Every built-in incoming filter logs its discard at That matters because the trigger is remote: a blacklisted tx does not produce 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.
Suggested change
|
||||||
| return result; | ||||||
| } | ||||||
|
|
||||||
| private static bool IsBlackListed(IXdcReleaseSpec spec, Address? address) => | ||||||
| address is not null && spec.BlackListedAddresses.Contains(address); | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| // SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited | ||
| // SPDX-License-Identifier: LGPL-3.0-only | ||
|
|
||
| using Nethermind.TxPool; | ||
|
|
||
| namespace Nethermind.Xdc.TxPool; | ||
|
|
||
| internal static class XdcAcceptTxResult | ||
| { | ||
| private const int BlackListedAddressId = 1000; | ||
| private const string BlackListedAddressCode = "BlackListedAddress"; | ||
|
|
||
| public static AcceptTxResult BlackListedSender { get; } = new(BlackListedAddressId, BlackListedAddressCode, "Transaction sender is blacklisted"); | ||
| public static AcceptTxResult BlackListedRecipient { get; } = new(BlackListedAddressId, BlackListedAddressCode, "Transaction recipient is blacklisted"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Low — this is a source-breaking change to a public constructor; the PR is checkboxed as non-breaking.
TxPooland this constructor arepublic, andIIncomingTxFilter? incomingTxFilter→IIncomingTxFilter[]? incomingTxFiltersbreaks 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, andnull/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 dropCompositeIncomingTxFilter.Two smaller notes while here:
tx.SenderAddress— would earn its place. That guarantee is exactly what makesBlackListedAddressFilter's sender check safe.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.