Skip to content

fix(frames): decode pre-2D scalar gas_used in stored frame-tx receipts - #12957

Open
AnkushinDaniil wants to merge 1 commit into
eip8141-frame-txs-devnet7from
daniil/frame-receipt-storage-compat
Open

fix(frames): decode pre-2D scalar gas_used in stored frame-tx receipts#12957
AnkushinDaniil wants to merge 1 commit into
eip8141-frame-txs-devnet7from
daniil/frame-receipt-storage-compat

Conversation

@AnkushinDaniil

Copy link
Copy Markdown
Contributor

PR #12942 changed a frame-tx receipt's on-disk gas_used from a scalar to a [execution, state] list. A node that already persisted frame-tx receipts under eip8141-frame-txs-devnet7 throws RlpException reading its own receipts DB after upgrading (eth_getTransactionReceipt/eth_getLogs 500s), with no fallback.

The two storage decoders (ReceiptStorageDecoder, CompactReceiptStorageDecoder) now detect the item shape at gas_used: a sequence decodes as [execution, state] (new); a scalar decodes as execution, state = 0 (pre-2D). The network ReceiptMessageDecoder is 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.

@claude

claude Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Claude finished @AnkushinDaniil's task in 6m 7s —— View job


Review: fix(frames): decode pre-2D scalar gas_used in stored frame-tx receipts

  • Gather context (diff vs daniil/frame-2d-gas-d7, read both decoders)
  • Verify IsSequenceNext shape discrimination is unambiguous
  • Hand-decode both golden hex fixtures
  • Audit every remaining frame-receipt decode path for coverage gaps
  • Check repo rules (DRY, comments, test conventions)
  • Post review

The core change is correct and the approach is the right one. Findings are about the blast radius of the silently-lost split and about repo-rule housekeeping — nothing that changes the decode logic.

What I verified

The shape sniff cannot be ambiguous. An RLP-encoded ulong is always < 0xc0 (single byte < 0x80, or an 0x81..0x88 prefix), and the 2D encoder always emits StartSequence for gas_used — including for [0, 0], which becomes c2 80 80. So IsSequenceNext() partitions old from new cleanly, in both directions, for every value. Check(frameEnd) still runs on both branches, so a malformed frame is still caught.

The semantic mapping preserves totals. TxFrameReceipt.GasUsed => ExecutionGasUsed + StateGasUsed (TxFrameReceipt.cs:30), so folding the old scalar into execution with state = 0 keeps every per-frame gasUsed and the receipt's GasUsedTotal exactly right. Only the split is unrecoverable, which it inherently is.

Decoder coverage is complete. The only two decoders that read frame receipts off disk are the two patched here; OptimismCompactReceiptStorageDecoder has no frame handling, and both DecodeStructRef implementations realign to receiptEnd without ever touching the gas field.

The golden fixtures are genuine. Hand-decoded OldNonCompactHex: f90207 array → b9020406 (FrameTx) → status 01, sender, gasUsedTotal 82c738 = 51000, bloom, union log, ff 80 txHash mark, 80 error, payer, then f888 frames = 01 825208 … / 80 827530 … / c3 02 80 c0. Scalar gas confirmed in both blobs, and the third frame's 80 is a free check that a zero scalar takes the fallback instead of being read as a list.

Findings

# 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 ReceiptMessageDecoder untouched 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

Comment on lines +203 to +216
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;
}

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

Fix this →

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

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

  1. 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 existing StructRefIteration_OverArrayWithFrameTxReceipt_DoesNotThrowOrCorruptNeighbours.
  2. DecodeStructRef. The PR description lists eth_getLogs 500s. eth_getLogs reaches the frame gas field only via LogFinder.FilterLogsInBlockHighMemoryAllocation_receiptFinder.Get (full decode); the ReceiptsIterator path uses DecodeStructRef, which realigns to receiptEnd and 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.

Fix this →

@AnkushinDaniil
AnkushinDaniil force-pushed the daniil/frame-receipt-storage-compat branch 3 times, most recently from 045fb47 to 58ae0a0 Compare August 22, 2026 12:03
Base automatically changed from daniil/frame-2d-gas-d7 to eip8141-frame-txs-devnet7 August 22, 2026 12:45
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.

@wurdum wurdum left a comment

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.

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 = [];

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.

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>

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.

trim comment

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.

4 participants