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

Merged
ak88 merged 6 commits into from
Aug 19, 2026
+76 −4
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
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
50 changes: 50 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,50 @@
// SPDX-FileCopyrightText: 2026 Demerzel Solutions Limited
// SPDX-License-Identifier: LGPL-3.0-only

using System.IO;
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 the system contract addresses our chain specs deserialize into <see cref="XdcChainSpecEngineParameters"/>.
/// </summary>
/// <remarks>
/// A key that does not match its property name deserializes to <c>null</c> instead of failing, and the affected
/// contract then never matches a transaction recipient - the DEX/lending addresses silently lose their
/// special-transaction handling, which diverges from the reference client.
/// </remarks>
[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 = LoadEngineParameters(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")));
});
}

private static XdcChainSpecEngineParameters LoadEngineParameters(string chainSpecFile)
{
string path = Path.Combine(TestContext.CurrentContext.WorkDirectory, "../../../../", "Chains", chainSpecFile);
ChainSpec chainSpec = new ChainSpecFileLoader(new EthereumJsonSerializer(), LimboLogs.Instance).LoadEmbeddedOrFromFile(path);

return chainSpec.EngineChainSpecParametersProvider.GetChainSpecParameters<XdcChainSpecEngineParameters>();

@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 — duplicated chainspec loader. These three lines are verbatim XdcForkIdConformanceTests.ForkInfo (XdcForkIdConformanceTests.cs:68-71), including the "../../../../", "Chains" path dance. Per AGENTS.md ("when only parts of tests are similar (shared setup …), factor those parts into helper methods or helper types"), a single internal static helper in Nethermind.Xdc.Test — e.g. XdcChainSpecs.LoadEngineParameters(string file) returning (ChainSpec, XdcChainSpecEngineParameters) — would keep the path knowledge in one place for both fixtures and for the next one.

Not a correctness issue; the path pattern is the established one (TaikoChainSpecEngineParametersTests.LoadChainSpec does the same), so resolution should work in CI.

}
}
Loading