Skip to content

fix(xdc): restore the XDCX special-transaction window on Apothem and mainnet #12872

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
ak88 wants to merge 6 commits into
base: master
Choose a base branch
from
+76 −4
Open

fix(xdc): restore the XDCX special-transaction window on Apothem and mainnet #12872

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f48b896
fix(xdc): align the Apothem XDCX address chainspec key with the engin…
ak88 Aug 18, 2026
03753f3
fix(xdc): give the XDCX fork blocks their own release spec boundaries
ak88 Aug 18, 2026
f82ef4e
test(xdc): load the chain specs by name rather than by relative path
ak88 Aug 18, 2026
b14796f
test(xdc): load the fork ID chain specs by name as well
ak88 Aug 18, 2026
0c2fcc3
Apply suggestions from code review
ak88 Aug 18, 2026
7e6f659
test(xdc): disambiguate the chain spec test case names
ak88 Aug 18, 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
2 changes: 1 addition & 1 deletion src/Nethermind/Chains/xdc-testnet.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@
"masternodeVotingContract": "0x0000000000000000000000000000000000000088",
"blockSignerContract": "0x0000000000000000000000000000000000000089",
"randomizeSMCBinary": "0x0000000000000000000000000000000000000090",
"XDCXAddrBinary": "0x0000000000000000000000000000000000000091",
"XDCXAddressBinary": "0x0000000000000000000000000000000000000091",

@claude claude Bot Aug 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.

Medium — the divergence window is only half closed by this rename.

IsTIPXDCXReceiver is baked into each release spec from releaseStartBlock:

// XdcChainSpecBasedSpecProvider.cs:82
releaseSpec.IsTIPXDCXReceiver = (…TipXDCX ?? ulong.MaxValue) <= releaseStartBlock
                                && releaseStartBlock < (…TIPXDCXReceiverDisable ?? ulong.MaxValue);

but TIPXDCXReceiverDisable (and TipXDCX, TIPXDCXMinerDisable) are not registered in XdcChainSpecEngineParameters.AddTransitions, so the flag can only flip on whichever unrelated transition encloses the block — the exact caveat already documented for DynamicGasLimitBlock at XdcChainSpecEngineParameters.cs:110.

Concretely:

chain disable block is it a transition? flag actually flips at
Apothem TIPXDCXReceiverDisable 66,825,000 no (nearest 61,290,000 / 71,550,000) 71,550,000 (~4.7M blocks late)
mainnet TIPXDCXReceiverDisable 80,370,900 no (nearest 76,321,000 / 98,800,200) 98,800,200 (~18.4M blocks late)
mainnet TIPXDCXMinerDisable 80,370,000 no 98,800,200

TipXDCX (23,779,191 / 38,383,838) does coincide with eip152Transition, so activation is fine — only deactivation is late.

Net effect of this PR on Apothem: trading txs to 0x…91 go from "never special" (wrong for 23,779,191–66,824,999) to "special up to 71,549,999" (wrong for 66,825,000–71,549,999). Strictly better, but the special-transaction path still diverges from the reference client, which evaluates TIPXDCXReceiver per block. The lending / trading-state keys (0x…92–0x…94) were already spelled correctly, so this part is pre-existing rather than introduced here — but it is the other half of the same bug the PR description sets out to fix.

Suggested fix (same shape as the existing DynamicGasLimitBlock handling), plus a spec-provider test case pinning IsTIPXDCXReceiver at 66,825,000 and at 66,824,999:

if (TipXDCX is not null)
    blockNumbers.Add(TipXDCX.Value);
if (TIPXDCXMinerDisable is not null)
    blockNumbers.Add(TIPXDCXMinerDisable.Value);
if (TIPXDCXReceiverDisable is not null)
    blockNumbers.Add(TIPXDCXReceiverDisable.Value);

If you'd rather keep this PR minimal, a follow-up issue plus a note here is fine — but as it stands the PR body's claim of parity with the reference client on Apothem DEX handling doesn't hold for blocks ≥ 66,825,000.

@ak88 ak88 Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed and folded into this PR in 03753f3 — thanks, this was the more consequential half.

Verified your analysis against the chainspec transition sets independently: activation lands on a transition on both chains, deactivation does not, except Apothem TIPXDCXMinerDisable 61,290,000 which coincides with eip1559Transition. AddTransitions now registers all three blocks, following the DynamicGasLimitBlock precedent.

XdcChainSpecTests.XDCX_flags_flip_on_their_own_blocks pins the boundaries, deriving them from the engine parameters so the numbers stay in one place (the schedule itself is already pinned to the reference client by XdcForkIdConformanceTests). Without the fix it fails with exactly the spread you predicted: 2 assertions on mainnet, 1 on apothem. Full suite green at 621 tests, and the fork IDs are unperturbed.

Findings 2 and 3 I left alone deliberately: the duplicated loader is three lines, and the unmapped-key guard needs rewardCheckpoint resolved first. Both are noted in the PR body as follow-ups.

"tradingStateAddressBinary": "0x0000000000000000000000000000000000000092",
"XDCXLendingAddressBinary": "0x0000000000000000000000000000000000000093",
"XDCXLendingFinalizedTradeAddressBinary": "0x0000000000000000000000000000000000000094"
Expand Down
68 changes: 68 additions & 0 deletions src/Nethermind/Nethermind.Xdc.Test/XdcChainSpecTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
// SPDX-License-Identifier: LGPL-3.0-only

using Nethermind.Core;
using Nethermind.Logging;
using Nethermind.Serialization.Json;
using Nethermind.Specs.ChainSpecStyle;
using Nethermind.Xdc.Spec;
using NUnit.Framework;

namespace Nethermind.Xdc.Test;

/// <summary>
/// Pins what our chain specs produce for the XDCX special-transaction path on both networks.
/// </summary>
[TestFixture, Parallelizable(ParallelScope.All)]
public class XdcChainSpecTests
{
[TestCase("xdc.json", TestName = "mainnet")]
[TestCase("xdc-testnet.json", TestName = "apothem")]
public void System_contract_addresses_are_deserialized(string chainSpecFile)
{

@claude claude Bot Aug 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.

Low — the test pins seven constants, but not the bug class. The root cause is that engine.XDPoS.params is deserialized with PropertyNameCaseInsensitive = true and no unmapped-member handling (ChainSpecLoader.LoadEngine → ChainSpecParametersProvider:59), so any key that matches no property is dropped without a word. That failure mode is still live for every other key — e.g. rewardCheckpoint (xdc.json:9, xdc-testnet.json:9) maps to no property on XdcChainSpecEngineParameters and is silently discarded today.

A cheap generic guard that would have caught XDCXAddrBinary and any future typo, without needing a new Assert per parameter:

// every key under engine.XDPoS.params must bind to a property
foreach (string key in keysFromJson)
    Assert.That(typeof(XdcChainSpecEngineParameters).GetProperty(key,
        BindingFlags.Public | BindingFlags.Instance | BindingFlags.IgnoreCase), Is.Not.Null, key);

(rewardCheckpoint would have to be dropped from both chainspecs, or added as a property, for that to pass — it's unused by Nethermind and equals epoch, so removing it looks right.) The stricter alternative is [JsonUnmappedMemberHandling(JsonUnmappedMemberHandling.Disallow)] on the parameters type, which fails chainspec load loudly instead of at consensus time — nicer, but it also affects third-party/custom specs, so the test-only version is the safer first step.

Related observation while checking this (pre-existing, not for this PR): LimitPenaltyEpochV2 is set by neither chainspec nor XdcReleaseSpec.ApplyV2Config, so it stays 0 where PenaltyHandler uses it (PenaltyHandler.cs:87,90).

XdcChainSpecEngineParameters engineParameters = EngineParameters(LoadChainSpec(chainSpecFile));

Assert.Multiple(() =>
{
Assert.That(engineParameters.MasternodeVotingContract, Is.EqualTo(new Address("0x0000000000000000000000000000000000000088")));
Assert.That(engineParameters.BlockSignerContract, Is.EqualTo(new Address("0x0000000000000000000000000000000000000089")));
Assert.That(engineParameters.RandomizeSMCBinary, Is.EqualTo(new Address("0x0000000000000000000000000000000000000090")));
Assert.That(engineParameters.XDCXAddressBinary, Is.EqualTo(new Address("0x0000000000000000000000000000000000000091")));
Assert.That(engineParameters.TradingStateAddressBinary, Is.EqualTo(new Address("0x0000000000000000000000000000000000000092")));
Assert.That(engineParameters.XDCXLendingAddressBinary, Is.EqualTo(new Address("0x0000000000000000000000000000000000000093")));
Assert.That(engineParameters.XDCXLendingFinalizedTradeAddressBinary, Is.EqualTo(new Address("0x0000000000000000000000000000000000000094")));
});
}

[TestCase("xdc.json", TestName = "mainnet")]
[TestCase("xdc-testnet.json", TestName = "apothem")]

@claude claude Bot Aug 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.

Low — the two methods produce identical NUnit test names. TestCaseAttribute.TestName replaces the method name, so all four cases in this fixture resolve to just two fully-qualified names:

Nethermind.Xdc.Test.XdcChainSpecTests.mainnet   ← both methods
Nethermind.Xdc.Test.XdcChainSpecTests.apothem   ← both methods

A failure report then can't distinguish "addresses didn't deserialize" from "flags flipped on the wrong block", and --filter FullyQualifiedName~XdcChainSpecTests.mainnet selects both. The sibling fixture already sidesteps this by prefixing (TestName = "mainnet genesis", "mainnet byzantium", … in XdcForkIdConformanceTests.cs:46-53).

Suggested change
[TestCase("xdc.json", TestName = "mainnet")]
[TestCase("xdc-testnet.json", TestName = "apothem")]
[TestCase("xdc.json", TestName = "mainnet XDCX flags")]
[TestCase("xdc-testnet.json", TestName = "apothem XDCX flags")]

(and correspondingly "mainnet addresses" / "apothem addresses" on lines 19-20 — or simply drop TestName and let NUnit append the argument, which is unambiguous for a single string parameter.)

@ak88 ak88 Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7e6f659 — confirmed by filtering: --filter FullyQualifiedName~XdcChainSpecTests.XDCX_flags_flip_on_their_own_blocks now selects exactly 2 cases, where previously the method name was gone entirely. I dropped TestName rather than prefixing it, so the cases read System_contract_addresses_are_deserialized("xdc.json") — unambiguous without hand-maintained names.

public void XDCX_flags_flip_on_their_own_blocks(string chainSpecFile)
{
ChainSpec chainSpec = LoadChainSpec(chainSpecFile);
XdcChainSpecEngineParameters engineParameters = EngineParameters(chainSpec);
XdcChainSpecBasedSpecProvider specProvider = new(chainSpec, engineParameters, LimboLogs.Instance);

ulong activation = engineParameters.TipXDCX!.Value;
ulong minerDisable = engineParameters.TIPXDCXMinerDisable!.Value;
ulong receiverDisable = engineParameters.TIPXDCXReceiverDisable!.Value;

Assert.Multiple(() =>
{
Assert.That(specProvider.GetXdcSpec(activation - 1).IsTIPXDCXMiner, Is.False);
Assert.That(specProvider.GetXdcSpec(activation).IsTIPXDCXMiner, Is.True);
Assert.That(specProvider.GetXdcSpec(minerDisable - 1).IsTIPXDCXMiner, Is.True);
Assert.That(specProvider.GetXdcSpec(minerDisable).IsTIPXDCXMiner, Is.False);
Comment on lines +51 to +54

@claude claude Bot Aug 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.

Low (informational) — the four IsTIPXDCXMiner assertions pin a flag with no reader. IsTIPXDCXMiner is written in XdcChainSpecBasedSpecProvider.cs:81 and declared on IXdcReleaseSpec/XdcReleaseSpec, but grepping *.cs finds no production consumer — only this test and RpcModuleTests.cs:132 (which sets it). The special-transaction predicates all gate on IsTIPXDCXReceiver:

// XdcExtensions.Transactions.cs:21-24 — all four use IsTIPXDCXReceiver
public static bool IsTradingTransaction(this Transaction currentTx, IXdcReleaseSpec spec)
    => currentTx.To is not null && currentTx.To == spec.XDCXAddressBinary && spec.IsTIPXDCXReceiver;

No change needed here — pinning it is cheap and correct for when a consumer lands. But it does mean the PR body's impact table overstates the mainnet blast radius: the TIPXDCXMinerDisable 80,370,000 row (80,370,000 – 98,800,199) is behaviourally inert today, so the resync guidance in the release note should rest on the TIPXDCXReceiverDisable rows alone (mainnet 80,370,900 – 98,800,199, apothem 56,828,700 – 71,549,999). Worth trimming so operators don't over-scope.

@ak88 ak88 Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed and corrected in the PR body. Grepping *.cs, IsTIPXDCXMiner is written at XdcChainSpecBasedSpecProvider.cs:81 and declared on the spec, but has no production reader; all four predicates in XdcExtensions.Transactions.cs:21-24 gate on IsTIPXDCXReceiver.

I dropped the inert mainnet 80,370,000 – 98,800,199 miner row from the impact table and rescoped the release note to the receiver ranges only, with a note that the miner flag is fixed and pinned for when a consumer lands. Good catch on the over-scoping — that line would have sent operators resyncing further back than needed.


Assert.That(specProvider.GetXdcSpec(activation - 1).IsTIPXDCXReceiver, Is.False);
Assert.That(specProvider.GetXdcSpec(activation).IsTIPXDCXReceiver, Is.True);
Assert.That(specProvider.GetXdcSpec(receiverDisable - 1).IsTIPXDCXReceiver, Is.True);
Assert.That(specProvider.GetXdcSpec(receiverDisable).IsTIPXDCXReceiver, Is.False);
});
}

private static ChainSpec LoadChainSpec(string chainSpecFile) =>
new ChainSpecFileLoader(new EthereumJsonSerializer(), LimboLogs.Instance).LoadEmbeddedOrFromFile(chainSpecFile);

private static XdcChainSpecEngineParameters EngineParameters(ChainSpec chainSpec) =>
chainSpec.EngineChainSpecParametersProvider.GetChainSpecParameters<XdcChainSpecEngineParameters>();
}
4 changes: 1 addition & 3 deletions src/Nethermind/Nethermind.Xdc.Test/XdcForkIdConformanceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
// SPDX-License-Identifier: LGPL-3.0-only

using System.IO;
using Nethermind.Core;
using Nethermind.Core.Crypto;
using Nethermind.Core.Test.Builders;
Expand Down Expand Up @@ -65,8 +64,7 @@ private static void AssertForkId(string chainSpecFile, Hash256 genesisHash, ulon

private static XdcForkInfo ForkInfo(string chainSpecFile, Hash256 genesisHash)
{
string path = Path.Combine(TestContext.CurrentContext.WorkDirectory, "../../../../", "Chains", chainSpecFile);
ChainSpec chainSpec = new ChainSpecFileLoader(new EthereumJsonSerializer(), LimboLogs.Instance).LoadEmbeddedOrFromFile(path);
ChainSpec chainSpec = new ChainSpecFileLoader(new EthereumJsonSerializer(), LimboLogs.Instance).LoadEmbeddedOrFromFile(chainSpecFile);
XdcChainSpecEngineParameters engineParameters =
chainSpec.EngineChainSpecParametersProvider.GetChainSpecParameters<XdcChainSpecEngineParameters>();

Expand Down
6 changes: 6 additions & 0 deletions src/Nethermind/Nethermind.Xdc/Spec/XdcChainSpecEngineParameters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,12 @@ public void AddTransitions(SortedSet<ulong> blockNumbers, SortedSet<ulong> times
// Without its own release spec boundary the flag would only flip on whichever transition encloses it.
if (DynamicGasLimitBlock is not null)
blockNumbers.Add(DynamicGasLimitBlock.Value);
if (TipXDCX is not null)
blockNumbers.Add(TipXDCX.Value);
if (TIPXDCXMinerDisable is not null)
blockNumbers.Add(TIPXDCXMinerDisable.Value);
if (TIPXDCXReceiverDisable is not null)
blockNumbers.Add(TIPXDCXReceiverDisable.Value);
}
Comment on lines +112 to 118

@claude claude Bot Aug 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.

Low — two siblings with the identical defect are still unregistered. XdcChainSpecBasedSpecProvider.CreateReleaseSpec bakes seven engine-parameter-gated flags from releaseStartBlock:

flag gating parameter in AddTransitions?
IsTipTrc21FeeEnabled TipTrc21Fee yes
IsTipUpgradeRewardEnabled TipUpgradeReward yes
IsTipUpgradePenaltyEnabled TipUpgradePenalty yes
IsDynamicGasLimitBlock DynamicGasLimitBlock yes
IsTIPXDCXMiner / IsTIPXDCXReceiver TipXDCX, TIPXDCX*Disable yes, as of this commit
IsTIP2019 TIP2019Block no
IsBlackListingEnabled BlackListHFNumber no

I checked both chainspecs and neither is live today — the values happen to coincide with existing EIP transitions:

parameter mainnet apothem coincides with
tip2019Block 1 1 eip7Transition: 1
BlackListHFNumber 38,383,838 23,779,191 eip152Transition

So no divergence to fix, but the same silent-failure mode is one chainspec edit away: IsBlackListingEnabled gates denylist enforcement, so a mis-timed flip is a consensus fault of the same shape as the one this PR closes. Since the fix is four more lines in the block you just added, folding them in makes the invariant complete — every block that gates a release-spec flag gets its own boundary — rather than leaving two entries that only work by coincidence:

if (TIP2019Block > 0)
    blockNumbers.Add(TIP2019Block);
if (BlackListHFNumber is not null)
    blockNumbers.Add(BlackListHFNumber.Value);

Fork IDs stay neutral for the same reason as the three you added: XdcForkInfo.GetForkActivations already contributes TIP2019Block and BlackListHFNumber explicitly (XdcForkInfo.cs:34,39), and the SortedSet<ForkActivation> collapses the duplicates. Neither value is the max of its chain's transition set, so biggestBlockTransition is unaffected. Your call whether it belongs in this PR or a follow-up — flagging it so the "only accidentally correct" status is recorded either way.

}

Expand Down
Loading