From 80a2f1f586230b09276295c3f1fd1591f1645fb6 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 14:03:42 +1000 Subject: [PATCH 01/49] fix(chain): retry transaction sends across every RPC provider Broadcasting a transaction built its own one-shot connection to whichever endpoint the pool happened to point at, so the one call that spends money got none of the retry and failover every read call gets. One provider answering an error abandoned the send with a healthy one unused. --- bin/dipper-service/src/chain_client/client.rs | 178 ++++++++++++++++-- .../src/chain_client/rpc_provider.rs | 121 +++++++++--- 2 files changed, 251 insertions(+), 48 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index c95550c9..ee75b5f2 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -21,6 +21,7 @@ use thegraph_core::alloy::{ rpc::types::TransactionRequest, signers::local::PrivateKeySigner, sol_types::{SolCall, SolValue}, + transports::TransportErrorKind, }; use tokio::sync::Mutex; @@ -455,29 +456,35 @@ impl AlloyChainClient { classify_fill_nonce_gap_outcome(nonce, self.send_transaction(&tx).await) } + /// Broadcast a transaction, retrying and rotating providers on transport faults the + /// way every read call already does. Rebroadcasting is safe because the caller fixed + /// the nonce, gas limit and both fees, so every endpoint sees one identical hash. async fn send_transaction(&self, tx: &TransactionRequest) -> Result { - let wallet = EthereumWallet::from(self.inner.signer.clone()); - let url = self.inner.rpc_pool.current_url().clone(); - - // Build HTTP client with timeout - let client = reqwest::Client::builder() - .timeout(self.inner.rpc_pool.request_timeout()) - .build() - .map_err(|e| { - ChainClientError::ConfigError(format!("Failed to build HTTP client: {e}")) - })?; - - // Build wallet-enabled provider - let provider = ProviderBuilder::new() - .wallet(wallet) - .connect_reqwest(client, url); + let request_timeout = self.inner.rpc_pool.request_timeout(); - let pending = provider - .send_transaction(tx.clone()) + self.inner + .rpc_pool + .execute_on_url("send_transaction", |url| { + let wallet = EthereumWallet::from(self.inner.signer.clone()); + let tx = tx.clone(); + async move { + let client = reqwest::Client::builder() + .timeout(request_timeout) + .build() + .map_err(|e| { + TransportErrorKind::custom(ChainClientError::ConfigError(format!( + "Failed to build HTTP client: {e}" + ))) + })?; + + let provider = ProviderBuilder::new() + .wallet(wallet) + .connect_reqwest(client, url); + + Ok(*provider.send_transaction(tx).await?.tx_hash()) + } + }) .await - .map_err(|e| ChainClientError::SubmitFailed(anyhow::anyhow!("Send failed: {e}")))?; - - Ok(*pending.tx_hash()) } /// Poll `eth_getTransactionReceipt` until the tx has mined or the timeout @@ -712,8 +719,139 @@ impl ChainClient for AlloyChainClient { #[cfg(test)] mod tests { + use url::Url; + use wiremock::{Mock, MockServer, Request, Respond, ResponseTemplate, matchers::method}; + use super::*; + /// Answers every JSON-RPC call with a fixed transaction hash, echoing the request id + /// so alloy's transport accepts the response. + struct SendResponder { + tx_hash: B256, + } + + impl Respond for SendResponder { + fn respond(&self, request: &Request) -> ResponseTemplate { + let body: serde_json::Value = + serde_json::from_slice(&request.body).expect("JSON-RPC request body"); + ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "jsonrpc": "2.0", + "id": body["id"], + "result": format!("{tx_hash:#x}", tx_hash = self.tx_hash), + })) + } + } + + async fn server_answering_with(tx_hash: B256) -> MockServer { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(SendResponder { tx_hash }) + .mount(&server) + .await; + server + } + + async fn server_answering_500() -> MockServer { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(500).set_body_string( + r#"{"id":0,"jsonrpc":"2.0","error":{"message":"Temporary internal error. Please retry","code":19}}"#, + )) + .mount(&server) + .await; + server + } + + fn client_over(providers: Vec) -> AlloyChainClient { + let config = ChainClientConfig { + enabled: true, + providers, + request_timeout: Duration::from_secs(5), + max_retries: 0, + domain_refresh_interval: Duration::from_secs(3600), + gas_price_multiplier: 1.2, + max_gas_price_gwei: 100, + gas_buffer_multiplier: 2.0, + gas_floor: 100_000, + gas_max_addition: 200_000, + }; + + AlloyChainClient::new( + &config, + 1337, + Address::repeat_byte(0x11), + Address::repeat_byte(0x22), + &[0x42; 32], + ) + .expect("chain client") + } + + /// A transaction with everything filled, so no filler needs to reach the network and + /// the bytes signed are identical whichever provider receives them. + fn ready_to_send_tx(from: Address) -> TransactionRequest { + TransactionRequest::default() + .from(from) + .to(Address::repeat_byte(0x33)) + .value(thegraph_core::alloy::primitives::U256::ZERO) + .with_gas_limit(21_000) + .with_max_fee_per_gas(2_000_000_000) + .with_max_priority_fee_per_gas(1_000_000_000) + .with_chain_id(1337) + .with_nonce(7) + } + + /// Submitting a transaction must fail over to the next provider, exactly as every read + /// call does. On 2026-07-29 the send bypassed the pool, so one endpoint answering 500 + /// stranded an accepted agreement while a healthy second endpoint was never tried. + #[tokio::test] + async fn send_transaction_rotates_to_the_next_provider_on_server_fault() { + let sick = server_answering_500().await; + let expected_hash = B256::repeat_byte(0xab); + let healthy = server_answering_with(expected_hash).await; + + let client = client_over(vec![ + sick.uri().parse().expect("sick provider URL"), + healthy.uri().parse().expect("healthy provider URL"), + ]); + let tx = ready_to_send_tx(client.inner.signer.address()); + + let tx_hash = client + .send_transaction(&tx) + .await + .expect("send should succeed on the second provider"); + + assert_eq!(tx_hash, expected_hash); + assert!( + !sick + .received_requests() + .await + .unwrap_or_default() + .is_empty(), + "the failing provider should have been tried first" + ); + assert!( + !healthy + .received_requests() + .await + .unwrap_or_default() + .is_empty(), + "the healthy provider should have been tried after rotation" + ); + } + + /// With a single provider there is nowhere to rotate to, so the fault must surface + /// rather than be retried forever or silently swallowed. + #[tokio::test] + async fn send_transaction_reports_failure_when_every_provider_is_sick() { + let sick = server_answering_500().await; + let client = client_over(vec![sick.uri().parse().expect("sick provider URL")]); + let tx = ready_to_send_tx(client.inner.signer.address()); + + let result = client.send_transaction(&tx).await; + + assert!(result.is_err(), "a 500 from the only provider must error"); + } + #[test] fn test_is_nonce_error() { // Nonce errors diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 5dd7bf84..9625121f 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -31,9 +31,8 @@ fn extract_chain_client_error(err: TransportError) -> Option { } } -/// Error patterns that indicate a transient failure worth retrying. -/// -/// These patterns are matched case-insensitively against the error message. +/// Error text that indicates a transient failure worth retrying, used only for faults +/// that arrive as prose rather than as a status code or JSON-RPC error object. const RETRYABLE_ERROR_PATTERNS: &[&str] = &[ "connection refused", "connection reset", @@ -42,13 +41,20 @@ const RETRYABLE_ERROR_PATTERNS: &[&str] = &[ "timed out", "rate limit", "too many requests", - "503", - "502", - "429", "service unavailable", "bad gateway", ]; +/// Build a read-only provider for one URL, with the pool's request timeout applied. +fn build_provider(url: Url, request_timeout: Duration) -> Result { + let client = reqwest::Client::builder() + .timeout(request_timeout) + .build() + .map_err(|e| ChainClientError::ConfigError(format!("Failed to build HTTP client: {e}")))?; + + Ok(ProviderBuilder::new().connect_reqwest(client, url)) +} + /// Type alias for the provider with default fillers. pub type HttpProvider = FillProvider< JoinFill< @@ -116,24 +122,6 @@ impl RpcProviderPool { &self.providers[idx] } - /// Build a provider for the current URL. - pub fn get_provider(&self) -> Result { - let url = self.current_url(); - - // Build HTTP client with timeout - let client = reqwest::Client::builder() - .timeout(self.request_timeout) - .build() - .map_err(|e| { - ChainClientError::ConfigError(format!("Failed to build HTTP client: {e}")) - })?; - - // Build alloy provider using the connect_reqwest method - let provider = ProviderBuilder::new().connect_reqwest(client, url.clone()); - - Ok(provider) - } - /// Rotate to the next provider. /// /// Returns the new provider URL after rotation. @@ -163,18 +151,41 @@ impl RpcProviderPool { where F: Fn(HttpProvider) -> Fut, Fut: Future>, + { + let f = &f; + self.execute_on_url(operation, move |url| { + let provider = build_provider(url, self.request_timeout); + async move { + match provider { + Ok(provider) => f(provider).await, + Err(e) => Err(TransportErrorKind::custom(e)), + } + } + }) + .await + } + + /// Same retry and rotation as [`Self::execute`], but the closure receives the URL + /// rather than a provider, so a caller needing a wallet attached can build its own + /// without reimplementing the retry, backoff and rotation policy. + pub async fn execute_on_url( + &self, + operation: &str, + f: F, + ) -> Result + where + F: Fn(Url) -> Fut, + Fut: Future>, { let mut last_error: Option = None; let mut providers_tried = 0; loop { - // Get current provider - let provider = self.get_provider()?; let current_url = self.current_url().clone(); // Retry loop for current provider for attempt in 0..=self.max_retries { - match f(provider.clone()).await { + match f(current_url.clone()).await { Ok(result) => return Ok(result), Err(e) if Self::is_retryable(&e) && attempt < self.max_retries => { let delay = Self::backoff_delay(attempt); @@ -231,8 +242,19 @@ impl RpcProviderPool { } } - /// Check if an error is retryable. + /// Whether an error is worth trying again rather than giving up on. Reads the HTTP + /// status where the transport reports one, since a status is unambiguous while the + /// same digits inside a revert reason are not, and matches text only without one. fn is_retryable(error: &TransportError) -> bool { + if let RpcError::Transport(kind) = error { + if let Some(http) = kind.as_http_error() { + // A 5xx is the server failing for its own reasons and 429 is it + // declining; either can succeed on a retry or another provider. Other + // 4xx means the request is wrong, so repeating it cannot help. + return http.status >= 500 || http.status == 429; + } + } + let error_str = error.to_string().to_lowercase(); RETRYABLE_ERROR_PATTERNS .iter() @@ -255,6 +277,49 @@ impl RpcProviderPool { mod tests { use super::*; + /// A provider answering 500 is the failure that stranded an accepted agreement in + /// production on 2026-07-29: the status was never read, the digits 500 were not in + /// the text patterns, and the submission was abandoned as unretryable. + #[test] + fn server_faults_are_retryable_by_status() { + for status in [500, 502, 503, 504, 429] { + let err = TransportErrorKind::http_error(status, "provider fault".to_string()); + assert!( + RpcProviderPool::is_retryable(&err), + "HTTP {status} should be retryable" + ); + } + } + + /// A 4xx other than 429 means the request itself is wrong, so resending it unchanged + /// to the same or another provider cannot succeed. + #[test] + fn client_faults_are_not_retryable_by_status() { + for status in [400, 401, 403, 404] { + let err = TransportErrorKind::http_error(status, "bad request".to_string()); + assert!( + !RpcProviderPool::is_retryable(&err), + "HTTP {status} should not be retryable" + ); + } + } + + /// Reading the status rather than the message keeps a revert reason that happens to + /// contain 502 or 429 from being mistaken for a transport fault and retried. + #[test] + fn revert_text_containing_status_digits_is_not_retryable() { + for body in [ + "execution reverted: price 502 below floor", + "execution reverted: only 429 tokens remain", + ] { + let err = TransportErrorKind::http_error(200, body.to_string()); + assert!( + !RpcProviderPool::is_retryable(&err), + "{body} should not be retryable" + ); + } + } + #[test] fn test_backoff_delay_calculation() { // 1s, 2s, 4s, 8s, 16s, 32s->30s From 9d6f62a5952b9df390d179ee75f7f4e081b4cfe1 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 14:11:54 +1000 Subject: [PATCH 02/49] fix(chain): collapse the nested status check so clippy passes --- .../src/chain_client/rpc_provider.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 9625121f..32d1021e 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -246,13 +246,13 @@ impl RpcProviderPool { /// status where the transport reports one, since a status is unambiguous while the /// same digits inside a revert reason are not, and matches text only without one. fn is_retryable(error: &TransportError) -> bool { - if let RpcError::Transport(kind) = error { - if let Some(http) = kind.as_http_error() { - // A 5xx is the server failing for its own reasons and 429 is it - // declining; either can succeed on a retry or another provider. Other - // 4xx means the request is wrong, so repeating it cannot help. - return http.status >= 500 || http.status == 429; - } + if let RpcError::Transport(kind) = error + && let Some(http) = kind.as_http_error() + { + // A 5xx is the server failing for its own reasons and 429 is it declining; + // either can succeed on a retry or another provider. Other 4xx means the + // request is wrong, so repeating it unchanged cannot help. + return http.status >= 500 || http.status == 429; } let error_str = error.to_string().to_lowercase(); From 8d76d9bae8bfd2a3dd49eb15edcc130cc2112a57 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 14:23:05 +1000 Subject: [PATCH 03/49] fix(chain): retry providers that report overload as a JSON-RPC error Some providers answer with HTTP 200 and put the overload in the JSON-RPC error object, each with its own code, so reading the HTTP status alone still treated those as permanent. Defer to alloy, which knows the provider codes and reports an execution error as not worth retrying. --- bin/dipper-service/src/chain_client/rpc_provider.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 32d1021e..8e2a56a4 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -255,6 +255,13 @@ impl RpcProviderPool { return http.status >= 500 || http.status == 429; } + // Some providers answer 200 and report being overloaded in the JSON-RPC error + // instead, each with its own code. Alloy knows those codes, and reports a genuine + // execution error such as a revert as not worth retrying. + if let RpcError::ErrorResp(payload) = error { + return payload.is_retry_err(); + } + let error_str = error.to_string().to_lowercase(); RETRYABLE_ERROR_PATTERNS .iter() From 1847adfffb28f07d51516bf6dc4f46f61f65316b Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 14:39:26 +1000 Subject: [PATCH 04/49] docs(chain): fix the deadline the receipt timeout is judged against The note explaining why the receipt poll waits 15 seconds cited a 300 second agreement deadline, while the configured default is 600, so anyone checking whether the retry budget fits inside the deadline started from a figure twice too small. --- bin/dipper-service/src/chain_client/client.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index ee75b5f2..134e161d 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -41,11 +41,9 @@ use crate::{ /// RecurringCollectorInvalidOfferType(0). const OFFER_TYPE_NEW: u8 = 1; -/// Time to wait for a tx receipt to appear before declaring the tx dropped -/// from the mempool. On hardhat this is ~15 blocks at 1s each; on Arbitrum -/// at 0.25s block time this is 60 confirmations. Short enough that the -/// pgmq retry budget can recover within the 300s RCA deadline, long enough -/// to tolerate typical network glitches. +/// Time to wait for a tx receipt before declaring the tx dropped from the mempool: ~15 +/// blocks on hardhat at 1s each, 60 confirmations on Arbitrum at 0.25s. Short enough that +/// the pgmq retry budget recovers inside the RCA deadline `deadline_seconds` sets. const RECEIPT_POLL_TIMEOUT: Duration = Duration::from_secs(15); /// Interval between `eth_getTransactionReceipt` polls while waiting for a From 64d45fff3261e49fb1f5aa8925880928c726ab41 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 15:11:28 +1000 Subject: [PATCH 05/49] fix(chain): keep the provider's own words when every provider fails Callers read that text to recognise a stale nonce and resync from the chain, and it is the only record of why a submission was refused, but the pool replaced it with a count of providers tried. A rejection therefore stopped being recognisable once the send went through the pool. --- bin/dipper-service/src/chain_client/client.rs | 98 ++++++++++++++++++- .../src/chain_client/rpc_provider.rs | 70 +++++-------- 2 files changed, 124 insertions(+), 44 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 134e161d..f9a68721 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -760,12 +760,32 @@ mod tests { server } + /// Answers HTTP 200 carrying a JSON-RPC error, which is how a chain reports a rejection + /// such as a stale nonce and how some providers report being overloaded. + async fn server_answering_rpc_error(code: i64, message: &str) -> MockServer { + let server = MockServer::start().await; + let body = serde_json::json!({ + "id": 0, + "jsonrpc": "2.0", + "error": { "code": code, "message": message }, + }); + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(200).set_body_json(body)) + .mount(&server) + .await; + server + } + fn client_over(providers: Vec) -> AlloyChainClient { + client_over_retrying(providers, 0) + } + + fn client_over_retrying(providers: Vec, max_retries: u32) -> AlloyChainClient { let config = ChainClientConfig { enabled: true, providers, request_timeout: Duration::from_secs(5), - max_retries: 0, + max_retries, domain_refresh_interval: Duration::from_secs(3600), gas_price_multiplier: 1.2, max_gas_price_gwei: 100, @@ -850,6 +870,82 @@ mod tests { assert!(result.is_err(), "a 500 from the only provider must error"); } + /// What the chain said has to survive the pool, because `sign_and_send` reads this text to + /// recognise a stale nonce and resync from the chain. Routing the send through the pool + /// once replaced the reason with a generic summary, which silently disabled that recovery. + #[tokio::test] + async fn send_transaction_surfaces_what_the_provider_said() { + let rejecting = server_answering_rpc_error(-32000, "nonce too low: next nonce 12").await; + let client = client_over(vec![rejecting.uri().parse().expect("provider URL")]); + let tx = ready_to_send_tx(client.inner.signer.address()); + + let err = client + .send_transaction(&tx) + .await + .expect_err("a rejected transaction must error"); + + let text = err.to_string(); + assert!( + is_nonce_error(&text), + "the nonce reason must still be recognisable, got: {text}" + ); + } + + /// A provider reporting overload in the JSON-RPC error rather than the HTTP status earns a + /// retry on that provider, while a chain rejection does not, since it will answer the same. + #[tokio::test] + async fn json_rpc_errors_decide_whether_the_provider_is_retried() { + let overloaded = + server_answering_rpc_error(-32005, "project ID request rate exceeded").await; + let healthy = server_answering_with(B256::repeat_byte(0xcd)).await; + let client = client_over_retrying( + vec![ + overloaded.uri().parse().expect("overloaded provider URL"), + healthy.uri().parse().expect("healthy provider URL"), + ], + 1, + ); + let tx = ready_to_send_tx(client.inner.signer.address()); + + client + .send_transaction(&tx) + .await + .expect("send should succeed on the healthy provider"); + + assert_eq!( + overloaded + .received_requests() + .await + .unwrap_or_default() + .len(), + 2, + "a rate-limited provider should be retried once before rotating" + ); + + let rejecting = server_answering_rpc_error(-32000, "nonce too low: next nonce 12").await; + let spare = server_answering_with(B256::repeat_byte(0xef)).await; + let client = client_over_retrying( + vec![ + rejecting.uri().parse().expect("rejecting provider URL"), + spare.uri().parse().expect("spare provider URL"), + ], + 1, + ); + let tx = ready_to_send_tx(client.inner.signer.address()); + + let _ = client.send_transaction(&tx).await; + + assert_eq!( + rejecting + .received_requests() + .await + .unwrap_or_default() + .len(), + 1, + "a chain rejection should not be retried against the same provider" + ); + } + #[test] fn test_is_nonce_error() { // Nonce errors diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 8e2a56a4..0be8318b 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -116,12 +116,6 @@ impl RpcProviderPool { self.request_timeout } - /// Get the current provider URL. - pub fn current_url(&self) -> &Url { - let idx = self.current_index.load(Ordering::Relaxed) % self.providers.len(); - &self.providers[idx] - } - /// Rotate to the next provider. /// /// Returns the new provider URL after rotation. @@ -180,8 +174,14 @@ impl RpcProviderPool { let mut last_error: Option = None; let mut providers_tried = 0; + // Walk the ring by local offset from wherever the pool points. Re-reading the shared + // index each time lets a concurrent rotation send this call back to a provider it + // already tried, so it can give up without ever reaching the healthy one. + let start = self.current_index.load(Ordering::Relaxed); + loop { - let current_url = self.current_url().clone(); + let current_url = + self.providers[(start + providers_tried) % self.providers.len()].clone(); // Retry loop for current provider for attempt in 0..=self.max_retries { @@ -215,6 +215,11 @@ impl RpcProviderPool { let final_err = last_error.unwrap_or_else(|| TransportErrorKind::custom_str("unknown error")); + // Keep what the provider actually said. Callers read this text to tell a + // nonce rejection from anything else, and it is the only record of why every + // provider refused, since a non-retryable attempt logs nothing. + let cause = final_err.to_string(); + // Preserve structured ChainClientError instances boxed in via // TransportErrorKind::custom (e.g. ContractRevert from gas // estimation). Otherwise fall back to the generic wrap. @@ -223,20 +228,24 @@ impl RpcProviderPool { } return Err(ChainClientError::RpcError(anyhow::anyhow!( - "All {} RPC providers failed for '{}'", + "All {} RPC providers failed for '{}': {}", self.providers.len(), operation, + cause, ))); } - // Rotate to next provider - let new_url = self.rotate(); + // Advance the shared index as well, so later calls start from a provider that has + // not just failed rather than repeating this one's discovery. + self.rotate(); + let next_url = &self.providers[(start + providers_tried) % self.providers.len()]; tracing::warn!( operation, old_provider = %current_url, - new_provider = %new_url, + new_provider = %next_url, providers_tried, total_providers = self.providers.len(), + error = last_error.as_ref().map(|e| e.to_string()).unwrap_or_default(), "Rotating RPC provider after failures" ); } @@ -284,9 +293,9 @@ impl RpcProviderPool { mod tests { use super::*; - /// A provider answering 500 is the failure that stranded an accepted agreement in - /// production on 2026-07-29: the status was never read, the digits 500 were not in - /// the text patterns, and the submission was abandoned as unretryable. + /// 500 is the status a provider answered on 2026-07-29 while an accepted agreement went + /// unfunded. Reading it here is what earns a retry on that provider before rotating; the + /// rotation itself is unconditional, so this decides attempts rather than failover. #[test] fn server_faults_are_retryable_by_status() { for status in [500, 502, 503, 504, 429] { @@ -311,22 +320,6 @@ mod tests { } } - /// Reading the status rather than the message keeps a revert reason that happens to - /// contain 502 or 429 from being mistaken for a transport fault and retried. - #[test] - fn revert_text_containing_status_digits_is_not_retryable() { - for body in [ - "execution reverted: price 502 below floor", - "execution reverted: only 429 tokens remain", - ] { - let err = TransportErrorKind::http_error(200, body.to_string()); - assert!( - !RpcProviderPool::is_retryable(&err), - "{body} should not be retryable" - ); - } - } - #[test] fn test_backoff_delay_calculation() { // 1s, 2s, 4s, 8s, 16s, 32s->30s @@ -401,19 +394,10 @@ mod tests { let pool = RpcProviderPool::new(providers.clone(), Duration::from_secs(30), 3).unwrap(); - // Initially at index 0 - assert_eq!(pool.current_url().as_str(), "https://rpc1.example.com/"); - - // Rotate to index 1 - pool.rotate(); - assert_eq!(pool.current_url().as_str(), "https://rpc2.example.com/"); - - // Rotate to index 2 - pool.rotate(); - assert_eq!(pool.current_url().as_str(), "https://rpc3.example.com/"); + assert_eq!(pool.rotate().as_str(), "https://rpc2.example.com/"); + assert_eq!(pool.rotate().as_str(), "https://rpc3.example.com/"); - // Rotate wraps back to index 0 - pool.rotate(); - assert_eq!(pool.current_url().as_str(), "https://rpc1.example.com/"); + // Wraps back round rather than running off the end. + assert_eq!(pool.rotate().as_str(), "https://rpc1.example.com/"); } } From 91f71bf42a292c2283f9b664ca63be9c7ec48d32 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 16:02:47 +1000 Subject: [PATCH 06/49] docs(chain): correct and trim the RPC pool's comments One comment claimed the final error message was the only record of why every endpoint refused, which stopped being true once the rotation warning started carrying each one. The pool's other doc comments restated the function signatures, so they lose the padding. --- .../src/chain_client/rpc_provider.rs | 36 +++++-------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 0be8318b..16fa9d72 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -64,10 +64,8 @@ pub type HttpProvider = FillProvider< RootProvider, >; -/// RPC provider pool with automatic rotation and retry. -/// -/// Manages multiple RPC provider URLs and automatically rotates between them -/// on failure. Uses exponential backoff for retries within a single provider. +/// Several RPC endpoints treated as one, retried with exponential backoff on the current +/// endpoint and rotated through on failure. #[derive(Debug)] pub struct RpcProviderPool { /// Provider URLs (primary first, then fallbacks) @@ -81,11 +79,7 @@ pub struct RpcProviderPool { } impl RpcProviderPool { - /// Create a new RPC provider pool. - /// - /// # Errors - /// - /// Returns an error if no providers are configured. + /// Create a new RPC provider pool. Errors if no providers are configured. pub fn new( providers: Vec, request_timeout: Duration, @@ -125,22 +119,8 @@ impl RpcProviderPool { &self.providers[new_idx] } - /// Execute an RPC operation with retry and provider rotation. - /// - /// The closure receives a provider and should return a `Result`. On retryable - /// errors, the operation is retried with exponential backoff. After exhausting - /// retries, the pool rotates to the next provider and continues. - /// - /// # Arguments - /// - /// * `operation` - Name of the operation (for logging) - /// * `f` - Closure that performs the RPC call - /// - /// # Type Parameters - /// - /// * `F` - Closure type - /// * `Fut` - Future type returned by the closure - /// * `T` - Success type + /// Run an RPC call, retrying the current endpoint with backoff and then rotating on to + /// the next. `operation` names the call for logging. pub async fn execute(&self, operation: &str, f: F) -> Result where F: Fn(HttpProvider) -> Fut, @@ -215,9 +195,9 @@ impl RpcProviderPool { let final_err = last_error.unwrap_or_else(|| TransportErrorKind::custom_str("unknown error")); - // Keep what the provider actually said. Callers read this text to tell a - // nonce rejection from anything else, and it is the only record of why every - // provider refused, since a non-retryable attempt logs nothing. + // Keep what the provider actually said, because callers read this text to + // tell a nonce rejection from anything else. The rotation warning below + // carries each earlier provider's reason; this one carries the last. let cause = final_err.to_string(); // Preserve structured ChainClientError instances boxed in via From d443c7bf5c5322e7ca73782cb0a897b1097bb17d Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 16:03:51 +1000 Subject: [PATCH 07/49] refactor(chain): give the provider ring one indexing helper Working out which endpoint a ring position lands on was written out three times, once in the rotation counter and twice inside the retry loop, so a change to one could silently disagree with the others. They now share a single small helper. --- bin/dipper-service/src/chain_client/rpc_provider.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 16fa9d72..9f185225 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -115,8 +115,12 @@ impl RpcProviderPool { /// Returns the new provider URL after rotation. pub fn rotate(&self) -> &Url { let old_idx = self.current_index.fetch_add(1, Ordering::Relaxed); - let new_idx = (old_idx + 1) % self.providers.len(); - &self.providers[new_idx] + self.url_at(old_idx + 1) + } + + /// The endpoint an unbounded ring position lands on. + fn url_at(&self, position: usize) -> &Url { + &self.providers[position % self.providers.len()] } /// Run an RPC call, retrying the current endpoint with backoff and then rotating on to @@ -160,8 +164,7 @@ impl RpcProviderPool { let start = self.current_index.load(Ordering::Relaxed); loop { - let current_url = - self.providers[(start + providers_tried) % self.providers.len()].clone(); + let current_url = self.url_at(start + providers_tried).clone(); // Retry loop for current provider for attempt in 0..=self.max_retries { @@ -218,7 +221,7 @@ impl RpcProviderPool { // Advance the shared index as well, so later calls start from a provider that has // not just failed rather than repeating this one's discovery. self.rotate(); - let next_url = &self.providers[(start + providers_tried) % self.providers.len()]; + let next_url = self.url_at(start + providers_tried); tracing::warn!( operation, old_provider = %current_url, From 18e50e9da735fcde6f75c0d2ed190498197c84d2 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 16:05:08 +1000 Subject: [PATCH 08/49] fix(chain): report what every RPC endpoint said, not just the last A submission rejected for a stale nonce is recognised by reading the error text, but that text only carried the last endpoint's reason, so a sick second endpoint hid the first one's rejection and the recovery never ran. Every reason is now in the message. --- .../src/chain_client/rpc_provider.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 9f185225..b324cce3 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -156,6 +156,10 @@ impl RpcProviderPool { Fut: Future>, { let mut last_error: Option = None; + // What each endpoint said, in the order they were tried. `sign_and_send` matches + // this text to tell a stale nonce from a transport fault, so a rejection from the + // first endpoint has to survive a different kind of failure on the next. + let mut reasons: Vec = Vec::with_capacity(self.providers.len()); let mut providers_tried = 0; // Walk the ring by local offset from wherever the pool points. Re-reading the shared @@ -191,6 +195,13 @@ impl RpcProviderPool { } } + reasons.push(format!( + "{current_url}: {}", + last_error + .as_ref() + .map(|e| e.to_string()) + .unwrap_or_else(|| "unknown error".to_string()) + )); providers_tried += 1; // Check if we've tried all providers @@ -198,11 +209,6 @@ impl RpcProviderPool { let final_err = last_error.unwrap_or_else(|| TransportErrorKind::custom_str("unknown error")); - // Keep what the provider actually said, because callers read this text to - // tell a nonce rejection from anything else. The rotation warning below - // carries each earlier provider's reason; this one carries the last. - let cause = final_err.to_string(); - // Preserve structured ChainClientError instances boxed in via // TransportErrorKind::custom (e.g. ContractRevert from gas // estimation). Otherwise fall back to the generic wrap. @@ -214,7 +220,7 @@ impl RpcProviderPool { "All {} RPC providers failed for '{}': {}", self.providers.len(), operation, - cause, + reasons.join("; "), ))); } From d61152a92284bb6f3d1976d27657b452fdf8d1c4 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 16:06:16 +1000 Subject: [PATCH 09/49] refactor(chain): keep each endpoint's failure reason with that endpoint One variable carried the last failure across every endpoint, so reading it back always meant handling an "absent" case that could not happen and left the log line unsure whose failure it was printing. --- .../src/chain_client/rpc_provider.rs | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index b324cce3..bdc4e04d 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -155,7 +155,6 @@ impl RpcProviderPool { F: Fn(Url) -> Fut, Fut: Future>, { - let mut last_error: Option = None; // What each endpoint said, in the order they were tried. `sign_and_send` matches // this text to tell a stale nonce from a transport fault, so a rejection from the // first endpoint has to survive a different kind of failure on the next. @@ -171,6 +170,7 @@ impl RpcProviderPool { let current_url = self.url_at(start + providers_tried).clone(); // Retry loop for current provider + let mut endpoint_error: Option = None; for attempt in 0..=self.max_retries { match f(current_url.clone()).await { Ok(result) => return Ok(result), @@ -186,33 +186,28 @@ impl RpcProviderPool { "Retryable RPC error, backing off" ); tokio::time::sleep(delay).await; - last_error = Some(e); + endpoint_error = Some(e); } Err(e) => { - last_error = Some(e); + endpoint_error = Some(e); break; } } } + // Every attempt records why it failed before stopping, so the fallback only + // covers a configuration that somehow allows no attempt at all. + let endpoint_error = endpoint_error + .unwrap_or_else(|| TransportErrorKind::custom_str("no attempt was made")); - reasons.push(format!( - "{current_url}: {}", - last_error - .as_ref() - .map(|e| e.to_string()) - .unwrap_or_else(|| "unknown error".to_string()) - )); + reasons.push(format!("{current_url}: {endpoint_error}")); providers_tried += 1; // Check if we've tried all providers if providers_tried >= self.providers.len() { - let final_err = - last_error.unwrap_or_else(|| TransportErrorKind::custom_str("unknown error")); - // Preserve structured ChainClientError instances boxed in via // TransportErrorKind::custom (e.g. ContractRevert from gas // estimation). Otherwise fall back to the generic wrap. - if let Some(typed) = extract_chain_client_error(final_err) { + if let Some(typed) = extract_chain_client_error(endpoint_error) { return Err(typed); } @@ -234,7 +229,7 @@ impl RpcProviderPool { new_provider = %next_url, providers_tried, total_providers = self.providers.len(), - error = last_error.as_ref().map(|e| e.to_string()).unwrap_or_default(), + error = %endpoint_error, "Rotating RPC provider after failures" ); } From 7dd71053aa1f9121fdec222338ec6ce416df01f3 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:29:11 +1000 Subject: [PATCH 10/49] test(chain): cover a nonce rejection followed by a sick second endpoint The existing test used a single endpoint, which is the one arrangement where the rejection reason could not be lost. With two endpoints the reason has to survive the second one failing a different way, which is the case that stranded a submission in practice. --- bin/dipper-service/src/chain_client/client.rs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index f9a68721..d4993ad8 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -891,6 +891,31 @@ mod tests { ); } + /// The nonce reason has to survive a second provider failing a different way. Reporting + /// only the last provider's reason hid the rejection behind a transport fault, and + /// `sign_and_send` then skipped the resync that would have unstuck the wallet. + #[tokio::test] + async fn send_transaction_surfaces_a_nonce_reason_from_any_provider() { + let rejecting = server_answering_rpc_error(-32000, "nonce too low: next nonce 12").await; + let sick = server_answering_500().await; + let client = client_over(vec![ + rejecting.uri().parse().expect("rejecting provider URL"), + sick.uri().parse().expect("sick provider URL"), + ]); + let tx = ready_to_send_tx(client.inner.signer.address()); + + let err = client + .send_transaction(&tx) + .await + .expect_err("both providers refused, so the send must error"); + + let text = err.to_string(); + assert!( + is_nonce_error(&text), + "the nonce reason must survive the later transport fault, got: {text}" + ); + } + /// A provider reporting overload in the JSON-RPC error rather than the HTTP status earns a /// retry on that provider, while a chain rejection does not, since it will answer the same. #[tokio::test] From 3146ff5cf2d4678eee6c60368070f97ebf0ae734 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:29:57 +1000 Subject: [PATCH 11/49] fix(chain): stop the status check from vetoing the other retry checks Reading the HTTP status and the error code was written so that either one answering "no" ended the question, which quietly dropped faults only the wording identifies, such as a throttling notice sent under a 403 or a timeout described in a 200 response. --- .../src/chain_client/rpc_provider.rs | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index bdc4e04d..518055cc 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -235,24 +235,26 @@ impl RpcProviderPool { } } - /// Whether an error is worth trying again rather than giving up on. Reads the HTTP - /// status where the transport reports one, since a status is unambiguous while the - /// same digits inside a revert reason are not, and matches text only without one. + /// Whether an error is worth trying again rather than giving up on. Each check can only + /// say yes, so a fault the status and the error code both miss still gets read as text. fn is_retryable(error: &TransportError) -> bool { + // A 5xx is the server failing for its own reasons and 429 is it declining; either + // can succeed on a retry or another endpoint. A status is worth reading before the + // text, because the same digits inside a revert reason mean nothing. if let RpcError::Transport(kind) = error && let Some(http) = kind.as_http_error() + && (http.status >= 500 || http.status == 429) { - // A 5xx is the server failing for its own reasons and 429 is it declining; - // either can succeed on a retry or another provider. Other 4xx means the - // request is wrong, so repeating it unchanged cannot help. - return http.status >= 500 || http.status == 429; + return true; } // Some providers answer 200 and report being overloaded in the JSON-RPC error // instead, each with its own code. Alloy knows those codes, and reports a genuine // execution error such as a revert as not worth retrying. - if let RpcError::ErrorResp(payload) = error { - return payload.is_retry_err(); + if let RpcError::ErrorResp(payload) = error + && payload.is_retry_err() + { + return true; } let error_str = error.to_string().to_lowercase(); From 26969c782fd8f1ab77fc0640d8980e0083e82bee Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:30:36 +1000 Subject: [PATCH 12/49] test(chain): cover throttling reported under a client-fault status Providers and the proxies in front of them sometimes report throttling with a status that otherwise means the request was malformed. Only the wording distinguishes the two, so this pins that a retry still follows. --- bin/dipper-service/src/chain_client/rpc_provider.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 518055cc..005070b0 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -306,6 +306,18 @@ mod tests { } } + /// Some providers and the proxies in front of them report throttling under a 4xx rather + /// than a 429. The status alone reads as "your request is wrong", so the wording is what + /// tells this apart from a request that will be refused however often it is sent. + #[test] + fn throttling_described_in_a_client_fault_body_is_retryable() { + let err = TransportErrorKind::http_error(403, "rate limit exceeded".to_string()); + assert!( + RpcProviderPool::is_retryable(&err), + "a 403 that explains it is throttling should be retryable" + ); + } + #[test] fn test_backoff_delay_calculation() { // 1s, 2s, 4s, 8s, 16s, 32s->30s From 7f46a8bdf717eb323bd73f95bcc59745b1372645 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:32:06 +1000 Subject: [PATCH 13/49] test(chain): cover a transient fault described only in the error body The list of error codes that mean "throttled" is well known and already handled, but a gateway reporting a dropped connection inside an otherwise normal response has no such code. The wording is the only signal, so this pins that a retry still follows. --- .../src/chain_client/rpc_provider.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 005070b0..a8974ea5 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -318,6 +318,20 @@ mod tests { ); } + /// Alloy recognises the error codes providers use for throttling, but not every + /// transient fault has one. A gateway that describes a timeout in an otherwise ordinary + /// response is still worth another go, and only the wording says so. + #[test] + fn transient_faults_described_in_a_json_rpc_error_are_retryable() { + let payload = serde_json::from_str(r#"{"code":-32603,"message":"connection reset"}"#) + .expect("JSON-RPC error payload"); + let err: TransportError = RpcError::ErrorResp(payload); + assert!( + RpcProviderPool::is_retryable(&err), + "a reset described in the error body should be retryable" + ); + } + #[test] fn test_backoff_delay_calculation() { // 1s, 2s, 4s, 8s, 16s, 32s->30s From a84567825106aacc9ad7aa71717acd0e60a961a9 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:33:27 +1000 Subject: [PATCH 14/49] test(chain): check the retry decision itself, not a copy of its rules This test rebuilt the wording comparison inline and compared strings against the list, so it passed no matter what the real decision did. It now calls that decision, and the cases naming bare status numbers are gone since those are judged by status elsewhere. --- .../src/chain_client/rpc_provider.rs | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index a8974ea5..ee4942fb 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -344,29 +344,28 @@ mod tests { assert_eq!(RpcProviderPool::backoff_delay(10), Duration::from_secs(30)); // stays capped } + /// Faults that arrive with no status and no error code, only a description, which is + /// what a connection that never got a reply looks like. #[test] fn test_retryable_error_detection() { - // Test retryable patterns let retryable_errors = [ "connection refused by remote host", "Connection Reset by peer", "request TIMEOUT exceeded", - "HTTP 429 Too Many Requests", - "503 Service Unavailable", - "502 Bad Gateway", + "Service Unavailable", + "Bad Gateway", "rate limit exceeded", + "too many requests, slow down", ]; for err_str in retryable_errors { - // Create a mock transport error by using the error message - // In practice, TransportError wraps various error types - let is_match = RETRYABLE_ERROR_PATTERNS - .iter() - .any(|p| err_str.to_lowercase().contains(p)); - assert!(is_match, "Expected '{}' to be retryable", err_str); + let err = TransportErrorKind::custom_str(err_str); + assert!( + RpcProviderPool::is_retryable(&err), + "expected '{err_str}' to be retryable" + ); } - // Test non-retryable patterns let non_retryable_errors = [ "nonce too low", "insufficient funds", @@ -375,10 +374,11 @@ mod tests { ]; for err_str in non_retryable_errors { - let is_match = RETRYABLE_ERROR_PATTERNS - .iter() - .any(|p| err_str.to_lowercase().contains(p)); - assert!(!is_match, "Expected '{}' to NOT be retryable", err_str); + let err = TransportErrorKind::custom_str(err_str); + assert!( + !RpcProviderPool::is_retryable(&err), + "expected '{err_str}' to not be retryable" + ); } } From 7cc380ccd2ab54f1e3ef9efb0f50d5de2324f4a9 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:34:15 +1000 Subject: [PATCH 15/49] test(chain): make the send mock reject calls it cannot sensibly answer The mock replied to every request with a transaction hash, so a future change that made the client look up the chain id or a gas price first would have been handed a hash where a number belongs and failed somewhere confusing. It now says what went wrong. --- bin/dipper-service/src/chain_client/client.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index d4993ad8..86b15f3f 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -722,8 +722,9 @@ mod tests { use super::*; - /// Answers every JSON-RPC call with a fixed transaction hash, echoing the request id - /// so alloy's transport accepts the response. + /// Answers a send with a fixed transaction hash, echoing the request id so alloy's + /// transport accepts the response. Any other call is a mistake in the test rather than + /// something to answer with a hash, so say so instead of returning nonsense. struct SendResponder { tx_hash: B256, } @@ -732,6 +733,11 @@ mod tests { fn respond(&self, request: &Request) -> ResponseTemplate { let body: serde_json::Value = serde_json::from_slice(&request.body).expect("JSON-RPC request body"); + let method = body["method"].as_str().unwrap_or_default(); + assert!( + method.starts_with("eth_send"), + "this mock only answers sends, got {method}" + ); ResponseTemplate::new(200).set_body_json(serde_json::json!({ "jsonrpc": "2.0", "id": body["id"], From 6c62ad7da111c7f5fd7909d75f4ab348b8d21246 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:37:54 +1000 Subject: [PATCH 16/49] fix(chain): treat a transaction already in a mempool as sent Offering identical bytes to a second endpoint is answered with "I already have this", which was read as a stale nonce and prompted a second transaction at a fresh nonce, paying twice for one agreement offer. Signing now happens once up front, so that answer carries a hash. --- bin/dipper-service/src/chain_client/client.rs | 151 +++++++++++++++--- .../src/chain_client/rpc_provider.rs | 36 +---- 2 files changed, 132 insertions(+), 55 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 86b15f3f..5e8eb279 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -14,14 +14,14 @@ use std::{ use async_trait::async_trait; use dipper_rpc::indexer::indexer_client::sol::RecurringCollectionAgreement; use thegraph_core::alloy::{ - eips::BlockNumberOrTag, + eips::{BlockNumberOrTag, eip2718::Encodable2718}, network::{EthereumWallet, TransactionBuilder}, primitives::{Address, B256, FixedBytes}, - providers::{Provider, ProviderBuilder}, + providers::Provider, rpc::types::TransactionRequest, signers::local::PrivateKeySigner, sol_types::{SolCall, SolValue}, - transports::TransportErrorKind, + transports::TransportError, }; use tokio::sync::Mutex; @@ -78,6 +78,18 @@ fn is_nonce_error(error: &str) -> bool { NONCE_ERROR_PATTERNS.iter().any(|p| lower.contains(p)) } +/// How nodes say they are already holding the transaction being offered to them. +const ALREADY_BROADCAST_PATTERNS: &[&str] = + &["already known", "already imported", "known transaction"]; + +/// Whether a rejection means this exact transaction is already in a mempool. Offering the +/// same signed bytes to a second endpoint is expected to land here, and it means the +/// broadcast succeeded, so it must not be mistaken for a reason to send another. +fn is_already_broadcast(error: &TransportError) -> bool { + let lower = error.to_string().to_lowercase(); + ALREADY_BROADCAST_PATTERNS.iter().any(|p| lower.contains(p)) +} + /// Classify the outcome of a nonce-gap fill submission. Pure so the /// swallow rule (`is_nonce_error` → `Ok(())`) is unit-testable without /// an RPC mock. @@ -454,32 +466,30 @@ impl AlloyChainClient { classify_fill_nonce_gap_outcome(nonce, self.send_transaction(&tx).await) } - /// Broadcast a transaction, retrying and rotating providers on transport faults the - /// way every read call already does. Rebroadcasting is safe because the caller fixed - /// the nonce, gas limit and both fees, so every endpoint sees one identical hash. + /// Broadcast a transaction, retrying and rotating endpoints the way every read call + /// already does. Signing happens once, up front, so every endpoint is offered the same + /// bytes under one hash and the hash is known before anyone is asked to accept them. async fn send_transaction(&self, tx: &TransactionRequest) -> Result { - let request_timeout = self.inner.rpc_pool.request_timeout(); + let wallet = EthereumWallet::from(self.inner.signer.clone()); + let signed = tx.clone().build(&wallet).await.map_err(|e| { + ChainClientError::SubmitFailed(anyhow::anyhow!("Failed to sign transaction: {e}")) + })?; + let signed_hash = *signed.tx_hash(); + let raw = signed.encoded_2718(); self.inner .rpc_pool - .execute_on_url("send_transaction", |url| { - let wallet = EthereumWallet::from(self.inner.signer.clone()); - let tx = tx.clone(); + .execute("send_transaction", |provider| { + let raw = raw.clone(); async move { - let client = reqwest::Client::builder() - .timeout(request_timeout) - .build() - .map_err(|e| { - TransportErrorKind::custom(ChainClientError::ConfigError(format!( - "Failed to build HTTP client: {e}" - ))) - })?; - - let provider = ProviderBuilder::new() - .wallet(wallet) - .connect_reqwest(client, url); - - Ok(*provider.send_transaction(tx).await?.tx_hash()) + match provider.send_raw_transaction(&raw).await { + Ok(pending) => Ok(*pending.tx_hash()), + // This endpoint already holds these exact bytes, which is the + // outcome we were after. Answer with the hash we signed rather + // than reporting a failure that would send a second transaction. + Err(e) if is_already_broadcast(&e) => Ok(signed_hash), + Err(e) => Err(e), + } } }) .await @@ -897,6 +907,99 @@ mod tests { ); } + /// Answers the nonce lookup that precedes a send, then reports every send as already + /// held. Counting the sends is how a duplicate submission shows up. + struct AlreadyHeldResponder { + pending_nonce: u64, + } + + impl Respond for AlreadyHeldResponder { + fn respond(&self, request: &Request) -> ResponseTemplate { + let body: serde_json::Value = + serde_json::from_slice(&request.body).expect("JSON-RPC request body"); + match body["method"].as_str().unwrap_or_default() { + "eth_getTransactionCount" => { + ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "jsonrpc": "2.0", + "id": body["id"], + "result": format!("{:#x}", self.pending_nonce), + })) + } + "eth_sendRawTransaction" => { + ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "jsonrpc": "2.0", + "id": body["id"], + "error": { "code": -32000, "message": "already known" }, + })) + } + other => panic!("unexpected method {other}"), + } + } + } + + fn sends_among(requests: &[wiremock::Request]) -> usize { + requests + .iter() + .filter(|r| { + let body: serde_json::Value = + serde_json::from_slice(&r.body).expect("JSON-RPC request body"); + body["method"] == "eth_sendRawTransaction" + }) + .count() + } + + /// The first endpoint takes the bytes but its reply is lost, so the same bytes go to the + /// second, which already holds them. That is the outcome we wanted, so it has to be + /// reported with the hash rather than as a failure. + #[tokio::test] + async fn send_transaction_accepts_a_transaction_already_in_a_mempool() { + let sick = server_answering_500().await; + let holding = server_answering_rpc_error(-32000, "already known").await; + let client = client_over(vec![ + sick.uri().parse().expect("sick provider URL"), + holding.uri().parse().expect("holding provider URL"), + ]); + let tx = ready_to_send_tx(client.inner.signer.address()); + + let tx_hash = client + .send_transaction(&tx) + .await + .expect("a transaction already in a mempool is a successful broadcast"); + + let wallet = EthereumWallet::from(client.inner.signer.clone()); + let signed = tx.clone().build(&wallet).await.expect("sign the same tx"); + assert_eq!( + tx_hash, + *signed.tx_hash(), + "the hash reported must be the one we signed" + ); + } + + /// Treating "we already have it" as a stale nonce made the caller resync and send a + /// second transaction at a different nonce, which for an agreement offer means paying + /// twice for one offer. One broadcast has to stay one broadcast. + #[tokio::test] + async fn sign_and_send_does_not_resend_a_transaction_already_in_a_mempool() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(AlreadyHeldResponder { pending_nonce: 6 }) + .mount(&server) + .await; + let client = client_over(vec![server.uri().parse().expect("provider URL")]); + + client + .sign_and_send(ready_to_send_tx(client.inner.signer.address()), &[0u8; 16]) + .await + .expect("a transaction already in a mempool is a successful broadcast"); + + let requests = server.received_requests().await.unwrap_or_default(); + assert_eq!( + sends_among(&requests), + 1, + "the transaction must be broadcast once, not resent at a new nonce" + ); + } + /// The nonce reason has to survive a second provider failing a different way. Reporting /// only the last provider's reason hid the rejection behind a transport fault, and /// `sign_and_send` then skipped the resync that would have unstuck the wallet. diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index ee4942fb..8c13df35 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -105,11 +105,6 @@ impl RpcProviderPool { }) } - /// Get the configured request timeout. - pub fn request_timeout(&self) -> Duration { - self.request_timeout - } - /// Rotate to the next provider. /// /// Returns the new provider URL after rotation. @@ -129,31 +124,6 @@ impl RpcProviderPool { where F: Fn(HttpProvider) -> Fut, Fut: Future>, - { - let f = &f; - self.execute_on_url(operation, move |url| { - let provider = build_provider(url, self.request_timeout); - async move { - match provider { - Ok(provider) => f(provider).await, - Err(e) => Err(TransportErrorKind::custom(e)), - } - } - }) - .await - } - - /// Same retry and rotation as [`Self::execute`], but the closure receives the URL - /// rather than a provider, so a caller needing a wallet attached can build its own - /// without reimplementing the retry, backoff and rotation policy. - pub async fn execute_on_url( - &self, - operation: &str, - f: F, - ) -> Result - where - F: Fn(Url) -> Fut, - Fut: Future>, { // What each endpoint said, in the order they were tried. `sign_and_send` matches // this text to tell a stale nonce from a transport fault, so a rejection from the @@ -172,7 +142,11 @@ impl RpcProviderPool { // Retry loop for current provider let mut endpoint_error: Option = None; for attempt in 0..=self.max_retries { - match f(current_url.clone()).await { + let outcome = match build_provider(current_url.clone(), self.request_timeout) { + Ok(provider) => f(provider).await, + Err(e) => Err(TransportErrorKind::custom(e)), + }; + match outcome { Ok(result) => return Ok(result), Err(e) if Self::is_retryable(&e) && attempt < self.max_retries => { let delay = Self::backoff_delay(attempt); From 8c66b1a1a7d1b4522fe73807dd4901c81706d97d Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:38:37 +1000 Subject: [PATCH 17/49] test(chain): check both endpoints are offered the same transaction Rotating between endpoints is only safe while each is offered identical bytes, otherwise two transactions could both be mined and both be paid for. That property was argued in a comment and never checked, so this compares what each endpoint actually received. --- bin/dipper-service/src/chain_client/client.rs | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 5e8eb279..8931258e 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -948,6 +948,46 @@ mod tests { .count() } + /// The raw transaction each endpoint was asked to accept. + fn broadcast_bytes(requests: &[wiremock::Request]) -> Vec { + requests + .iter() + .filter_map(|r| { + let body: serde_json::Value = + serde_json::from_slice(&r.body).expect("JSON-RPC request body"); + (body["method"] == "eth_sendRawTransaction") + .then(|| body["params"][0].as_str().expect("raw tx").to_string()) + }) + .collect() + } + + /// Rotating between endpoints is only safe while they are all offered the same bytes: two + /// different transactions would mean two chances of both being mined and paid for. This + /// is what makes a rebroadcast a retry of one transaction rather than a second one. + #[tokio::test] + async fn every_endpoint_is_offered_the_same_bytes() { + let sick = server_answering_500().await; + let healthy = server_answering_with(B256::repeat_byte(0xab)).await; + let client = client_over(vec![ + sick.uri().parse().expect("sick provider URL"), + healthy.uri().parse().expect("healthy provider URL"), + ]); + let tx = ready_to_send_tx(client.inner.signer.address()); + + client.send_transaction(&tx).await.expect("send"); + + let offered = [ + broadcast_bytes(&sick.received_requests().await.unwrap_or_default()), + broadcast_bytes(&healthy.received_requests().await.unwrap_or_default()), + ] + .concat(); + assert_eq!(offered.len(), 2, "both endpoints should have been asked"); + assert_eq!( + offered[0], offered[1], + "the two endpoints were offered different transactions" + ); + } + /// The first endpoint takes the bytes but its reply is lost, so the same bytes go to the /// second, which already holds them. That is the outcome we wanted, so it has to be /// reported with the hash rather than as a failure. From 5431e2a8192eef61e0b51a347ca6321030134691 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:40:56 +1000 Subject: [PATCH 18/49] fix(chain): move a submission on rather than pressing a sick endpoint A submission holds a lock that every other submission queues behind, and with the shipped settings retrying each of 2 endpoints 4 times could hold it for roughly 500 seconds against a 600 second acceptance window. Each endpoint now gets one attempt, bounding this near 120. --- bin/dipper-service/src/chain_client/client.rs | 19 +++++----- .../src/chain_client/rpc_provider.rs | 35 +++++++++++++++++-- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 8931258e..b4d56577 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -479,7 +479,7 @@ impl AlloyChainClient { self.inner .rpc_pool - .execute("send_transaction", |provider| { + .execute_trying_each_once("send_transaction", |provider| { let raw = raw.clone(); async move { match provider.send_raw_transaction(&raw).await { @@ -1065,10 +1065,11 @@ mod tests { ); } - /// A provider reporting overload in the JSON-RPC error rather than the HTTP status earns a - /// retry on that provider, while a chain rejection does not, since it will answer the same. + /// A submission moves on to the next endpoint rather than pressing a struggling one, even + /// when the complaint is the kind a retry would normally clear. Retries here are expensive: + /// they hold up every other submission and eat into the window an offer has to be accepted. #[tokio::test] - async fn json_rpc_errors_decide_whether_the_provider_is_retried() { + async fn send_transaction_tries_each_endpoint_once() { let overloaded = server_answering_rpc_error(-32005, "project ID request rate exceeded").await; let healthy = server_answering_with(B256::repeat_byte(0xcd)).await; @@ -1077,7 +1078,7 @@ mod tests { overloaded.uri().parse().expect("overloaded provider URL"), healthy.uri().parse().expect("healthy provider URL"), ], - 1, + 3, ); let tx = ready_to_send_tx(client.inner.signer.address()); @@ -1092,8 +1093,8 @@ mod tests { .await .unwrap_or_default() .len(), - 2, - "a rate-limited provider should be retried once before rotating" + 1, + "a struggling endpoint should be left alone once it has refused" ); let rejecting = server_answering_rpc_error(-32000, "nonce too low: next nonce 12").await; @@ -1103,7 +1104,7 @@ mod tests { rejecting.uri().parse().expect("rejecting provider URL"), spare.uri().parse().expect("spare provider URL"), ], - 1, + 3, ); let tx = ready_to_send_tx(client.inner.signer.address()); @@ -1116,7 +1117,7 @@ mod tests { .unwrap_or_default() .len(), 1, - "a chain rejection should not be retried against the same provider" + "a chain rejection should not be retried against the same endpoint" ); } diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 8c13df35..8603aa6d 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -121,6 +121,35 @@ impl RpcProviderPool { /// Run an RPC call, retrying the current endpoint with backoff and then rotating on to /// the next. `operation` names the call for logging. pub async fn execute(&self, operation: &str, f: F) -> Result + where + F: Fn(HttpProvider) -> Fut, + Fut: Future>, + { + self.execute_with_retries(operation, self.max_retries, f) + .await + } + + /// Run an RPC call, giving each endpoint a single attempt instead of several. Submitting + /// a transaction uses this: it holds a lock that every other submission queues behind, + /// and a different endpoint is likelier to help than asking a sick one four times. + pub async fn execute_trying_each_once( + &self, + operation: &str, + f: F, + ) -> Result + where + F: Fn(HttpProvider) -> Fut, + Fut: Future>, + { + self.execute_with_retries(operation, 0, f).await + } + + async fn execute_with_retries( + &self, + operation: &str, + max_retries: u32, + f: F, + ) -> Result where F: Fn(HttpProvider) -> Fut, Fut: Future>, @@ -141,20 +170,20 @@ impl RpcProviderPool { // Retry loop for current provider let mut endpoint_error: Option = None; - for attempt in 0..=self.max_retries { + for attempt in 0..=max_retries { let outcome = match build_provider(current_url.clone(), self.request_timeout) { Ok(provider) => f(provider).await, Err(e) => Err(TransportErrorKind::custom(e)), }; match outcome { Ok(result) => return Ok(result), - Err(e) if Self::is_retryable(&e) && attempt < self.max_retries => { + Err(e) if Self::is_retryable(&e) && attempt < max_retries => { let delay = Self::backoff_delay(attempt); tracing::warn!( operation, provider = %current_url, attempt = attempt + 1, - max_retries = self.max_retries, + max_retries, delay_ms = delay.as_millis(), error = %e, "Retryable RPC error, backing off" From 3834ae18ff04daa7d9575c5e343da29145d989fd Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:42:01 +1000 Subject: [PATCH 19/49] test(chain): split the two submission behaviours into separate tests One test checked both how a struggling endpoint is treated and how a chain rejection is treated, so a failure did not say which of the two had broken and neither could be run on its own. --- bin/dipper-service/src/chain_client/client.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index b4d56577..b44c59b3 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -1069,7 +1069,7 @@ mod tests { /// when the complaint is the kind a retry would normally clear. Retries here are expensive: /// they hold up every other submission and eat into the window an offer has to be accepted. #[tokio::test] - async fn send_transaction_tries_each_endpoint_once() { + async fn send_transaction_leaves_a_struggling_endpoint_alone() { let overloaded = server_answering_rpc_error(-32005, "project ID request rate exceeded").await; let healthy = server_answering_with(B256::repeat_byte(0xcd)).await; @@ -1096,7 +1096,12 @@ mod tests { 1, "a struggling endpoint should be left alone once it has refused" ); + } + /// A chain rejection is the chain's answer, not one endpoint's, so asking the same + /// endpoint again would only collect the same refusal at the cost of the delay. + #[tokio::test] + async fn send_transaction_does_not_repeat_a_chain_rejection() { let rejecting = server_answering_rpc_error(-32000, "nonce too low: next nonce 12").await; let spare = server_answering_with(B256::repeat_byte(0xef)).await; let client = client_over_retrying( From 768f02dd533948ce303be878a14e75451d759c87 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:42:34 +1000 Subject: [PATCH 20/49] test(chain): check the submission succeeded rather than ignoring it The test threw away what the submission returned and only counted requests, so it would have passed just as happily if the submission had failed outright rather than moving on to the spare endpoint as intended. --- bin/dipper-service/src/chain_client/client.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index b44c59b3..6aa84562 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -1113,7 +1113,10 @@ mod tests { ); let tx = ready_to_send_tx(client.inner.signer.address()); - let _ = client.send_transaction(&tx).await; + client + .send_transaction(&tx) + .await + .expect("the spare endpoint accepts, so the send succeeds"); assert_eq!( rejecting From edc97d7d6a07361961028a88501ed840ca3c128a Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:43:03 +1000 Subject: [PATCH 21/49] perf(chain): reuse one connection across an endpoint's retries Each attempt built its own HTTP client, so every retry opened a new connection and repeated the TLS handshake, on exactly the path that is already short of time. The connection is now built once per endpoint and shared by that endpoint's attempts. --- bin/dipper-service/src/chain_client/rpc_provider.rs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 8603aa6d..11b9d59b 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -168,12 +168,18 @@ impl RpcProviderPool { loop { let current_url = self.url_at(start + providers_tried).clone(); + // One connection per endpoint, reused across its attempts, so a retry does not + // pay for a fresh TLS handshake on the path that is already running out of time. + let provider = build_provider(current_url.clone(), self.request_timeout); + // Retry loop for current provider let mut endpoint_error: Option = None; for attempt in 0..=max_retries { - let outcome = match build_provider(current_url.clone(), self.request_timeout) { - Ok(provider) => f(provider).await, - Err(e) => Err(TransportErrorKind::custom(e)), + let outcome = match &provider { + Ok(provider) => f(provider.clone()).await, + Err(e) => Err(TransportErrorKind::custom(ChainClientError::ConfigError( + e.to_string(), + ))), }; match outcome { Ok(result) => return Ok(result), From 1809abd24ae031883569e694b43ec00c94e6f54c Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:43:52 +1000 Subject: [PATCH 22/49] refactor(chain): build the HTTP client once when the pool is created Building it per call meant every call carried an error path for "could not make an HTTP client", which has nothing to do with any endpoint being reachable, and gave each call its own connection pool so nothing was ever reused between them. --- .../src/chain_client/rpc_provider.rs | 39 ++++++++----------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 11b9d59b..cdf734b2 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -45,16 +45,6 @@ const RETRYABLE_ERROR_PATTERNS: &[&str] = &[ "bad gateway", ]; -/// Build a read-only provider for one URL, with the pool's request timeout applied. -fn build_provider(url: Url, request_timeout: Duration) -> Result { - let client = reqwest::Client::builder() - .timeout(request_timeout) - .build() - .map_err(|e| ChainClientError::ConfigError(format!("Failed to build HTTP client: {e}")))?; - - Ok(ProviderBuilder::new().connect_reqwest(client, url)) -} - /// Type alias for the provider with default fillers. pub type HttpProvider = FillProvider< JoinFill< @@ -72,8 +62,9 @@ pub struct RpcProviderPool { providers: Vec, /// Current provider index (atomic for thread-safety) current_index: AtomicUsize, - /// Request timeout per RPC call - request_timeout: Duration, + /// Shared across every endpoint, which is what lets connections be pooled and reused + /// rather than reopened per call. Built once, so a call can never fail for lack of one. + http: reqwest::Client, /// Maximum retries per provider before rotating max_retries: u32, } @@ -91,6 +82,13 @@ impl RpcProviderPool { )); } + let http = reqwest::Client::builder() + .timeout(request_timeout) + .build() + .map_err(|e| { + ChainClientError::ConfigError(format!("Failed to build HTTP client: {e}")) + })?; + tracing::info!( provider_count = providers.len(), primary = %providers[0], @@ -100,7 +98,7 @@ impl RpcProviderPool { Ok(Self { providers, current_index: AtomicUsize::new(0), - request_timeout, + http, max_retries, }) } @@ -168,20 +166,15 @@ impl RpcProviderPool { loop { let current_url = self.url_at(start + providers_tried).clone(); - // One connection per endpoint, reused across its attempts, so a retry does not - // pay for a fresh TLS handshake on the path that is already running out of time. - let provider = build_provider(current_url.clone(), self.request_timeout); + // Reused across this endpoint's attempts, so a retry does not pay for a fresh + // TLS handshake on the path that is already running out of time. + let provider = + ProviderBuilder::new().connect_reqwest(self.http.clone(), current_url.clone()); // Retry loop for current provider let mut endpoint_error: Option = None; for attempt in 0..=max_retries { - let outcome = match &provider { - Ok(provider) => f(provider.clone()).await, - Err(e) => Err(TransportErrorKind::custom(ChainClientError::ConfigError( - e.to_string(), - ))), - }; - match outcome { + match f(provider.clone()).await { Ok(result) => return Ok(result), Err(e) if Self::is_retryable(&e) && attempt < max_retries => { let delay = Self::backoff_delay(attempt); From d371332e844347397aaf1ad5b7cb41328a47eedf Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:44:33 +1000 Subject: [PATCH 23/49] fix(chain): report a failed submission as a submission failure Routing submissions through the shared endpoint pool made them come back labelled as a generic RPC problem, which reads the same as a failed read and loses the fact that money was about to move. Anything the pool can name precisely keeps its own label. --- bin/dipper-service/src/chain_client/client.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 6aa84562..07fc2252 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -493,6 +493,13 @@ impl AlloyChainClient { } }) .await + .map_err(|e| match e { + // A submission that got nowhere is a failed submission, whatever the + // endpoints happened to say. Anything the pool could name precisely, such + // as a rejection from the contract, keeps the name it already has. + ChainClientError::RpcError(cause) => ChainClientError::SubmitFailed(cause), + other => other, + }) } /// Poll `eth_getTransactionReceipt` until the tx has mined or the timeout From 52c67229b9eb9ceae6b3a08044e077e0486fe68f Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:46:49 +1000 Subject: [PATCH 24/49] test(chain): cover one call reaching every endpoint while others rotate Each call keeps its own place in the rotation rather than reading the shared one, so that concurrent calls cannot send it back to an endpoint it has already tried and leave it giving up before it reaches a working one. That reasoning had nothing checking it. --- .../src/chain_client/rpc_provider.rs | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index cdf734b2..62197df7 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -279,8 +279,67 @@ impl RpcProviderPool { #[cfg(test)] mod tests { + use thegraph_core::alloy::providers::Provider; + use wiremock::{Mock, MockServer, ResponseTemplate, matchers::method}; + use super::*; + /// Refuses every call for a reason no retry would clear, so the pool moves straight on. + async fn server_refusing() -> MockServer { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "jsonrpc": "2.0", + "id": 0, + "error": { "code": -32000, "message": "refused" }, + }))) + .mount(&server) + .await; + server + } + + /// A call walks its own way round the ring, so it reaches every endpoint even while other + /// calls move the shared starting point underneath it. Reading that shared point afresh + /// each time let a call revisit one endpoint and give up without trying another. + #[tokio::test] + async fn a_call_reaches_every_endpoint_even_while_others_rotate() { + let servers = [ + server_refusing().await, + server_refusing().await, + server_refusing().await, + ]; + let pool = RpcProviderPool::new( + servers + .iter() + .map(|s| s.uri().parse().expect("server URL")) + .collect(), + Duration::from_secs(5), + 0, + ) + .expect("pool"); + + let call = pool.execute("probe", |provider| async move { + tokio::task::yield_now().await; + provider.get_block_number().await + }); + let others_rotating = async { + for _ in 0..64 { + pool.rotate(); + tokio::task::yield_now().await; + } + }; + let (result, ()) = tokio::join!(call, others_rotating); + + assert!(result.is_err(), "every endpoint refused, so the call fails"); + for (i, server) in servers.iter().enumerate() { + assert_eq!( + server.received_requests().await.unwrap_or_default().len(), + 1, + "endpoint {i} should have been tried exactly once" + ); + } + } + /// 500 is the status a provider answered on 2026-07-29 while an accepted agreement went /// unfunded. Reading it here is what earns a retry on that provider before rotating; the /// rotation itself is unconditional, so this decides attempts rather than failover. From dfac280d6bfceef76239b4eaf17744699ec8e0de Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:48:09 +1000 Subject: [PATCH 25/49] test(chain): cover a read failing over to the next endpoint Every read the service makes shares the retry and failover machinery that submissions use, and it was reworked here without anything checking a read still behaves. This asks for a block number across one sick endpoint and one working one. --- .../src/chain_client/rpc_provider.rs | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 62197df7..7e779c2b 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -298,6 +298,62 @@ mod tests { server } + /// Answers a block-number lookup, which is the shape of the read calls the service makes. + async fn server_answering_block(number: u64) -> MockServer { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "jsonrpc": "2.0", + "id": 0, + "result": format!("{number:#x}"), + }))) + .mount(&server) + .await; + server + } + + /// Every read the service makes goes through this path, so a read has to fail over the same + /// way a submission does, and a fault worth another go has to earn one before it rotates. + /// One retry rather than the usual several, to keep the backoff this waits out short. + #[tokio::test] + async fn a_read_retries_a_sick_endpoint_then_rotates() { + let sick = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(503).set_body_string("service unavailable")) + .mount(&sick) + .await; + let healthy = server_answering_block(0x2a).await; + + let pool = RpcProviderPool::new( + vec![ + sick.uri().parse().expect("sick URL"), + healthy.uri().parse().expect("healthy URL"), + ], + Duration::from_secs(5), + 1, + ) + .expect("pool"); + + let block = pool + .execute("get_block_number", |provider| async move { + provider.get_block_number().await + }) + .await + .expect("the read should succeed on the second endpoint"); + + assert_eq!(block, 0x2a); + assert_eq!( + sick.received_requests().await.unwrap_or_default().len(), + 2, + "a read should use its retry budget before rotating" + ); + assert_eq!( + healthy.received_requests().await.unwrap_or_default().len(), + 1, + "the healthy endpoint should answer on the first ask" + ); + } + /// A call walks its own way round the ring, so it reaches every endpoint even while other /// calls move the shared starting point underneath it. Reading that shared point afresh /// each time let a call revisit one endpoint and give up without trying another. From d6a6f1e3521aa00dc111497d32d7ec2e03695bca Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 18:49:48 +1000 Subject: [PATCH 26/49] fix(chain): recover onto the next free transaction slot Transactions from one wallet are numbered and must be mined in order. Recovery from a rejected number aimed one past the next free slot, leaving a hole, and a transaction behind a hole waits indefinitely, so the wallet could sit stuck until something else filled it. --- bin/dipper-service/src/chain_client/client.rs | 65 ++++++++++++++++--- 1 file changed, 57 insertions(+), 8 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 07fc2252..1761eef1 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -342,16 +342,12 @@ impl AlloyChainClient { Ok(self.inner.nonce.fetch_add(1, Ordering::SeqCst)) } - /// Ratchet the in-memory nonce counter up to `chain_pending + 1`. - /// Never decreases the counter, so it is safe to call from any - /// context without `submit_lock` held: an in-flight reservation - /// from another caller cannot be invalidated. Callers needing a - /// reservation call `next_nonce()` after. + /// Ratchet the in-memory counter up to the next slot the chain will accept. Never + /// decreases it, so a reservation another caller is still submitting cannot be handed out + /// again: that reservation has already pushed the counter past what the chain reports. async fn resync_nonce(&self) -> Result<(), ChainClientError> { let chain_nonce = self.fetch_chain_nonce().await?; - self.inner - .nonce - .fetch_max(chain_nonce + 1, Ordering::SeqCst); + self.inner.nonce.fetch_max(chain_nonce, Ordering::SeqCst); Ok(()) } @@ -995,6 +991,59 @@ mod tests { ); } + /// Recovering from a stale nonce has to land on the slot the chain will actually take. + /// Aiming one past it leaves that slot empty, and a transaction behind an empty slot never + /// gets mined, so a wallet could stay stuck until something else happened to fill it. + #[tokio::test] + async fn resync_lands_on_the_next_slot_the_chain_will_accept() { + let pending = Arc::new(AtomicU64::new(3)); + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(PendingNonceResponder { + pending: pending.clone(), + }) + .mount(&server) + .await; + let client = client_over(vec![server.uri().parse().expect("provider URL")]); + + assert_eq!( + client.next_nonce().await.expect("first reservation"), + 3, + "the first reservation takes the slot the chain reports" + ); + + // Something else spent slots 3 to 9, so 10 is now the next one free. + pending.store(10, Ordering::SeqCst); + client.resync_nonce().await.expect("resync"); + + assert_eq!( + client.next_nonce().await.expect("reservation after resync"), + 10, + "the reservation after a resync must not skip the free slot" + ); + } + + /// Reports whatever the pending count currently is, so a test can move the chain on. + struct PendingNonceResponder { + pending: Arc, + } + + impl Respond for PendingNonceResponder { + fn respond(&self, request: &Request) -> ResponseTemplate { + let body: serde_json::Value = + serde_json::from_slice(&request.body).expect("JSON-RPC request body"); + assert_eq!( + body["method"], "eth_getTransactionCount", + "this mock only answers nonce lookups" + ); + ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "jsonrpc": "2.0", + "id": body["id"], + "result": format!("{:#x}", self.pending.load(Ordering::SeqCst)), + })) + } + } + /// The first endpoint takes the bytes but its reply is lost, so the same bytes go to the /// second, which already holds them. That is the outcome we wanted, so it has to be /// reported with the hash rather than as a failure. From 94424f875864f80584da6746aadc1dbc4082a472 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 19:28:06 +1000 Subject: [PATCH 27/49] fix(chain): report the transaction hash we signed A broadcast reported whatever hash the endpoint echoed back, though the hash follows from the bytes we signed. A wrong hash sends the receipt poll after a transaction that never mines, so the offer is reported dropped and resubmitted while the first one is still live. --- bin/dipper-service/src/chain_client/client.rs | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 1761eef1..4ce17142 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -479,7 +479,10 @@ impl AlloyChainClient { let raw = raw.clone(); async move { match provider.send_raw_transaction(&raw).await { - Ok(pending) => Ok(*pending.tx_hash()), + // The hash follows from the bytes, so report the one we signed + // instead of what this endpoint echoed back. A wrong hash sends the + // receipt poll after a transaction that never mines. + Ok(_) => Ok(signed_hash), // This endpoint already holds these exact bytes, which is the // outcome we were after. Answer with the hash we signed rather // than reporting a failure that would send a second transaction. @@ -837,14 +840,23 @@ mod tests { .with_nonce(7) } + /// The hash the signed bytes carry, which is what a send reports. + async fn signed_hash_of(client: &AlloyChainClient, tx: &TransactionRequest) -> B256 { + let wallet = EthereumWallet::from(client.inner.signer.clone()); + *tx.clone() + .build(&wallet) + .await + .expect("sign the transaction") + .tx_hash() + } + /// Submitting a transaction must fail over to the next provider, exactly as every read /// call does. On 2026-07-29 the send bypassed the pool, so one endpoint answering 500 /// stranded an accepted agreement while a healthy second endpoint was never tried. #[tokio::test] async fn send_transaction_rotates_to_the_next_provider_on_server_fault() { let sick = server_answering_500().await; - let expected_hash = B256::repeat_byte(0xab); - let healthy = server_answering_with(expected_hash).await; + let healthy = server_answering_with(B256::repeat_byte(0xab)).await; let client = client_over(vec![ sick.uri().parse().expect("sick provider URL"), @@ -857,7 +869,11 @@ mod tests { .await .expect("send should succeed on the second provider"); - assert_eq!(tx_hash, expected_hash); + assert_eq!( + tx_hash, + signed_hash_of(&client, &tx).await, + "the hash reported must be the one we signed, not the one the endpoint made up" + ); assert!( !sick .received_requests() @@ -1062,11 +1078,9 @@ mod tests { .await .expect("a transaction already in a mempool is a successful broadcast"); - let wallet = EthereumWallet::from(client.inner.signer.clone()); - let signed = tx.clone().build(&wallet).await.expect("sign the same tx"); assert_eq!( tx_hash, - *signed.tx_hash(), + signed_hash_of(&client, &tx).await, "the hash reported must be the one we signed" ); } From ded24324d67a4364e44cc9debe99231423c1d204 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 19:31:27 +1000 Subject: [PATCH 28/49] fix(chain): name RPC endpoints by host so keys stay out of logs Hosted RPC endpoints carry their API key in the URL path or query, and the whole URL was written into the failure text and the log fields. Endpoints are now named by host and port, which still says which one refused without carrying the key along with it. --- .../src/chain_client/rpc_provider.rs | 50 +++++++++++++++++-- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 7e779c2b..a066b882 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -31,6 +31,16 @@ fn extract_chain_client_error(err: TransportError) -> Option { } } +/// How an endpoint is named in logs and errors. Hosted RPC endpoints carry their API key in +/// the path or the query, so naming one by host says which endpoint it was without the key. +fn endpoint_name(url: &Url) -> String { + match (url.host_str(), url.port()) { + (Some(host), Some(port)) => format!("{host}:{port}"), + (Some(host), None) => host.to_string(), + (None, _) => "unnamed endpoint".to_string(), + } +} + /// Error text that indicates a transient failure worth retrying, used only for faults /// that arrive as prose rather than as a status code or JSON-RPC error object. const RETRYABLE_ERROR_PATTERNS: &[&str] = &[ @@ -91,7 +101,7 @@ impl RpcProviderPool { tracing::info!( provider_count = providers.len(), - primary = %providers[0], + primary = %endpoint_name(&providers[0]), "RPC provider pool initialized" ); @@ -165,6 +175,7 @@ impl RpcProviderPool { loop { let current_url = self.url_at(start + providers_tried).clone(); + let endpoint = endpoint_name(¤t_url); // Reused across this endpoint's attempts, so a retry does not pay for a fresh // TLS handshake on the path that is already running out of time. @@ -180,7 +191,7 @@ impl RpcProviderPool { let delay = Self::backoff_delay(attempt); tracing::warn!( operation, - provider = %current_url, + provider = %endpoint, attempt = attempt + 1, max_retries, delay_ms = delay.as_millis(), @@ -201,7 +212,7 @@ impl RpcProviderPool { let endpoint_error = endpoint_error .unwrap_or_else(|| TransportErrorKind::custom_str("no attempt was made")); - reasons.push(format!("{current_url}: {endpoint_error}")); + reasons.push(format!("{endpoint}: {endpoint_error}")); providers_tried += 1; // Check if we've tried all providers @@ -227,8 +238,8 @@ impl RpcProviderPool { let next_url = self.url_at(start + providers_tried); tracing::warn!( operation, - old_provider = %current_url, - new_provider = %next_url, + old_provider = %endpoint, + new_provider = %endpoint_name(next_url), providers_tried, total_providers = self.providers.len(), error = %endpoint_error, @@ -396,6 +407,35 @@ mod tests { } } + /// Hosted RPC endpoints carry their API key in the path, and this failure text reaches the + /// logs and every error built from it, so it has to say which endpoint refused without + /// repeating the key that gets it in. + #[tokio::test] + async fn a_failure_names_the_endpoint_without_its_api_key() { + let refusing = server_refusing().await; + let keyed: Url = format!("{}/v2/super-secret-key", refusing.uri()) + .parse() + .expect("keyed endpoint URL"); + + let pool = RpcProviderPool::new(vec![keyed], Duration::from_secs(5), 0).expect("pool"); + let err = pool + .execute("probe", |provider| async move { + provider.get_block_number().await + }) + .await + .expect_err("the endpoint refused, so the call fails"); + + let text = err.to_string(); + assert!( + !text.contains("super-secret-key"), + "the API key must not appear in the failure: {text}" + ); + assert!( + text.contains("127.0.0.1"), + "the failure should still say which endpoint refused: {text}" + ); + } + /// 500 is the status a provider answered on 2026-07-29 while an accepted agreement went /// unfunded. Reading it here is what earns a retry on that provider before rotating; the /// rotation itself is unconditional, so this decides attempts rather than failover. From bd3063d5657c590a35ae7f7a652f4f9c50cf3b25 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 19:32:39 +1000 Subject: [PATCH 29/49] fix(chain): refuse to sign for a chain the client is not configured for Signing a transaction no longer fills in missing fields, and a request that names no chain is signed for Ethereum mainnet rather than refused. Every submission passes through one place, so that place now checks the chain matches before a signature exists. --- bin/dipper-service/src/chain_client/client.rs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 4ce17142..8fc5a085 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -466,6 +466,16 @@ impl AlloyChainClient { /// already does. Signing happens once, up front, so every endpoint is offered the same /// bytes under one hash and the hash is known before anyone is asked to accept them. async fn send_transaction(&self, tx: &TransactionRequest) -> Result { + // Nothing fills a field in on this path any more, and a request that names no chain is + // signed for chain 1 rather than refused, so check before the signature exists. + if tx.chain_id() != Some(self.inner.chain_id) { + return Err(ChainClientError::ConfigError(format!( + "refusing to sign for chain {:?} while configured for chain {}", + tx.chain_id(), + self.inner.chain_id + ))); + } + let wallet = EthereumWallet::from(self.inner.signer.clone()); let signed = tx.clone().build(&wallet).await.map_err(|e| { ChainClientError::SubmitFailed(anyhow::anyhow!("Failed to sign transaction: {e}")) @@ -1110,6 +1120,35 @@ mod tests { ); } + /// Nothing fills a field in on the send path, and a request naming no chain is signed for + /// chain 1 rather than refused, so a transaction has to say which chain it is for. + #[tokio::test] + async fn send_transaction_refuses_a_transaction_that_names_another_chain() { + let server = server_answering_with(B256::repeat_byte(0xab)).await; + let client = client_over(vec![server.uri().parse().expect("provider URL")]); + let from = client.inner.signer.address(); + + let mut names_no_chain = ready_to_send_tx(from); + names_no_chain.chain_id = None; + + for tx in [ready_to_send_tx(from).with_chain_id(1), names_no_chain] { + let err = client + .send_transaction(&tx) + .await + .expect_err("a transaction for another chain must not be signed"); + assert!(matches!(err, ChainClientError::ConfigError(_)), "got {err}"); + } + + assert!( + server + .received_requests() + .await + .unwrap_or_default() + .is_empty(), + "no endpoint should have been asked to accept either transaction" + ); + } + /// The nonce reason has to survive a second provider failing a different way. Reporting /// only the last provider's reason hid the rejection behind a transport fault, and /// `sign_and_send` then skipped the resync that would have unstuck the wallet. From 814083dd11406cd681a5e0723c3a7140486ebd28 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 19:33:29 +1000 Subject: [PATCH 30/49] test(chain): pin the error a failed submission reports A send that no endpoint accepts is reported as a failed submission rather than as the pool's generic RPC fault, since that is what tells a caller the transaction went nowhere. The test only checked that something went wrong, so the distinction could have been lost unnoticed. --- bin/dipper-service/src/chain_client/client.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 8fc5a085..ce20b157 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -902,17 +902,24 @@ mod tests { ); } - /// With a single provider there is nowhere to rotate to, so the fault must surface - /// rather than be retried forever or silently swallowed. + /// With a single provider there is nowhere to rotate to, so the fault must surface rather + /// than be retried forever or silently swallowed. It surfaces as a failed submission rather + /// than the pool's generic RPC fault, which is what says the transaction went nowhere. #[tokio::test] async fn send_transaction_reports_failure_when_every_provider_is_sick() { let sick = server_answering_500().await; let client = client_over(vec![sick.uri().parse().expect("sick provider URL")]); let tx = ready_to_send_tx(client.inner.signer.address()); - let result = client.send_transaction(&tx).await; + let err = client + .send_transaction(&tx) + .await + .expect_err("a 500 from the only provider must error"); - assert!(result.is_err(), "a 500 from the only provider must error"); + assert!( + matches!(err, ChainClientError::SubmitFailed(_)), + "got {err}" + ); } /// What the chain said has to survive the pool, because `sign_and_send` reads this text to From 518e5b5ace98a37b64daae7cabbed3f88f9cc681 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 19:34:49 +1000 Subject: [PATCH 31/49] test(chain): check every endpoint's reason reaches the failure When no endpoint accepts a call, the failure gathers what each one said in the order they were tried, because a caller reads that text to tell a rejection from the chain apart from a broken connection. Nothing checked that reasons from earlier endpoints survived the later ones. --- .../src/chain_client/rpc_provider.rs | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index a066b882..1edae0b5 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -436,6 +436,41 @@ mod tests { ); } + /// Reporting only the last endpoint's reason hid what the earlier ones said, and a caller + /// reading this text to tell a chain rejection from a transport fault would then miss the + /// rejection and skip the recovery it calls for. + #[tokio::test] + async fn a_failure_names_every_endpoint_that_was_tried() { + let servers = [server_refusing().await, server_refusing().await]; + let pool = RpcProviderPool::new( + servers + .iter() + .map(|s| s.uri().parse().expect("server URL")) + .collect(), + Duration::from_secs(5), + 0, + ) + .expect("pool"); + + let err = pool + .execute("probe", |provider| async move { + provider.get_block_number().await + }) + .await + .expect_err("every endpoint refused, so the call fails"); + + let text = err.to_string(); + for server in &servers { + let named = endpoint_name(&server.uri().parse().expect("server URL")); + assert!(text.contains(&named), "{named} is missing from: {text}"); + } + assert_eq!( + text.matches("refused").count(), + servers.len(), + "every endpoint's own reason should appear: {text}" + ); + } + /// 500 is the status a provider answered on 2026-07-29 while an accepted agreement went /// unfunded. Reading it here is what earns a retry on that provider before rotating; the /// rotation itself is unconditional, so this decides attempts rather than failover. From 8fbef873441ea2f2bc766efec36a40e6626192d2 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 19:36:07 +1000 Subject: [PATCH 32/49] fix(chain): read a temporary internal error as worth another go The provider that stranded an accepted agreement on 2026-07-29 answered `code 19 Temporary internal error. Please retry`. That was only recognised because it arrived with an HTTP 500; sent under a 200, the same complaint bought no retry, because nothing read the wording. --- .../src/chain_client/rpc_provider.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 1edae0b5..debd1220 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -53,6 +53,7 @@ const RETRYABLE_ERROR_PATTERNS: &[&str] = &[ "too many requests", "service unavailable", "bad gateway", + "temporary internal error", ]; /// Type alias for the provider with default fillers. @@ -524,6 +525,22 @@ mod tests { ); } + /// The provider that stranded an accepted agreement on 2026-07-29 answered `code 19 + /// Temporary internal error. Please retry`, a code alloy does not know. Sent under a 200 + /// the status says nothing either, so the wording is the only thing left to read. + #[test] + fn a_temporary_internal_error_without_a_status_is_retryable() { + let payload = serde_json::from_str( + r#"{"code":19,"message":"Temporary internal error. Please retry"}"#, + ) + .expect("JSON-RPC error payload"); + let err: TransportError = RpcError::ErrorResp(payload); + assert!( + RpcProviderPool::is_retryable(&err), + "the fault behind the outage should be retryable however it is reported" + ); + } + #[test] fn test_backoff_delay_calculation() { // 1s, 2s, 4s, 8s, 16s, 32s->30s From 0c7a0c7c096df5da28c5e6a1db186ec94b23f982 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 19:36:42 +1000 Subject: [PATCH 33/49] docs(chain): say what rotating the shared start actually does The note claimed later calls start from an endpoint that has not just failed. Only calls that begin afterwards get that, a call already walking the ring keeps the start it took, and a sweep that fails on every endpoint leaves the shared start back where it began. --- bin/dipper-service/src/chain_client/rpc_provider.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index debd1220..43043457 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -233,8 +233,9 @@ impl RpcProviderPool { ))); } - // Advance the shared index as well, so later calls start from a provider that has - // not just failed rather than repeating this one's discovery. + // Move the shared start on too, so a call beginning after this one skips the + // endpoint that just failed. A call already under way keeps its own start, and a + // sweep that fails everywhere lands back where it began. self.rotate(); let next_url = self.url_at(start + providers_tried); tracing::warn!( From 64c6644230e32db406422f86fbbeee4e515903d7 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 19:38:09 +1000 Subject: [PATCH 34/49] docs(chain): record why the read failover test runs in real time The one-second backoff this test waits out looks like something tokio's clock control could skip. It cannot: with time paused, tokio advances the clock while the HTTP response is still arriving, the request timeout fires, and every endpoint then looks dead. --- bin/dipper-service/src/chain_client/rpc_provider.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 43043457..16141ede 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -325,9 +325,9 @@ mod tests { server } - /// Every read the service makes goes through this path, so a read has to fail over the same - /// way a submission does, and a fault worth another go has to earn one before it rotates. - /// One retry rather than the usual several, to keep the backoff this waits out short. + /// Every read goes through this path, so a read has to fail over the way a submission does, + /// and a fault worth another go has to earn one before it rotates. One retry keeps the + /// backoff short; real time, since pausing it jumps the clock to the request timeout. #[tokio::test] async fn a_read_retries_a_sick_endpoint_then_rotates() { let sick = MockServer::start().await; From 8782680e1634cd8ba76a2174307981cd80a57e24 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 19:39:34 +1000 Subject: [PATCH 35/49] test(chain): name the chain client tests after what they check Half the tests in these two files were named after the function under test and half after the behaviour they pin, so a failure told you where to look but not what broke. They now all read as a sentence about the behaviour. --- bin/dipper-service/src/chain_client/client.rs | 10 +++++----- bin/dipper-service/src/chain_client/rpc_provider.rs | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index ce20b157..41a3d29f 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -1246,7 +1246,7 @@ mod tests { } #[test] - fn test_is_nonce_error() { + fn nonce_rejections_are_told_apart_from_other_refusals() { // Nonce errors assert!(is_nonce_error("nonce too low")); assert!(is_nonce_error("Nonce Too Low for account")); @@ -1262,13 +1262,13 @@ mod tests { } #[test] - fn test_classify_fill_nonce_gap_outcome_success_returns_ok() { + fn a_nonce_gap_fill_that_is_accepted_is_a_success() { let result = classify_fill_nonce_gap_outcome(42, Ok(B256::ZERO)); assert!(result.is_ok()); } #[test] - fn test_classify_fill_nonce_gap_outcome_swallows_nonce_error() { + fn a_nonce_gap_fill_refused_on_the_nonce_is_still_a_success() { // Each of these strings flips `is_nonce_error` to true; the gap // fill must treat them as success because the original tx is // either still in flight or the slot is already filled. @@ -1288,7 +1288,7 @@ mod tests { } #[test] - fn test_classify_fill_nonce_gap_outcome_propagates_other_error() { + fn a_nonce_gap_fill_that_fails_for_another_reason_is_reported() { // Errors that don't match `is_nonce_error` mean the noop tx itself // failed for a real reason (RPC down, gas estimation broken, etc.), // so the wallet may stay wedged. Surface to the caller. @@ -1301,7 +1301,7 @@ mod tests { } #[tokio::test] - async fn test_nonce_reservation_unique_under_concurrent_callers() { + async fn concurrent_callers_each_reserve_a_different_nonce() { use std::collections::HashSet; let counter = Arc::new(AtomicU64::new(NONCE_UNINITIALIZED)); diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 16141ede..09d81ef1 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -543,7 +543,7 @@ mod tests { } #[test] - fn test_backoff_delay_calculation() { + fn each_retry_waits_twice_as_long_up_to_a_ceiling() { // 1s, 2s, 4s, 8s, 16s, 32s->30s assert_eq!(RpcProviderPool::backoff_delay(0), Duration::from_secs(1)); assert_eq!(RpcProviderPool::backoff_delay(1), Duration::from_secs(2)); @@ -557,7 +557,7 @@ mod tests { /// Faults that arrive with no status and no error code, only a description, which is /// what a connection that never got a reply looks like. #[test] - fn test_retryable_error_detection() { + fn faults_described_only_in_words_are_read_from_the_text() { let retryable_errors = [ "connection refused by remote host", "Connection Reset by peer", @@ -593,7 +593,7 @@ mod tests { } #[test] - fn test_provider_pool_requires_at_least_one_provider() { + fn a_pool_with_no_endpoints_is_refused() { let result = RpcProviderPool::new(vec![], Duration::from_secs(30), 3); assert!(result.is_err()); @@ -607,7 +607,7 @@ mod tests { } #[test] - fn test_provider_pool_rotation() { + fn rotating_walks_the_endpoints_and_wraps_round() { let providers = vec![ Url::parse("https://rpc1.example.com").unwrap(), Url::parse("https://rpc2.example.com").unwrap(), From 167c62f4b4398413e37f27f5b779a876ed6eb801 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 20:08:55 +1000 Subject: [PATCH 36/49] fix(chain): keep endpoint URLs out of RPC failure text An endpoint is already named by host alone so its API key stays out of the logs, but the reason a call failed was passed through as the HTTP client wrote it, and that names the whole URL. Take the URL out of the reason too, leaving the host in its place. --- .../src/chain_client/rpc_provider.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 09d81ef1..586f48a1 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -41,6 +41,18 @@ fn endpoint_name(url: &Url) -> String { } } +/// How a failure reads once the endpoint's URL is taken out of it. A connection that never +/// got a reply is described by the HTTP client, which names the URL it was reaching for, and +/// that is where a hosted endpoint carries the API key that gets us in. +fn describe_failure(url: &Url, error: &TransportError) -> String { + let name = endpoint_name(url); + // A URL is as likely to be printed with its trailing slash as without, so take out both. + error + .to_string() + .replace(url.as_str(), &name) + .replace(url.as_str().trim_end_matches('/'), &name) +} + /// Error text that indicates a transient failure worth retrying, used only for faults /// that arrive as prose rather than as a status code or JSON-RPC error object. const RETRYABLE_ERROR_PATTERNS: &[&str] = &[ @@ -196,7 +208,7 @@ impl RpcProviderPool { attempt = attempt + 1, max_retries, delay_ms = delay.as_millis(), - error = %e, + error = %describe_failure(¤t_url, &e), "Retryable RPC error, backing off" ); tokio::time::sleep(delay).await; @@ -213,7 +225,8 @@ impl RpcProviderPool { let endpoint_error = endpoint_error .unwrap_or_else(|| TransportErrorKind::custom_str("no attempt was made")); - reasons.push(format!("{endpoint}: {endpoint_error}")); + let reason = describe_failure(¤t_url, &endpoint_error); + reasons.push(format!("{endpoint}: {reason}")); providers_tried += 1; // Check if we've tried all providers @@ -244,7 +257,7 @@ impl RpcProviderPool { new_provider = %endpoint_name(next_url), providers_tried, total_providers = self.providers.len(), - error = %endpoint_error, + error = %reason, "Rotating RPC provider after failures" ); } From cd3d75d5a6ace0bf753c1dbb6658c82dbe58fef2 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 20:12:39 +1000 Subject: [PATCH 37/49] test(chain): check an unanswered connection hides the API key The endpoint the existing check points at answers every call, and an endpoint that answers describes its own refusal without ever naming a URL. Point a second check at a port nothing listens on, which is what makes the HTTP client write the URL into the failure. --- .../src/chain_client/rpc_provider.rs | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 586f48a1..6186a23f 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -451,6 +451,34 @@ mod tests { ); } + /// An endpoint that answers can only describe its own refusal, so it never repeats the + /// URL. One that never answers is described by the HTTP client instead, which says which + /// URL it was reaching for, and that is where the key sits. Nothing listens on port 1. + #[tokio::test] + async fn a_connection_that_gets_no_answer_hides_the_api_key() { + let keyed: Url = "http://127.0.0.1:1/v2/super-secret-key" + .parse() + .expect("keyed endpoint URL"); + + let pool = RpcProviderPool::new(vec![keyed], Duration::from_secs(5), 0).expect("pool"); + let err = pool + .execute("probe", |provider| async move { + provider.get_block_number().await + }) + .await + .expect_err("nothing is listening, so the call fails"); + + let text = err.to_string(); + assert!( + !text.contains("super-secret-key"), + "the API key must not appear in the failure: {text}" + ); + assert!( + text.contains("127.0.0.1:1"), + "the failure should still say which endpoint was unreachable: {text}" + ); + } + /// Reporting only the last endpoint's reason hid what the earlier ones said, and a caller /// reading this text to tell a chain rejection from a transport fault would then miss the /// rejection and skip the recovery it calls for. From 1ceddfc6efbdb95e48ad9cded20390b1990db701 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 20:13:42 +1000 Subject: [PATCH 38/49] fix(chain): keep a named rejection when a later endpoint is unreachable Only the last endpoint's error was inspected for a structured rejection, so a contract refusing the call on one endpoint was downgraded to a generic RPC fault whenever the next endpoint was merely unreachable. Callers then retried a refusal that will never clear. --- .../src/chain_client/rpc_provider.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 6186a23f..7f3eb589 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -179,6 +179,10 @@ impl RpcProviderPool { // this text to tell a stale nonce from a transport fault, so a rejection from the // first endpoint has to survive a different kind of failure on the next. let mut reasons: Vec = Vec::with_capacity(self.providers.len()); + // The first endpoint to name its refusal precisely, such as a contract rejecting the + // call during gas estimation. Callers act on a named refusal and only log a generic + // one, so a later endpoint merely being unreachable must not bury it. + let mut named_refusal: Option = None; let mut providers_tried = 0; // Walk the ring by local offset from wherever the pool points. Re-reading the shared @@ -229,13 +233,14 @@ impl RpcProviderPool { reasons.push(format!("{endpoint}: {reason}")); providers_tried += 1; + // Structured errors reach us boxed in through `TransportErrorKind::custom`, which + // is how gas estimation hands back a contract rejection. Keep the first one. + named_refusal = named_refusal.or_else(|| extract_chain_client_error(endpoint_error)); + // Check if we've tried all providers if providers_tried >= self.providers.len() { - // Preserve structured ChainClientError instances boxed in via - // TransportErrorKind::custom (e.g. ContractRevert from gas - // estimation). Otherwise fall back to the generic wrap. - if let Some(typed) = extract_chain_client_error(endpoint_error) { - return Err(typed); + if let Some(named) = named_refusal { + return Err(named); } return Err(ChainClientError::RpcError(anyhow::anyhow!( From d702dddf8f896162882a5e040fcc8580c4b308e9 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 20:14:40 +1000 Subject: [PATCH 39/49] test(chain): cover a contract refusal outliving an unreachable endpoint One endpoint reports the contract refusing the call while the next cannot be reached at all, which is the shape that used to hide the refusal behind a generic fault. --- .../src/chain_client/rpc_provider.rs | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 7f3eb589..78617d71 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -456,6 +456,46 @@ mod tests { ); } + /// A contract refusing the call is the chain's answer, not one endpoint's, and the caller + /// reads it to decide whether to give up rather than retry. A later endpoint being merely + /// unreachable must not turn that into a generic fault that reads as worth another go. + #[tokio::test] + async fn a_named_refusal_outlives_a_later_endpoint_going_dark() { + let pool = RpcProviderPool::new( + vec![ + Url::parse("http://refusing.invalid").expect("refusing endpoint URL"), + Url::parse("http://dark.invalid").expect("dark endpoint URL"), + ], + Duration::from_secs(5), + 0, + ) + .expect("pool"); + + let calls = AtomicUsize::new(0); + let err = pool + .execute("probe", |_provider| { + let refusing = calls.fetch_add(1, Ordering::Relaxed) == 0; + async move { + let outcome: Result<(), TransportError> = Err(if refusing { + TransportErrorKind::custom(ChainClientError::ContractRevert { + selector: [0xde, 0xad, 0xbe, 0xef], + data: Default::default(), + }) + } else { + TransportErrorKind::custom_str("connection refused") + }); + outcome + } + }) + .await + .expect_err("both endpoints failed, so the call fails"); + + assert!( + matches!(err, ChainClientError::ContractRevert { .. }), + "the contract's refusal should have survived, got {err}" + ); + } + /// An endpoint that answers can only describe its own refusal, so it never repeats the /// URL. One that never answers is described by the HTTP client instead, which says which /// URL it was reaching for, and that is where the key sits. Nothing listens on port 1. From 221f02f39fe514823ac4d9e70f0f73c8c230f59a Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 20:15:51 +1000 Subject: [PATCH 40/49] docs(chain): say which check claims an already-held transaction first A node reporting that it already holds the transaction is listed as a nonce problem, which reads as though refreshing the nonce and resending were the answer. The send path claims that wording first and reports the successful broadcast it is, so say so in both places. --- bin/dipper-service/src/chain_client/client.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 41a3d29f..c2e1c3f2 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -69,6 +69,8 @@ const NONCE_ERROR_PATTERNS: &[&str] = &[ "nonce is too low", "invalid nonce", "replacement transaction underpriced", + // A backstop only. `is_already_broadcast` claims this wording first and reports the + // broadcast as the success it is, so a send never reaches here saying it. "already known", ]; @@ -1252,6 +1254,7 @@ mod tests { assert!(is_nonce_error("Nonce Too Low for account")); assert!(is_nonce_error("invalid nonce: expected 5, got 3")); assert!(is_nonce_error("replacement transaction underpriced")); + // A backstop, not a live path: a send reports this as the successful broadcast it is. assert!(is_nonce_error("transaction already known")); // Non-nonce errors From bb527d9e6179ac3afa8dd00fa176ce6ba6fa5ecc Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 20:17:15 +1000 Subject: [PATCH 41/49] fix(chain): say a transaction is not ready rather than blaming signing No field is filled in for the caller on the send path, so a request missing one stops before the signature exists. Calling that a signing failure sends a reader after the wrong thing, when the wording underneath already names the field that was left out. --- bin/dipper-service/src/chain_client/client.rs | 36 ++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index c2e1c3f2..67967cbd 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -479,8 +479,11 @@ impl AlloyChainClient { } let wallet = EthereumWallet::from(self.inner.signer.clone()); + // A caller leaving out a field fails here rather than having it filled in, so this is + // as likely to be an incomplete request as a signing fault. Alloy's own wording names + // which, so leave the reason to it. let signed = tx.clone().build(&wallet).await.map_err(|e| { - ChainClientError::SubmitFailed(anyhow::anyhow!("Failed to sign transaction: {e}")) + ChainClientError::SubmitFailed(anyhow::anyhow!("Transaction not ready to send: {e}")) })?; let signed_hash = *signed.tx_hash(); let raw = signed.encoded_2718(); @@ -1158,6 +1161,37 @@ mod tests { ); } + /// A field a caller leaves out is no longer filled in for them, so the send stops rather + /// than putting an incomplete transaction on the wire. What it says has to name the field, + /// because a caller reading only "signing failed" would go looking at the wrong thing. + #[tokio::test] + async fn send_transaction_refuses_a_transaction_missing_a_field() { + let server = server_answering_with(B256::repeat_byte(0xab)).await; + let client = client_over(vec![server.uri().parse().expect("provider URL")]); + + let mut no_gas_limit = ready_to_send_tx(client.inner.signer.address()); + no_gas_limit.gas = None; + + let err = client + .send_transaction(&no_gas_limit) + .await + .expect_err("a transaction with no gas limit must not be sent"); + + let text = err.to_string(); + assert!( + text.contains("gas_limit"), + "the failure should name the missing field, got: {text}" + ); + assert!( + server + .received_requests() + .await + .unwrap_or_default() + .is_empty(), + "no endpoint should have been asked to accept it" + ); + } + /// The nonce reason has to survive a second provider failing a different way. Reporting /// only the last provider's reason hid the rejection behind a transport fault, and /// `sign_and_send` then skipped the resync that would have unstuck the wallet. From 3cec514f48a63b6a046c6594dcb69d1f235912f9 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 22:38:53 +1000 Subject: [PATCH 42/49] fix(chain): retry a submission on one endpoint before rotating away A submission got a single attempt per endpoint, on the reasoning that a different endpoint was likelier to help than pressing a struggling one. Against an endpoint that fails quickly that gave up in milliseconds, and it left anyone running one endpoint with no retry at all. --- bin/dipper-service/src/chain_client/client.rs | 45 ++++++++++++++++--- .../src/chain_client/rpc_provider.rs | 15 ------- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 67967cbd..94317fa7 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -490,7 +490,7 @@ impl AlloyChainClient { self.inner .rpc_pool - .execute_trying_each_once("send_transaction", |provider| { + .execute("send_transaction", |provider| { let raw = raw.clone(); async move { match provider.send_raw_transaction(&raw).await { @@ -1217,11 +1217,11 @@ mod tests { ); } - /// A submission moves on to the next endpoint rather than pressing a struggling one, even - /// when the complaint is the kind a retry would normally clear. Retries here are expensive: - /// they hold up every other submission and eat into the window an offer has to be accepted. + /// An endpoint saying it is overloaded is asked again before the submission gives up on + /// it, because that complaint often clears on its own and rotating away costs a provider. + /// One retry rather than the usual several, to keep the backoff this waits out short. #[tokio::test] - async fn send_transaction_leaves_a_struggling_endpoint_alone() { + async fn send_transaction_retries_a_struggling_endpoint_before_rotating() { let overloaded = server_answering_rpc_error(-32005, "project ID request rate exceeded").await; let healthy = server_answering_with(B256::repeat_byte(0xcd)).await; @@ -1230,7 +1230,7 @@ mod tests { overloaded.uri().parse().expect("overloaded provider URL"), healthy.uri().parse().expect("healthy provider URL"), ], - 3, + 1, ); let tx = ready_to_send_tx(client.inner.signer.address()); @@ -1245,8 +1245,39 @@ mod tests { .await .unwrap_or_default() .len(), + 2, + "the struggling endpoint should get its retry before the rotation" + ); + assert_eq!( + healthy.received_requests().await.unwrap_or_default().len(), + 1, + "the healthy endpoint should answer on the first ask" + ); + } + + /// Offering the same bytes twice is what makes a retry safe: an endpoint that took them + /// and then failed to say so recognises them the second time, and reports the broadcast + /// that already happened rather than accepting a second transaction. + #[tokio::test] + async fn a_retry_that_lands_on_bytes_already_held_is_a_success() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(AlreadyHeldResponder { pending_nonce: 6 }) + .mount(&server) + .await; + let client = client_over_retrying(vec![server.uri().parse().expect("provider URL")], 1); + let tx = ready_to_send_tx(client.inner.signer.address()); + + let tx_hash = client + .send_transaction(&tx) + .await + .expect("bytes already held are a successful broadcast"); + + assert_eq!(tx_hash, signed_hash_of(&client, &tx).await); + assert_eq!( + sends_among(&server.received_requests().await.unwrap_or_default()), 1, - "a struggling endpoint should be left alone once it has refused" + "an accepted broadcast should not be offered again" ); } diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 78617d71..aff756a5 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -150,21 +150,6 @@ impl RpcProviderPool { .await } - /// Run an RPC call, giving each endpoint a single attempt instead of several. Submitting - /// a transaction uses this: it holds a lock that every other submission queues behind, - /// and a different endpoint is likelier to help than asking a sick one four times. - pub async fn execute_trying_each_once( - &self, - operation: &str, - f: F, - ) -> Result - where - F: Fn(HttpProvider) -> Fut, - Fut: Future>, - { - self.execute_with_retries(operation, 0, f).await - } - async fn execute_with_retries( &self, operation: &str, From dff292d1dfb9056733a37fbfe2cf455e856f309a Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 22:40:24 +1000 Subject: [PATCH 43/49] fix(chain): stop one submission holding the lock without limit Submissions run one at a time, and how long one takes grew with the number of endpoints configured and the retries each is given, so a slow spell could park everything behind it. Cap a submission at 60 seconds, which frees the lock and lets the job run again later. --- bin/dipper-service/src/chain_client/client.rs | 72 ++++++++++++++++++- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 94317fa7..cb840afc 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -4,6 +4,7 @@ //! alloy for Ethereum interactions. use std::{ + future::Future, sync::{ Arc, atomic::{AtomicU64, Ordering}, @@ -80,6 +81,27 @@ fn is_nonce_error(error: &str) -> bool { NONCE_ERROR_PATTERNS.iter().any(|p| lower.contains(p)) } +/// How long one submission may hold `submit_lock` before giving up. Every other submission +/// queues behind it, so without a cap the wait grows with the endpoint count and the retry +/// budget. Comfortably inside the 300s a worker job gets and the RCA acceptance deadline. +const SUBMIT_DEADLINE: Duration = Duration::from_secs(60); + +/// Run a submission under [`SUBMIT_DEADLINE`], reporting a failed submission if it runs over. +/// Giving up releases the lock, and the caller re-runs the job rather than losing the work. +async fn under_submit_deadline(work: F) -> Result +where + F: Future>, +{ + tokio::time::timeout(SUBMIT_DEADLINE, work) + .await + .unwrap_or_else(|_| { + Err(ChainClientError::SubmitFailed(anyhow::anyhow!( + "gave up submitting after {}s holding the submission lock", + SUBMIT_DEADLINE.as_secs() + ))) + }) +} + /// How nodes say they are already holding the transaction being offered to them. const ALREADY_BROADCAST_PATTERNS: &[&str] = &["already known", "already imported", "known transaction"]; @@ -376,14 +398,24 @@ impl AlloyChainClient { /// retry loop so concurrent reservations cannot interleave with each /// other's submissions; released before receipt polling. async fn sign_and_send( + &self, + tx: TransactionRequest, + agreement_id: &[u8; 16], + ) -> Result { + let _submit_guard = self.inner.submit_lock.lock().await; + under_submit_deadline(self.sign_and_send_locked(tx, agreement_id)).await + } + + /// The work `sign_and_send` does while holding `submit_lock`, split out so the deadline + /// wraps the work rather than the wait for the lock: a caller queueing politely behind + /// someone else should not be charged for the time it spent waiting. + async fn sign_and_send_locked( &self, mut tx: TransactionRequest, agreement_id: &[u8; 16], ) -> Result { const MAX_NONCE_RETRIES: u32 = 2; - let _submit_guard = self.inner.submit_lock.lock().await; - for attempt in 0..MAX_NONCE_RETRIES { if attempt > 0 { self.resync_nonce().await?; @@ -1255,6 +1287,42 @@ mod tests { ); } + /// Every other submission queues behind the one holding the lock, so a submission that + /// never finishes has to be cut off rather than waited out. Paused time so the deadline + /// is reached without the test spending it. + #[tokio::test(start_paused = true)] + async fn a_submission_that_never_finishes_is_given_up_on() { + let err = under_submit_deadline(std::future::pending()) + .await + .expect_err("a submission that never finishes must not be waited out"); + + assert!( + matches!(err, ChainClientError::SubmitFailed(_)), + "got {err}" + ); + assert!( + err.to_string().contains("60"), + "the failure should say how long it waited, got: {err}" + ); + } + + /// A submission that finishes inside the deadline is left alone, so the cap only ever + /// catches the case it is there for. + #[tokio::test(start_paused = true)] + async fn a_submission_that_finishes_in_time_is_left_alone() { + let submitted = under_submit_deadline(async { + tokio::time::sleep(SUBMIT_DEADLINE / 2).await; + Ok(SubmittedTx { + hash: B256::repeat_byte(0x77), + nonce: 3, + }) + }) + .await + .expect("a submission inside the deadline should stand"); + + assert_eq!(submitted.hash, B256::repeat_byte(0x77)); + } + /// Offering the same bytes twice is what makes a retry safe: an endpoint that took them /// and then failed to say so recognises them the second time, and reports the broadcast /// that already happened rather than accepting a second transaction. From 3ea204a8fca53cc8e679a62c3a5ef14ffd3e0fd4 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 22:41:03 +1000 Subject: [PATCH 44/49] docs(chain): note that concurrent callers can undo a rotation Whether a call starts on an endpoint that just failed depends on how many other calls failed alongside it, because the shared starting point counts failures. Say so where it happens, since the next reader will otherwise wonder whether it is a bug. --- bin/dipper-service/src/chain_client/rpc_provider.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index aff756a5..156d7060 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -236,9 +236,9 @@ impl RpcProviderPool { ))); } - // Move the shared start on too, so a call beginning after this one skips the - // endpoint that just failed. A call already under way keeps its own start, and a - // sweep that fails everywhere lands back where it began. + // Move the shared start on, so a call beginning after this one skips the endpoint + // that just failed. Counting failures this way lets concurrent callers wind it + // back round to a failing endpoint, costing them the one wasted first ask. self.rotate(); let next_url = self.url_at(start + providers_tried); tracing::warn!( From c8e592e18bced10919cc6d715db96b136d99c19b Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 22:42:11 +1000 Subject: [PATCH 45/49] fix(chain): report a chain mismatch as a failed submission A transaction naming the wrong chain was refused as a configuration fault, and the path that cancels a stale agreement reads a configuration fault as the chain client being switched off: it would skip the cancel and mark the agreement abandoned while it was still live on-chain. --- bin/dipper-service/src/chain_client/client.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index cb840afc..2957ffb2 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -501,9 +501,10 @@ impl AlloyChainClient { /// bytes under one hash and the hash is known before anyone is asked to accept them. async fn send_transaction(&self, tx: &TransactionRequest) -> Result { // Nothing fills a field in on this path any more, and a request that names no chain is - // signed for chain 1 rather than refused, so check before the signature exists. + // signed for chain 1 rather than refused, so check before the signature exists. Not a + // `ConfigError`: the cancel path reads that as the chain client being switched off. if tx.chain_id() != Some(self.inner.chain_id) { - return Err(ChainClientError::ConfigError(format!( + return Err(ChainClientError::SubmitFailed(anyhow::anyhow!( "refusing to sign for chain {:?} while configured for chain {}", tx.chain_id(), self.inner.chain_id @@ -1165,7 +1166,8 @@ mod tests { } /// Nothing fills a field in on the send path, and a request naming no chain is signed for - /// chain 1 rather than refused, so a transaction has to say which chain it is for. + /// chain 1 rather than refused, so a transaction has to say which chain it is for. It has + /// to read as a failed submission: a config fault means "chain client off" to the caller. #[tokio::test] async fn send_transaction_refuses_a_transaction_that_names_another_chain() { let server = server_answering_with(B256::repeat_byte(0xab)).await; @@ -1180,7 +1182,10 @@ mod tests { .send_transaction(&tx) .await .expect_err("a transaction for another chain must not be signed"); - assert!(matches!(err, ChainClientError::ConfigError(_)), "got {err}"); + assert!( + matches!(err, ChainClientError::SubmitFailed(_)), + "got {err}" + ); } assert!( From eeb9f308aa3b2fb5596111b81669b5dce2b3e6e5 Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 22:43:30 +1000 Subject: [PATCH 46/49] fix(config): wait 10 seconds for an RPC call rather than 30 Every call the chain client makes is a small one, so 30 seconds was patience it never needed, and it multiplies: an endpoint that stops answering is asked 4 times before the next is tried. Waiting 10 thirds how long a stalled endpoint delays a submission or a receipt check. --- bin/dipper-service/src/config.rs | 5 +++-- k8s/configmap-example.yaml | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/bin/dipper-service/src/config.rs b/bin/dipper-service/src/config.rs index 9be975c8..ccee80c5 100644 --- a/bin/dipper-service/src/config.rs +++ b/bin/dipper-service/src/config.rs @@ -663,7 +663,8 @@ pub struct ChainClientConfig { /// rotating to the next on persistent failures. pub providers: Vec, - /// Request timeout per RPC call in seconds (default: 30s) + /// Request timeout per RPC call in seconds (default: 10s). Every call here is a small one, + /// so this is headroom rather than a working limit, and it multiplies with `max_retries`. #[serde(default = "default_chain_client_request_timeout")] #[serde_as(as = "serde_with::DurationSeconds")] pub request_timeout: Duration, @@ -717,7 +718,7 @@ fn default_chain_client_enabled() -> bool { } fn default_chain_client_request_timeout() -> Duration { - Duration::from_secs(30) + Duration::from_secs(10) } fn default_domain_refresh_interval() -> Duration { diff --git a/k8s/configmap-example.yaml b/k8s/configmap-example.yaml index 5d48fc60..6b857d13 100644 --- a/k8s/configmap-example.yaml +++ b/k8s/configmap-example.yaml @@ -94,7 +94,7 @@ data: "https://arb1.arbitrum.io/rpc", "https://arbitrum-one.publicnode.com" ], - "request_timeout": 30, + "request_timeout": 10, "max_retries": 3, "gas_price_multiplier": 1.2, "max_gas_price_gwei": 100, From f7a889ade26689f5b8658925bdc472ceb77e9b2b Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 23:48:06 +1000 Subject: [PATCH 47/49] docs(chain): tighten the chain client's comments Several comment blocks said in 4 to 8 lines what fits in 3, so trim them without losing the reasoning they carry. No code changes. --- bin/dipper-service/src/chain_client/client.rs | 116 ++++++------------ 1 file changed, 39 insertions(+), 77 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 2957ffb2..6f02766c 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -1,6 +1,4 @@ -//! AlloyChainClient implementation. -//! -//! This is the production implementation of the `ChainClient` trait using +//! AlloyChainClient: the production implementation of the `ChainClient` trait, using //! alloy for Ethereum interactions. use std::{ @@ -36,9 +34,8 @@ use crate::{ config::ChainClientConfig, }; -/// OFFER_TYPE_NEW from `RecurringCollector.sol`. Used when submitting a new -/// agreement offer on-chain. The contract defines OFFER_TYPE_NONE=0, -/// OFFER_TYPE_NEW=1, OFFER_TYPE_UPDATE=2; passing 0 reverts with +/// OFFER_TYPE_NEW from `RecurringCollector.sol`, used when submitting a new agreement +/// offer on-chain. The contract defines NONE=0, NEW=1, UPDATE=2; passing 0 reverts with /// RecurringCollectorInvalidOfferType(0). const OFFER_TYPE_NEW: u8 = 1; @@ -152,10 +149,8 @@ fn classify_fill_nonce_gap_outcome( } } -/// Production implementation of `ChainClient` using alloy. -/// -/// This struct is `Clone` via internal `Arc` wrapping, allowing it to be shared -/// across async task contexts. +/// Production implementation of `ChainClient` using alloy. `Clone` via internal `Arc` +/// wrapping, so it can be shared across async task contexts. #[derive(Clone)] pub struct AlloyChainClient { /// Inner state wrapped in Arc for Clone support @@ -193,26 +188,19 @@ struct AlloyChainClientInner { gas_price_multiplier: f64, /// Maximum gas price in gwei max_gas_price_gwei: u64, - /// In-memory nonce counter. Concurrent callers atomically increment this - /// to get unique nonces without querying the chain, avoiding - /// "replacement transaction underpriced" errors when multiple offer() - /// transactions are submitted in parallel. + /// In-memory nonce counter. Concurrent callers atomically increment this to get + /// unique nonces without querying the chain, avoiding "replacement transaction + /// underpriced" errors when multiple offer() transactions are submitted in parallel. nonce: AtomicU64, - /// Serializes nonce reservation through mempool submission so the RPC - /// sees txs in nonce order and the wallet's queue can never strand a - /// higher-nonce tx behind an unfilled gap. Released before receipt - /// polling so multiple confirmations pipeline concurrently. + /// Serializes nonce reservation through mempool submission so the RPC sees txs in + /// nonce order and the wallet's queue can never strand a higher-nonce tx behind an + /// unfilled gap. Released before receipt polling so confirmations pipeline concurrently. submit_lock: Mutex<()>, } impl AlloyChainClient { - /// Create a new AlloyChainClient from configuration. - /// - /// # Errors - /// - /// Returns an error if: - /// - No RPC providers are configured - /// - The signer cannot be constructed + /// Create a new AlloyChainClient from configuration. Errors if no RPC providers are + /// configured or the signer cannot be constructed. pub fn new( config: &ChainClientConfig, chain_id: u64, @@ -259,10 +247,8 @@ impl AlloyChainClient { }) } - /// Build, gas-estimate, and send a call to any contract. - /// - /// Shared entry point for the manager-routed offer and cancel calls. - /// `log_agreement_id` is used only for structured logging. + /// Build, gas-estimate, and send a call to any contract. Shared entry point for the + /// manager-routed offer and cancel calls; `log_agreement_id` is only for logging. async fn build_and_send_call( &self, to: Address, @@ -275,12 +261,9 @@ impl AlloyChainClient { .to(to) .input(calldata.into()); - // 2. Estimate gas with safety bounds. - // - // The estimator may surface a structured contract revert (e.g. an - // already-canceled agreement). Box the typed error through alloy's - // Custom transport variant so `rpc_pool.execute` can hand it back - // without losing the selector and revert payload. + // 2. Estimate gas with safety bounds. The estimator may surface a structured + // contract revert (e.g. an already-canceled agreement); box it through alloy's + // Custom transport variant so the pool hands back the selector and payload intact. let gas_limit = self .inner .rpc_pool @@ -342,11 +325,9 @@ impl AlloyChainClient { self.sign_and_send(tx, log_agreement_id).await } - /// Get the next nonce, initializing from chain on first call. - /// - /// Concurrent callers each get a unique nonce via atomic - /// fetch-and-increment, avoiding the "replacement transaction - /// underpriced" race when multiple offer() calls fire in parallel. + /// Get the next nonce, initializing from chain on first call. Concurrent callers each + /// get a unique nonce via atomic fetch-and-increment, avoiding the "replacement + /// transaction underpriced" race when multiple offer() calls fire in parallel. async fn next_nonce(&self) -> Result { let current = self.inner.nonce.load(Ordering::SeqCst); if current == NONCE_UNINITIALIZED { @@ -375,12 +356,8 @@ impl AlloyChainClient { Ok(()) } - /// Fetch the pending transaction count from chain. - /// - /// Uses the "pending" block tag so the count includes transactions - /// sitting in the mempool from our wallet. Querying "latest" would - /// return a stale count when prior txs are awaiting confirmation, - /// causing the next tx to reuse a nonce that's already in-flight. + /// Fetch the pending transaction count from chain. The "pending" tag counts our own + /// mempool transactions too; "latest" would miss them and reuse an in-flight nonce. async fn fetch_chain_nonce(&self) -> Result { self.inner .rpc_pool @@ -391,12 +368,9 @@ impl AlloyChainClient { .await } - /// Sign and send a transaction with nonce error handling. - /// - /// Uses the in-memory nonce counter for the first attempt. On nonce - /// errors, re-syncs from chain and retries. `submit_lock` spans the - /// retry loop so concurrent reservations cannot interleave with each - /// other's submissions; released before receipt polling. + /// Sign and send a transaction, re-syncing the nonce from chain and retrying on nonce + /// errors. `submit_lock` spans the retry loop so concurrent reservations cannot + /// interleave with each other's submissions; released before receipt polling. async fn sign_and_send( &self, tx: TransactionRequest, @@ -459,12 +433,9 @@ impl AlloyChainClient { ))) } - /// Submit a self-transfer of 0 wei at `nonce` so the chain has - /// something to mine in a slot left empty by an evicted tx, - /// releasing higher-nonce txs from the same wallet that were - /// stuck behind the gap. Best-effort: an `is_nonce_error` - /// rejection means the original is still in flight or the gap is - /// already filled, so we treat that as success. + /// Submit a self-transfer of 0 wei at `nonce` so the chain has something to mine in a + /// slot left empty by an evicted tx, releasing higher-nonce txs stuck behind the gap. + /// Best-effort: an `is_nonce_error` rejection means the slot is spoken for, so success. async fn fill_nonce_gap(&self, nonce: u64) -> Result<(), ChainClientError> { let _submit_guard = self.inner.submit_lock.lock().await; @@ -549,10 +520,9 @@ impl AlloyChainClient { }) } - /// Poll `eth_getTransactionReceipt` until the tx has mined or the timeout - /// elapses. `Ok(Some(status))` reports the receipt's success flag; - /// `Ok(None)` signals the tx never appeared in time (dropped from the - /// mempool). Transient RPC errors are retried silently until timeout. + /// Poll `eth_getTransactionReceipt` until the tx has mined or the timeout elapses. + /// `Ok(Some(status))` reports the receipt's success flag; `Ok(None)` says the tx never + /// appeared in time (dropped from the mempool). Transient RPC errors keep polling. async fn wait_for_receipt( &self, tx_hash: B256, @@ -572,10 +542,8 @@ impl AlloyChainClient { Ok(Some(r)) => return Ok(Some(r.status())), Ok(None) => {} // not mined yet Err(e) => { - // Transient RPC error — log and keep polling. If the - // error is persistent, the outer handler will see the - // eventual timeout as `Ok(None)` and resubmit, which is - // the safe default. + // Transient RPC error: log and keep polling. If it persists, the outer + // handler sees the timeout as `Ok(None)` and resubmits, the safe default. tracing::debug!( tx_hash = %tx_hash, error = %e, @@ -1410,9 +1378,8 @@ mod tests { #[test] fn a_nonce_gap_fill_refused_on_the_nonce_is_still_a_success() { - // Each of these strings flips `is_nonce_error` to true; the gap - // fill must treat them as success because the original tx is - // either still in flight or the slot is already filled. + // Each of these strings flips `is_nonce_error` to true; the gap fill must treat + // them as success because they all mean the slot is already spoken for. for msg in [ "nonce too low", "replacement transaction underpriced", @@ -1461,14 +1428,9 @@ mod tests { handles.push(tokio::spawn(async move { let _guard = lock.lock().await; - // Mirror the entry shape of `next_nonce`/`resync_nonce`: - // - First caller initializes via fetch+CAS. - // - Every seventh subsequent caller hits a "nonce error" - // path: ratchets the counter via fetch_max(chain + 1) - // then reserves via fetch_add. The ratchet never lowers - // the counter, so an in-flight reservation cannot be - // invalidated even if the chain reports a lower pending. - // - Everyone else takes the next slot via fetch_add. + // Mirror the entry shape of `next_nonce`/`resync_nonce`: the first caller + // initializes via fetch+CAS, every seventh ratchets via fetch_max(chain + 1) + // before reserving via fetch_add, and the rest reserve via fetch_add. let nonce = if counter.load(Ordering::SeqCst) == NONCE_UNINITIALIZED { let chain_nonce = chain_pending.load(Ordering::SeqCst); match counter.compare_exchange( From 9b787d287aea1b22dcadbf3b6ee548f2154e4f4e Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 23:49:13 +1000 Subject: [PATCH 48/49] fix(chain): give a submission as long as its retry schedule allows A fixed 60s cut-off could cancel a submission mid-failover once endpoint count, retries and timeouts multiplied past it. Derive the deadline from the configured schedule instead, capped to leave a worker job room for the receipt poll that follows the broadcast. --- bin/dipper-service/src/chain_client/client.rs | 91 ++++++++++++++++--- .../src/chain_client/rpc_provider.rs | 30 ++++++ 2 files changed, 107 insertions(+), 14 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 6f02766c..6407a009 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -32,6 +32,7 @@ use super::{ use crate::{ chain_client::{ChainClient, ChainClientError}, config::ChainClientConfig, + worker::service::PROCESS_JOB_TIMEOUT, }; /// OFFER_TYPE_NEW from `RecurringCollector.sol`, used when submitting a new agreement @@ -78,23 +79,44 @@ fn is_nonce_error(error: &str) -> bool { NONCE_ERROR_PATTERNS.iter().any(|p| lower.contains(p)) } -/// How long one submission may hold `submit_lock` before giving up. Every other submission -/// queues behind it, so without a cap the wait grows with the endpoint count and the retry -/// budget. Comfortably inside the 300s a worker job gets and the RCA acceptance deadline. -const SUBMIT_DEADLINE: Duration = Duration::from_secs(60); +/// Number of attempts `sign_and_send` makes at finding a nonce the chain will take. +const MAX_NONCE_RETRIES: u32 = 2; + +/// How long one submission may hold `submit_lock` before giving up, derived from the retry +/// schedule so the cut-off can never pre-empt a retry the config allows: each nonce attempt +/// is at worst one full walk of the endpoint ring to read the nonce and another to send. +fn derive_submit_deadline(pool: &RpcProviderPool) -> Duration { + let budget = pool.worst_case_walk() * (2 * MAX_NONCE_RETRIES); + // The rest of the worker job's budget stays reserved for what follows the broadcast: + // the receipt poll and the nonce-gap fill. + let cap = PROCESS_JOB_TIMEOUT / 5 * 4; + if budget > cap { + tracing::warn!( + budget_secs = budget.as_secs(), + cap_secs = cap.as_secs(), + "retry schedule wants more time than a worker job allows; submissions may give \ + up before trying every endpoint" + ); + return cap; + } + budget +} -/// Run a submission under [`SUBMIT_DEADLINE`], reporting a failed submission if it runs over. +/// Run a submission under `deadline`, reporting a failed submission if it runs over. /// Giving up releases the lock, and the caller re-runs the job rather than losing the work. -async fn under_submit_deadline(work: F) -> Result +async fn under_submit_deadline( + deadline: Duration, + work: F, +) -> Result where F: Future>, { - tokio::time::timeout(SUBMIT_DEADLINE, work) + tokio::time::timeout(deadline, work) .await .unwrap_or_else(|_| { Err(ChainClientError::SubmitFailed(anyhow::anyhow!( "gave up submitting after {}s holding the submission lock", - SUBMIT_DEADLINE.as_secs() + deadline.as_secs() ))) }) } @@ -196,6 +218,8 @@ struct AlloyChainClientInner { /// nonce order and the wallet's queue can never strand a higher-nonce tx behind an /// unfilled gap. Released before receipt polling so confirmations pipeline concurrently. submit_lock: Mutex<()>, + /// How long one submission may hold `submit_lock`; see `derive_submit_deadline`. + submit_deadline: Duration, } impl AlloyChainClient { @@ -231,6 +255,8 @@ impl AlloyChainClient { "AlloyChainClient initialized" ); + let submit_deadline = derive_submit_deadline(&rpc_pool); + Ok(Self { inner: Arc::new(AlloyChainClientInner { rpc_pool, @@ -243,6 +269,7 @@ impl AlloyChainClient { max_gas_price_gwei: config.max_gas_price_gwei, nonce: AtomicU64::new(NONCE_UNINITIALIZED), submit_lock: Mutex::new(()), + submit_deadline, }), }) } @@ -377,7 +404,11 @@ impl AlloyChainClient { agreement_id: &[u8; 16], ) -> Result { let _submit_guard = self.inner.submit_lock.lock().await; - under_submit_deadline(self.sign_and_send_locked(tx, agreement_id)).await + under_submit_deadline( + self.inner.submit_deadline, + self.sign_and_send_locked(tx, agreement_id), + ) + .await } /// The work `sign_and_send` does while holding `submit_lock`, split out so the deadline @@ -388,8 +419,6 @@ impl AlloyChainClient { mut tx: TransactionRequest, agreement_id: &[u8; 16], ) -> Result { - const MAX_NONCE_RETRIES: u32 = 2; - for attempt in 0..MAX_NONCE_RETRIES { if attempt > 0 { self.resync_nonce().await?; @@ -1265,7 +1294,7 @@ mod tests { /// is reached without the test spending it. #[tokio::test(start_paused = true)] async fn a_submission_that_never_finishes_is_given_up_on() { - let err = under_submit_deadline(std::future::pending()) + let err = under_submit_deadline(Duration::from_secs(60), std::future::pending()) .await .expect_err("a submission that never finishes must not be waited out"); @@ -1283,8 +1312,8 @@ mod tests { /// catches the case it is there for. #[tokio::test(start_paused = true)] async fn a_submission_that_finishes_in_time_is_left_alone() { - let submitted = under_submit_deadline(async { - tokio::time::sleep(SUBMIT_DEADLINE / 2).await; + let submitted = under_submit_deadline(Duration::from_secs(60), async { + tokio::time::sleep(Duration::from_secs(30)).await; Ok(SubmittedTx { hash: B256::repeat_byte(0x77), nonce: 3, @@ -1296,6 +1325,40 @@ mod tests { assert_eq!(submitted.hash, B256::repeat_byte(0x77)); } + /// The deadline exists to stop one submission starving the queue, not to cut off retries + /// the config asks for, so it is derived from the schedule: each nonce attempt walks the + /// whole ring twice, once reading the chain's nonce and once broadcasting. + #[test] + fn the_submit_deadline_covers_the_retry_schedule() { + let client = client_over_retrying( + vec![ + "http://one.invalid".parse().expect("first URL"), + "http://two.invalid".parse().expect("second URL"), + ], + 1, + ); + + // Per endpoint: 2 attempts of 5s plus 1s of backoff; 2 endpoints make one walk of + // 22s; 2 walks for each of the 2 nonce attempts. + assert_eq!(client.inner.submit_deadline, Duration::from_secs(88)); + } + + /// A schedule that wants more time than a worker job has is capped rather than obeyed, + /// leaving room after the broadcast for the receipt poll and the nonce-gap fill. + #[test] + fn the_submit_deadline_stays_inside_a_worker_job() { + let providers = (0..20) + .map(|i| { + format!("http://rpc{i}.invalid") + .parse() + .expect("provider URL") + }) + .collect(); + let client = client_over_retrying(providers, 3); + + assert_eq!(client.inner.submit_deadline, PROCESS_JOB_TIMEOUT / 5 * 4); + } + /// Offering the same bytes twice is what makes a retry safe: an endpoint that took them /// and then failed to say so recognises them the second time, and reports the broadcast /// that already happened rather than accepting a second transaction. diff --git a/bin/dipper-service/src/chain_client/rpc_provider.rs b/bin/dipper-service/src/chain_client/rpc_provider.rs index 156d7060..0d038539 100644 --- a/bin/dipper-service/src/chain_client/rpc_provider.rs +++ b/bin/dipper-service/src/chain_client/rpc_provider.rs @@ -90,6 +90,9 @@ pub struct RpcProviderPool { http: reqwest::Client, /// Maximum retries per provider before rotating max_retries: u32, + /// The longest one walk of the ring can take; computed once here because the pool is + /// what knows the schedule. The submission deadline is derived from it. + worst_case_walk: Duration, } impl RpcProviderPool { @@ -118,14 +121,27 @@ impl RpcProviderPool { "RPC provider pool initialized" ); + // Every endpoint spending the full request timeout on every attempt, plus the + // backoff waited out between attempts, across one visit to each endpoint. + let backoff: Duration = (0..max_retries).map(Self::backoff_delay).sum(); + let worst_case_walk = + (request_timeout * (max_retries + 1) + backoff) * providers.len() as u32; + Ok(Self { providers, current_index: AtomicUsize::new(0), http, max_retries, + worst_case_walk, }) } + /// The longest [`execute`](Self::execute) can spend before giving up: every retry the + /// schedule allows, on every endpoint, with the backoff between them all waited out. + pub fn worst_case_walk(&self) -> Duration { + self.worst_case_walk + } + /// Rotate to the next provider. /// /// Returns the new provider URL after rotation. @@ -677,6 +693,20 @@ mod tests { } } + /// The submission deadline is derived from this figure, so it has to count every + /// attempt the schedule allows and the backoff waited out between them. + #[test] + fn the_worst_case_walk_counts_every_attempt_and_backoff() { + let providers = vec![ + Url::parse("https://rpc1.example.com").unwrap(), + Url::parse("https://rpc2.example.com").unwrap(), + ]; + let pool = RpcProviderPool::new(providers, Duration::from_secs(10), 3).unwrap(); + + // Per endpoint: 4 attempts of 10s plus 1+2+4s of backoff, across 2 endpoints. + assert_eq!(pool.worst_case_walk(), Duration::from_secs(94)); + } + #[test] fn rotating_walks_the_endpoints_and_wraps_round() { let providers = vec![ From ec60e40d98b3f67b45d2cf0968d7cc4be183cfcd Mon Sep 17 00:00:00 2001 From: MoonBoi9001 Date: Thu, 30 Jul 2026 23:50:19 +1000 Subject: [PATCH 49/49] fix(chain): spend a nonce only when its broadcast succeeds A submission that failed on every endpoint advanced the nonce counter anyway, leaving a slot the chain kept waiting on while every later transaction queued behind it until a restart. Advance the counter only after an endpoint accepts the transaction. --- bin/dipper-service/src/chain_client/client.rs | 184 +++++++++--------- 1 file changed, 89 insertions(+), 95 deletions(-) diff --git a/bin/dipper-service/src/chain_client/client.rs b/bin/dipper-service/src/chain_client/client.rs index 6407a009..d25ad58a 100644 --- a/bin/dipper-service/src/chain_client/client.rs +++ b/bin/dipper-service/src/chain_client/client.rs @@ -210,13 +210,13 @@ struct AlloyChainClientInner { gas_price_multiplier: f64, /// Maximum gas price in gwei max_gas_price_gwei: u64, - /// In-memory nonce counter. Concurrent callers atomically increment this to get - /// unique nonces without querying the chain, avoiding "replacement transaction - /// underpriced" errors when multiple offer() transactions are submitted in parallel. + /// The next nonce a submission should use. Read under `submit_lock` and advanced only + /// once a broadcast succeeds, so a failed submission cannot skip a slot the chain + /// would then sit waiting on while later transactions queue behind the gap. nonce: AtomicU64, - /// Serializes nonce reservation through mempool submission so the RPC sees txs in - /// nonce order and the wallet's queue can never strand a higher-nonce tx behind an - /// unfilled gap. Released before receipt polling so confirmations pipeline concurrently. + /// Serializes reading the nonce counter through committing it after the mempool + /// submission, so two submissions can never take the same slot. Released before + /// receipt polling so multiple confirmations pipeline concurrently. submit_lock: Mutex<()>, /// How long one submission may hold `submit_lock`; see `derive_submit_deadline`. submit_deadline: Duration, @@ -352,31 +352,28 @@ impl AlloyChainClient { self.sign_and_send(tx, log_agreement_id).await } - /// Get the next nonce, initializing from chain on first call. Concurrent callers each - /// get a unique nonce via atomic fetch-and-increment, avoiding the "replacement - /// transaction underpriced" race when multiple offer() calls fire in parallel. + /// The slot the next submission should use, read from the chain on first call. Only + /// `commit_nonce` moves the counter on, so a submission that never got a transaction + /// out leaves its slot to the next caller instead of leaving the chain waiting on it. async fn next_nonce(&self) -> Result { let current = self.inner.nonce.load(Ordering::SeqCst); - if current == NONCE_UNINITIALIZED { - let chain_nonce = self.fetch_chain_nonce().await?; - // CAS: if another task already initialized, use its value - match self.inner.nonce.compare_exchange( - NONCE_UNINITIALIZED, - chain_nonce + 1, - Ordering::SeqCst, - Ordering::SeqCst, - ) { - Ok(_) => return Ok(chain_nonce), - // Another task already initialized; get next unique nonce - Err(_) => return Ok(self.inner.nonce.fetch_add(1, Ordering::SeqCst)), - } + if current != NONCE_UNINITIALIZED { + return Ok(current); } - Ok(self.inner.nonce.fetch_add(1, Ordering::SeqCst)) + let chain_nonce = self.fetch_chain_nonce().await?; + self.inner.nonce.store(chain_nonce, Ordering::SeqCst); + Ok(chain_nonce) + } + + /// Record that `nonce` carried a successful broadcast, so the next submission moves on + /// to the following slot. + fn commit_nonce(&self, nonce: u64) { + self.inner.nonce.fetch_max(nonce + 1, Ordering::SeqCst); } /// Ratchet the in-memory counter up to the next slot the chain will accept. Never - /// decreases it, so a reservation another caller is still submitting cannot be handed out - /// again: that reservation has already pushed the counter past what the chain reports. + /// decreases it, so an endpoint reporting a stale pending count cannot wind the + /// counter back onto a slot a broadcast has already spent. async fn resync_nonce(&self) -> Result<(), ChainClientError> { let chain_nonce = self.fetch_chain_nonce().await?; self.inner.nonce.fetch_max(chain_nonce, Ordering::SeqCst); @@ -396,8 +393,8 @@ impl AlloyChainClient { } /// Sign and send a transaction, re-syncing the nonce from chain and retrying on nonce - /// errors. `submit_lock` spans the retry loop so concurrent reservations cannot - /// interleave with each other's submissions; released before receipt polling. + /// errors. `submit_lock` spans the retry loop so two submissions can never interleave; + /// released before receipt polling so confirmations pipeline concurrently. async fn sign_and_send( &self, tx: TransactionRequest, @@ -431,6 +428,7 @@ impl AlloyChainClient { match result { Ok(tx_hash) => { + self.commit_nonce(nonce); tracing::info!( agreement_id = %format_args!("0x{}", agreement_id.iter().map(|b| format!("{b:02x}")).collect::()), tx_hash = %tx_hash, @@ -1471,79 +1469,75 @@ mod tests { ); } - #[tokio::test] - async fn concurrent_callers_each_reserve_a_different_nonce() { - use std::collections::HashSet; - - let counter = Arc::new(AtomicU64::new(NONCE_UNINITIALIZED)); - let lock = Arc::new(Mutex::new(())); - let chain_pending = Arc::new(AtomicU64::new(100)); - let reserved: Arc>> = Arc::new(Mutex::new(HashSet::new())); - - const TASKS: usize = 50; - let mut handles = Vec::with_capacity(TASKS); - for i in 0..TASKS { - let counter = counter.clone(); - let lock = lock.clone(); - let chain_pending = chain_pending.clone(); - let reserved = reserved.clone(); - - handles.push(tokio::spawn(async move { - let _guard = lock.lock().await; - - // Mirror the entry shape of `next_nonce`/`resync_nonce`: the first caller - // initializes via fetch+CAS, every seventh ratchets via fetch_max(chain + 1) - // before reserving via fetch_add, and the rest reserve via fetch_add. - let nonce = if counter.load(Ordering::SeqCst) == NONCE_UNINITIALIZED { - let chain_nonce = chain_pending.load(Ordering::SeqCst); - match counter.compare_exchange( - NONCE_UNINITIALIZED, - chain_nonce + 1, - Ordering::SeqCst, - Ordering::SeqCst, - ) { - Ok(_) => chain_nonce, - Err(_) => counter.fetch_add(1, Ordering::SeqCst), - } - } else if i % 7 == 0 { - let chain_nonce = chain_pending.load(Ordering::SeqCst); - counter.fetch_max(chain_nonce + 1, Ordering::SeqCst); - counter.fetch_add(1, Ordering::SeqCst) - } else { - counter.fetch_add(1, Ordering::SeqCst) - }; - - // Simulate the time spent signing and submitting to the - // mempool while the lock is held; this is the window the - // bug exploited when the lock was missing. - tokio::time::sleep(Duration::from_micros(50)).await; - - // Simulate the chain accepting the tx into pending: the - // pending count cannot decrease, so use fetch_max. - let _ = chain_pending.fetch_update( - Ordering::SeqCst, - Ordering::SeqCst, - |cur| Some(cur.max(nonce + 1)), - ); + /// Answers nonce lookups with a fixed pending count, refuses the first send with a + /// server fault, and accepts every send after it: the shape of a submission failing + /// outright and the job being re-run. + struct FailsFirstSendResponder { + pending_nonce: u64, + sends: AtomicU64, + } - let mut reserved = reserved.lock().await; - assert!( - reserved.insert(nonce), - "nonce {nonce} reissued to a concurrent caller (counter rewound past in-flight reservation)" - ); - })); + impl Respond for FailsFirstSendResponder { + fn respond(&self, request: &Request) -> ResponseTemplate { + let body: serde_json::Value = + serde_json::from_slice(&request.body).expect("JSON-RPC request body"); + match body["method"].as_str().unwrap_or_default() { + "eth_getTransactionCount" => { + ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "jsonrpc": "2.0", + "id": body["id"], + "result": format!("{:#x}", self.pending_nonce), + })) + } + "eth_sendRawTransaction" if self.sends.fetch_add(1, Ordering::SeqCst) == 0 => { + ResponseTemplate::new(500).set_body_string("Temporary internal error") + } + "eth_sendRawTransaction" => { + ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "jsonrpc": "2.0", + "id": body["id"], + "result": format!("{:#x}", B256::repeat_byte(0xaa)), + })) + } + other => panic!("unexpected method {other}"), + } } + } - for h in handles { - h.await.expect("worker task panicked"); - } + /// A submission that never got a transaction out must leave its nonce for the next one. + /// Spending it anyway left a slot the chain kept waiting on: every later transaction + /// queued behind the empty slot, and nothing filled it short of a restart. + #[tokio::test] + async fn a_failed_submission_leaves_its_nonce_to_the_next() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with(FailsFirstSendResponder { + pending_nonce: 5, + sends: AtomicU64::new(0), + }) + .mount(&server) + .await; + let client = client_over(vec![server.uri().parse().expect("provider URL")]); + let tx = ready_to_send_tx(client.inner.signer.address()); - let reserved = reserved.lock().await; + client + .sign_and_send(tx.clone(), &[0u8; 16]) + .await + .expect_err("the only endpoint refused, so the submission fails"); + + let retry = client + .sign_and_send(tx.clone(), &[0u8; 16]) + .await + .expect("the endpoint accepts the re-run"); assert_eq!( - reserved.len(), - TASKS, - "expected {TASKS} unique nonces, got {}", - reserved.len() + retry.nonce, 5, + "the re-run must take the slot the failure never spent" ); + + let next = client + .sign_and_send(tx, &[0u8; 16]) + .await + .expect("a further submission succeeds"); + assert_eq!(next.nonce, 6, "a successful broadcast spends its slot"); } }