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 to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review. Check for regressions. |
|
Claude finished @manav2401's task in 2m 57s —— View job Code Review: v2.7.3 Release
SummaryOverall this is a clean release with mostly additive logging changes and one meaningful behavioral change (hard TTL for private tx store). The code is well-structured and the new test covers the TTL feature properly. I have one item to flag as a potential regression. Potential Regression:
|
| Scenario | Old behavior | New behavior |
|---|---|---|
SubmitTransaction returns non-ErrAlreadyKnown error |
Purge from private store, then still try SubmitPrivateTx to BPs |
Purge and return immediately (no BP submission) |
SubmitTransaction returns ErrAlreadyKnown |
Don't purge, then still try SubmitPrivateTx to BPs |
Don't purge, return immediately (no BP submission) |
SubmitTransaction succeeds |
Submit to BPs | Submit to BPs (unchanged) |
Case 1 (non-ErrAlreadyKnown error) looks like a bug fix — the old code would purge the tx from the private store and then still try to forward to BPs, which is inconsistent.
Case 2 (ErrAlreadyKnown) is the one worth double-checking: if a private tx arrives from a relay retry path and the local pool already has it, the old code would still ensure BPs receive it. The new code skips BP submission entirely. This is fine if the prior SendRawTransactionPrivate call already handled BP forwarding successfully, but could miss the BP notification if the tx entered the pool through a different path (e.g., regular eth_sendRawTransaction or P2P propagation).
Question for author: Is the
ErrAlreadyKnownearly-return intentional? If the tx was added to the local pool through a non-private path, this would skip the BP submission.
Findings
1. Private Tx Store Hard TTL — Looks Good
eth/relay/private_tx_store.go:189-191 — Clean implementation. The TTL check runs before the txpool-based eviction, so expired txs are removed regardless of pool state. The three-phase sweep pattern (snapshot → filter → delete with re-check) is preserved correctly. Test (TestPrivateTxStoreSweepTTL) covers the new path well.
One note: when TTL fires, the tx is no longer marked private and can be gossiped publicly. This matches the PR description ("making them public") — just confirming that's the intended consequence.
2. Multiclient Timing Logs — Looks Good
eth/relay/multiclient.go — callStart/elapsed are captured per-goroutine, so there's no shared-state concern. The timing accurately wraps the RPC call + context timeout. No behavioral changes.
3. Service Elapsed Logging — Looks Good
eth/relay/service.go:175,293 — Adding elapsed to existing log.Warn calls. The time.Since(start) is computed at log-time rather than captured immediately, so it includes a tiny bit of overhead from the error checks, but this is negligible and acceptable for Warn-level diagnostic logging.
4. RPC Read Timeout Logging — Looks Good, Minor Notes
rpc/server.go:195-229 — The diagnostic logging is thorough and will help identify upstream stalls vs. Bor delays.
codec.peerInfo()returns{Transport: "ipc", RemoteAddr: ...}for HTTP server codecs (becausehttpServerConnis wrapped injsonCodec), whilePeerInfoFromContext(ctx)returns the HTTP-enriched view with User-Agent/Host/Origin. Logging both is redundant forTransportbut useful for cross-referencingRemoteAddr.fmt.Sprintf("%T", err)is fine since this is server-side logging, not returned to callers.- This is Warn-level logging on every read error. On nodes with frequent upstream timeouts, this could be noisy. Consider whether Debug level would be more appropriate for steady-state, but for initial diagnosis Warn is fine.
5. Version Bump — Looks Good
params/version.go — VersionMeta changed from "beta" to "". Standard release promotion.
6. Test Coverage — Good
The new TestPrivateTxStoreSweepTTL test properly validates:
- Entries older than TTL are removed
- Fresh entries are preserved
- The
txsExpiredmetric is incremented correctly - No txpool checker is needed for TTL-based eviction (correct — TTL fires regardless)
Verdict
Approve with one question: please confirm the ErrAlreadyKnown early-return in SendRawTransactionPrivate is intentional (see regression analysis above). The rest of the changes are clean observability improvements and a well-tested feature addition.
There was a problem hiding this comment.
Pull request overview
This PR prepares the v2.7.3 release by adding more diagnostic logging around RPC read failures and relay submissions, and by introducing a hard TTL to evict old private transactions from the relay’s private-tx store (allowing them to become public after expiry). It also updates the build version metadata for the release.
Changes:
- Add richer RPC “read timeout / read error” logging before handler dispatch to help identify upstream stalls vs local delays.
- Add elapsed-time logging for relay submissions (preconf/private tx) and introduce a hard TTL (10 minutes) for private-tx store eviction (with tests).
- Update version metadata for the v2.7.3 release.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
rpc/server.go |
Adds timing + enriched peer metadata to logs for HTTP read failures. |
params/version.go |
Updates release metadata (VersionMeta). |
internal/ethapi/api.go |
Refactors private-tx purge logic on submission error. |
eth/relay/service.go |
Adds elapsed-time logging for relay submission failures. |
eth/relay/private_tx_store.go |
Adds hard TTL eviction to private tx store sweep logic. |
eth/relay/private_tx_store_test.go |
Adds unit test coverage for TTL-based eviction. |
eth/relay/multiclient.go |
Adds elapsed-time debug logging per block-producer RPC call failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code reviewFound 1 issue. Checked for bugs and CLAUDE.md compliance. |
|
@claude the return if the error is "already known" is intentional. It should not send the tx further for re-submission. |
|
Claude finished @manav2401's task in 16s —— View job Acknowledged:
|
|
test123 |
Bug: Behavioral regression in SendRawTransactionPrivate (internal/ethapi/api.go:2358-2365)The refactoring of the error-handling block changes control flow for ErrAlreadyKnown errors. Old code (link): New code (link): Suggested fix: Move |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2212 +/- ##
=======================================
Coverage 52.21% 52.22%
=======================================
Files 885 885
Lines 155613 155623 +10
=======================================
+ Hits 81257 81273 +16
+ Misses 69122 69115 -7
- Partials 5234 5235 +1
... and 18 files with indirect coverage changes
🚀 New features to boost your workflow:
|
Description
Few small tweaks (mostly aimed at relay nodes tested via bor-tester release).
Changes
Breaking changes
Please complete this section if any breaking changes have been made, otherwise delete it
Nodes audience
In case this PR includes changes that must be applied only to a subset of nodes, please specify how you handled it (e.g. by adding a flag with a default value...)
Checklist
Cross repository changes
Testing
Manual tests
Please complete this section with the steps you performed if you ran manual tests for this functionality, otherwise delete it
Additional comments
Please post additional comments in this section if you have them, otherwise delete it