eth: preserve preconfirmation RPC availability - #2394
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review |
There was a problem hiding this comment.
🔵 Needs a closer look
The synthetic pending-state behavior affects several externally visible RPC semantics and warrants final human validation.
Pull request overview
Restores synchronous transaction submission when no direct producer relay is configured, while adding canonical pending-state fallbacks.
Changes:
- Submit synchronous transactions through the local txpool and P2P gossip.
- Make producer relay optional while preserving preconfirmation caching.
- Provide pending block, receipt, log, and state fallbacks during Sequence Store gaps.
File summaries
| File | Description |
|---|---|
internal/ethapi/preconf_receipt.go |
Removes the obsolete synchronous submitter interface. |
internal/ethapi/api.go |
Uses local transaction admission for synchronous submissions. |
internal/ethapi/api_test.go |
Tests local admission and optional relay behavior. |
eth/relay/relay.go |
Exposes producer-relay availability. |
eth/relay/relay_test.go |
Tests relay availability states. |
eth/api_backend.go |
Preserves transaction caching and conditionally relays. |
eth/api_backend_test.go |
Updates optional-relay expectations. |
eth/api_backend_sequencer_test.go |
Tests canonical pending fallbacks. |
eth/api_backend_pending.go |
Implements canonical pending block and state views. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
930778c to
bdba942
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding on PreconfRelayAvailable, I also looked at canonicalPendingSnapshot's head-consistency retry loop for unbounded spinning — it re-checks ctx.Err() on every iteration, so it's bounded by the caller's context deadline rather than being a true infinite busy-loop.
Extended reasoning...
The confirmed inline finding (silent no-op in SubmitTxForPreconf when PreconfRelayAvailable() is false, even in cases where bp-rpc-endpoints were configured but failed to dial at startup) already covers the substantive issue for this run and will be posted as an inline comment; no need to restate it here. The one additional item examined was the tight for-loop in canonicalPendingSnapshot (eth/api_backend_pending.go) that retries when the chain head changes between fetching the header and re-checking CurrentBlock — it lacks a sleep/backoff, but it does call ctx.Err() at the top of each iteration, so an RPC caller with a bounded context will not hang forever; this was investigated and ruled out as a standalone bug distinct from the confirmed finding, worth noting for the human reviewer's awareness given a prior review already exists on this PR.
| func (b *EthAPIBackend) SubmitTxForPreconf(tx *types.Transaction) error { | ||
| if err := b.cachePreconfTransaction(tx); err != nil { | ||
| return err | ||
| } | ||
| if !b.relay.PreconfRelayAvailable() { | ||
| return nil | ||
| } | ||
| return b.relay.SubmitPreconfTransaction(tx) |
There was a problem hiding this comment.
🟡 (optional) SubmitTxForPreconf now silently returns nil when RelayService.PreconfRelayAvailable() is false, so if bp-rpc-endpoints ARE configured but every producer fails to dial at Bor startup (newMultiClientWithLimit returns nil multiclient), preconf submissions permanently no-op with zero error/log — the pre-existing log.Error("...submission for preconf failed") signal is lost. Fix: log a warning (once or rate-limited) when relay is configured but unavailable, distinguishing "not configured" from "configured but all producers unreachable", so operators can detect the outage instead of relay silently going dark until restart.
Extended reasoning...
Before this diff, SubmitTxForPreconf always called b.relay.SubmitPreconfTransaction(tx), which returns an error when txRelay/multiclient is nil, and submitForPreconf() logged that error via log.Error. After this diff, PreconfRelayAvailable() short-circuits to return nil before ever calling the relay, so the same nil-multiclient condition (all bp-rpc-endpoints failed to dial at startup, per multiclient.go:121-124) now produces no error and no log line, even though the operator configured relay and expects it to run. The PR states existing bp-rpc-endpoints deployments are unaffected, but this specific transient-dial-failure case silently degrades observability.
Verification: nit. The behavior change is real and reachable but its impact is limited to lost log signal, not lost transactions. When bp-rpc-endpoints are configured yet every producer fails to dial at startup, NewService still builds a non-nil txRelay (eth/relay/relay.go:47-52) while newMultiClientWithLimit returns nil (eth/relay/multiclient.go:120-123), so `txRelay != nil && txRelay.multiclient ==…
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (46.15%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## cffls/sequence-publisher #2394 +/- ##
============================================================
+ Coverage 56.98% 56.99% +0.01%
============================================================
Files 951 951
Lines 174271 174278 +7
============================================================
+ Hits 99304 99331 +27
+ Misses 69299 69284 -15
+ Partials 5668 5663 -5
... and 21 files with indirect coverage changes
🚀 New features to boost your workflow:
|
Summary
This PR fixes
eth_sendRawTransactionSyncwhen preconfirmations are enabled but the RPC node has no direct block-producer endpoints configured.The synchronous path now submits the transaction to the local txpool first, allowing normal P2P propagation to the block producer. It then waits for either a speculative preconfirmation receipt or the canonical receipt. Direct relay submission remains optional and is used only when
bp-rpc-endpointsis configured.This prevents
eth_sendRawTransactionSyncfrom failing withrequest dropped: rpc client unavailable to submit transactionson standard RPC and sentry nodes.The change is intentionally limited to transaction submission and receipt waiting. It does not modify pending-block construction,
PendingBlockAndReceipts, pending-state RPC behavior, block import, or consensus processing.Executed tests
Manual Kurtosis validation was also performed against an RPC node with preconfirmations enabled and no
bp-rpc-endpoints.A transaction submitted using
cast send --syncreturned a successful speculative receipt withblockHash: null. The same transaction was subsequently available as a canonical receipt with a populated block hash.Rollout notes
This change is consensus-neutral and does not alter block execution, validation, import, or fork choice.
No database migration, resync, or coordinated network upgrade is required. The change is backward-compatible for nodes with direct producer endpoints configured.
RPC nodes no longer require
bp-rpc-endpointsforeth_sendRawTransactionSyncto function. Without those endpoints, transactions propagate through the local txpool and normal P2P networking. When producer endpoints are configured, the existing direct relay path remains available.