feat(xdc): read XDPoS rewards as XDC amounts - #12852
Conversation
The reference client states masternode, protector and observer rewards in XDC and scales them to wei on load, so Apothem's genesis carries values such as 63420000000000001704 rather than a round 6.342e19. Nethermind's chainspec copied those pre-converted integers, which left the client unable to read a reference genesis and left the odd constants unexplained. Add a converter that reproduces the reference conversion exactly: decode the literal to a float64, form the exact product with 10^18, round it to a 64-bit significand and truncate toward zero. Neither double nor decimal arithmetic reaches those values, so the product is built in BigInteger and rounded explicitly. Apply it per property, which confines the unit convention to engine.XDPoS.params: the converters registered on EthereumJsonSerializer are untouched, so the rest of the chainspec and JSON-RPC keep rejecting fractional numbers as EIP-1474 requires. Restate the Apothem rewards as the reference amounts; a test loads the shipped file and pins the wei they resolve to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @ak88's task in 9m 28s —— View job Review —
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| [JsonConverter(typeof(XdcToWeiConverter))] | ||
| public UInt256 MasternodeReward { get; init; } | ||
| [JsonConverter(typeof(XdcToWeiConverter))] | ||
| public UInt256 ProtectorReward { get; init; } | ||
| [JsonConverter(typeof(XdcToWeiConverter))] | ||
| public UInt256 ObserverReward { get; init; } |
There was a problem hiding this comment.
Critical — this breaks the XDPoS_networkInformation JSON-RPC method.
V2ConfigParams is not only a chainspec type; it is reused verbatim as an RPC response DTO:
RPC/XDPoSConfig.cs:17—public List<V2ConfigParams>? V2Configs { get; set; }RPC/NetworkInformation.cs:17—public XDPoSConfig? ConsensusConfigs { get; set; }RPC/XdcRpcModule.cs:626—V2Configs = spec.V2Configs
A property-level [JsonConverter] applies on write too, so serializing the XDPoS_networkInformation response now calls XdcToWeiConverter.Write and throws NotSupportedException. Because JSON-RPC serializes straight into the response stream, this doesn't just fail the call cleanly — the exception lands mid-object, after NetworkInformation/XDPoSConfig have already been partially written.
The PR description states "Nothing serializes engine parameters"; that is the assumption that doesn't hold here.
Before this PR the endpoint emitted these three fields through the default UInt256 converter. Options, roughly in the order the repo guidelines prefer:
- Make
Writereproduce the previous output (delegate to the defaultUInt256handling) rather than throwing. The read/write asymmetry is real but confined, and it keeps the wire format unchanged. - Give the RPC path its own DTO so the chainspec unit convention never reaches a response type.
Either way this needs a regression test — Nethermind.Xdc.Test currently has no coverage of XDPoS_networkInformation at all, which is why 590/590 passed.
|
|
||
| int byteCount = wei.GetByteCount(isUnsigned: true); | ||
| if (byteCount > UInt256ByteCount) ThrowNotAnAmount(literal); |
There was a problem hiding this comment.
High — the only upper bound is UInt256 overflow, so an un-migrated wei value loads silently.
This is the migration hazard the PR flags in the release notes, but nothing here catches it. Take the exact value this PR removes from xdc-testnet.json:
"MasternodeReward": 63420000000000001704
Read as XDC that is 6.342e19 × 10^18 ≈ 6.342e37 wei — comfortably under 2^256 ≈ 1.16e77, so byteCount > UInt256ByteCount never trips. The node starts and pays every masternode a reward inflated by 10^18, with no log line and no load failure. The rejection test only covers 1e60; 1e58 would be accepted.
That matters more than a normal config typo because XDC subnets are operator-deployed with hand-written chainspecs (XdcSubnetChainSpecEngineParameters), and the field name and JSON shape are unchanged — an operator gets no signal that the unit moved under them.
A plausibility bound would catch 100% of wei-denominated values, since they are 18 orders of magnitude above any real reward. Rejecting anything above, say, 10^12 XDC (still far above total supply, and ~25 orders of magnitude below the UInt256 ceiling) turns a silent consensus divergence into a startup error, at the cost of one comparison. Worth a named constant with the rationale in its doc comment.
There was a problem hiding this comment.
no subnets have even been deployed, this is not relevant
| /// <remarks> | ||
| /// The reference client decodes these fields into a float64 and scales them by 10^18 through a | ||
| /// <c>big.Float</c> carrying the 64-bit significand its wei multiplier is built with, then truncates | ||
| /// toward zero. Both steps are load-bearing: <c>63.42</c> has to produce | ||
| /// 63420000000000001704 — the value Apothem's genesis was generated with — and neither | ||
| /// <see cref="double"/> nor <see cref="decimal"/> arithmetic lands there. So the exact product is | ||
| /// formed in <see cref="BigInteger"/> and rounded explicitly. | ||
| /// <para> | ||
| /// Only a JSON number is accepted. A hex QUANTITY would have to mean wei, and carrying two units on | ||
| /// one field is how a reward ends up wrong by a factor of 10^18. | ||
| /// </para> | ||
| /// <para> | ||
| /// Applying this per property keeps the XDC unit convention inside <c>engine.XDPoS.params</c>. The | ||
| /// converters registered on <see cref="EthereumJsonSerializer"/> are untouched, so the rest of the | ||
| /// chainspec — and JSON-RPC, where EIP-1474 requires a QUANTITY — still rejects a fractional number. | ||
| /// </para> | ||
| /// </remarks> |
There was a problem hiding this comment.
The derivation checks out — I reproduced all three Apothem amounts by hand. Recording the working here since the PR asks for a second pair of eyes on it, and since it's the part no reviewer can check by reading the reference format alone.
For each literal: take the exact double, form the exact product with 10^18, round to a 64-bit significand (half-to-even), truncate.
| Literal | double significand (exact real value) |
Exact product | ulp @ 64 bits | Rounded → truncated |
|---|---|---|---|---|
63.42 |
8925571511494902 / 2⁴⁷ (exact: …901.76) |
63420000000000001705.30 |
4 (2⁶⁵ ≤ x < 2⁶⁶) | …1704 ✓ |
50.27 |
7074873539622339 / 2⁴⁷ (exact: …338.56) |
50270000000000003126.39 |
4 | …3128 ✓ |
25.13 |
7073466164738785 / 2⁴⁸ (exact: …785.28) |
25129999999999999005.24 |
2 (2⁶⁴ ≤ x < 2⁶⁵) | …9006 ✓ |
All three land exactly on the values Apothem's genesis carries. The two nearby hypotheses both fail, so the rule is pinned rather than merely consistent:
- Truncate the exact product, no intermediate rounding →
…1705/…3126/…9005. Wrong on all three. - Round to 53 bits (what you'd get if the Go receiver already had
prec == 53, i.e.bigval.Mul(bigval, coin)rather thannew(big.Float).Mul(...)) → ulp is 8192 at6.342e19, and63420000000000000000happens to be an exact multiple of 8192, so63.42collapses to the round value. Wrong.
Note the sign structure is what makes this convincing: two amounts round up past the round value and one rounds down, and the offsets (+1704, +3128, −994) aren't a common relative factor — they track the individual double representation error of each literal. That's hard to hit by accident.
Two small things on the comment itself:
ReferencePrecisionBits = 64is the load-bearing constant and its provenance is "the three Apothem values are consistent with it". Worth naming the reference source (repo/file/function) in the<remarks>so a future reader can confirm rather than re-derive. In Go terms the 64 comes fromSetIntraising a zero-precisionbig.Floatto 64, and it only survives into the product if the receiver's precision is unset — which is exactly the detail that would silently change the answer if the reference ever restructured that expression.- Line 24 says the multiplier "carr[ies] the 64-bit significand its wei multiplier is built with", but 10¹⁸ needs only 60 bits — the 64 is a floor, not a fit. Slightly clearer as "whose precision defaults to 64 bits".
| ReadOnlySpan<byte> literal = reader.HasValueSequence ? reader.ValueSequence.ToArray() : reader.ValueSpan; | ||
| if (!reader.TryGetDouble(out double amount) || !double.IsFinite(amount) || amount < 0) | ||
| { | ||
| ThrowNotAnAmount(literal); | ||
| } | ||
|
|
||
| return ToWei(amount, literal); |
There was a problem hiding this comment.
Low — -0.0 slips through. amount < 0 is false for negative zero, so "-0.0" is accepted and ToWei returns default. Harmless in effect, but it's the one negative literal the rejection path lets past, and double.IsNegative(amount) closes it without changing any other case.
Low — literal is computed on every read, including the success path. reader.ValueSequence.ToArray() allocates, and the value is only ever used to build an exception message. Chainspec load isn't a hot path so this is cosmetic, but deferring the materialization into the two throw helpers (they can take the reader's raw bytes, or the Utf8JsonReader by in) keeps the happy path allocation-free and matches the repo's low-allocation preference.
| [TestCase("-1", TestName = "Negative")] | ||
| [TestCase("1e60", TestName = "Beyond UInt256 once scaled")] | ||
| [TestCase("\"0x37f0e6c9e9dd0e0000\"", TestName = "Hex string, which would have to mean wei")] | ||
| [TestCase("\"63.42\"", TestName = "Quoted amount")] | ||
| public void Reward_that_is_not_an_xdc_amount_is_rejected(string literal) => | ||
| Assert.That(() => LoadEngineParameters(literal), Throws.TypeOf<InvalidDataException>()); |
There was a problem hiding this comment.
Medium — the rejection cases don't cover the one input the migration actually produces.
1e60 only proves the UInt256 ceiling works. The realistic bad input is an operator chainspec that still states the reward in wei — literally the value this PR deletes from xdc-testnet.json:
[TestCase("63420000000000001704", TestName = "Reward still stated in wei")]Today that case would fail, because 6.342e37 wei fits in a UInt256 and loads silently (see the comment on XdcToWeiConverter.cs). It's worth adding alongside whatever bound you settle on, so the test documents the migration boundary rather than just the arithmetic one.
Two more gaps worth closing while you're here:
- The write path is untested, which is why the
XDPoS_networkInformationbreakage got through. A test that serializes aV2ConfigParams(or the wholeNetworkInformationresponse) would have caught it. Shipped_testnet_rewards_are_unchangedreadsV2Configs[^1]with the comment "the newest config … sorts last by switch round". That's true, but the assertion is only meaningful because that entry happens to be the one carrying rewards. If a later config is appended without reward fields, this silently starts asserting three zeros against three non-zero expectations — it'd fail, so it's safe, just confusingly. Selecting bySwitchRoundwould say what the test means.
V2ConfigParams is not only a chainspec type: it is the XDPoS_networkInformation response DTO, reached through XDPoSConfig.V2Configs. A property-level JsonConverter applies on write, so the NotSupportedException thrown by XdcToWeiConverter.Write landed mid-object once the response had begun serializing. Write now emits wei, exactly what the endpoint produced before, and a test serializes the response to pin it. Reading XDC while writing wei is asymmetric, but deliberate: operators configure in XDC as the reference does, and the API reports wei as every other Ethereum endpoint does. Nothing round-trips the type. Bound the amount at 10^12 XDC. UInt256 overflow was the only ceiling, and a reward left in the previous wei spelling is ~10^18 times any real value yet still far inside UInt256 -- so it started the node and inflated every payout silently. Above the bound the load now fails with a message naming the unit. Also correct the two README entries still documenting these fields as Wei, reject negative zero, defer the literal to the throw paths, and select the Apothem config by switch round rather than position. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Re-review — follow-up commit
|
No subnets have been deployed, and the shipped chainspecs are embedded in the build and replaced on upgrade, so there is no wei-denominated config for the bound to catch. It was guarding a migration that cannot happen. Restore the UInt256 overflow check the bound had made unreachable, which also makes the "beyond UInt256 once scaled" case exercise what its name claims again. Assert all three rewards in the networkInformation test rather than the masternode one alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @ak88's task in 3m 1s —— View job Re-review —
|
| Wei | Hex | ✓ |
|---|---|---|
63420000000000001704 |
0x3702119fc874606a8 |
✓ |
50270000000000003128 |
0x2b9a2eaa87ae30c38 |
✓ |
25129999999999999006 |
0x15cbfb1db0590fc1e |
✓ |
Each is a 17-digit value = d·2⁶⁴ + …; all three reconcile exactly. Asserting all three now covers all three attributes, as suggested.
XdcSubnetChainSpecEngineParameters only overrides SealEngineType and inherits the reward properties, so the annotations apply on the subnet path too — no shadowing to worry about.
Nothing else in the tree reads these fields through a serializer: XdcReleaseSpec / XdcRewardCalculator / XdcChainSpecBasedSpecProvider all take the already-converted UInt256, and the // in Wei comments on XdcReleaseSpec.cs:29-31 remain accurate — correctly left alone.
On dropping the bound
Your call, and the reasoning in the commit message is sound for the shipped chainspecs. One loose end it leaves: the PR's release-note paragraph still says an operator chainspec passed via --Init.ChainSpecPath "would need to be restated in XDC" without saying what happens if it isn't. With the bound gone, a wei-denominated value loads and inflates payouts by 10¹⁸ with no error — worth one clause in the release note so the operator-facing text matches the behaviour. Purely a wording change; no code implied.
Optional nits
ThrowTooLarge's message — "exceeds the largest amount representable in wei" is a shade off; the ceiling isUInt256, not wei as a unit. "…representable as aUInt256of wei" would be exact.- No case near the real boundary.
1e60is a full order of magnitude past the ceiling (~1.1579e59XDC). A pair straddling it would pin the guard rather than just exercise it — genuinely optional, since the ceiling isn't a value anyone configures.
Verification caveat
I read the code and did the arithmetic by hand. dotnet build was denied by the tool allowlist in this job (as in the previous round), so I could not compile or run Nethermind.Xdc.Test — the test results are your CI's word, not mine.
Verdict
Ship it. The revert restores a correct guard, the test changes are accurate, and the earlier blockers stay fixed.
· branch claude/chainspec-float-compatibility-b4af83
|
Claude finished @ak88's task in 5m 3s —— View job Review —
|
|
|
||
| [DoesNotReturn, StackTraceHidden] | ||
| private static void ThrowTooLarge(in Utf8JsonReader reader) => | ||
| throw new JsonException($"'{Literal(in reader)}' XDC exceeds the largest amount representable in wei"); |
There was a problem hiding this comment.
Low — the message names the wrong ceiling, and nothing pins it.
wei is bounded by UInt256, not by wei as a unit — an amount larger than this is perfectly representable in wei, just not in 256 bits. Exact wording would be "…exceeds the largest amount representable as a UInt256 of wei".
Related: the only case exercising this guard is 1e60, a full order of magnitude past the ceiling ((2^256 − 1) / 10^18 ≈ 1.1579e59 XDC). It proves the guard fires, but a pair straddling the boundary would pin where it fires. Optional — the ceiling isn't a value anyone configures, and the guard's real job here is to stop the oversized wei.TryWriteBytes into bytes[(32 - byteCount)..] on line 101, which it does correctly by sitting before it.
flcl42
left a comment
flcl42
left a comment
There was a problem hiding this comment.
Found 1 high-severity issue in src/Nethermind/Nethermind.Xdc/Spec/XdcToWeiConverter.cs.
| ThrowNotAnAmount(in reader); | ||
| } | ||
|
|
||
| return ToWei(amount, in reader); |
There was a problem hiding this comment.
[HIGH] Legacy wei-denominated XDC chainspecs are silently rescaled
Before this PR, these UInt256 fields and the module documentation treated numeric values as wei. An existing custom XDPoS or subnet chainspec can therefore contain 63420000000000001704; this path now treats it as XDC and produces roughly 6.342e37 wei, which still fits UInt256, so startup succeeds. Once TIPUpgradeReward is active, that value is minted directly, causing approximately 10^18-overpayments and a state-root split from nodes using the previous interpretation. A migration discriminator or explicit rejection of the legacy representation would prevent the silent unit change.
There was a problem hiding this comment.
There is no production configs out there, so this is not a problem
|
Claude finished @ak88's task in 2m 44s —— View job Re-review — no code change since the last pass
On the wei-migration finding (@flcl42's HIGH, your "no production configs out there")The mechanism is real — it's the same finding I raised in round one, and neither of us disputes the code path: a chainspec still stating
One supporting point I hadn't made before, which I think settles it in your favour more cleanly than the deployment argument does:
The Remaining items — all Low, all optional, all unchanged
Say the word and I'll push 1–3; they're a few lines total and none touch the conversion. Re-confirmed clean this round
Verification caveatStatic review only. VerdictShip it. The only thing I'd still ask for is the release-note clause, and that's prose. |
3 participants
Changes
XdcToWeiConverter, which reads an XDPoS reward stated in XDC (63.42,1,2.5e1) and yields the amount in wei.MasternodeReward/ProtectorReward/ObserverRewardonXdcChainSpecEngineParametersandV2ConfigParams.xdc-testnet.jsonas the reference amounts (63.42/50.27/25.13) instead of pre-converted wei.Why
The reference client states these rewards in XDC and scales them to wei on load. That conversion is lossy in a specific way, so Apothem's genesis carries
63420000000000001704rather than a round6.342e19. Nethermind's chainspec copied those pre-converted integers, which left the client unable to read a reference genesis and left the constants unexplained.The converter reproduces the reference conversion exactly: decode the literal to a
float64, form the exact product with10^18, round that to a 64-bit significand (half-to-even), then truncate toward zero. Both steps are load-bearing — plainfloat64(63.42) * 1e18gives a round6.342e19, which is wrong by 1704 wei per block — and neitherdoublenordecimalarithmetic lands on the reference values, so the product is formed inBigIntegerand rounded explicitly.Scope
Applying the converter per property confines the XDC unit convention to
engine.XDPoS.params.ChainSpecJsonkeeps the engine section as rawJsonElement,JsonElement.ToString()preserves the literal text, and a property-level[JsonConverter]outranks the options-level ones — so the converters registered onEthereumJsonSerializerare untouched and the rest of the chainspec, plus JSON-RPC, keep rejecting fractional numbers as EIP-1474 requires. No code outsideNethermind.Xdcchanged.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
XdcChainSpecRewardTests— 19 cases, all driven through the realChainSpecLoaderrather than the converter in isolation, so they also cover theChainSpecParametersProviderhop.UInt256once scaled, hex strings and quoted amounts.Shipped_testnet_rewards_are_unchangedloads the embeddedxdc-testnet.jsonand asserts the three original wei integers, so any drift in the rounding fails the build.period), inparams.eip150Transition, and inparams.terminalTotalDifficulty.590/590
Nethermind.Xdc.Testpass;dotnet format whitespace --verify-no-changesis clean.Documentation
Requires documentation update
Requires explanation in Release Notes
MasternodeReward,ProtectorRewardandObserverRewardinengine.XDPoS.paramsare now read as XDC amounts rather than wei, matching the reference client. This affects whole numbers too:"MasternodeReward": 5now means 5 XDC. A hex string is no longer accepted on these fields, since it could only mean wei and carrying two units on one field is how a reward ends up wrong by a factor of 10^18.The shipped chainspecs are embedded in the build and replaced on upgrade, so no migration is needed for
xdcorxdc-testnet. An operator-supplied chainspec passed via--Init.ChainSpecPaththat states these rewards in wei would need to be restated in XDC.Remarks
The conversion rule was derived from the three amounts Apothem's genesis already carries and verified to reproduce all of them exactly. Worth a second pair of eyes on that derivation, since it is consensus-relevant: it is the only part of this change that cannot be checked by reading the reference format alone.
WritethrowsNotSupportedException(same asBlockRewardConverter) — wei to XDC is not invertible, and emitting the wei value would read back scaled by 10^18. Nothing serializes engine parameters.🤖 Generated with Claude Code