execution/types: dedup typed-transaction codec boilerplate#22179
Draft
yperbasis wants to merge 7 commits into
Draft
execution/types: dedup typed-transaction codec boilerplate#22179yperbasis wants to merge 7 commits into
yperbasis wants to merge 7 commits into
Conversation
Deterministic instances of every tx type (legacy incl. pre-EIP-155, access-list, dynamic-fee, blob incl. wrapped v0/v1, set-code with and without authorizations, account-abstraction; both contract-creation and non-nil To where the type allows it) with hex-pinned MarshalBinary, EncodeRLP, Hash, SigningHash and EncodingSize outputs, plus byte-identical decode/re-encode round-trips. These freeze the canonical wire and hash format ahead of deduplicating the codec implementations.
…r recovery The cachedSender bodies were identical across LegacyTx, AccessListTx, DynamicFeeTransaction and BlobTx and only touch TransactionMisc.from, so promote a single implementation from the embedded struct. Sender still needs the concrete type for signer.Sender, so each type keeps a one-line method delegating to a shared recoverSender helper (this also removes the cosmetic nested-if variant in AccessListTx, which was semantically identical). AccountAbstractionTransaction and BlobTxWrapper keep their intentionally different implementations.
AccessListTx, DynamicFeeTransaction, BlobTx and SetCodeTransaction all applied SignatureValues + ChainID to their copy with the same code; route them through one applySignature helper. LegacyTx stays as is since it does not set ChainID.
All five tx types ended their payload encoding with the same V, R, S writes; move that tail onto the embedded CommonTx. The typed decoders (dynamic-fee, blob, set-code) get the matching decodeVRS. LegacyTx and AccessListTx keep their inline V/R/S decoding because they wrap each field error with its own context.
DynamicFeeTransaction, BlobTx and SetCodeTransaction encoded the same leading payload fields (list prefix, ChainID, Nonce, TipCap, FeeCap, GasLimit, To, Value, Data, AccessList) byte-for-byte; hoist them into one method on the embedded DynamicFeeTransaction that the per-type encodePayload composes with its own trailing fields.
Mirror of encode1559Prefix for the decode side. DynamicFeeTransaction, BlobTx and SetCodeTransaction read the same leading payload fields; the only difference is that blob and set-code transactions require a 20-byte To (no contract creation), expressed via the strictTo flag.
…pers Every typed transaction wrapped its payload the same way: type byte for the canonical encoding, RLP string prefix + type byte for EncodeRLP. Hoist both envelopes into marshalTyped/encodeRLPTyped taking an encodePayload closure, mirroring the shape Hash already uses via prefixedPayloadHash. LegacyTx's EncodeRLP now delegates to its identical MarshalBinary. The closures do not escape (encode benchmarks stay at 0 allocs/op).
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors the execution/types typed-transaction encoding/decoding implementation to eliminate repeated boilerplate across transaction families, while adding a comprehensive golden-test suite to pin consensus-critical wire encodings and hashes.
Changes:
- Deduplicates common typed-transaction encoding/decoding blocks via shared helpers (
marshalTyped,encodeRLPTyped,encode1559Prefix/decode1559Prefix,encodeVRS/decodeVRS,recoverSender,applySignature). - Centralizes sender caching logic on embedded structs (via
TransactionMisc) and consolidatesWithSignatureimplementations. - Adds
codec_golden_test.goto pin byte-identical outputs forMarshalBinary,EncodeRLP,Hash,SigningHash, andEncodingSizeacross all transaction types (including wrapper variants) plus round-trip invariants.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| execution/types/transaction.go | Adds shared helpers for typed envelopes, sender recovery/caching, and signature application to remove repeated codec boilerplate. |
| execution/types/legacy_tx.go | Reuses shared V/R/S helpers and simplifies EncodeRLP to delegate to MarshalBinary. |
| execution/types/access_list_tx.go | Switches typed envelope + V/R/S encoding and WithSignature to shared helpers; sender recovery uses shared logic. |
| execution/types/dynamic_fee_tx.go | Extracts EIP-1559 shared prefix encode/decode helpers and uses typed envelope + V/R/S helpers. |
| execution/types/blob_tx.go | Uses shared 1559 prefix + typed envelope + V/R/S helpers and shared sender recovery/signature application. |
| execution/types/set_code_tx.go | Uses shared 1559 prefix (strict-to), typed envelope helpers, V/R/S helpers, and shared sender recovery/signature application. |
| execution/types/aa_transaction.go | Switches typed envelope encoding/decoding to shared helpers for the AA transaction family. |
| execution/types/codec_golden_test.go | Adds goldens and round-trip assertions to lock down consensus wire format and hashing behavior during refactors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pull Bot
pushed a commit
to Dustin4444/erigon
that referenced
this pull request
Jul 4, 2026
Helper extractions collapsing the package's most-repeated scaffolds. Behavior-preserving with the compat quirks kept byte-identical, except two deliberate changes called out below. 1. **Txn lookup + index derivation ×8** — two helpers rather than one bundle, because two sites must keep their inline bor fallbacks (`GetTransactionReceipt` runs its prune/index checks with the pre-fallback block number — live pruned-bor-node behavior; debug `GetRawTransaction` early-returns on `!ok` before its bor branch). `txnLookupWithBorFallback` covers the four sites whose fallback triggers on a missed lookup — the miss contract of both `TxnLookup` implementations is verified `(0,0,false,nil)`, and block 0 has no txns, so `GetTransactionByHash`'s historical `blockNum==0` trigger is equivalent to `!ok`. `txnIndexInBlock` owns the txNum→index math at all 8 sites with the exact underflow error, called at each site's original position so error precedence is unchanged; for bor state-sync txns it returns the established `-1` sentinel (the value the block readers guard and trace_filtering decodes). 2. **`getReceiptsWithBor` ×3** — returns `(receipts, borReceipt)` rather than appending (appending would break the marshal loops' `Transactions()[TransactionIndex]`); per-site marshal flags (`withBlockTimestamp` etc.) stay exact. Its events→`GenerateBorReceipt` tail is `borReceiptForBlock`, also reused by `GetTransactionReceipt`'s bor state-sync path (nil receipt ⇔ zero events — `GenerateBorReceipt` never returns `(nil, nil)` — mapped back to that site's "tx not found"). 3. **Log range resolution** — erigon_receipts' byte-identical latest-only pair extracted into `logRangeLatestOnly`, with its FromBlock/ToBlock twin blocks collapsed into `resolveLogBound` (the begin=0-on-latest quirk lives in the per-bound default); overlay's `getBeginEnd` promoted to `BaseAPI.resolveLogsRange` with a `checkFuture` flag (eth_getLogs keeps its inline into-the-future rejections — deferring them would change which error surfaces). The two variants are deliberately NOT merged. 4. **`subscribeRPC[T]` ×5** in eth_filters — the full guard/CreateSubscription/LogPanic-goroutine/unsubscribe/select skeleton, checking channel closure before notify; notify callbacks receive a pre-bound `emit` func so the Notify+Warn boilerplate lives once in the skeleton (all call sites used the identical warn message). `NewPendingTransactionsWithBody` delegates to the same core as `NewPendingTransactions` (the only deltas are fullTx and channel buffer 256 vs 512 — both parameterized). 5. **Replay-env helpers** — the inline BLOCKHASH closures reuse the pre-existing `transactions.MakeBlockHashProvider` instead of gaining a new helper (safe: both `CanonicalHash` impls return the zero hash on miss; where a timeout applies, the provider binds the deadline-carrying ctx so hash lookups run under the EVM deadline); `validateBundles` ×2 (pinned by `TestCallManyEmptyBundles` for both callers); `setupEVMTimeout` ×6 (CallBundle, CallMany, overlay `replayBlock`, `simulateCall`, trace_call, trace_rawTransaction), returning the deadline ctx, a `store` func for the current EVM, and a cleanup that preserves the stop-before-cancel order. The trace doCallBlock/doCall stacks and trace_filtering's parallel per-tx tracer keep their own shapes — different cancellation mechanisms (cooperative ctx checks; many concurrent EVMs). Two deliberate behavior changes: - The canonical-hash-miss log line is unified on the shared provider at Debug (message `[evm] canonical hash not found`): the four jsonrpc sites keep their historical Debug level, and the `eth_simulateV1` path drops from Error — the miss is user-triggerable (e.g. `eth_callMany` bundles advance `BlockNumber` past head, so BLOCKHASH near the simulated boundary legitimately misses), geth logs nothing there, and genuine read errors still propagate to the RPC response. - EVM-timeout cancellation is now airtight: `setupEVMTimeout`'s `store` re-checks `ctx.Err()` after every store, so a deadline or client disconnect firing before — or between — EVM (re)creations still cancels the current instance (previously CallBundle/CallMany could drop a one-shot cancellation that fired between loop iterations, leaving the call running with no timeout). Pinned by `TestSetupEVMTimeoutCancelsEVMStoredAfterExpiry`, verified red against a plain store. New tests: `TestGetRawReceipts` (previously untested debug site), `TestOverlayGetBeginEnd` (previously untested overlay path), a guard-behavior test across all five subscription methods, `TestCallManyEmptyBundles`, and the timeout test above. Every pinning test was run green against the unconverted code first. Net ≈ −284 production lines, +174 of tests. Verified: `go test ./rpc/...` green (plus a `-race` pass over the timeout-helper tests), scoped `golangci-lint` clean (×2), full `make lint` clean, `make erigon`. Part of a dedup series; siblings erigontech#22165–erigontech#22179.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The five typed-transaction codecs repeated the same mechanical blocks: the 1559 field prefix (ChainID…AccessList — byte-identical in dynamic-fee/blob/set-code), the V,R,S encode tail (×5), the DecodeRLP prefix incl. a byte-identical strict-To block, the MarshalBinary/EncodeRLP envelope pair (×5),
cachedSender(×5), andWithSignature(×4). ~535 lines of copies collapse onto helpers on the embedded structs — the patternSetCodeTransaction.payloadSizealready used:encode1559Prefix/decode1559PrefixonDynamicFeeTransaction,encodeVRS/decodeVRSonCommonTx,cachedSenderpromoted toTransactionMisc,applySignature, and Hash-stylemarshalTyped/encodeRLPTypedenvelope wrappers. Net −369 production lines; one commit per family.This is consensus wire format, so the safety net came first:
codec_golden_test.go(+449 lines, committed and green before any refactor) pins hex goldens of MarshalBinary, EncodeRLP, Hash, SigningHash, EncodingSize for every type — legacy EIP-155 and pre-155 contract creation, access-list ±To, dynamic-fee ±To, blob (2 versioned hashes) plus wrapped blob v0/v1, set-code with 2/0 auths, and account-abstraction with all fields — plus byte-identical decode/re-encode round-trips. Goldens stayed byte-identical after every family; the randomized TRand harness passes-count=5.Perf: no interface dispatch inside encode paths — plain method calls on embedded structs; the envelope closures were benchmarked at 0 allocs/op (~3–4% ns/op on ~200ns encodes).
Deliberate skips: legacy/access-list
decodeVRS(their per-field"read V: %w"error wrapping would change error text),payloadSize(no token-identical bodies remain),Senderstays per-type (signer needs the concrete type; bodies are now 1-liners over a sharedrecoverSender),BlobTxWrapper.MarshalBinaryWrapped(unique wire shape), and AA beyond the envelope+VRS (different field set by design).Verified: full
execution/typessuite green after each family,go test ./execution/... -shortgreen, scopedgolangci-lintclean (×2),make erigon integration.Part of a dedup series; siblings #22165–#22178.