Conversation
dimriou
force-pushed
the
read_multiplexer
branch
3 times, most recently
from
bd6d02d to
260b5f5
Compare
dimriou
force-pushed
the
read_multiplexer
branch
from
260b5f5 to
5aa8579
Compare
| defer cancel() | ||
|
|
||
| startedAt := time.Now() | ||
| raw, err := c.CallContextAllSequential(ctx, method, args...) |
There was a problem hiding this comment.
This means the total of the sequential read calls have max 1.5s in total to respond, isn't that too low? We can start with this, but do we have an idea of the avg/p99 read call latency?
There was a problem hiding this comment.
This is a bit of unknown, but here's my justification:
Across the Core node, our absolute minimum interval for each component is 2 seconds. Anything lower than that and you start having 100% utilization. For this case in particular, we need to come up with a number that is high enough but not too high so it adds additional latency for the components on chains with fast block time. Given how fast eth_transactionCount RPC calls are, I think 1.5s is decent enough to cycle through 3 RPCs (or even more) and not add potential latency. We have to take in consideration that if MultiNode is "locked" in a bad RPC, it means the max timeout we use might be consumed for an extended period of time, so we have to be careful with how high that value is.
Finally my thought process was this: I'd rather fail occasionally if this context is not enough than add latency that the node can't handle, the node would have failed without this anyway. I do agree that we should monitor this after we ship this change and see if this value is reasonable enough and revisit it if needed.
There was a problem hiding this comment.
Ok, that makes sense. Good to know about the 2s minimum interval for each core node component.
It's probably worthwhile to add some otel metric(s) to the code so that we can monitor how this change behaves after shipping it.
|
product-security-plaid-production
bot
requested review from
cedric-cordenier and
ilija42
There was a problem hiding this comment.
Pull request overview
Adds an optional “read multiplexer” for txm v2 nonce reads so the system can try multiple RPC nodes sequentially when fetching latest/pending nonces, improving resiliency when the currently-selected node is unhealthy.
Changes:
- Introduces
Client.CallContextAllSequentialand wires it through txm’sChainClientwrapper to multiplexNonceAt/PendingNonceAtreads. - Threads a new
ReadRequestsToMultipleNodesconfig flag through TOML/config interfaces into txm v2 client construction (including dualbroadcast selector). - Adds/updates unit tests for the new sequential call path and txm wrapper multiplexing.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/txmgr/builder.go | Wires new config flag into txm v2 client wrapper construction and dualbroadcast selector. |
| pkg/txm/clientwrappers/dualbroadcast/selector.go | Wraps the base client with the txm ChainClient wrapper (optionally multiplexing reads) before constructing dualbroadcast clients. |
| pkg/txm/clientwrappers/dualbroadcast/meta_client.go | Updates SendTransaction RPC interface and call sites to pass txm attempt context. |
| pkg/txm/clientwrappers/dualbroadcast/flashbots_client.go | Updates SendTransaction RPC interface and call sites to pass txm attempt context. |
| pkg/txm/clientwrappers/dualbroadcast/flashbots_client_test.go | Updates test RPC mock to match new SendTransaction signature. |
| pkg/txm/clientwrappers/chain_client.go | Adds multiplexed nonce reads (via raw JSON-RPC + decoding), timeout, and metrics wiring. |
| pkg/txm/clientwrappers/chain_client_metrics.go | Adds OTel histogram recording for multicall duration and outcome attributes. |
| pkg/txm/clientwrappers/chain_client_test.go | Adds tests for multicall sequencing and nonce decoding behavior. |
| pkg/config/toml/testdata/config-full.toml | Adds ReadRequestsToMultipleNodes to full config fixture. |
| pkg/config/toml/docs.toml | Documents new ReadRequestsToMultipleNodes option. |
| pkg/config/toml/config_test.go | Updates defaults/docs tests and full config struct to include new option. |
| pkg/config/toml/config.go | Adds TOML field and merge behavior for ReadRequestsToMultipleNodes. |
| pkg/config/config.go | Extends TransactionManagerV2 interface with ReadRequestsToMultipleNodes(). |
| pkg/config/chain_scoped_transactions.go | Implements new config accessor in chain-scoped config wrapper. |
| pkg/client/chain_client.go | Adds CallContextAllSequential to client interface and implements it using multinode iteration. |
| pkg/client/chain_client_test.go | Adds tests validating CallContextAllSequential behavior (success, cancellation, arg handling). |
| pkg/client/simulated_backend_client.go | Implements CallContextAllSequential shim for simulated backend client. |
| pkg/client/null_client.go | Implements CallContextAllSequential stub for null client. |
| pkg/client/clienttest/client.go | Extends autogenerated mock client with CallContextAllSequential. |
| CONFIG.md | Documents new ReadRequestsToMultipleNodes option in generated config docs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dimriou
force-pushed
the
read_multiplexer
branch
from
1bd17cc to
2d03da9
Compare
This PR allows v2 client wrappers to perform multiple read calls for the pending and the latest nonce. Every time a call is required, the client will cycle through the available alive RPCs and make a request until it receives a successful response. If it does it returns with the value. If it doesn't, it moves on to the next RPC. This change is important because it makes sure the confirmation logic will resume, even if the MultiNode has selected a bad RPC.