rpc/jsonrpc: extract BaseAPI helper families#22180
Merged
Merged
Conversation
Collapse the repeated txnLookup -> EventTxnLookup fallback -> _txNumReader.Min -> underflow check -> txnIndex derivation dance into two BaseAPI helpers: - txnLookupWithBorFallback: the !ok-triggered bor state-sync fallback shared verbatim by debug_traceTransaction, trace_replayTransaction and trace_transaction. GetTransactionReceipt, GetTransactionByHash and debug_getRawTransaction keep their inline fallbacks: their trigger is blockNum==0-based (fires also for ok lookups) and interleaves differently with prune/receipts checks, so folding them in would not be behavior-preserving. - txnIndexInBlock: Min + 'uint underflow txnums error' + index derivation, now shared by all eight call sites at the exact position the Min call used to occupy, so error precedence wrt prune checks and header fetches is unchanged. In GetTransactionReceipt the underflow check moves from after the bor fallback to the helper (before it); this is equivalent because TxnLookup returns blockNum==0 on every miss (both local BlockReader and RemoteBlockReader enforce this), so a failed lookup on a bor chain always sets isBorStateSyncTx and the check is skipped either way. Behavior-preserving: identical JSON responses, error strings and nil,nil not-found quirks at every site.
eth_getBlockReceipts, erigon_getBlockReceiptsByBlockHash and debug_getRawReceipts shared the same tail: getReceipts, then on bor chains fetch state sync events and generate the synthetic bor receipt. Collapse it into BaseAPI.getReceiptsWithBor. The bor receipt is returned separately rather than appended because the three call sites marshal it differently (eth marshals with block timestamp, erigon uses the legacy format without it, debug appends it raw); marshalling stays at the call sites with their exact flags. debug_getRawReceipts now reads chainConfig before generating receipts instead of after; both are adjacent error returns, and chainConfig is also read inside getReceipts itself, so the response is unchanged. TestGetRawReceipts pins the previously untested debug_getRawReceipts non-bor path against getReceipts+MarshalBinary; the bor tail is not reachable from this package's harness (no bor test chain) and is covered by code review.
Two deliberately separate helpers, matching the two distinct dialects in use: - logRangeLatestOnly (erigon_receipts.go): the byte-identical blocks in erigon_getLogs and erigon_getLatestLogs that accept only 'latest' as a negative tag and reject the rest with a CustomError. - BaseAPI.resolveLogsRange: the resolving dialect from overlay's getBeginEnd, now also used by eth_getLogs. Negative tags are resolved via rpchelper.GetBlockNumber against the latest executed block. The one real delta is parameterized: eth_getLogs rejects ranges past the head (checkFuture) as each bound is resolved, overlay does not. eth_getLogs' blockHash+range guard is hoisted above the helper call (fires under exactly the same condition as before), and each caller keeps its own end<begin error and MaxUint32 clamp: the error messages differ (CustomError vs fmt.Errorf) and sit between resolution and clamp, so they cannot move into the shared helper without changing responses. TestOverlayGetBeginEnd pins the previously untested getBeginEnd (explicit range, defaults, inverted-range error string, blockHash pin) and was green before and after the change.
The five subscription methods (eth_newHeads, eth_newPendingTransactions, eth_newPendingTransactionsWithBody, eth_logs, eth_transactionReceipts) shared the same wrapper: nil-filters guard, NotifierFromContext guard, CreateSubscription, and a goroutine with dbg.LogPanic pumping the filter channel into the notifier with defer-unsubscribe and channel-closed warning. Collapse it into subscribeRPC[T]; each method supplies its subscribe closure, per-item notify logic and closed-channel message. NewPendingTransactionsWithBody delegates to the same core as NewPendingTransactions with fullTx=true; their bodies differed only in that flag and the channel buffer size (256 vs 512), which stays a parameter. TestSubscriptionsRequireFiltersAndNotifier pins the guard behavior (ErrNotificationsUnsupported + empty subscription) for all five methods with both nil filters and a notifier-less context; the notify path is covered end-to-end by TestSendRawTransactionSync's eth_newPendingTransactions subscription.
Three helpers for the replay/simulation scaffolds:
- BaseAPI.blockHashGetter: the BLOCKHASH resolver closure (overrideBlockHash
map, then CanonicalHash with a debug log on miss) that appeared in
eth_callMany, debug_traceCallMany and twice in the overlay API. The
traceCallMany variant returned common.Hash{} explicitly on miss, but both
BlockReader and RemoteBlockReader already return the zero hash whenever
ok is false or err is set, so the variants are equivalent and merge into
one.
- validateBundles: the double empty-bundles check shared verbatim by
eth_callMany and debug_traceCallMany, same 'empty bundles' error.
- setupEVMTimeout: the evmPtr + WithTimeout/WithCancel + AfterFunc-cancel
scaffold shared by eth_callBundle and eth_callMany; the returned cleanup
preserves the original stop-before-cancel defer order, and per-call
timeout derivation (5s literal vs evmCallTimeout default with different
nil/positive guards) stays at the call sites.
TestCallManyEmptyBundles pins the empty-bundles error for both
eth_callMany and debug_traceCallMany before the loop+flag logic was
rewritten as early returns; trace_adhoc/trace_filtering block stacks and
overlay replayBlock's simpler timeout block are intentionally untouched.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the rpc/jsonrpc package by extracting several frequently repeated scaffolding patterns into shared helpers (transaction lookup/index derivation, Bor-aware receipts retrieval, log-range resolution, subscription skeletons, and replay-env utilities), aiming to reduce duplication while preserving RPC behavior (including Bor compatibility quirks).
Changes:
- Extracted shared helpers into
BaseAPI(txn lookup w/ Bor fallback, tx index derivation, Bor-aware receipts retrieval, log-range resolution, replay-env helpers). - Refactored multiple JSON-RPC endpoints to use the new helpers (eth/debug/trace/overlay/erigon receipts + filters subscriptions).
- Added/expanded tests to pin previously uncovered behaviors (overlay begin/end resolution, raw receipts, subscription guards, empty-bundle validation).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rpc/jsonrpc/tracing.go | Switches trace paths to shared txn lookup/index helpers; uses shared bundle validation and block-hash getter. |
| rpc/jsonrpc/trace_filtering.go | Uses shared txn lookup (w/ Bor fallback) and txn index helper. |
| rpc/jsonrpc/trace_adhoc.go | Uses shared txn lookup (w/ Bor fallback) and txn index helper. |
| rpc/jsonrpc/overlay_api.go | Reuses shared block-hash getter and shared log-range resolver in overlay range resolution. |
| rpc/jsonrpc/overlay_api_test.go | Adds test coverage for overlay begin/end block-range resolution. |
| rpc/jsonrpc/eth_txs.go | Replaces local txNum→index math with shared helper; adjusts types for RPC marshaling. |
| rpc/jsonrpc/eth_receipts.go | Adds getReceiptsWithBor and resolveLogsRange; refactors eth_getLogs and Bor receipt handling. |
| rpc/jsonrpc/eth_filters.go | Extracts a generic subscribeRPC[T] skeleton and refactors subscription endpoints to use it. |
| rpc/jsonrpc/eth_filters_test.go | Adds a guard-behavior test ensuring subscription methods reject missing filters/notifier. |
| rpc/jsonrpc/eth_callMany.go | Extracts bundle validation, block-hash getter, and EVM-timeout setup helper; refactors CallMany to use them. |
| rpc/jsonrpc/eth_callMany_test.go | Adds test coverage ensuring empty bundles are rejected consistently across CallMany and TraceCallMany. |
| rpc/jsonrpc/eth_block.go | Reuses txn index helper and shared EVM-timeout helper in CallBundle. |
| rpc/jsonrpc/eth_api.go | Adds txnLookupWithBorFallback and txnIndexInBlock helpers on BaseAPI. |
| rpc/jsonrpc/erigon_receipts.go | Extracts “latest-only” log-range parsing into helper; reuses Bor-aware receipts helper. |
| rpc/jsonrpc/debug_api.go | Refactors raw receipts + raw transaction paths to use Bor-aware receipts and shared txn index helper. |
| rpc/jsonrpc/debug_api_test.go | Adds test coverage for debug_getRawReceipts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lupin012
approved these changes
Jul 3, 2026
- reuse transactions.MakeBlockHashProvider instead of adding blockHashGetter, unifying the canonical-hash-miss log at Debug level - subscribeRPC: hand notify callbacks a pre-bound emit func, folding the Notify+Warn boilerplate into the skeleton - extract borReceiptForBlock, shared by getReceiptsWithBor and GetTransactionReceipt's bor state-sync path - setupEVMTimeout: return the derived context; convert replayBlock's inline timeout copy - logRangeLatestOnly: collapse the FromBlock/ToBlock twins into resolveLogBound - drop a comment restating the code in resolveLogsRange
- subscribeRPC: check channel closure before invoking notify, so the skeleton no longer relies on callbacks guarding the zero value - setupEVMTimeout: derive the context before the EVM exists (callers Store the instance after construction) and build the BLOCKHASH provider from the derived context, restoring the pre-extraction behavior where CallMany/replayBlock hash lookups ran under the EVM-timeout deadline (the old inline closures late-bound the reassigned ctx variable)
…ixes setupEVMTimeout armed its one-shot AfterFunc before any EVM was stored, so a cancellation firing in the arm-to-Store window was silently dropped (the nil-guarded Load skips, AfterFunc never re-fires) and the call then ran with no timeout at all: ApplyMessage takes no ctx and the loops abort only via evm.Cancelled(). The helper now returns a store func that re-checks ctx.Err() after each store and cancels the just-stored EVM if the deadline has already fired. The re-check runs on every store, so the EVM-recreation loops in CallBundle/CallMany are also covered between iterations (a hole that predated the extraction). Pinned by TestSetupEVMTimeoutCancelsEVMStoredAfterExpiry, verified red against the plain-Store semantics. Further review fixes: - txnIndexInBlock returns the established -1 bor state-sync sentinel instead of a documented-meaningless wraparound value (RemoteBlockReader already guards i < 0, the local snapshot path guards == -1, and trace_filtering decodes bor-ness from txIndex == -1); the two trace-site -1 overwrites become redundant and are removed. The Min call stays first so error propagation is unchanged. - GetTransactionByHash adopts txnLookupWithBorFallback - the one remaining site where the collapse is behavior-preserving: its prune check runs post-fallback inside "if ok", and both TxnLookup implementations return blockNum 0 exactly on miss, so the blockNum==0 trigger is equivalent to the helper's !ok trigger. - simulateCall, trace_call and trace_rawTransaction adopt setupEVMTimeout. They previously bound evm.Cancel after the EVM existed, so converting them without the store re-check would have imported the cancellation window; with it, the conversion is a strict dedup. - validateBundles drops its redundant len==0 branch: an empty slice skips the loop and falls through to the identical error, pinned by TestCallManyEmptyBundles.
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.
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.
GetTransactionReceiptruns its prune/index checks with the pre-fallback block number — live pruned-bor-node behavior; debugGetRawTransactionearly-returns on!okbefore its bor branch).txnLookupWithBorFallbackcovers the four sites whose fallback triggers on a missed lookup — the miss contract of bothTxnLookupimplementations is verified(0,0,false,nil), and block 0 has no txns, soGetTransactionByHash's historicalblockNum==0trigger is equivalent to!ok.txnIndexInBlockowns 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-1sentinel (the value the block readers guard and trace_filtering decodes).getReceiptsWithBor×3 — returns(receipts, borReceipt)rather than appending (appending would break the marshal loops'Transactions()[TransactionIndex]); per-site marshal flags (withBlockTimestampetc.) stay exact. Its events→GenerateBorReceipttail isborReceiptForBlock, also reused byGetTransactionReceipt's bor state-sync path (nil receipt ⇔ zero events —GenerateBorReceiptnever returns(nil, nil)— mapped back to that site's "tx not found").logRangeLatestOnly, with its FromBlock/ToBlock twin blocks collapsed intoresolveLogBound(the begin=0-on-latest quirk lives in the per-bound default); overlay'sgetBeginEndpromoted toBaseAPI.resolveLogsRangewith acheckFutureflag (eth_getLogs keeps its inline into-the-future rejections — deferring them would change which error surfaces). The two variants are deliberately NOT merged.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-boundemitfunc so the Notify+Warn boilerplate lives once in the skeleton (all call sites used the identical warn message).NewPendingTransactionsWithBodydelegates to the same core asNewPendingTransactions(the only deltas are fullTx and channel buffer 256 vs 512 — both parameterized).transactions.MakeBlockHashProviderinstead of gaining a new helper (safe: bothCanonicalHashimpls 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 byTestCallManyEmptyBundlesfor both callers);setupEVMTimeout×6 (CallBundle, CallMany, overlayreplayBlock,simulateCall, trace_call, trace_rawTransaction), returning the deadline ctx, astorefunc 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:
[evm] canonical hash not found): the four jsonrpc sites keep their historical Debug level, and theeth_simulateV1path drops from Error — the miss is user-triggerable (e.g.eth_callManybundles advanceBlockNumberpast head, so BLOCKHASH near the simulated boundary legitimately misses), geth logs nothing there, and genuine read errors still propagate to the RPC response.setupEVMTimeout'sstorere-checksctx.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 byTestSetupEVMTimeoutCancelsEVMStoredAfterExpiry, 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-racepass over the timeout-helper tests), scopedgolangci-lintclean (×2), fullmake lintclean,make erigon.Part of a dedup series; siblings #22165–#22179.