Skip to content

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

Open
ak88 wants to merge 6 commits into
masterfrom
claude/xdcxaddrbinary-chainspec-align-9e43e9
Open

fix(xdc): restore the XDCX special-transaction window on Apothem and mainnet#12872
ak88 wants to merge 6 commits into
masterfrom
claude/xdcxaddrbinary-chainspec-align-9e43e9

Conversation

@ak88

@ak88 ak88 commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Changes

  • Rename XDCXAddrBinary to XDCXAddressBinary in xdc-testnet.json, matching the mainnet chainspec and XdcChainSpecEngineParameters.XDCXAddressBinary
  • Register TipXDCX, TIPXDCXMinerDisable and TIPXDCXReceiverDisable in XdcChainSpecEngineParameters.AddTransitions, as already done for DynamicGasLimitBlock
  • Add XdcChainSpecTests, pinning the system contract addresses (0x…880x…94) and the blocks at which IsTIPXDCXMiner/IsTIPXDCXReceiver flip, for both networks

Two independent, silent faults closed the XDCX special-transaction path at the wrong points.

1. The testnet contract address was dropped. XDCXAddrBinary matched no property, and engine.XDPoS.params is deserialized with no unmapped-member handling, so XDCXAddressBinary stayed null on Apothem. IsTradingTransaction compares the recipient against it behind a To is not null guard, so the predicate was permanently false rather than throwing: DEX transactions to 0x…91 ran through the EVM instead of taking the special path (no intrinsic gas, EVM skipped, empty successful receipt).

2. The XDCX fork blocks had no release spec boundary. IsTIPXDCXMiner/IsTIPXDCXReceiver are baked into each release spec from its releaseStartBlock, so a fork block absent from AddTransitions can only take effect at whichever unrelated transition encloses it — the hazard already documented for DynamicGasLimitBlock. Activation happened to land on a transition on both chains; deactivation did not:

chain block was a transition? flag flipped at
Apothem TIPXDCXReceiverDisable 66,825,000 no 71,550,000 (~4.7M late)
mainnet TIPXDCXMinerDisable 80,370,000 no 98,800,200 (~18.4M late)
mainnet TIPXDCXReceiverDisable 80,370,900 no 98,800,200

Fault 2 was pre-existing and affected mainnet as well as Apothem; fixing only the rename would have moved Apothem from "never special" to "special ~4.7M blocks too long", so both are fixed here.

Scoping the impact to what is officially supported — blocks at or after switchBlock (80,370,000 mainnet, 56,828,700 Apothem) — the TipXDCX activation itself is pre-switch on both chains and therefore academic, but every affected deactivation range is post-switch:

chain supported range affected wrong behaviour
Apothem 56,828,700 – 66,824,999 trading txs to 0x…91 never took the special path (fault 1)
Apothem 66,825,000 – 71,549,999 IsTIPXDCXReceiver stayed on past its disable block (fault 2)
mainnet 80,370,900 – 98,800,199 IsTIPXDCXReceiver stayed on past its disable block (fault 2)

IsTIPXDCXMiner is written to the release spec but has no production reader today — all four special-transaction predicates gate on IsTIPXDCXReceiver — so its late flip (mainnet 80,370,000, Apothem 61,290,000) is currently inert. It is fixed and pinned here so it behaves when a consumer lands, but the resync guidance rests on the receiver rows alone.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Both new tests were verified to fail before their respective fix and pass after:

  • System_contract_addresses_are_deserialized — apothem case fails on the pre-rename chainspec (XDCXAddressBinary null)
  • XDCX_flags_flip_on_their_own_blocks — fails without the AddTransitions change with exactly the predicted spread (2 assertions on mainnet, 1 on apothem, since TIPXDCXMinerDisable 61,290,000 already coincides with a transition there)

Fork IDs are unaffected, which matters because the added transitions feed ISpecProvider.TransitionActivations. XdcForkInfo.GetForkActivations already contributed all three blocks explicitly, and collects them into a SortedSet<ForkActivation> whose CompareTo compares block numbers whenever either timestamp is null; block transitions are created as new ForkActivation(blockNumber) with a null timestamp, so Activation == blockNumber on both paths and the now-duplicate entry collapses rather than being appended twice. Neither chainspec has a timestamp transition, and the added blocks are neither the minimum (Skip(1)) nor the maximum of the transition set. Verified by dumping the full activation list and every fork ID with and without the change: identical on both chains, including the terminal-block checksum, which is a running CRC over every prior activation. Release spec count rises (mainnet 8→10, apothem 9→10) as intended, while the fork set is unchanged.

Full Nethermind.Xdc.Test suite green (621 tests), including the 16 reference-client checksums pinned by XdcForkIdConformanceTests. No other project loads these chainspecs.

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Fixes the XDCX DEX/lending special-transaction window on XDC. Within the officially supported range (blocks at or after switchBlock): on Apothem, DEX transactions to 0x…91 were not given special handling from 56,828,700 to 66,824,999 because the chainspec entry was ignored, and the window then stayed open until 71,550,000 instead of 66,825,000; on mainnet the window stayed open until 98,800,200 instead of 80,370,900. Nodes that processed those ranges need to resync. Fork IDs are unchanged, so peering is unaffected.

Remarks

Three related pre-existing issues found while investigating, none addressed here:

  • rewardCheckpoint (line 9 of both chainspecs) maps to no property and is silently dropped. A generic guard — asserting every engine.XDPoS.params key binds to a property, or [JsonUnmappedMemberHandling(Disallow)] — would catch this whole class of typo, but needs that key removed or added first.
  • RelayerRegistrationSMC and TRC21IssuerSMC are set by neither chainspec and copied into no release spec.
  • LimitPenaltyEpochV2 is set by neither chainspec nor XdcReleaseSpec.ApplyV2Config, so it stays 0 where PenaltyHandler uses it.

🤖 Generated with Claude Code

…e parameter

The testnet chainspec spelled the DEX contract key `XDCXAddrBinary`, which does
not match `XdcChainSpecEngineParameters.XDCXAddressBinary`, so it deserialized to
null and every DEX transaction on Apothem lost its special-transaction handling.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ak88's task in 7m 38s —— View job


Review: fix(xdc): align the Apothem XDCX address chainspec key with the engine parameter

  • Gather PR diff and context
  • Inspect XdcChainSpecEngineParameters and both chainspecs
  • Verify the claimed consensus impact (IsTradingTransaction path)
  • Check the new test for correctness / repo test conventions
  • Post review

Verdict

The rename is correct, minimal, and the diagnosis in the PR body checks out end to end. One notable gap: the special-transaction path still diverges from the reference client after the disable block, so Apothem parity isn't fully restored by this change alone.

What I verified

  • The key really was dropped. ChainSpecLoader.LoadEngine (ChainSpecLoader.cs:315-317) hands engine.XDPoS.params to ChainSpecParametersProvider, which deserializes it into XdcChainSpecEngineParameters (ChainSpecParametersProvider.cs:59) using EthereumJsonSerializer options with PropertyNameCaseInsensitive = true and no unmapped-member handling. XDCXAddrBinary matched nothing → dropped silently, XDCXAddressBinary stayed null. Confirmed.
  • No crash, silent divergence — as described. IsTradingTransaction (XdcExtensions.Transactions.cs:21) guards To is not null before comparing, so a null address made the predicate permanently false rather than throwing. Same shape as the sibling lending/trading-state predicates, whose keys were already spelled correctly.
  • Blast radius on Apothem: the special path additionally requires spec.IsTIPXDCXReceiver, which on Apothem covers blocks [TipXDCX 23,779,191, TIPXDCXReceiverDisable 66,825,000). So this bug affected historical processing of that window (i.e. full sync), not the current head.
  • No stragglers: XDCXAddrBinary no longer appears anywhere in the repo; xdc.json already used the correct key; the seven addresses (0x…880x…94) match between the two chainspecs.
  • Test plumbing: the WorkDirectory + "../../../../" + "Chains" pattern and LoadEmbeddedOrFromFile usage match the established precedent (XdcForkIdConformanceTests.cs:68, TaikoChainSpecEngineParametersTests.cs:288), so path resolution should hold in CI. Copyright header year, file-scoped namespace, no var, [TestFixture, Parallelizable], XML docs — all in line with repo style.
  • I could not execute the test here: dotnet test was not permitted in this sandbox, so correctness of the assertions was checked by reading, not by running. Worth confirming CI is green on Nethermind.Xdc.Test.

Findings

# Severity Finding
1 Medium TIPXDCXReceiverDisable / TIPXDCXMinerDisable / TipXDCX are not in AddTransitions, so the XDCX special-transaction window closes late — Apothem at 71,550,000 instead of 66,825,000 (~4.7M blocks), mainnet at 98,800,200 instead of 80,370,900 (~18.4M blocks). Pre-existing, but it's the other half of the divergence this PR sets out to fix. (details) · Fix this →
2 Low New test duplicates the chainspec-loading helper from XdcForkIdConformanceTests.ForkInfo; factor out a shared test helper. (details)
3 Low The test pins seven constants but not the bug class — unmapped keys are still dropped silently (rewardCheckpoint is dropped in both chainspecs today). A reflection assertion over engine.XDPoS.params keys, or [JsonUnmappedMemberHandling(Disallow)], would catch the next typo. (details)

No security or performance concerns: the change is load-time only, and no hot path is touched.

Two informational notes:

  • A node that previously full-synced Apothem across 23,779,191–66,824,999 would have produced state roots diverging from the network for any block containing a DEX transaction, so it most likely could not sync past those blocks at all rather than silently holding bad state. Still, "resync Apothem" is a reasonable release-note line alongside the one you already drafted.
  • RelayerRegistrationSMC and TRC21IssuerSMC exist on XdcChainSpecEngineParameters but are set by neither chainspec and copied into no release spec — dead config, unrelated to this PR.

Nothing here blocks the rename itself; finding 1 just needs a decision (fold in, or file a follow-up) so Apothem parity isn't assumed to be complete.
· branch claude/xdcxaddrbinary-chainspec-align-9e43e9

"blockSignerContract": "0x0000000000000000000000000000000000000089",
"randomizeSMCBinary": "0x0000000000000000000000000000000000000090",
"XDCXAddrBinary": "0x0000000000000000000000000000000000000091",
"XDCXAddressBinary": "0x0000000000000000000000000000000000000091",

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…920x…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.

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.

Comment on lines +44 to +48
{
string path = Path.Combine(TestContext.CurrentContext.WorkDirectory, "../../../../", "Chains", chainSpecFile);
ChainSpec chainSpec = new ChainSpecFileLoader(new EthereumJsonSerializer(), LimboLogs.Instance).LoadEmbeddedOrFromFile(path);

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

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.

Comment on lines +26 to +28
[TestCase("xdc-testnet.json", TestName = "apothem")]
public void System_contract_addresses_are_deserialized(string chainSpecFile)
{

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.LoadEngineChainSpecParametersProvider: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).

`TipXDCX`, `TIPXDCXMinerDisable` and `TIPXDCXReceiverDisable` were not registered
in `AddTransitions`, so `IsTIPXDCXMiner`/`IsTIPXDCXReceiver` could only flip on
whichever unrelated transition enclosed them: the special-transaction window
closed at 71,550,000 instead of 66,825,000 on Apothem and at 98,800,200 instead
of 80,370,000/80,370,900 on mainnet.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ak88 ak88 changed the title fix(xdc): align the Apothem XDCX address chainspec key with the engine parameter fix(xdc): restore the XDCX special-transaction window on Apothem and mainnet Aug 18, 2026
ak88 and others added 2 commits August 18, 2026 15:20
Reaching up from the NUnit work directory assumed both that it is the test
assembly's directory and that the source tree sits four levels above it.
`LoadEmbeddedOrFromFile` resolves a bare name against the copy Nethermind.Config
embeds, so the test no longer depends on the output layout.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same reasoning as the previous commit: the relative path assumed the NUnit work
directory is the assembly's and that the source tree sits four levels above it.
The pinned reference client checksums are unchanged, so the embedded copy the
loader resolves is equivalent to the file it was reading.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread src/Nethermind/Nethermind.Xdc.Test/XdcChainSpecTests.cs Outdated
Comment thread src/Nethermind/Nethermind.Xdc.Test/XdcChainSpecTests.cs Outdated
Co-authored-by: ak88 <anders.holmbjerg@hotmail.com>
@ak88
ak88 marked this pull request as ready for review August 18, 2026 15:47
@ak88
ak88 requested a review from cicr99 as a code owner August 18, 2026 15:47
@claude

claude Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @ak88's task in 6m 49s —— View job


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

  • Gather PR diff and context
  • Verify the AddTransitions fix and its fork-ID neutrality claim
  • Verify the new tests and the by-name chainspec loading change
  • Check for unintended release-spec regressions from the new boundaries
  • Post review

Verdict

Both fixes are correct and the reasoning in the PR body holds up under independent verification, including the fork-ID neutrality claim and the "no other release spec changes" claim, which I checked exhaustively rather than taking on trust. No Critical or High findings; nothing blocking. Three Low notes, one of which affects the release note rather than the code.

What I verified

The AddTransitions fix is safe by construction, and I confirmed nothing else moved. Adding a boundary at block B can only change flags whose threshold lies in (prev_boundary, B] — every other flag is a monotone X <= releaseStartBlock predicate, so subdividing is idempotent. I enumerated every threshold in the newly separated intervals:

chain new boundaries previously enclosed by other thresholds in the split range
mainnet 80,370,000 / 80,370,900 76,321,000 → 98,800,200 none¹
apothem 66,825,000 61,290,000 → 71,550,000 none

¹ Gas50xBlock and switchBlock are both 80,370,000, but Gas50xBlock is referenced only by XdcForkInfo.cs:45 (fork ID) and SwitchBlock is copied verbatim, not block-derived. So IsTIPXDCXMiner/IsTIPXDCXReceiver are the only flags that change — exactly the intent. TipXDCX (38,383,838 / 23,779,191) already coincided with eip152Transition, so those two adds are no-ops that dedupe in the SortedSet.

The divergence table checks out. Confirmed against the chainspecs: mainnet TIPXDCXMinerDisable 80,370,000 and TIPXDCXReceiverDisable 80,370,900 both fell between transitions 76,321,000 and 98,800,200; apothem TIPXDCXReceiverDisable 66,825,000 fell between 61,290,000 and 71,550,000; apothem TIPXDCXMinerDisable 61,290,000 already coincided with eip2565/eip2718/eip2930, which is why only 1 of the 2 assertions fails there pre-fix, as the author reported.

Fork IDs really are unaffected. XdcForkInfo.GetForkActivations already contributes all three blocks (XdcForkInfo.cs:41,46,47) into a SortedSet<ForkActivation>, and ForkActivation.CompareTo falls back to BlockNumber whenever either Timestamp is null — both CreateTransitionActivations and XdcForkInfo.Add build new ForkActivation(block) with a null timestamp, so the entries are compare-equal and collapse. Independently corroborated by the existing pins: XdcForkIdConformanceTests.cs:40 already asserts 66,825,000 is an activation with next 71,550,000, and :50-51 that 80,370,000 is one. Also verified biggestBlockTransition is unchanged (max is 98,802,000 mainnet / 83,600,000 apothem, both above the added blocks) and that neither chainspec has a single *TransitionTimestamp key — so the timestamp-spec path can't shift either.

The by-name chainspec loading is an improvement, not a risk. Nethermind.Config.csproj embeds ..\Chains\**\*.* as chainspec\%(Filename)%(Extension), so FileNameToResource("xdc.json")Nethermind.Config.chainspec.xdc.json resolves against typeof(IConfig).Assembly. If it ever didn't, LoadEmbeddedOrFromFile falls through to LoadFromFile, which throws FileNotFoundException — so there's no silent-wrong-file mode, and the test now exercises the copy a released node actually reads. Dropping the "../../../../" path dance from XdcForkIdConformanceTests is a genuine fragility removal.

Boundary assertions are arithmetically right (spot-checked all eight): e.g. mainnet GetXdcSpec(80370899).IsTIPXDCXReceiver → spec at 80,370,000 → 38383838 <= 80370000 && 80370000 < 80370900 → true; at 80,370,900 → false.

Style/robustness: matches repo conventions — file-scoped namespace, no var, [TestFixture, Parallelizable] with no shared state, XML doc on the fixture, TestCase parameterization instead of copy-paste, minimal diff. The !.Value dereferences are safe for these two chainspecs and would fail loudly rather than silently otherwise. Code Lint, Code formatting, Build solutions, and Spell check are all green in CI.

I could not run the tests locally — dotnet was not permitted in this sandbox, so assertions were verified by reading the derivation, not by execution. Nethermind tests (Flat DB) was still in progress at review time; worth a glance before merge.

Findings

# Severity Finding
1 Low Both new methods use TestName = "mainnet" / "apothem", and TestName replaces the method name — so all four cases collapse to two fully-qualified names, and a failure can't be attributed to a method. The sibling fixture already prefixes ("mainnet genesis", …) to avoid exactly this. (details) · Fix this →
2 Low TIP2019Block and BlackListHFNumber gate release-spec flags the same way but are still absent from AddTransitions. Verified not live — both coincide with eip7Transition: 1 and eip152Transition respectively on both chains — so they work only by coincidence, and IsBlackListingEnabled gates denylist enforcement. Four lines to make the invariant complete; fork IDs stay neutral for the same reason as the three added here. (details) · Fix this →
3 Low IsTIPXDCXMiner has no production reader — only XdcExtensions.Transactions.cs:21-24 gates the special path, and all four predicates use IsTIPXDCXReceiver. The fix and the assertions are still right, but the PR body's mainnet TIPXDCXMinerDisable row is behaviourally inert, so the release-note resync guidance should rest on the receiver rows alone. (details)

No security concerns (load-time only, no untrusted input). No performance concerns: the extra release specs are built once at startup, and GetXdcSpec binary-searches a list that grows by two entries.

Two notes on the earlier review thread, now settled:

  • Finding 1 from the previous round is fully addressed03753f3 registers all three blocks and XdcChainSpecTests.XDCX_flags_flip_on_their_own_blocks pins the boundaries. I re-derived the numbers independently and they match.
  • Findings 2 and 3 were explicitly declined with rationale (here) and recorded as follow-ups in the PR body. The duplicated loader is in fact now gone — f82ef4e/b14796f reduced it to a one-line call in each fixture, which is a better outcome than the shared helper I suggested. The unmapped-key guard genuinely does need rewardCheckpoint resolved first; deferring is the right call.

One judgement call worth stating plainly: XDCX_flags_flip_on_their_own_blocks derives its boundaries from the engine parameters, so it pins that the boundary lands where configured and cannot catch a wrong number in the chainspec. That's the author's stated intent and it's sound — the schedule itself is pinned to the reference client by XdcForkIdConformanceTests — but it does mean the two fixtures are load-bearing together, not separately.
· branch claude/xdcxaddrbinary-chainspec-align-9e43e9

Comment on lines +37 to +38
[TestCase("xdc.json", TestName = "mainnet")]
[TestCase("xdc-testnet.json", TestName = "apothem")]

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

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.

Comment on lines +112 to 118
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);
}

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.

Comment on lines +51 to +54
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);

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.

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.

`TestName` replaces the method name, so all four cases resolved to just
`XdcChainSpecTests.mainnet` and `XdcChainSpecTests.apothem`, leaving failure
reports unable to say which of the two methods failed. Dropping it lets NUnit
append the argument instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants