fix(txpool-rpc): batch validity transaction insertion - #5064
Conversation
✅ Heimdall Review Status
|
|
✅ All benchmarks green — 14 within ±2% (deterministic instruction counts). View run Benchmark details (14)
|
| pool: Pool, | ||
| } | ||
|
|
||
| type ValidityTransactionSender = Arc< |
There was a problem hiding this comment.
ValidityTransactionSender is used in the public method with_validity_limits_and_sender (line 203) but is declared as a crate-private type alias. External consumers of this crate cannot name the type, which makes calling that constructor difficult and would trigger the workspace's unnameable-types = "warn" lint.
Per the project convention ("Make all structs, types, enums, and functions within modules pub, and re-export them from lib.rs"), this should be pub type ValidityTransactionSender = ... and re-exported from lib.rs.
| type ValidityTransactionSender = Arc< | |
| pub type ValidityTransactionSender = Arc< |
There was a problem hiding this comment.
Accepted: made ValidityTransactionSender public, re-exported it from the crate root, and kept the constructor public. Re-ran fmt, 33 unit tests, clippy with -D warnings, and git diff --check.
🤖 Reply generated with Toshi
|
<!-- CLAUDE_REVIEW_SUMMARY --> Review SummaryClean, well-scoped change that routes Findings
No block-production-critical issues identified. The change is in the RPC ingress path and |
d24e4ec to
769e5c7
Compare
| pub fn with_validity_limits_and_sender( | ||
| pool: Pool, | ||
| provider: Provider, | ||
| max_validity_predicates: usize, | ||
| max_validity_expiry_secs: u64, | ||
| transaction_sender: ValidityTransactionSender, | ||
| ) -> Self { | ||
| Self { | ||
| pool, | ||
| provider, | ||
| max_validity_predicates, | ||
| max_validity_expiry_secs, | ||
| transaction_sender: Some(transaction_sender), | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: When transaction_sender is Some (the production path via SendRawTransactionValidityExtension), the pool field is never read — it's only used in the else fallback for tests that construct the impl directly.
This means callers of with_validity_limits_and_sender must pass a pool that's never used in production. Consider making the pool Option<Pool> or removing it from the sender-backed constructor — though this may require trait-bound gymnastics. Alternatively, a brief doc comment on with_validity_limits_and_sender noting that pool is unused when the sender is present would clarify the intent.
There was a problem hiding this comment.
Addressed in 3d77731: the RPC now stores the concrete reth BatchTxRequest sender, has no direct-pool fallback, and uses that one route for all submissions. The obsolete callback/API surface is removed.
🤖 Reply generated with Toshi
| let transaction = transaction.with_validity_predicates(options.validity); | ||
| if let Some(transaction_sender) = &self.transaction_sender { | ||
| transaction_sender(TransactionOrigin::Private, transaction).await?; | ||
| } else { | ||
| self.pool | ||
| .add_transaction(TransactionOrigin::Private, transaction) | ||
| .await | ||
| .map_err(|error| ErrorObjectOwned::from(RpcPoolError::from(error)))?; | ||
| } |
There was a problem hiding this comment.
Error-code divergence between the two insertion paths.
The direct-pool fallback converts via RpcPoolError::from(error), which produces pool-specific JSON-RPC error codes (e.g., ALREADY_KNOWN, UNDERPRICED). The transaction_sender path converts via ErrorObjectOwned::from(BaseEthApiError), which can produce different error codes/messages for the same pool rejection.
RPC callers that pattern-match on error codes will see different responses depending on whether the shared sender is wired in. If the error contract matters for consumers of base_sendRawTransactionValidity, consider normalizing the error conversion so both paths produce the same RpcPoolError-style codes.
There was a problem hiding this comment.
Addressed in 3d77731: the RPC now stores the concrete reth BatchTxRequest sender, has no direct-pool fallback, and uses that one route for all submissions. The obsolete callback/API surface is removed.
🤖 Reply generated with Toshi
Review SummaryThis PR routes Findings
No block-production-critical findings. This PR touches the RPC ingress path only — transaction pool insertion via |
Co-authored-by: Toshi <[email protected]>
769e5c7 to
3d77731
Compare
Review SummaryClean refactor that replaces direct pool insertion with the shared Correctness: The change is sound. The validation pipeline (predicate limits, expiry bounds, fork gates, transaction decoding) runs entirely before the channel send, so validation behavior is preserved. The channel send + oneshot response pattern correctly propagates pool insertion errors back to the RPC caller. Test coverage: Tests are well-structured with two helpers — Block production sensitivity: Not on the critical path. This is purely an RPC ingress endpoint; payload builders read directly from the pool via No new findings beyond existing inline comments. The prior review comments about error-code divergence and type alias verbosity remain relevant to this iteration, though the specific code references in those comments are stale (they reference |
Change
Route
base_sendRawTransactionValiditythrough reth's shared transaction insertion sender instead of calling the pool directly.The RPC keeps
TransactionOrigin::Private, retains attached validity predicates, and returns the sender's RPC error. The extension injects the registered eth API sender; direct constructors retain the existing direct-pool path for focused tests.Validation
cargo test --locked -p base-txpool-rpc --lib(33 passed)cargo clippy --locked -p base-txpool-rpc --lib -- -D warningscargo +nightly fmt -p base-txpool-rpc -- --checkgit diff --checkGenerated with Toshi