From 12fac99d800e3d7e7adebd87201f1292df1b35a7 Mon Sep 17 00:00:00 2001 From: Mohan <86064887+binarybaron@users.noreply.github.com> Date: Fri, 20 Jun 2025 11:43:03 +0200 Subject: [PATCH] feat(wallet): Cache fee estimations for up to 2 minutes (#411) * feat(wallet): Cache fee estimations for up to 2 minutes * remove complicated type alias * fmt, changelog entry --- CHANGELOG.md | 2 + swap/src/bitcoin/wallet.rs | 392 +++++++++++++++++++++++++++++++++++-- swap/src/cli/api.rs | 2 +- 3 files changed, 376 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 59cb78c0..c1dee8c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- ASB + GUI + CLI: We now cache fee estimates for the Bitcoin wallet for up to 2 minutes. This improves the speed of fee estimation and reduces the number of requests to the Electrum servers. + ## [2.3.0-beta.1] - 2025-06-19 - ASB + CLI + GUI: Introduce a load-balancing proxy for Monero RPC nodes that automatically discovers healthy nodes and routes requests to improve connection reliability. diff --git a/swap/src/bitcoin/wallet.rs b/swap/src/bitcoin/wallet.rs index f471b913..9d7517b9 100644 --- a/swap/src/bitcoin/wallet.rs +++ b/swap/src/bitcoin/wallet.rs @@ -43,6 +43,7 @@ use super::bitcoin_address::revalidate_network; use super::BlockHeight; use crate::bitcoin::electrum_balancer::ElectrumBalancer; use derive_builder::Builder; +use moka; /// We allow transaction fees of up to 20% of the transferred amount to ensure /// that lock transactions can always be published, even when fees are high. @@ -66,8 +67,10 @@ pub struct Wallet { persister: Arc>, /// The electrum client. electrum_client: Arc>, - /// The mempool client. - mempool_client: Arc>, + /// The cached fee estimator for the electrum client. + cached_electrum_fee_estimator: Arc>, + /// The cached fee estimator for the mempool client. + cached_mempool_fee_estimator: Arc>>, /// The network this wallet is on. network: Network, /// The number of confirmations (blocks) we require for a transaction @@ -83,6 +86,7 @@ pub struct Wallet { } /// This is our wrapper around a bdk electrum client. +#[derive(Clone)] pub struct Client { /// The underlying electrum balancer for load balancing across multiple servers. inner: Arc, @@ -130,7 +134,7 @@ impl WalletBuilder { /// Asynchronously builds the `Wallet` using the configured parameters. /// This method contains the core logic for wallet initialization, including /// database setup, key derivation, and potential migration from older wallet formats. - pub async fn build(self) -> Result> { + pub async fn build(self) -> Result> { let config = self .validate_config() .map_err(|e| anyhow!("Builder validation failed: {e}"))?; @@ -293,6 +297,83 @@ pub trait EstimateFeeRate { fn min_relay_fee(&self) -> impl std::future::Future> + Send; } +/// A caching wrapper around EstimateFeeRate implementations. +/// +/// Uses Moka cache with TTL (Time To Live) expiration for both fee rate estimates +/// and minimum relay fees to reduce the frequency of network calls to Electrum and mempool.space APIs. +#[derive(Clone)] +pub struct CachedFeeEstimator { + inner: T, + fee_cache: Arc>, + min_relay_cache: Arc>, +} + +impl CachedFeeEstimator { + /// Cache duration for fee estimates (2 minutes) + const CACHE_DURATION: Duration = Duration::from_secs(120); + /// Maximum number of cached fee rate entries (different target blocks) + const MAX_CACHE_SIZE: u64 = 10; + + /// Create a new caching wrapper around an EstimateFeeRate implementation. + pub fn new(inner: T) -> Self { + Self { + inner, + fee_cache: Arc::new( + moka::future::Cache::builder() + .max_capacity(Self::MAX_CACHE_SIZE) + .time_to_live(Self::CACHE_DURATION) + .build(), + ), + min_relay_cache: Arc::new( + moka::future::Cache::builder() + .max_capacity(1) // Only one min relay fee value + .time_to_live(Self::CACHE_DURATION) + .build(), + ), + } + } +} + +impl EstimateFeeRate for CachedFeeEstimator { + async fn estimate_feerate(&self, target_block: u32) -> Result { + // Check cache first + if let Some(cached_rate) = self.fee_cache.get(&target_block).await { + return Ok(cached_rate); + } + + // If not in cache, fetch from underlying estimator + let fee_rate = self.inner.estimate_feerate(target_block).await?; + + // Store in cache + self.fee_cache.insert(target_block, fee_rate).await; + + Ok(fee_rate) + } + + async fn min_relay_fee(&self) -> Result { + // Check cache first + if let Some(cached_rate) = self.min_relay_cache.get(&()).await { + return Ok(cached_rate); + } + + // If not in cache, fetch from underlying estimator + let min_relay_fee = self.inner.min_relay_fee().await?; + + // Store in cache + self.min_relay_cache.insert((), min_relay_fee).await; + + Ok(min_relay_fee) + } +} + +impl std::ops::Deref for CachedFeeEstimator { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.inner + } +} + impl Wallet { /// If this many consequent addresses are unused, we stop the full scan. /// On old wallets we used to generate a ton of unused addresses @@ -362,7 +443,7 @@ impl Wallet { sync_interval: Duration, env_config: crate::env::Config, tauri_handle: Option, - ) -> Result> { + ) -> Result> { // Construct the private key, directory and wallet file for the new (>= 1.0.0) bdk wallet let xprivkey = seed.derive_extended_private_key(env_config.bitcoin_network)?; let wallet_dir = data_dir @@ -425,7 +506,7 @@ impl Wallet { target_block: u32, sync_interval: Duration, tauri_handle: Option, - ) -> Result> { + ) -> Result> { Self::create_new( seed.derive_extended_private_key(network)?, network, @@ -458,7 +539,7 @@ impl Wallet { old_wallet: Option, tauri_handle: Option, use_mempool_space_fee_estimation: bool, - ) -> Result> + ) -> Result> where Persister: WalletPersister + Sized, ::Error: std::error::Error + Send + Sync + 'static, @@ -550,10 +631,16 @@ impl Wallet { None }; + // Create cached fee estimators + let cached_electrum_fee_estimator = Arc::new(CachedFeeEstimator::new(client.clone())); + let cached_mempool_fee_estimator = + Arc::new(mempool_client.clone().map(CachedFeeEstimator::new)); + Ok(Wallet { wallet: wallet.into_arc_mutex_async(), electrum_client: client.into_arc_mutex_async(), - mempool_client: Arc::new(mempool_client), + cached_electrum_fee_estimator, + cached_mempool_fee_estimator, persister: persister.into_arc_mutex_async(), tauri_handle, network, @@ -573,7 +660,7 @@ impl Wallet { target_block: u32, tauri_handle: Option, use_mempool_space_fee_estimation: bool, - ) -> Result> + ) -> Result> where Persister: WalletPersister + Sized, ::Error: std::error::Error + Send + Sync + 'static, @@ -596,19 +683,23 @@ impl Wallet { .context("Failed to open database")? .context("No wallet found in database")?; - // Create the mempool client - let mempool_client = if use_mempool_space_fee_estimation { + // Create the mempool client with caching + let cached_mempool_fee_estimator = if use_mempool_space_fee_estimation { mempool_client::MempoolClient::new(network).inspect_err(|e| { tracing::warn!("Failed to create mempool client: {:?}. We will only use the Electrum server for fee estimation.", e); - }).ok() + }).ok().map(CachedFeeEstimator::new) } else { None }; + // Wrap the electrum client with caching + let cached_electrum_fee_estimator = Arc::new(CachedFeeEstimator::new(client.clone())); + let wallet = Wallet { wallet: wallet.into_arc_mutex_async(), electrum_client: client.into_arc_mutex_async(), - mempool_client: Arc::new(mempool_client), + cached_electrum_fee_estimator, + cached_mempool_fee_estimator: Arc::new(cached_mempool_fee_estimator), persister: persister.into_arc_mutex_async(), tauri_handle, network, @@ -1095,10 +1186,11 @@ where /// If either of the clients fail but the other is successful, we use the successful one. /// If both clients fail, we return an error async fn combined_fee_rate(&self) -> Result { - let electrum_client = self.electrum_client.lock().await; - let electrum_future = electrum_client.estimate_feerate(self.target_block); + let electrum_future = self + .cached_electrum_fee_estimator + .estimate_feerate(self.target_block); let mempool_future = async { - match self.mempool_client.as_ref() { + match self.cached_mempool_fee_estimator.as_ref() { Some(mempool_client) => mempool_client .estimate_feerate(self.target_block) .await @@ -1174,10 +1266,9 @@ where /// /// Only fails if both sources fail. Always chooses the higher value. async fn combined_min_relay_fee(&self) -> Result { - let electrum_client = self.electrum_client.lock().await; - let electrum_future = electrum_client.min_relay_fee(); + let electrum_future = self.cached_electrum_fee_estimator.min_relay_fee(); let mempool_future = async { - match self.mempool_client.as_ref() { + match self.cached_mempool_fee_estimator.as_ref() { Some(mempool_client) => mempool_client.min_relay_fee().await.map(Some), None => Ok(None), } @@ -2455,6 +2546,7 @@ mod mempool_client { /// A client for the mempool.space API. /// /// This client is used to estimate the fee rate for a transaction. + #[derive(Clone)] pub struct MempoolClient { client: reqwest::Client, base_url: String, @@ -2751,6 +2843,7 @@ impl IntoArcMutex for T { } #[cfg(test)] +#[derive(Clone)] pub struct StaticFeeRate { fee_rate: FeeRate, min_relay_fee: bitcoin::Amount, @@ -2856,10 +2949,13 @@ impl TestWalletBuilder { bitcoin::Amount::from_sat(self.min_relay_sats_per_vb), ); + let cached_electrum_fee_estimator = Arc::new(CachedFeeEstimator::new(client.clone())); + let wallet = Wallet { wallet: bdk_core_wallet.into_arc_mutex_async(), electrum_client: client.into_arc_mutex_async(), - mempool_client: Arc::new(None), // We don't use mempool client in tests + cached_electrum_fee_estimator, + cached_mempool_fee_estimator: Arc::new(None), // We don't use mempool client in tests persister: persister.into_arc_mutex_async(), tauri_handle: None, network: Network::Regtest, @@ -3309,6 +3405,264 @@ TRACE swap::bitcoin::wallet: Bitcoin transaction status changed txid=00000000000 }); } } + + mod cached_fee_estimator_tests { + use super::*; + use std::sync::atomic::{AtomicU32, Ordering}; + use std::sync::Arc; + use tokio::time::{sleep, Duration}; + + /// Mock fee estimator that tracks how many times methods are called + #[derive(Clone)] + struct MockFeeEstimator { + estimate_calls: Arc, + min_relay_calls: Arc, + fee_rate: FeeRate, + min_relay_fee: FeeRate, + delay: Duration, + } + + impl MockFeeEstimator { + fn new(fee_rate: FeeRate, min_relay_fee: FeeRate) -> Self { + Self { + estimate_calls: Arc::new(AtomicU32::new(0)), + min_relay_calls: Arc::new(AtomicU32::new(0)), + fee_rate, + min_relay_fee, + delay: Duration::from_millis(0), + } + } + + fn with_delay(mut self, delay: Duration) -> Self { + self.delay = delay; + self + } + + fn estimate_call_count(&self) -> u32 { + self.estimate_calls.load(Ordering::SeqCst) + } + + fn min_relay_call_count(&self) -> u32 { + self.min_relay_calls.load(Ordering::SeqCst) + } + } + + impl EstimateFeeRate for MockFeeEstimator { + async fn estimate_feerate(&self, _target_block: u32) -> Result { + self.estimate_calls.fetch_add(1, Ordering::SeqCst); + if !self.delay.is_zero() { + sleep(self.delay).await; + } + Ok(self.fee_rate) + } + + async fn min_relay_fee(&self) -> Result { + self.min_relay_calls.fetch_add(1, Ordering::SeqCst); + if !self.delay.is_zero() { + sleep(self.delay).await; + } + Ok(self.min_relay_fee) + } + } + + #[tokio::test] + async fn caches_fee_rate_estimates() { + let mock = MockFeeEstimator::new( + FeeRate::from_sat_per_vb(50).unwrap(), + FeeRate::from_sat_per_vb(1).unwrap(), + ); + let cached = CachedFeeEstimator::new(mock.clone()); + + // First call should hit the underlying estimator + let fee1 = cached.estimate_feerate(6).await.unwrap(); + assert_eq!(fee1, FeeRate::from_sat_per_vb(50).unwrap()); + assert_eq!(mock.estimate_call_count(), 1); + + // Second call with same target should use cache + let fee2 = cached.estimate_feerate(6).await.unwrap(); + assert_eq!(fee2, FeeRate::from_sat_per_vb(50).unwrap()); + assert_eq!(mock.estimate_call_count(), 1); // Still 1, not 2 + + // Different target should hit the underlying estimator again + let fee3 = cached.estimate_feerate(12).await.unwrap(); + assert_eq!(fee3, FeeRate::from_sat_per_vb(50).unwrap()); + assert_eq!(mock.estimate_call_count(), 2); + } + + #[tokio::test] + async fn caches_min_relay_fee() { + let mock = MockFeeEstimator::new( + FeeRate::from_sat_per_vb(50).unwrap(), + FeeRate::from_sat_per_vb(1).unwrap(), + ); + let cached = CachedFeeEstimator::new(mock.clone()); + + // First call should hit the underlying estimator + let fee1 = cached.min_relay_fee().await.unwrap(); + assert_eq!(fee1, FeeRate::from_sat_per_vb(1).unwrap()); + assert_eq!(mock.min_relay_call_count(), 1); + + // Second call should use cache + let fee2 = cached.min_relay_fee().await.unwrap(); + assert_eq!(fee2, FeeRate::from_sat_per_vb(1).unwrap()); + assert_eq!(mock.min_relay_call_count(), 1); // Still 1, not 2 + } + + #[tokio::test] + async fn concurrent_requests_dont_duplicate_calls() { + let mock = MockFeeEstimator::new( + FeeRate::from_sat_per_vb(25).unwrap(), + FeeRate::from_sat_per_vb(1).unwrap(), + ) + .with_delay(Duration::from_millis(50)); // Add delay to simulate network call + + let cached = CachedFeeEstimator::new(mock.clone()); + + // First, make one call to populate the cache + let _initial = cached.estimate_feerate(6).await.unwrap(); + assert_eq!(mock.estimate_call_count(), 1); + + // Now make multiple concurrent requests for the same target + // These should all hit the cache + let handles: Vec<_> = (0..5) + .map(|_| { + let cached = cached.clone(); + tokio::spawn(async move { cached.estimate_feerate(6).await }) + }) + .collect(); + + // Wait for all requests to complete + let results: Vec<_> = futures::future::join_all(handles).await; + + // All should succeed with the same value + for result in results { + let fee = result.unwrap().unwrap(); + assert_eq!(fee, FeeRate::from_sat_per_vb(25).unwrap()); + } + + // The underlying estimator should still only have been called once + // since all subsequent requests should hit the cache + assert_eq!( + mock.estimate_call_count(), + 1, + "Expected exactly 1 call, got {}", + mock.estimate_call_count() + ); + } + + #[tokio::test] + async fn different_target_blocks_cached_separately() { + let mock = MockFeeEstimator::new( + FeeRate::from_sat_per_vb(30).unwrap(), + FeeRate::from_sat_per_vb(1).unwrap(), + ); + let cached = CachedFeeEstimator::new(mock.clone()); + + // Request different target blocks + let _fee1 = cached.estimate_feerate(1).await.unwrap(); + let _fee2 = cached.estimate_feerate(6).await.unwrap(); + let _fee3 = cached.estimate_feerate(12).await.unwrap(); + + assert_eq!(mock.estimate_call_count(), 3); + + // Request same targets again - should use cache + let _fee1_cached = cached.estimate_feerate(1).await.unwrap(); + let _fee2_cached = cached.estimate_feerate(6).await.unwrap(); + let _fee3_cached = cached.estimate_feerate(12).await.unwrap(); + + assert_eq!(mock.estimate_call_count(), 3); // Still 3, no additional calls + } + + #[tokio::test] + async fn cache_respects_ttl() { + let mock = MockFeeEstimator::new( + FeeRate::from_sat_per_vb(40).unwrap(), + FeeRate::from_sat_per_vb(1).unwrap(), + ); + let cached = CachedFeeEstimator::new(mock.clone()); + + // First call + let _fee1 = cached.estimate_feerate(6).await.unwrap(); + assert_eq!(mock.estimate_call_count(), 1); + + // Wait for cache to expire (2 minutes + small buffer) + // Note: In a real test environment, you might want to use a shorter TTL + // or mock the time. For now, we'll just verify the cache works within TTL. + + // Immediate second call should use cache + let _fee2 = cached.estimate_feerate(6).await.unwrap(); + assert_eq!(mock.estimate_call_count(), 1); + } + + #[tokio::test] + async fn error_propagation() { + #[derive(Clone)] + struct FailingEstimator; + + impl EstimateFeeRate for FailingEstimator { + async fn estimate_feerate(&self, _target_block: u32) -> Result { + Err(anyhow::anyhow!("Network error")) + } + + async fn min_relay_fee(&self) -> Result { + Err(anyhow::anyhow!("Network error")) + } + } + + let cached = CachedFeeEstimator::new(FailingEstimator); + + // Errors should be propagated, not cached + let result1 = cached.estimate_feerate(6).await; + assert!(result1.is_err()); + assert!(result1.unwrap_err().to_string().contains("Network error")); + + let result2 = cached.min_relay_fee().await; + assert!(result2.is_err()); + assert!(result2.unwrap_err().to_string().contains("Network error")); + } + + #[tokio::test] + async fn cache_capacity_limits() { + let mock = MockFeeEstimator::new( + FeeRate::from_sat_per_vb(35).unwrap(), + FeeRate::from_sat_per_vb(1).unwrap(), + ); + let cached = CachedFeeEstimator::new(mock.clone()); + + // Fill cache beyond capacity (MAX_CACHE_SIZE = 10) + for target in 1..=15 { + let _fee = cached.estimate_feerate(target).await.unwrap(); + } + + assert_eq!(mock.estimate_call_count(), 15); + + // Request some of the earlier targets - some might have been evicted + // Due to LRU eviction, the earliest entries might be gone + let _fee = cached.estimate_feerate(1).await.unwrap(); + + // The exact behavior depends on Moka's eviction policy, + // but we should see that the cache is working within its limits + assert!(mock.estimate_call_count() >= 15); + } + + #[tokio::test] + async fn clone_shares_cache() { + let mock = MockFeeEstimator::new( + FeeRate::from_sat_per_vb(45).unwrap(), + FeeRate::from_sat_per_vb(1).unwrap(), + ); + let cached1 = CachedFeeEstimator::new(mock.clone()); + let cached2 = cached1.clone(); + + // First estimator makes a call + let _fee1 = cached1.estimate_feerate(6).await.unwrap(); + assert_eq!(mock.estimate_call_count(), 1); + + // Second estimator should use the shared cache + let _fee2 = cached2.estimate_feerate(6).await.unwrap(); + assert_eq!(mock.estimate_call_count(), 1); // Still 1, cache was shared + } + } } #[derive(Clone)] diff --git a/swap/src/cli/api.rs b/swap/src/cli/api.rs index 8740611f..849f99ee 100644 --- a/swap/src/cli/api.rs +++ b/swap/src/cli/api.rs @@ -556,7 +556,7 @@ async fn init_bitcoin_wallet( env_config: EnvConfig, bitcoin_target_block: u16, tauri_handle_option: Option, -) -> Result { +) -> Result> { let mut builder = bitcoin::wallet::WalletBuilder::default() .seed(seed.clone()) .network(env_config.bitcoin_network)