internal/ethapi: unify relay endpoints and rate limit tx submissions#2203
internal/ethapi: unify relay endpoints and rate limit tx submissions#2203
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.
There was a problem hiding this comment.
Pull request overview
This PR updates the internal/ethapi transaction submission flow so eth_sendRawTransaction can drive both preconfirmation and private-tx relay submission (depending on node flags), while keeping eth_sendRawTransactionPrivate working as before.
Changes:
- Updated
TransactionAPI.SendRawTransactionto (whenPrivateTxEnabled()): record private tx hashes, purge on local submission failure, and submit private txs to BPs with a user-facing wrapped error on relay failure. - Improved logging to include tx hashes for preconf/private relay submission failures and unified the wrapped error message between
SendRawTransactionandSendRawTransactionPrivate. - Added/updated unit tests to cover the unified behavior across flag combinations and key failure modes (local pool failure,
ErrAlreadyKnown, relay failures).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/ethapi/api.go | Implements unified private+preconf behavior in SendRawTransaction, adjusts logging, and aligns SendRawTransactionPrivate error/log output. |
| internal/ethapi/api_test.go | Expands test coverage for unified submission behavior and updates expected wrapped error message text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
manav2401
changed the title
|
@claude - review once |
|
Claude finished @pratikspatil024's task in 3m 38s —— View job Code Review: Unify Relay Endpoints & Rate Limit TX Submissions
SummaryThe PR achieves its goals: Bug: Type mismatch in test assertions will fail at runtime
require.Equal(t, defaultConfig.maxConcurrentPreconfs, cap(service.preconfSemaphore), ...)
require.Equal(t, defaultConfig.maxConcurrentPrivateTxs, cap(service.privateSemaphore), ...)
Fix: either cast to Medium: Blocking semaphore in
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| taskCh: make(chan TxTask, config.maxQueuedTasks), | ||
| preconfSemaphore: make(chan struct{}, config.maxConcurrentPreconfs), | ||
| privateSemaphore: make(chan struct{}, config.maxConcurrentPrivateTxs), | ||
| closeCh: make(chan struct{}), |
There was a problem hiding this comment.
ServiceConfig.maxConcurrentPreconfs / maxConcurrentPrivateTxs are uint64, but make(chan struct{}, ...) requires an int capacity. As written, this won’t compile. Convert safely (with bounds checking/clamping) or change these config fields to int (and validate CLI/config inputs) before using them for channel capacities.
| case task := <-s.taskCh: | ||
| // Acquire semaphore to limit concurrent submissions | ||
| select { | ||
| case s.semaphore <- struct{}{}: |
There was a problem hiding this comment.
This previous semaphore just existed for private submissions. Because I see this being replaced by preconfSemaphore and privateSemaphore. But what about Txs that doesnt fit on any of these tasks? Is it possible?
There was a problem hiding this comment.
That's not really possible - it could only belong to preconf or private tx task.
I separated them out so that we can have different limits for both if needed.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (88.57%) 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 @@
## master #2203 +/- ##
==========================================
- Coverage 52.21% 52.21% -0.01%
==========================================
Files 884 884
Lines 155479 155528 +49
==========================================
+ Hits 81190 81203 +13
- Misses 69060 69086 +26
- Partials 5229 5239 +10
... and 25 files with indirect coverage changes
🚀 New features to boost your workflow:
|
Description
This PR does the following things:
eth_sendRawTransactionendpoint to be used for both preconf and private transaction submission instead of using separate endpoints for both of them.eth_sendRawTransactionPrivateendpoint will still operate as it does now.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