From 7202c83223f97937326d896719f835866ff22510 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Fri, 30 Aug 2024 08:34:04 +0200 Subject: [PATCH 1/9] Make `Wallet` and `WalletKeysManager` `pub(crate)` --- src/wallet.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet.rs b/src/wallet.rs index 0da3f6db8..7a77a4ece 100644 --- a/src/wallet.rs +++ b/src/wallet.rs @@ -43,7 +43,7 @@ enum WalletSyncStatus { InProgress { subscribers: tokio::sync::broadcast::Sender> }, } -pub struct Wallet +pub(crate) struct Wallet where D: BatchDatabase, B::Target: BroadcasterInterface, @@ -485,7 +485,7 @@ where /// Similar to [`KeysManager`], but overrides the destination and shutdown scripts so they are /// directly spendable by the BDK wallet. -pub struct WalletKeysManager +pub(crate) struct WalletKeysManager where D: BatchDatabase, B::Target: BroadcasterInterface, From bf4655af1d5c910b9ce188c7234cdbe63e69bc65 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Fri, 30 Aug 2024 08:33:52 +0200 Subject: [PATCH 2/9] Refactor `FeeEstimator` to introduce local target variants .. previously we used LDK's `FeeEstimator` and `ConfirmationTarget` and ~misused some of the latter's variants for our non-Lightning operations. Here, we introduce our own `FeeEstimator` and `ConfirmationTarget` allowing to add specific variants for `ChannelFunding` and `OnchainPayment`s, for example. --- src/event.rs | 4 +- src/fee_estimator.rs | 111 +++++++++++++++++++++++++++++++------------ src/wallet.rs | 16 +++---- 3 files changed, 88 insertions(+), 43 deletions(-) diff --git a/src/event.rs b/src/event.rs index c4c5034ff..d76f0b05e 100644 --- a/src/event.rs +++ b/src/event.rs @@ -6,6 +6,7 @@ use crate::{ }; use crate::connection::ConnectionManager; +use crate::fee_estimator::ConfirmationTarget; use crate::payment::store::{ PaymentDetails, PaymentDetailsUpdate, PaymentDirection, PaymentKind, PaymentStatus, @@ -18,7 +19,6 @@ use crate::io::{ }; use crate::logger::{log_debug, log_error, log_info, Logger}; -use lightning::chain::chaininterface::ConfirmationTarget; use lightning::events::bump_transaction::BumpTransactionEvent; use lightning::events::{ClosureReason, PaymentPurpose}; use lightning::events::{Event as LdkEvent, PaymentFailureReason}; @@ -398,7 +398,7 @@ where } => { // Construct the raw transaction with the output that is paid the amount of the // channel. - let confirmation_target = ConfirmationTarget::NonAnchorChannelFee; + let confirmation_target = ConfirmationTarget::ChannelFunding; // We set nLockTime to the current height to discourage fee sniping. let cur_height = self.channel_manager.current_best_block().height; diff --git a/src/fee_estimator.rs b/src/fee_estimator.rs index 329cc6e42..78dfe0140 100644 --- a/src/fee_estimator.rs +++ b/src/fee_estimator.rs @@ -2,9 +2,9 @@ use crate::config::FEE_RATE_CACHE_UPDATE_TIMEOUT_SECS; use crate::logger::{log_error, log_trace, Logger}; use crate::{Config, Error}; -use lightning::chain::chaininterface::{ - ConfirmationTarget, FeeEstimator, FEERATE_FLOOR_SATS_PER_KW, -}; +use lightning::chain::chaininterface::ConfirmationTarget as LdkConfirmationTarget; +use lightning::chain::chaininterface::FeeEstimator as LdkFeeEstimator; +use lightning::chain::chaininterface::FEERATE_FLOOR_SATS_PER_KW; use bdk::FeeRate; use esplora_client::AsyncClient as EsploraClient; @@ -17,6 +17,26 @@ use std::ops::Deref; use std::sync::{Arc, RwLock}; use std::time::Duration; +#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] +pub(crate) enum ConfirmationTarget { + /// The default target for onchain payments. + OnchainPayment, + /// The target which we use for funding transactions. + ChannelFunding, + /// Targets used by LDK. + Lightning(LdkConfirmationTarget), +} + +pub(crate) trait FeeEstimator { + fn estimate_fee_rate(&self, confirmation_target: ConfirmationTarget) -> FeeRate; +} + +impl From for ConfirmationTarget { + fn from(value: LdkConfirmationTarget) -> Self { + Self::Lightning(value) + } +} + pub(crate) struct OnchainFeeEstimator where L::Target: Logger, @@ -61,23 +81,30 @@ where } let confirmation_targets = vec![ - ConfirmationTarget::OnChainSweep, - ConfirmationTarget::MinAllowedAnchorChannelRemoteFee, - ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee, - ConfirmationTarget::AnchorChannelFee, - ConfirmationTarget::NonAnchorChannelFee, - ConfirmationTarget::ChannelCloseMinimum, - ConfirmationTarget::OutputSpendingFee, + ConfirmationTarget::OnchainPayment, + ConfirmationTarget::ChannelFunding, + LdkConfirmationTarget::OnChainSweep.into(), + LdkConfirmationTarget::MinAllowedAnchorChannelRemoteFee.into(), + LdkConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee.into(), + LdkConfirmationTarget::AnchorChannelFee.into(), + LdkConfirmationTarget::NonAnchorChannelFee.into(), + LdkConfirmationTarget::ChannelCloseMinimum.into(), + LdkConfirmationTarget::OutputSpendingFee.into(), ]; + for target in confirmation_targets { let num_blocks = match target { - ConfirmationTarget::OnChainSweep => 6, - ConfirmationTarget::MinAllowedAnchorChannelRemoteFee => 1008, - ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee => 144, - ConfirmationTarget::AnchorChannelFee => 1008, - ConfirmationTarget::NonAnchorChannelFee => 12, - ConfirmationTarget::ChannelCloseMinimum => 144, - ConfirmationTarget::OutputSpendingFee => 12, + ConfirmationTarget::OnchainPayment => 6, + ConfirmationTarget::ChannelFunding => 12, + ConfirmationTarget::Lightning(ldk_target) => match ldk_target { + LdkConfirmationTarget::OnChainSweep => 6, + LdkConfirmationTarget::MinAllowedAnchorChannelRemoteFee => 1008, + LdkConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee => 144, + LdkConfirmationTarget::AnchorChannelFee => 1008, + LdkConfirmationTarget::NonAnchorChannelFee => 12, + LdkConfirmationTarget::ChannelCloseMinimum => 144, + LdkConfirmationTarget::OutputSpendingFee => 12, + }, }; let converted_estimates = @@ -96,7 +123,9 @@ where // LDK 0.0.118 introduced changes to the `ConfirmationTarget` semantics that // require some post-estimation adjustments to the fee rates, which we do here. let adjusted_fee_rate = match target { - ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee => { + ConfirmationTarget::Lightning( + LdkConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee, + ) => { let slightly_less_than_background = fee_rate.fee_wu(Weight::from_wu(1000)) - 250; FeeRate::from_sat_per_kwu(slightly_less_than_background as f32) @@ -115,33 +144,53 @@ where } Ok(()) } +} - pub(crate) fn estimate_fee_rate(&self, confirmation_target: ConfirmationTarget) -> FeeRate { +impl FeeEstimator for OnchainFeeEstimator +where + L::Target: Logger, +{ + fn estimate_fee_rate(&self, confirmation_target: ConfirmationTarget) -> FeeRate { let locked_fee_rate_cache = self.fee_rate_cache.read().unwrap(); let fallback_sats_kwu = match confirmation_target { - ConfirmationTarget::OnChainSweep => 5000, - ConfirmationTarget::MinAllowedAnchorChannelRemoteFee => FEERATE_FLOOR_SATS_PER_KW, - ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee => FEERATE_FLOOR_SATS_PER_KW, - ConfirmationTarget::AnchorChannelFee => 500, - ConfirmationTarget::NonAnchorChannelFee => 1000, - ConfirmationTarget::ChannelCloseMinimum => 500, - ConfirmationTarget::OutputSpendingFee => 1000, + ConfirmationTarget::OnchainPayment => 5000, + ConfirmationTarget::ChannelFunding => 1000, + ConfirmationTarget::Lightning(ldk_target) => match ldk_target { + LdkConfirmationTarget::OnChainSweep => 5000, + LdkConfirmationTarget::MinAllowedAnchorChannelRemoteFee => { + FEERATE_FLOOR_SATS_PER_KW + }, + LdkConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee => { + FEERATE_FLOOR_SATS_PER_KW + }, + LdkConfirmationTarget::AnchorChannelFee => 500, + LdkConfirmationTarget::NonAnchorChannelFee => 1000, + LdkConfirmationTarget::ChannelCloseMinimum => 500, + LdkConfirmationTarget::OutputSpendingFee => 1000, + }, }; // We'll fall back on this, if we really don't have any other information. let fallback_rate = FeeRate::from_sat_per_kwu(fallback_sats_kwu as f32); - *locked_fee_rate_cache.get(&confirmation_target).unwrap_or(&fallback_rate) + let estimate = *locked_fee_rate_cache.get(&confirmation_target).unwrap_or(&fallback_rate); + + // Currently we assume every transaction needs to at least be relayable, which is why we + // enforce a lower bound of `FEERATE_FLOOR_SATS_PER_KW`. + let weight_units = Weight::from_wu(1000); + FeeRate::from_wu( + estimate.fee_wu(weight_units).max(FEERATE_FLOOR_SATS_PER_KW as u64), + weight_units, + ) } } -impl FeeEstimator for OnchainFeeEstimator +impl LdkFeeEstimator for OnchainFeeEstimator where L::Target: Logger, { - fn get_est_sat_per_1000_weight(&self, confirmation_target: ConfirmationTarget) -> u32 { - (self.estimate_fee_rate(confirmation_target).fee_wu(Weight::from_wu(1000)) as u32) - .max(FEERATE_FLOOR_SATS_PER_KW) + fn get_est_sat_per_1000_weight(&self, confirmation_target: LdkConfirmationTarget) -> u32 { + self.estimate_fee_rate(confirmation_target.into()).fee_wu(Weight::from_wu(1000)) as u32 } } diff --git a/src/wallet.rs b/src/wallet.rs index 7a77a4ece..996ec57da 100644 --- a/src/wallet.rs +++ b/src/wallet.rs @@ -1,9 +1,10 @@ use crate::logger::{log_error, log_info, log_trace, Logger}; use crate::config::BDK_WALLET_SYNC_TIMEOUT_SECS; +use crate::fee_estimator::{ConfirmationTarget, FeeEstimator}; use crate::Error; -use lightning::chain::chaininterface::{BroadcasterInterface, ConfirmationTarget, FeeEstimator}; +use lightning::chain::chaininterface::BroadcasterInterface; use lightning::events::bump_transaction::{Utxo, WalletSource}; use lightning::ln::msgs::{DecodeError, UnsignedGossipMessage}; @@ -18,8 +19,7 @@ use lightning::util::message_signing; use bdk::blockchain::EsploraBlockchain; use bdk::database::BatchDatabase; use bdk::wallet::AddressIndex; -use bdk::{Balance, FeeRate}; -use bdk::{SignOptions, SyncOptions}; +use bdk::{Balance, SignOptions, SyncOptions}; use bitcoin::address::{Payload, WitnessVersion}; use bitcoin::bech32::u5; @@ -153,9 +153,7 @@ where &self, output_script: ScriptBuf, value_sats: u64, confirmation_target: ConfirmationTarget, locktime: LockTime, ) -> Result { - let fee_rate = FeeRate::from_sat_per_kwu( - self.fee_estimator.get_est_sat_per_1000_weight(confirmation_target) as f32, - ); + let fee_rate = self.fee_estimator.estimate_fee_rate(confirmation_target); let locked_wallet = self.inner.lock().unwrap(); let mut tx_builder = locked_wallet.build_tx(); @@ -240,10 +238,8 @@ where pub(crate) fn send_to_address( &self, address: &bitcoin::Address, amount_msat_or_drain: Option, ) -> Result { - let confirmation_target = ConfirmationTarget::OutputSpendingFee; - let fee_rate = FeeRate::from_sat_per_kwu( - self.fee_estimator.get_est_sat_per_1000_weight(confirmation_target) as f32, - ); + let confirmation_target = ConfirmationTarget::OnchainPayment; + let fee_rate = self.fee_estimator.estimate_fee_rate(confirmation_target); let tx = { let locked_wallet = self.inner.lock().unwrap(); From 9b1ea5a4b78df4786d7af48ca34dada921860e0a Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Fri, 30 Aug 2024 08:37:54 +0200 Subject: [PATCH 3/9] f `s/which we use/used/` --- src/fee_estimator.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fee_estimator.rs b/src/fee_estimator.rs index 78dfe0140..b023ae964 100644 --- a/src/fee_estimator.rs +++ b/src/fee_estimator.rs @@ -21,7 +21,7 @@ use std::time::Duration; pub(crate) enum ConfirmationTarget { /// The default target for onchain payments. OnchainPayment, - /// The target which we use for funding transactions. + /// The target used for funding transactions. ChannelFunding, /// Targets used by LDK. Lightning(LdkConfirmationTarget), From 01c499e45f5a7d62f9a329585d5402ada2afda81 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 29 Aug 2024 11:15:59 +0200 Subject: [PATCH 4/9] Log HTTP 400 at `TRACE` instead of ignoring it .. while it's most often bitcoind already knowing about a transaction already, the error sometimes holds additional information (e.g., not meeting the mempool min). --- src/tx_broadcaster.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/tx_broadcaster.rs b/src/tx_broadcaster.rs index 4492bcfc6..2db621ed1 100644 --- a/src/tx_broadcaster.rs +++ b/src/tx_broadcaster.rs @@ -56,11 +56,16 @@ where Err(e) => match e { esplora_client::Error::Reqwest(err) => { if err.status() == StatusCode::from_u16(400).ok() { - // Ignore 400, as this just means bitcoind already knows the + // Log 400 at lesser level, as this often just means bitcoind already knows the // transaction. // FIXME: We can further differentiate here based on the error // message which will be available with rust-esplora-client 0.7 and // later. + log_trace!( + self.logger, + "Failed to broadcast due to HTTP connection error: {}", + err + ); } else { log_error!( self.logger, From 02236984c71eccef0983a44103b8654ab5a2884d Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Fri, 16 Aug 2024 16:35:05 +0200 Subject: [PATCH 5/9] Allow honoring reserve in `send_all_to_address` Previously, `OnchainPayment::send_all_to_address` could only be used to fully drain the onchain wallet, i.e., would not retain any reserves. Here, we try to introduce a `retain_reserves` bool that allows users to send all funds while honoring the configured on-chain reserves. While we're at it, we move the reserve checks for `send_to_address` also to the internal wallet's method, which makes the checks more accurate as they now are checked against the final transaction value, including transaction fees. --- bindings/ldk_node.udl | 2 +- src/error.rs | 1 + src/payment/onchain.rs | 41 ++++---- src/wallet.rs | 160 +++++++++++++++++++++++++------- tests/integration_tests_rust.rs | 2 +- 5 files changed, 153 insertions(+), 53 deletions(-) diff --git a/bindings/ldk_node.udl b/bindings/ldk_node.udl index 1c9497264..8bf6c9591 100644 --- a/bindings/ldk_node.udl +++ b/bindings/ldk_node.udl @@ -146,7 +146,7 @@ interface OnchainPayment { [Throws=NodeError] Txid send_to_address([ByRef]Address address, u64 amount_sats); [Throws=NodeError] - Txid send_all_to_address([ByRef]Address address); + Txid send_all_to_address([ByRef]Address address, boolean retain_reserve); }; interface UnifiedQrPayment { diff --git a/src/error.rs b/src/error.rs index deaf6db31..94dbbc48b 100644 --- a/src/error.rs +++ b/src/error.rs @@ -181,6 +181,7 @@ impl From for Error { fn from(e: bdk::Error) -> Self { match e { bdk::Error::Signer(_) => Self::OnchainTxSigningFailed, + bdk::Error::InsufficientFunds { .. } => Self::InsufficientFunds, _ => Self::WalletOperationFailed, } } diff --git a/src/payment/onchain.rs b/src/payment/onchain.rs index 5c1365de3..ca09c85b8 100644 --- a/src/payment/onchain.rs +++ b/src/payment/onchain.rs @@ -2,8 +2,9 @@ use crate::config::Config; use crate::error::Error; -use crate::logger::{log_error, log_info, FilesystemLogger, Logger}; +use crate::logger::{log_info, FilesystemLogger, Logger}; use crate::types::{ChannelManager, Wallet}; +use crate::wallet::OnchainSendType; use bitcoin::{Address, Txid}; @@ -53,33 +54,39 @@ impl OnchainPayment { let cur_anchor_reserve_sats = crate::total_anchor_channels_reserve_sats(&self.channel_manager, &self.config); - let spendable_amount_sats = - self.wallet.get_spendable_amount_sats(cur_anchor_reserve_sats).unwrap_or(0); - - if spendable_amount_sats < amount_sats { - log_error!(self.logger, - "Unable to send payment due to insufficient funds. Available: {}sats, Required: {}sats", - spendable_amount_sats, amount_sats - ); - return Err(Error::InsufficientFunds); - } - self.wallet.send_to_address(address, Some(amount_sats)) + let send_amount = + OnchainSendType::SendRetainingReserve { amount_sats, cur_anchor_reserve_sats }; + self.wallet.send_to_address(address, send_amount) } - /// Send an on-chain payment to the given address, draining all the available funds. + /// Send an on-chain payment to the given address, draining the available funds. /// /// This is useful if you have closed all channels and want to migrate funds to another /// on-chain wallet. /// - /// Please note that this will **not** retain any on-chain reserves, which might be potentially + /// Please note that if `retain_reserves` is set to `false` this will **not** retain any on-chain reserves, which might be potentially /// dangerous if you have open Anchor channels for which you can't trust the counterparty to - /// spend the Anchor output after channel closure. - pub fn send_all_to_address(&self, address: &bitcoin::Address) -> Result { + /// spend the Anchor output after channel closure. If `retain_reserves` is set to `true`, this + /// will try to send all spendable onchain funds, i.e., + /// [`BalanceDetails::spendable_onchain_balance_sats`]. + /// + /// [`BalanceDetails::spendable_onchain_balance_sats`]: crate::balance::BalanceDetails::spendable_onchain_balance_sats + pub fn send_all_to_address( + &self, address: &bitcoin::Address, retain_reserves: bool, + ) -> Result { let rt_lock = self.runtime.read().unwrap(); if rt_lock.is_none() { return Err(Error::NotRunning); } - self.wallet.send_to_address(address, None) + let send_amount = if retain_reserves { + let cur_anchor_reserve_sats = + crate::total_anchor_channels_reserve_sats(&self.channel_manager, &self.config); + OnchainSendType::SendAllRetainingReserve { cur_anchor_reserve_sats } + } else { + OnchainSendType::SendAllDrainingReserve + }; + + self.wallet.send_to_address(address, send_amount) } } diff --git a/src/wallet.rs b/src/wallet.rs index 996ec57da..ae2c2d635 100644 --- a/src/wallet.rs +++ b/src/wallet.rs @@ -43,6 +43,12 @@ enum WalletSyncStatus { InProgress { subscribers: tokio::sync::broadcast::Sender> }, } +pub(crate) enum OnchainSendType { + SendRetainingReserve { amount_sats: u64, cur_anchor_reserve_sats: u64 }, + SendAllRetainingReserve { cur_anchor_reserve_sats: u64 }, + SendAllDrainingReserve, +} + pub(crate) struct Wallet where D: BatchDatabase, @@ -231,12 +237,8 @@ where self.get_balances(total_anchor_channels_reserve_sats).map(|(_, s)| s) } - /// Send funds to the given address. - /// - /// If `amount_msat_or_drain` is `None` the wallet will be drained, i.e., all available funds will be - /// spent. pub(crate) fn send_to_address( - &self, address: &bitcoin::Address, amount_msat_or_drain: Option, + &self, address: &bitcoin::Address, send_amount: OnchainSendType, ) -> Result { let confirmation_target = ConfirmationTarget::OnchainPayment; let fee_rate = self.fee_estimator.estimate_fee_rate(confirmation_target); @@ -245,23 +247,69 @@ where let locked_wallet = self.inner.lock().unwrap(); let mut tx_builder = locked_wallet.build_tx(); - if let Some(amount_sats) = amount_msat_or_drain { - tx_builder - .add_recipient(address.script_pubkey(), amount_sats) - .fee_rate(fee_rate) - .enable_rbf(); - } else { - tx_builder - .drain_wallet() - .drain_to(address.script_pubkey()) - .fee_rate(fee_rate) - .enable_rbf(); + // Prepare the tx_builder. We properly check the reserve requirements (again) further down. + match send_amount { + OnchainSendType::SendRetainingReserve { amount_sats, .. } => { + tx_builder + .add_recipient(address.script_pubkey(), amount_sats) + .fee_rate(fee_rate) + .enable_rbf(); + }, + OnchainSendType::SendAllRetainingReserve { cur_anchor_reserve_sats } => { + let spendable_amount_sats = + self.get_spendable_amount_sats(cur_anchor_reserve_sats).unwrap_or(0); + // TODO: can we make this closer resemble the actual transaction? + // As draining the wallet always will only add one output, this method likely + // under-estimates the fee rate a bit. + let mut tmp_tx_builder = locked_wallet.build_tx(); + tmp_tx_builder + .drain_wallet() + .drain_to(address.script_pubkey()) + .fee_rate(fee_rate) + .enable_rbf(); + let tmp_tx_details = match tmp_tx_builder.finish() { + Ok((_, tmp_tx_details)) => tmp_tx_details, + Err(err) => { + log_error!( + self.logger, + "Failed to create temporary transaction: {}", + err + ); + return Err(err.into()); + }, + }; + + let estimated_tx_fee_sats = tmp_tx_details.fee.unwrap_or(0); + let estimated_spendable_amount_sats = + spendable_amount_sats.saturating_sub(estimated_tx_fee_sats); + + if estimated_spendable_amount_sats == 0 { + log_error!(self.logger, + "Unable to send payment without infringing on Anchor reserves. Available: {}sats, estimated fee required: {}sats.", + spendable_amount_sats, + estimated_tx_fee_sats, + ); + return Err(Error::InsufficientFunds); + } + + tx_builder + .add_recipient(address.script_pubkey(), estimated_spendable_amount_sats) + .fee_absolute(estimated_tx_fee_sats) + .enable_rbf(); + }, + OnchainSendType::SendAllDrainingReserve => { + tx_builder + .drain_wallet() + .drain_to(address.script_pubkey()) + .fee_rate(fee_rate) + .enable_rbf(); + }, } - let mut psbt = match tx_builder.finish() { - Ok((psbt, _)) => { + let (mut psbt, tx_details) = match tx_builder.finish() { + Ok((psbt, tx_details)) => { log_trace!(self.logger, "Created PSBT: {:?}", psbt); - psbt + (psbt, tx_details) }, Err(err) => { log_error!(self.logger, "Failed to create transaction: {}", err); @@ -269,6 +317,38 @@ where }, }; + // Check the reserve requirements (again) and return an error if they aren't met. + match send_amount { + OnchainSendType::SendRetainingReserve { amount_sats, cur_anchor_reserve_sats } => { + let spendable_amount_sats = + self.get_spendable_amount_sats(cur_anchor_reserve_sats).unwrap_or(0); + let tx_fee_sats = tx_details.fee.unwrap_or(0); + if spendable_amount_sats < amount_sats + tx_fee_sats { + log_error!(self.logger, + "Unable to send payment due to insufficient funds. Available: {}sats, Required: {}sats + {}sats fee", + spendable_amount_sats, + amount_sats, + tx_fee_sats, + ); + return Err(Error::InsufficientFunds); + } + }, + OnchainSendType::SendAllRetainingReserve { cur_anchor_reserve_sats } => { + let spendable_amount_sats = + self.get_spendable_amount_sats(cur_anchor_reserve_sats).unwrap_or(0); + let drain_amount_sats = tx_details.sent - tx_details.received; + if spendable_amount_sats < drain_amount_sats { + log_error!(self.logger, + "Unable to send payment due to insufficient funds. Available: {}sats, Required: {}sats", + spendable_amount_sats, + drain_amount_sats, + ); + return Err(Error::InsufficientFunds); + } + }, + _ => {}, + } + match locked_wallet.sign(&mut psbt, SignOptions::default()) { Ok(finalized) => { if !finalized { @@ -287,21 +367,33 @@ where let txid = tx.txid(); - if let Some(amount_sats) = amount_msat_or_drain { - log_info!( - self.logger, - "Created new transaction {} sending {}sats on-chain to address {}", - txid, - amount_sats, - address - ); - } else { - log_info!( - self.logger, - "Created new transaction {} sending all available on-chain funds to address {}", - txid, - address - ); + match send_amount { + OnchainSendType::SendRetainingReserve { amount_sats, .. } => { + log_info!( + self.logger, + "Created new transaction {} sending {}sats on-chain to address {}", + txid, + amount_sats, + address + ); + }, + OnchainSendType::SendAllRetainingReserve { cur_anchor_reserve_sats } => { + log_info!( + self.logger, + "Created new transaction {} sending available on-chain funds retaining a reserve of {}sats to address {}", + txid, + address, + cur_anchor_reserve_sats, + ); + }, + OnchainSendType::SendAllDrainingReserve => { + log_info!( + self.logger, + "Created new transaction {} sending all available on-chain funds to address {}", + txid, + address + ); + }, } Ok(txid) diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index b3788f9d4..27f8c03a5 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -286,7 +286,7 @@ fn onchain_spend_receive() { assert!(node_b.list_balances().spendable_onchain_balance_sats < 100000); let addr_b = node_b.onchain_payment().new_address().unwrap(); - let txid = node_a.onchain_payment().send_all_to_address(&addr_b).unwrap(); + let txid = node_a.onchain_payment().send_all_to_address(&addr_b, false).unwrap(); generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6); wait_for_tx(&electrsd.client, txid); From e86d84354cb30e0aaa229dbe79b221eb187b2262 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 28 Aug 2024 11:37:34 +0200 Subject: [PATCH 6/9] f Fix logged argument order --- src/wallet.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet.rs b/src/wallet.rs index ae2c2d635..f8462663d 100644 --- a/src/wallet.rs +++ b/src/wallet.rs @@ -382,8 +382,8 @@ where self.logger, "Created new transaction {} sending available on-chain funds retaining a reserve of {}sats to address {}", txid, - address, cur_anchor_reserve_sats, + address, ); }, OnchainSendType::SendAllDrainingReserve => { From 326cc38154606a5f419e28f1e6b650201164f928 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 29 Aug 2024 12:51:36 +0200 Subject: [PATCH 7/9] f Make sure transaction is relayable .. rather than undercutting the fee, we now choose a method that might slightly overestimate fees, ensuring its relayability. We also make sure the net fee is always larger than the fee rate floor. --- src/wallet.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/wallet.rs b/src/wallet.rs index f8462663d..df8ab2fbe 100644 --- a/src/wallet.rs +++ b/src/wallet.rs @@ -4,7 +4,7 @@ use crate::config::BDK_WALLET_SYNC_TIMEOUT_SECS; use crate::fee_estimator::{ConfirmationTarget, FeeEstimator}; use crate::Error; -use lightning::chain::chaininterface::BroadcasterInterface; +use lightning::chain::chaininterface::{BroadcasterInterface, FEERATE_FLOOR_SATS_PER_KW}; use lightning::events::bump_transaction::{Utxo, WalletSource}; use lightning::ln::msgs::{DecodeError, UnsignedGossipMessage}; @@ -258,13 +258,9 @@ where OnchainSendType::SendAllRetainingReserve { cur_anchor_reserve_sats } => { let spendable_amount_sats = self.get_spendable_amount_sats(cur_anchor_reserve_sats).unwrap_or(0); - // TODO: can we make this closer resemble the actual transaction? - // As draining the wallet always will only add one output, this method likely - // under-estimates the fee rate a bit. let mut tmp_tx_builder = locked_wallet.build_tx(); tmp_tx_builder - .drain_wallet() - .drain_to(address.script_pubkey()) + .add_recipient(address.script_pubkey(), spendable_amount_sats) .fee_rate(fee_rate) .enable_rbf(); let tmp_tx_details = match tmp_tx_builder.finish() { @@ -279,7 +275,8 @@ where }, }; - let estimated_tx_fee_sats = tmp_tx_details.fee.unwrap_or(0); + let estimated_tx_fee_sats = + tmp_tx_details.fee.unwrap_or(0).max(FEERATE_FLOOR_SATS_PER_KW as u64); let estimated_spendable_amount_sats = spendable_amount_sats.saturating_sub(estimated_tx_fee_sats); From 156a9708172d9405541eaf9280a13ca61a00ff54 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Fri, 30 Aug 2024 09:39:17 +0200 Subject: [PATCH 8/9] f Make temporary estimation a bit more precise --- src/wallet.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/wallet.rs b/src/wallet.rs index df8ab2fbe..e288cc528 100644 --- a/src/wallet.rs +++ b/src/wallet.rs @@ -256,11 +256,21 @@ where .enable_rbf(); }, OnchainSendType::SendAllRetainingReserve { cur_anchor_reserve_sats } => { + let change_address_info = + locked_wallet.get_internal_address(AddressIndex::Peek(0)).map_err(|e| { + log_error!(self.logger, "Unable to retrieve address: {}", e); + e + })?; let spendable_amount_sats = self.get_spendable_amount_sats(cur_anchor_reserve_sats).unwrap_or(0); let mut tmp_tx_builder = locked_wallet.build_tx(); tmp_tx_builder - .add_recipient(address.script_pubkey(), spendable_amount_sats) + .drain_wallet() + .drain_to(address.script_pubkey()) + .add_recipient( + change_address_info.address.script_pubkey(), + cur_anchor_reserve_sats, + ) .fee_rate(fee_rate) .enable_rbf(); let tmp_tx_details = match tmp_tx_builder.finish() { @@ -275,8 +285,7 @@ where }, }; - let estimated_tx_fee_sats = - tmp_tx_details.fee.unwrap_or(0).max(FEERATE_FLOOR_SATS_PER_KW as u64); + let estimated_tx_fee_sats = tmp_tx_details.fee.unwrap_or(0); let estimated_spendable_amount_sats = spendable_amount_sats.saturating_sub(estimated_tx_fee_sats); From b0f8d6c644cca231101ec31f27ba650a6468489f Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Wed, 28 Aug 2024 12:04:17 +0200 Subject: [PATCH 9/9] Add test for `send_all_to_address` behavior --- tests/integration_tests_rust.rs | 63 +++++++++++++++++++++++++++------ 1 file changed, 53 insertions(+), 10 deletions(-) diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index 27f8c03a5..5197ea6bb 100644 --- a/tests/integration_tests_rust.rs +++ b/tests/integration_tests_rust.rs @@ -258,20 +258,39 @@ fn onchain_spend_receive() { let addr_a = node_a.onchain_payment().new_address().unwrap(); let addr_b = node_b.onchain_payment().new_address().unwrap(); + let premine_amount_sat = 1_100_000; premine_and_distribute_funds( &bitcoind.client, &electrsd.client, - vec![addr_b.clone()], - Amount::from_sat(100000), + vec![addr_a.clone(), addr_b.clone()], + Amount::from_sat(premine_amount_sat), ); node_a.sync_wallets().unwrap(); node_b.sync_wallets().unwrap(); - assert_eq!(node_b.list_balances().spendable_onchain_balance_sats, 100000); + assert_eq!(node_a.list_balances().spendable_onchain_balance_sats, premine_amount_sat); + assert_eq!(node_b.list_balances().spendable_onchain_balance_sats, premine_amount_sat); + + let channel_amount_sat = 1_000_000; + let reserve_amount_sat = 25_000; + open_channel(&node_b, &node_a, channel_amount_sat, true, &electrsd); + generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6); + + node_a.sync_wallets().unwrap(); + node_b.sync_wallets().unwrap(); + expect_channel_ready_event!(node_a, node_b.node_id()); + expect_channel_ready_event!(node_b, node_a.node_id()); + + let node_a_balance = premine_amount_sat - reserve_amount_sat; + let node_b_balance_lower = premine_amount_sat - channel_amount_sat - reserve_amount_sat - 1000; + let node_b_balance_upper = premine_amount_sat - channel_amount_sat - reserve_amount_sat; + assert_eq!(node_a.list_balances().spendable_onchain_balance_sats, node_a_balance); + assert!(node_b.list_balances().spendable_onchain_balance_sats > node_b_balance_lower); + assert!(node_b.list_balances().spendable_onchain_balance_sats < node_b_balance_upper); assert_eq!( Err(NodeError::InsufficientFunds), - node_a.onchain_payment().send_to_address(&addr_b, 1000) + node_a.onchain_payment().send_to_address(&addr_b, node_a_balance + 1) ); let txid = node_b.onchain_payment().send_to_address(&addr_a, 1000).unwrap(); @@ -281,9 +300,28 @@ fn onchain_spend_receive() { node_a.sync_wallets().unwrap(); node_b.sync_wallets().unwrap(); - assert_eq!(node_a.list_balances().spendable_onchain_balance_sats, 1000); - assert!(node_b.list_balances().spendable_onchain_balance_sats > 98000); - assert!(node_b.list_balances().spendable_onchain_balance_sats < 100000); + let node_a_balance = node_a_balance + 1000; + let node_b_balance_lower = node_b_balance_lower - 1000; + let node_b_balance_upper = node_b_balance_upper - 1000; + assert_eq!(node_a.list_balances().spendable_onchain_balance_sats, node_a_balance); + assert!(node_b.list_balances().spendable_onchain_balance_sats > node_b_balance_lower); + assert!(node_b.list_balances().spendable_onchain_balance_sats < node_b_balance_upper); + + let addr_b = node_b.onchain_payment().new_address().unwrap(); + let txid = node_a.onchain_payment().send_all_to_address(&addr_b, true).unwrap(); + generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6); + wait_for_tx(&electrsd.client, txid); + + node_a.sync_wallets().unwrap(); + node_b.sync_wallets().unwrap(); + + let node_b_balance_lower = node_b_balance_lower + node_a_balance; + let node_b_balance_upper = node_b_balance_upper + node_a_balance; + let node_a_balance = 0; + assert_eq!(node_a.list_balances().spendable_onchain_balance_sats, node_a_balance); + assert_eq!(node_a.list_balances().total_onchain_balance_sats, reserve_amount_sat); + assert!(node_b.list_balances().spendable_onchain_balance_sats > node_b_balance_lower); + assert!(node_b.list_balances().spendable_onchain_balance_sats < node_b_balance_upper); let addr_b = node_b.onchain_payment().new_address().unwrap(); let txid = node_a.onchain_payment().send_all_to_address(&addr_b, false).unwrap(); @@ -293,9 +331,14 @@ fn onchain_spend_receive() { node_a.sync_wallets().unwrap(); node_b.sync_wallets().unwrap(); - assert_eq!(node_a.list_balances().total_onchain_balance_sats, 0); - assert!(node_b.list_balances().spendable_onchain_balance_sats > 99000); - assert!(node_b.list_balances().spendable_onchain_balance_sats < 100000); + let node_b_balance_lower = node_b_balance_lower + reserve_amount_sat; + let node_b_balance_upper = node_b_balance_upper + reserve_amount_sat; + let node_a_balance = 0; + + assert_eq!(node_a.list_balances().spendable_onchain_balance_sats, node_a_balance); + assert_eq!(node_a.list_balances().total_onchain_balance_sats, node_a_balance); + assert!(node_b.list_balances().spendable_onchain_balance_sats > node_b_balance_lower); + assert!(node_b.list_balances().spendable_onchain_balance_sats < node_b_balance_upper); } #[test]