fix(docgen): document nullable JSON-RPC result payloads (#12837)

[hudem1]

Closes #12837

Changes

#12837 reports that several methods return a successful top-level null result where the generated API docs declare a non-null object/array/string. The implementation is correct — returning null on a missing entity matches the official execution-apis spec (result schema oneOf: [notFound, T], notFound being literally type: null) and Geth. The mismatch is a documentation defect.

• JsonRpcMethodAttribute — new opt-in ResultCanBeNull flag.
• tools/DocGen/JsonRpcGenerator.cs — emits `result` may be `null` in a successful response. when the flag is set.
• eth/debug/parity/rbuilder interfaces — set the flag on the 25 methods whose implementation actually returns Success(null).

Why an explicit flag instead of inferring from the return type

ResultWrapper<T?> is used throughout the codebase merely so the failure path can carry a default value — so the nullable annotation is not a reliable signal that a successful result can be null. Inferring from it would document a null result for methods that never return one on success (eth_gasPrice, eth_maxPriorityFeePerGas, eth_blockNumber, eth_getBalance, eth_estimateGas, the eth_new*Filter/eth_uninstallFilter methods, eth_getAccountInfo, …). The flag is set only after verifying each implementation's Success(null) path, so the note is correct by construction. This also picks up methods the annotation alone would miss (debug_getRawTransaction, parity_getBlockReceipts — issue item 7).

Flagged methods (all verified against their call sites): eth_getBlockByHash/ByNumber, eth_getHeaderByHash/ByNumber, eth_getUncleByBlock{Hash,Number}AndIndex, eth_getTransactionByHash, eth_getTransactionByBlock{Hash,Number}AndIndex, eth_getRawTransactionByHash, eth_getRawTransactionByBlock{Hash,Number}AndIndex, eth_getTransactionReceipt, eth_getBlockReceipts, eth_getBlockTransactionCountByHash/ByNumber, eth_getUncleCountByBlockHash/ByNumber, eth_baseFee, eth_blobBaseFee, eth_getBlockAccessList, eth_getAccount, debug_getRawTransaction, parity_getBlockReceipts, rbuilder_getAccount.

Types of changes

• Bugfix (a non-breaking change that fixes an issue)
• Documentation update

Not a breaking change: no method signatures change — only the [JsonRpcMethod] attribute gains a property. No binary/plugin break.

Testing

Requires testing

• No

Notes on testing

No behaviour change — the affected methods already returned Success(null), and existing EthRpcModuleTests assert "result":null for these lookups. Verified DocGen builds and that regeneration adds the note to exactly the 25 flagged methods (22 eth + debug + parity + rbuilder) with zero false positives.

Documentation

Requires documentation update

• Yes

The published docs (NethermindEth/docs) pick up the note on the next DocGen run against this branch.

Requires explanation in Release Notes

• No

[claude]

Claude finished @hudem1's task in 6m 8s —— View job

---

Review: fix(docgen): document nullable JSON-RPC result payloads

• Gather context (diff, linked issue successful responses return a top-level null result where the documentation declares a non-null object, array, or string #12837, repo rules)
• Review DocGen nullability detection
• Review interface/impl signature changes for breaking-change risk
• Check call sites, implementers, and overrides
• Post review (2 inline comments)

---

The diagnosis in the PR body is right: the implementation matches execution-apis (oneOf: [null, T]) and Geth, so this is a docs defect. The direction is good, and most of the methods in the issue (eth_getAccount, eth_getTransactionByHash, eth_getHeaderBy*, eth_getUncleBy*, eth_getRawTransactionBy*, eth_getTransactionReceipt, eth_getBlockReceipts) were already annotated nullable, so the DocGen change alone fixes them. My concern is with the inference the note is driven by, and with two methods left behind.

Findings: 0 Critical · 0 High · 2 Medium · 4 Low

Medium

1. The nullable annotation is not a reliable proxy for "result may be null on success" — this introduces ~10 new incorrect doc statements. (inline, tools/DocGen/JsonRpcGenerator.cs:237,251)

ResultWrapper<T?> is used widely in this repo purely so the failure path can carry a default, not to signal a nullable success payload. Verified against EthRpcModule, these all gain the note but never return a successful null: eth_gasPrice (L119), eth_maxPriorityFeePerGas (L155), eth_blockNumber (L175), eth_newFilter / eth_newBlockFilter / eth_newPendingTransactionFilter (L829-847), eth_uninstallFilter (L849), eth_getBalance, eth_estimateGas, eth_getAccountInfo (L1153, returns AccountInfoForRpc.Empty). Repo-wide there are 17 value-type ResultWrapper<T?> methods in this shape.

So the change trades ~12 correct notes for ~10 incorrect ones — the same class of defect it sets out to fix, pointing the other way. Suggestion: drive the note off an explicit ResultCanBeNull on JsonRpcMethodAttribute (additive, no existing code touched, correct by construction) instead of inferring it. Skipping Nullable<T> payloads would remove the bulk of the false positives as a cheaper partial fix, but eth_getAccountInfo / eth_createAccessList would remain wrong.

2. Annotation sweep is incomplete — #12837 is not fully closed. (inline, EthRpcModule.cs:619)

Still Success(null) behind a non-nullable payload, so still undocumented after this PR:

• debug_getRawTransaction — DebugRpcModule.cs:517-527, declares ResultWrapper<ArrayPoolList<byte>>.
• parity_getBlockReceipts — ParityRpcModule.cs:54, declares ResultWrapper<ReceiptForRpc[]> while delegating to ReceiptFinderExtensions.GetBlockReceipts (ResultWrapper<ReceiptForRpc[]?>, Success(null) on a block miss). This is item 7 of the issue, which names parity_getBlockReceipts explicitly.

Low

• Plugin surface. The four IEthRpcModule return types change annotation only, so this is not a binary break (BlockForRpc? and BlockForRpc are the same runtime type) — worth stating explicitly in the PR body given the AGENTS.md plugin-compat rule. In-tree there is exactly one implementer (EthRpcModule) plus the empty IOptimismEthRpcModule, and the four methods aren't virtual, so nothing in-tree can break; out-of-tree implementers get new nullability warnings only.
• XML docs. IsReturnNullable uses // lines; AGENTS.md asks for <summary> / <remarks> / <returns> for member-level documentation.
• Redundant check + duplicated unwrapping. Nullable.GetUnderlyingType(payload.Type) is not null (L264) appears redundant — NullabilityInfoContext already reports NullabilityState.Nullable for Nullable<T>. And the Task<…> unwrapping duplicates GetReturnType (L367-376); folding them together prevents drift.
• Diff noise / test nits. JsonRpcGenerator.cs:243 changes """\n<12 spaces>\n to """\n\n — identical raw-string output, pure noise per the "minimise the diff" rule. ResultWrapper<BlockForRpc?>.Success(...) in JsonRpcServiceTests.cs:216 is likewise a no-op (same runtime type). In EngineModuleTests.V1.cs:645,677, \.Data! immediately followed by Assert.That(blockForRpc, Is.Not.Null) reads oddly — asserting on .Data directly would be cleaner.

Notes on verification

Confirmed statically: L2Api.RetryGetBlock still type-checks (it already returned Task<BlockForRpc?>); WriteExpandedType always terminates with a newline on every path, so the inserted note renders as a well-formed Markdown paragraph; DocGen only reflects Nethermind.Consensus.Clique, Nethermind.Era1, Nethermind.Flashbots, Nethermind.HealthChecks, Nethermind.JsonRpc, so Optimism/Taiko/Merge nullable payloads (engine_getPayloadV*, taiko_*, rbuilder_*) are out of scope; DocGen.csproj sets no trimming, so NullabilityInfoContext is safe here.

I could not execute DocGen to diff the generated Markdown (dotnet run was not in the allowed tool set), so the false-positive/false-negative counts above come from reading every ResultWrapper<…?> declaration and its implementation rather than from generated output. Adding dotnet run --project tools/DocGen/DocGen.csproj to --allowedTools would let me confirm the rendered pages directly.

No correctness, security, or performance impact on the node — the RPC behaviour is unchanged and DocGen is offline tooling.
· branch fix/jsonrpc-nullable-result-docs

[claude]

Medium — the C# nullable annotation on the payload is not a reliable proxy for "the JSON-RPC result may be null on success", so this emits a batch of newly incorrect notes.

In this codebase ResultWrapper<T?> is very often used just so the failure path can carry a default value, not because success can be null. Verified in EthRpcModule (all of these will now get the note, none of them can return a successful null):

method	success path
eth_gasPrice	Success(await _gasPriceOracle.GetGasPriceEstimate()) (L119)
eth_maxPriorityFeePerGas	Success(gasPriceWithBaseFee) (L155-158)
eth_blockNumber	Success(number) (L175-179)
eth_newFilter / eth_newBlockFilter / eth_newPendingTransactionFilter	Success((UInt256)filterId) (L829-847)
eth_uninstallFilter	Success(false) / Success(true) (L849-859)
eth_getBalance	Success(balance) (L181+)
eth_estimateGas	executor always yields a value
eth_getAccountInfo	… : AccountInfoForRpc.Empty (L1153)

That's ~10 methods gaining a false claim, against ~12 gaining a correct one — the heuristic is close to a coin flip, and the generated sentence is a much stronger statement than the annotation actually supports.

Suggested alternative: drive the note off an explicit opt-in rather than inference, e.g. a ResultCanBeNull property on JsonRpcMethodAttribute (purely additive, no existing behaviour touched, and self-documenting at the declaration site). A cheaper partial mitigation is to skip Nullable<T> payloads (removes all 17 value-type cases repo-wide), but eth_getAccountInfo / eth_createAccessList would still be wrong.

Two smaller points on this method:

• Per AGENTS.md member-level docs should use XML doc comments (<summary> / <remarks> / <returns>) rather than // lines above the signature.
• Nullable.GetUnderlyingType(payload.Type) is not null looks redundant: NullabilityInfoContext already reports ReadState == Nullable for Nullable<T> payloads.
• The unwrapping of Task<ResultWrapper<T>> duplicates the logic in GetReturnType (L367-376); worth folding together so the two can't drift.

Fix this →

[claude]

Medium — the annotation sweep is incomplete, so #12837 is not fully closed. Two methods still return a successful null behind a non-nullable declared payload, and will therefore not get the new doc note:

• debug_getRawTransaction — DebugRpcModule.cs:517-527 declares ResultWrapper<ArrayPoolList<byte>> and returns Success(null) when the tx is not found.
• parity_getBlockReceipts — ParityRpcModule.cs:54 declares ResultWrapper<ReceiptForRpc[]> while delegating to ReceiptFinderExtensions.GetBlockReceipts, which is ResultWrapper<ReceiptForRpc[]?> and returns Success(null) on a block miss. This is item 7 of successful responses return a top-level null result where the documentation declares a non-null object, array, or string #12837, which names parity_getBlockReceipts explicitly.

Also note eth_getTransactionReceipt (L1157-1167) writes ResultWrapper<ReceiptForRpc>.Success(null) in a method whose signature is ResultWrapper<ReceiptForRpc?> — harmless, but it illustrates why the annotation isn't a load-bearing signal today.

Fix this →

[hudem1]

Thanks for the thorough review — force-pushed a redesign that adopts the explicit-opt-in approach. Point by point:

Medium 1 (annotation is not a reliable proxy → false notes). Agreed, and fixed at the root. Dropped the nullability inference entirely and added an opt-in ResultCanBeNull flag on [JsonRpcMethod]; the note is emitted only when it's set. I audited every Success(null) call site (plus eth_getAccount's inline … : null ternary) and set the flag on exactly those 25 methods. All the false positives you listed — eth_gasPrice, eth_maxPriorityFeePerGas, eth_blockNumber, eth_getBalance, eth_estimateGas, eth_new*Filter, eth_uninstallFilter, eth_getAccountInfo, eth_createAccessList — are not flagged (verified in the regenerated output: 0 notes on them).

Medium 2 (incomplete sweep). Both now flagged: debug_getRawTransaction and parity_getBlockReceipts (issue item 7). Also picked up rbuilder_getAccount, eth_baseFee, eth_blobBaseFee, and eth_getBlockAccessList, which are genuine Success(null) paths too. eth_feeHistory is not flagged — its Success(null) is an internal validation sentinel (_validationPassed), never surfaced.

Low — plugin surface. No signatures change anymore (the four IEthRpcModule return-type edits are gone), so there's no binary/plugin concern at all now. Called out explicitly in the PR body.

Low — XML docs. ResultCanBeNull has <summary>/<remarks>. The IsReturnNullable helper (with the // comment, the redundant Nullable.GetUnderlyingType check, and the duplicated Task<> unwrapping) is gone entirely.

Low — diff noise. The generator change is now a pure 6-line addition; the existing </TabItem> WriteLine block is byte-unchanged. The L2Api/test edits are gone since no signatures move.

Regeneration adds the note to exactly 25 methods (22 eth + debug + parity + rbuilder), zero false positives. Full solution + DocGen build clean.

[wurdum]

Worth adding ResultCanBeNull to debug_getConfigValue too.