fix(frames): decode pre-2D scalar gas_used in stored frame-tx receipts - #12957
fix(frames): decode pre-2D scalar gas_used in stored frame-tx receipts#12957AnkushinDaniil wants to merge 1 commit into
Conversation
|
Claude finished @AnkushinDaniil's task in 6m 7s —— View job Review:
|
| # | Severity | Finding |
|---|---|---|
| 1 | Medium | Scalar fallback silently produces a receipt that is not wire-faithful — the [total, 0] split is re-encoded onto the network by SyncServer.GetReceipts, and re-persisted by ReceiptsRecovery. Needs an acknowledgement/doc note, not code. (inline) |
| 2 | Low | No why comment at the branch — nothing marks it as eip8141-frame-txs-devnet7 read-compat or says when it can be deleted. (inline) |
| 3 | Low | The 14-line block is byte-identical across the two decoders (plus a third near-copy in ReceiptMessageDecoder); coding-style.md asks for 5+ line repeats to be extracted. (inline) |
| 4 | Low | New fixture re-implements Log(byte) / AssertFrameReceiptsEqual, which already exist in FrameTxReceiptDecoderTests.cs in the same folder and namespace. (inline) |
| 5 | Low | Coverage gap: no mixed pre-2D/2D array (the realistic post-upgrade DB shape), and no DecodeStructRef case over pre-2D bytes. (inline) |
On #1 — the substance is that stored receipts get re-encoded with ReceiptMessageDecoder69 when serving GetReceipts, and the wire format carries the two-element gas_used. For a pre-2D record we serve [total, 0], so the requesting peer's receipts-root check fails on those blocks; and LogFinder.cs:290 can re-insert the wrong split permanently. This is inherent to the format break — those blocks' header roots were computed under the old format anyway, and before this PR the same read threw outright — so I'm not asking for code. But it converts a loud failure into a silent wrong value, and right now the trade-off is recorded only in the PR description. A <remarks> on DecodeFrameReceipts would put it where the next reader will find it.
Not an issue, for the record: the PR description's eth_getLogs claim holds. eth_getLogs reaches a full decode via LogFinder.FilterLogsInBlockHighMemoryAllocation → _receiptFinder.Get, even though the ReceiptsIterator/DecodeStructRef path skips the frame extension entirely.
Scope notes
- Leaving
ReceiptMessageDecoderuntouched is the right call for a local-DB read-compat PR, and is stated in the description. Worth being aware that it means receipt sync from a not-yet-upgraded devnet7 peer still throws — devnet coordination territory, not this PR's. - Nothing here touches a hot path in a way that matters: one span index per frame, no allocation.
Caveat on verification
I could not run dotnet test or the formatter — both commands were denied in this environment, so the "without the fallback this throws RlpException" claim is taken from the PR description rather than reproduced, and the new test is reviewed statically only. Everything above is from reading the code and hand-decoding the fixtures. Adding Bash(dotnet test) / Bash(dotnet format) to --allowedTools would let me confirm.
--- · branch daniil/frame-receipt-storage-compat
| ulong executionGasUsed; | ||
| ulong stateGasUsed; | ||
| if (decoderContext.IsSequenceNext()) | ||
| { | ||
| int gasUsedEnd = decoderContext.ReadSequenceLength() + decoderContext.Position; | ||
| executionGasUsed = decoderContext.DecodeULong(); | ||
| stateGasUsed = decoderContext.DecodeULong(); | ||
| decoderContext.Check(gasUsedEnd); | ||
| } | ||
| else | ||
| { | ||
| executionGasUsed = decoderContext.DecodeULong(); | ||
| stateGasUsed = 0; | ||
| } |
There was a problem hiding this comment.
Medium — the scalar fallback is silent, and the lost split leaks out of the local DB.
The shape sniff itself is sound: a ulong RLP scalar is always < 0xc0 (either a single byte < 0x80 or an 0x81..0x88 prefix), so IsSequenceNext() can never confuse a pre-2D scalar with the new [execution, state] list. And because TxFrameReceipt.GasUsed => ExecutionGasUsed + StateGasUsed, mapping total → execution, 0 → state preserves the per-frame total, so GasUsedTotal and the frame gasUsed reported over JSON-RPC stay correct. Good.
What isn't local-only, though, is the split. Stored receipts are re-encoded with ReceiptMessageDecoder69 when serving GetReceipts (SyncServer.GetReceipts → ReceiptsMessageSerializer69), and the wire encoding carries gas_used as the two-element list. For a pre-2D record we will serve [total, 0], which no correctly-executing peer computed — so the requester's receipts-root check fails on those blocks. The same wrong split also gets baked in permanently if ReceiptsRecovery re-inserts the block (LogFinder.cs:290 → _receiptStorage.Insert).
This is inherent to the format break rather than introduced here (the old headers' receipt roots were computed with the old format anyway, and before this PR the same read threw outright), so I don't think it needs code. But it moves a loud failure to a silent-wrong-value, which is exactly what .agents/rules/robustness.md warns about, and right now the only record of that trade-off is the PR description. Please capture it where a future reader will see it — e.g. a short <remarks> on DecodeFrameReceipts noting that the scalar path is devnet7 read-compat, that it attributes the whole scalar to the execution dimension, that the resulting receipt is not wire-faithful, and when the branch can be deleted.
Low — no why at the branch itself. As written this is a bare shape test with no hint that it exists for eip8141-frame-txs-devnet7 on-disk data. Per AGENTS.md, this is precisely the non-obvious why that warrants a comment.
Low — DRY. These 14 lines are now byte-identical to CompactReceiptStorageDecoder.cs:188-201, and ReceiptMessageDecoder.cs:127-130 is a third near-copy of the 2D read. .agents/rules/coding-style.md asks for 5+ line repeats to be extracted; the two DecodeFrameReceipts bodies differ only in the log decoder and the list type, so a shared internal static void DecodeFrameGasUsed(ref RlpReader, out ulong execution, out ulong state) would collapse this without adding public surface.
| Assert.That(frames[2].Logs, Is.Empty); | ||
| } | ||
|
|
||
| private static void AssertScalarFrame(TxFrameReceipt frame, byte expectedStatus, ulong expectedExecution, byte logMarker) | ||
| { | ||
| Assert.That(frame.Status, Is.EqualTo(expectedStatus)); | ||
| Assert.That(frame.ExecutionGasUsed, Is.EqualTo(expectedExecution), | ||
| "the pre-2D scalar gas_used maps to the execution dimension"); | ||
| Assert.That(frame.StateGasUsed, Is.EqualTo(0UL), | ||
| "an old-format receipt carries no state dimension"); | ||
| Assert.That(frame.Logs, Has.Length.EqualTo(1)); | ||
| Assert.That(frame.Logs[0].Address, Is.EqualTo(TestItem.AddressB)); | ||
| Assert.That(frame.Logs[0].Data, Is.EqualTo(new[] { logMarker })); | ||
| Assert.That(frame.Logs[0].Topics, Is.EqualTo(new[] { Keccak.Compute(new[] { logMarker }) })); | ||
| } |
There was a problem hiding this comment.
Low — duplicates helpers that already exist one file over. FrameTxReceiptDecoderTests.cs, same folder and same namespace, already has AssertFrameReceiptsEqual, AssertLogsEqual, CreateStorageFrameReceipt, and
private static LogEntry Log(byte marker) =>
new(TestItem.AddressB, [marker], [Keccak.Compute([marker])]);AssertScalarFrame re-derives exactly that shape inline (AddressB, [marker], [Keccak.Compute([marker])]), and the frames-0/1 assertions re-implement AssertFrameReceiptsEqual. AGENTS.md is explicit here: "Before adding a new test, check whether an existing one can be extended" and "factor those parts into helper methods". Since that fixture already owns the storage round-trip (StorageRoundtrip_PreservesPayerFrameReceiptsAndUnionLogs, same [Values(true, false)] compact axis), the cleanest shape is one more test case there reusing Log / AssertFrameReceiptsEqual, with the golden hex as the only new state.
Low — coverage gap on the two paths the reported symptom actually goes through.
- Mixed-format array. A real post-upgrade DB has pre-2D records for old blocks sitting next to 2D records for new ones. The fixture is a single-receipt array, so it never exercises the discrimination and neighbour realignment together. A golden array of
[legacy, pre-2D frame, 2D frame, legacy]would pin both — this is the mirror of the existingStructRefIteration_OverArrayWithFrameTxReceipt_DoesNotThrowOrCorruptNeighbours. DecodeStructRef. The PR description listseth_getLogs500s.eth_getLogsreaches the frame gas field only viaLogFinder.FilterLogsInBlockHighMemoryAllocation→_receiptFinder.Get(full decode); theReceiptsIteratorpath usesDecodeStructRef, which realigns toreceiptEndand never reads it. So the struct-ref path should already be immune — but nothing asserts that against pre-2D bytes, and it's the hot iteration path. Worth one case.
Nit — the golden blobs are unverifiable in review. 500+ hex chars with no way to check them short of hand-decoding. (I did: 06 → FrameTx, sender, 82c738 = 51000, then frames 01 825208 … / 80 827530 … / c3 02 80 c0 — they're correct, and frame 2's 80 is a nice free check that a zero scalar takes the fallback rather than being mistaken for a list.) A frozen blob is a legitimate on-disk-compat fixture, so this is fine as-is; a small local helper that writes the pre-2D layout and is asserted equal to the blob would both document the format and make the extra shapes above cheap to add.
I could not run dotnet test in this environment (command not permitted), so the assertions are reviewed statically only. LogEntry.Data is byte[], so Is.EqualTo(new[] { logMarker }) compares fine.
045fb47 to
58ae0a0
Compare
A frame-tx receipt persisted under eip8141-frame-txs-devnet7 stored gas_used as a scalar. After PR #12942 changed it to an [execution, state] list, a node reading its own receipts DB would throw RlpException (eth_getTransactionReceipt /eth_getLogs 500s) with no fallback. The two storage decoders (ReceiptStorageDecoder, CompactReceiptStorageDecoder) now detect the shape at gas_used via a shared FrameReceiptGasRlp.DecodeGasUsed: a sequence decodes as [execution, state]; a scalar decodes as execution with state = 0. The scalar path preserves the per-frame total but is not wire-faithful when re-encoded for GetReceipts; that trade-off is documented on the helper's remarks. The network ReceiptMessageDecoder is intentionally left strict list-only. Regression fixtures are golden blobs produced by the base-branch encoder (c265e33): a standalone pre-2D receipt, and an array [legacy, pre-2D frame, legacy] exercised through both full decode and DecodeStructRef iteration.
58ae0a0 to
e157b11
Compare
wurdum
left a comment
There was a problem hiding this comment.
This restores the receipt side of the #12942 format break, but the same devnet7 datadir also holds frame transactions whose limits was a scalar, and TxFrameDecoder.cs:32 still requires the sequence. eth_getTransactionReceipt resolves the block before the receipt (PersistentReceiptStorage.cs:482 calls FindBlock), so it still 500s with RlpException on that database, just from the tx decoder.
// TxFrameDecoder.cs:32 - storage-gated only; the wire read stays strict
ulong executionGasLimit, stateGasLimit;
if ((rlpBehaviors & RlpBehaviors.Storage) != 0 && !decoderContext.IsSequenceNext())
{
executionGasLimit = decoderContext.DecodeULong();
stateGasLimit = 0;
}
else
{
int limitsCheck = decoderContext.ReadSequenceLength() + decoderContext.Position;
executionGasLimit = decoderContext.DecodeULong();
stateGasLimit = decoderContext.DecodeULong();
decoderContext.Check(limitsCheck);
}| private static TxFrameReceipt[] DecodeFrameReceipts(ref RlpReader decoderContext) | ||
| { | ||
| int framesEnd = decoderContext.ReadSequenceLength() + decoderContext.Position; | ||
| List<TxFrameReceipt> frameReceipts = []; |
There was a problem hiding this comment.
Not for this PR, more a note while the lines are in view: DecodeFrameReceipts here allocates a List<TxFrameReceipt> plus a List<LogEntry> per frame, while CompactReceiptStorageDecoder.cs:183 does the identical work with ArrayPoolListRef<T>. Both predate this change, so bringing the non-compact side into line is worth a separate follow-up rather than widening a read-compat diff.
using ArrayPoolListRef<TxFrameReceipt> frameReceipts = new(Eip8141Constants.MaxFrames);
...
using ArrayPoolListRef<LogEntry> frameLogs = new(4);|
|
||
| internal static class FrameReceiptGasRlp | ||
| { | ||
| /// <summary> |
PR #12942 changed a frame-tx receipt's on-disk
gas_usedfrom a scalar to a[execution, state]list. A node that already persisted frame-tx receipts undereip8141-frame-txs-devnet7throwsRlpExceptionreading its own receipts DB after upgrading (eth_getTransactionReceipt/eth_getLogs500s), with no fallback.The two storage decoders (
ReceiptStorageDecoder,CompactReceiptStorageDecoder) now detect the item shape atgas_used: a sequence decodes as[execution, state](new); a scalar decodes asexecution,state = 0(pre-2D). The networkReceiptMessageDecoderis untouched — this is a local-DB read-compat change only, no consensus or wire effect.The regression test decodes a real devnet7-format stored receipt through both storage decoders; the bytes come from the base-branch encoder. Without the fallback the decode throws
RlpException(verified).Split out of #12942 so the storage-compat change is reviewable on its own.