fix(ReceiptForRpc): omit non-spec error field from receipt JSON-RPC responses

[manusw7]

Fixes #11207

Changes

• Mark ReceiptForRpc.Error with [JsonIgnore] so it is no longer emitted in JSON-RPC responses. The Ethereum execution-apis ReceiptInfo schema declares additionalProperties: false, prohibiting the non-standard error field that Nethermind previously included for failed transactions.
• Endpoints affected: eth_getBlockReceipts, eth_getTransactionReceipt, parity_getBlockReceipts, proof receipt sub-object in ProofRpcModule, and the internal DataFeed monitoring stream.
• Update existing EthRpcModuleTests hardcoded JSON expectations to drop ,"error":"error".
• Add regression test Error_field_is_not_serialized in ReceiptsForRpcTests asserting the field is absent from serialized output.

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

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

• Added unit regression test in Nethermind.JsonRpc.Test/Data/ReceiptsForRpcTests.cs.
• We should verify end-to-end against rpc-tests fixtures: eth_getBlockReceipts/test_01 (pre-Byzantium block 0x41B582 with BadInstruction) and test_07 (post-Byzantium block with revert reasons) now match Geth-aligned expectations.

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

JSON-RPC receipt responses no longer include the non-standard error field on failed transactions. This aligns Nethermind with the Ethereum execution-apis spec and Geth behavior.
Clients that relied on this field for failure reasons should use debug_traceTransaction instead.

[claude]

Claude finished @manusw7's task in 3m 29s —— View job

---

PR Review

• Gather context and read changed files
• Load relevant agent rules
• Review ReceiptForRpc.cs changes
• Review test changes
• Assess correctness, edge cases, and regressions
• Post final review

---

Summary

The fix is spec-correct: ReceiptInfo in execution-apis declares additionalProperties: false, so emitting the non-standard error field was a protocol violation. Removing it via [JsonIgnore] is the right direction, and the regression test is well-written.

One medium issue needs to be addressed or explicitly acknowledged before merge.

---

Findings

Medium — [JsonIgnore] silently suppresses deserialization of Error too

[JsonIgnore] without a condition is JsonIgnoreCondition.Always, which excludes the property from both serialization and deserialization. The serialization suppression is the intended fix, but there is a deserialization regression:

debug_insertReceipts (DebugRpcModule.cs:213) deserializes the caller's JSON body directly into ReceiptForRpc[] and then calls ToReceipt(). Before this PR, an "error" field the caller provided was round-tripped into the stored TxReceipt.Error. After this PR it is silently dropped — ToReceipt() always produces TxReceipt { Error = null }.

TxReceiptConverter.Read() is not additionally affected: its Write() counterpart never emitted error, so the round-trip was already lossy there before this PR.

The practical impact is low (debug_insertReceipts is debug-only; error is non-spec data), but the behavioural change should be explicitly acknowledged — either with a short comment on the property, or a test that documents the new inbound behaviour.

Low — No coverage of the deserialization path

The new Error_field_is_not_serialized test in ReceiptsForRpcTests covers outbound serialization correctly. There is no test documenting that debug_insertReceipts now ignores incoming error fields. Worth adding to prevent silent confusion later.

---

What looks good

• The [JsonIgnore] placement and the constructor assignment (Error = string.IsNullOrEmpty(receipt.Error) ? null : receipt.Error) are both correct — the field remains available as internal state for ToReceipt() on instances constructed from a TxReceipt, which is the right design.
• EthRpcModuleTests golden-string updates are minimal and accurate.
• The regression test correctly checks both the key name and the value are absent from the serialized output.
• The Optimism derivation path (EthereumEthApi.GetReceiptsByHash) is unaffected in practice because the pipeline uses Logs for deposit derivation, not Error.

[claude]

Medium — [JsonIgnore] suppresses deserialization too, silently breaking debug_insertReceipts

Plain [JsonIgnore] is equivalent to JsonIgnoreCondition.Always, which excludes the property from both serialization and deserialization in System.Text.Json. The serialization suppression is the intended fix, but the deserialization side effect is a regression.

Affected path — DebugRpcModule.debug_insertReceipts (line 213):

debugBridge.InsertReceipts(blockParameter, receiptForRpc.Select(static r => r.ToReceipt()).ToArray());

ReceiptForRpc is deserialized from the caller's JSON body. With [JsonIgnore], any "error" field the caller includes is now silently dropped, so ToReceipt() always produces TxReceipt { Error = null }. Previously the caller's error string was preserved.

TxReceiptConverter.Read() is not additionally affected: TxReceiptConverter.Write() never emitted error, so the round-trip was already lossy there before this PR.

Since error is non-spec data and debug_insertReceipts is a debug-only endpoint the practical impact is low, but the behavioural change should be acknowledged. If you want to preserve read-side behaviour, a [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] won't help (it would still emit non-null values); a custom converter or a separate setter-only property would be needed — likely overkill here. Explicitly documenting the intent (e.g. a comment or a test) would be sufficient.

Fix this →

[manusw7]

@claude review again

[claude]

Claude finished @manusw7's task in 1m 10s —— View job

---

PR Review

• Gather context and read changed files
• Load relevant agent rules
• Review ReceiptForRpc.cs changes
• Review test changes
• Assess correctness, edge cases, and regressions
• Post final review

---

Summary

The previous Medium finding (deserialization side-effect on debug_insertReceipts) has been fully addressed by the new Error_field_is_not_deserialized test in ReceiptsForRpcTests.cs. The test explicitly asserts both that receiptForRpc.Error is null and that ToReceipt().Error is null when incoming JSON contains an "error" field — exactly the documentation that was requested.

The PR is spec-correct, well-tested, and no outstanding Medium (or above) issues remain.

---

What's now in the PR

File	Change
ReceiptForRpc.cs	[JsonIgnore] on Error property — suppresses emission in all JSON-RPC responses
ReceiptsForRpcTests.cs	Error_field_is_not_serialized — outbound path; Error_field_is_not_deserialized — inbound path (new, addresses prior Medium)
EthRpcModuleTests.cs	Three golden-string assertions updated to drop ,"error":"error"

---

What looks good

• [JsonIgnore] (≡ JsonIgnoreCondition.Always) is the correct attribute — the field must never be emitted regardless of value.
• The constructor still populates Error from TxReceipt.Error, so the in-memory property is correct for server-side receipts; only the JSON projection is suppressed.
• Error_field_is_not_deserialized correctly asserts both receiptForRpc.Error and receiptForRpc.ToReceipt().Error are null after deserialization of a JSON body containing "error". This explicitly documents and validates the debug_insertReceipts behavior change.
• All three EthRpcModuleTests golden strings are accurately updated.
• parity_getBlockReceipts, eth_getTransactionReceipt, eth_getBlockReceipts, ProofRpcModule, and DataFeed all inherit the fix through the shared ReceiptForRpc serialization.
• No LINQ-in-hot-path issues introduced; no new allocations.

---

Low — No remaining issues

No Critical, High, or Medium findings.

[svlachakis]

@manusw7 please attach hive tests job run

[manusw7]

@manusw7 please attach hive tests job run

https://github.com/NethermindEth/nethermind/actions/runs/25318851815

[LukaszRozmej]

why not remove the property entirely?

[manusw7]

my though was that even if it should not be in the rpc response, keeping error internally can still be useful for internal use/debugging

[LukaszRozmej]

not really, it is there in Receipt, we can drop it from RPC type