Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bindings/ldk_node.udl
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,8 @@ interface OnchainPayment {
Txid send_to_address([ByRef]Address address, u64 amount_sats, FeeRate? fee_rate);
[Throws=NodeError]
Txid send_all_to_address([ByRef]Address address, boolean retain_reserve, FeeRate? fee_rate);
[Throws=NodeError]
Txid bump_fee_rbf(PaymentId payment_id);
};

interface FeeRate {
Expand Down
15 changes: 15 additions & 0 deletions src/payment/onchain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use std::sync::{Arc, RwLock};

use bitcoin::{Address, Txid};
use lightning::ln::channelmanager::PaymentId;

use crate::config::Config;
use crate::error::Error;
Expand Down Expand Up @@ -120,4 +121,18 @@ impl OnchainPayment {
let fee_rate_opt = maybe_map_fee_rate_opt!(fee_rate);
self.wallet.send_to_address(address, send_amount, fee_rate_opt)
}

/// Attempt to bump the fee of an unconfirmed transaction using Replace-by-Fee (RBF).
///
/// This creates a new transaction that replaces the original one, increasing the fee by the
/// specified increment to improve its chances of confirmation. The original transaction must
/// be signaling RBF replaceability for this to succeed.
///
/// The new transaction will have the same outputs as the original but with a
/// higher fee, resulting in faster confirmation potential.
///
/// Returns the Txid of the new replacement transaction if successful.
pub fn bump_fee_rbf(&self, payment_id: PaymentId) -> Result<Txid, Error> {
self.wallet.bump_fee_rbf(payment_id)
}
}
11 changes: 6 additions & 5 deletions src/payment/pending_payment_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,11 @@ impl StorableObjectUpdate<PendingPaymentDetails> for PendingPaymentDetailsUpdate

impl From<&PendingPaymentDetails> for PendingPaymentDetailsUpdate {
fn from(value: &PendingPaymentDetails) -> Self {
Self {
id: value.id(),
payment_update: Some(value.details.to_update()),
conflicting_txids: Some(value.conflicting_txids.clone()),
}
let conflicting_txids = if value.conflicting_txids.is_empty() {
None
} else {
Some(value.conflicting_txids.clone())
};
Self { id: value.id(), payment_update: Some(value.details.to_update()), conflicting_txids }
}
}
18 changes: 15 additions & 3 deletions src/payment/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,15 @@ impl StorableObject for PaymentDetails {
}
}

if let Some(tx_id) = update.txid {
match self.kind {
PaymentKind::Onchain { ref mut txid, .. } => {
update_if_necessary!(*txid, tx_id);
},
_ => {},
}
}

if updated {
self.latest_update_timestamp = SystemTime::now()
.duration_since(UNIX_EPOCH)
Expand Down Expand Up @@ -540,6 +549,7 @@ pub(crate) struct PaymentDetailsUpdate {
pub direction: Option<PaymentDirection>,
pub status: Option<PaymentStatus>,
pub confirmation_status: Option<ConfirmationStatus>,
pub txid: Option<Txid>,
}

impl PaymentDetailsUpdate {
Expand All @@ -555,6 +565,7 @@ impl PaymentDetailsUpdate {
direction: None,
status: None,
confirmation_status: None,
txid: None,
}
}
}
Expand All @@ -570,9 +581,9 @@ impl From<&PaymentDetails> for PaymentDetailsUpdate {
_ => (None, None, None),
};

let confirmation_status = match value.kind {
PaymentKind::Onchain { status, .. } => Some(status),
_ => None,
let (confirmation_status, txid) = match &value.kind {
PaymentKind::Onchain { status, txid, .. } => (Some(*status), Some(*txid)),
_ => (None, None),
};

let counterparty_skimmed_fee_msat = match value.kind {
Expand All @@ -593,6 +604,7 @@ impl From<&PaymentDetails> for PaymentDetailsUpdate {
direction: Some(value.direction),
status: Some(value.status),
confirmation_status,
txid,
}
}
}
Expand Down
201 changes: 181 additions & 20 deletions src/wallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use std::sync::{Arc, Mutex};

use bdk_chain::spk_client::{FullScanRequest, SyncRequest};
use bdk_wallet::descriptor::ExtendedDescriptor;
use bdk_wallet::error::{BuildFeeBumpError, CreateTxError};
use bdk_wallet::event::WalletEvent;
#[allow(deprecated)]
use bdk_wallet::SignOptions;
Expand All @@ -29,6 +30,7 @@ use bitcoin::{
Address, Amount, FeeRate, OutPoint, ScriptBuf, Transaction, TxOut, Txid, WPubkeyHash, Weight,
WitnessProgram, WitnessVersion,
};

use lightning::chain::chaininterface::BroadcasterInterface;
use lightning::chain::channelmonitor::ANTI_REORG_DELAY;
use lightning::chain::{BestBlock, Listen};
Expand Down Expand Up @@ -244,31 +246,54 @@ impl Wallet {
self.pending_payment_store.insert_or_update(pending_payment)?;
},
WalletEvent::ChainTipChanged { new_tip, .. } => {
// Get all payments that are Pending with Confirmed status
// Get all on-chain payments that are Pending
let pending_payments: Vec<PendingPaymentDetails> =
self.pending_payment_store.list_filter(|p| {
p.details.status == PaymentStatus::Pending
&& matches!(
p.details.kind,
PaymentKind::Onchain {
status: ConfirmationStatus::Confirmed { .. },
..
}
)
&& matches!(p.details.kind, PaymentKind::Onchain { .. })
});

let mut unconfirmed_outbound_txids: Vec<Txid> = Vec::new();

for mut payment in pending_payments {
if let PaymentKind::Onchain {
status: ConfirmationStatus::Confirmed { height, .. },
..
} = payment.details.kind
{
let payment_id = payment.details.id;
if new_tip.height >= height + ANTI_REORG_DELAY - 1 {
payment.details.status = PaymentStatus::Succeeded;
self.payment_store.insert_or_update(payment.details)?;
self.pending_payment_store.remove(&payment_id)?;
}
match payment.details.kind {
PaymentKind::Onchain {
status: ConfirmationStatus::Confirmed { height, .. },
..
} => {
let payment_id = payment.details.id;
if new_tip.height >= height + ANTI_REORG_DELAY - 1 {
payment.details.status = PaymentStatus::Succeeded;
self.payment_store.insert_or_update(payment.details)?;
self.pending_payment_store.remove(&payment_id)?;
}
},
PaymentKind::Onchain {
txid,
status: ConfirmationStatus::Unconfirmed,
} if payment.details.direction == PaymentDirection::Outbound => {
unconfirmed_outbound_txids.push(txid);
},
_ => {},
}
}

if !unconfirmed_outbound_txids.is_empty() {
let txs_to_broadcast: Vec<Transaction> = unconfirmed_outbound_txids
.iter()
.filter_map(|txid| {
locked_wallet.tx_details(*txid).map(|d| (*d.tx).clone())
})
.collect();

if !txs_to_broadcast.is_empty() {
let tx_refs: Vec<&Transaction> = txs_to_broadcast.iter().collect();
self.broadcaster.broadcast_transactions(&tx_refs);
log_info!(
self.logger,
"Rebroadcast {} unconfirmed transactions on chain tip change",
txs_to_broadcast.len()
);
}
}
},
Expand Down Expand Up @@ -299,9 +324,11 @@ impl Wallet {
let conflict_txids: Vec<Txid> =
conflicts.iter().map(|(_, conflict_txid)| *conflict_txid).collect();

// Use the last transaction id in the conflicts as the new txid
let new_txid = conflicts.last().map(|(_, new_tx)| *new_tx).unwrap_or(txid);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this seems very unreliable? I don't think it's part of the API guarantees that it's always the last Txid that will be our original Txid? Can we lean on that or do we need more explicit APIs from BDK here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my tests, it happened to be the last one, but looking at the implementation, direct_conflicts used to get the conflicts for a replaced txid, comes from BDK's tx_graph and doesn't guarantee any ordering. I think this needs to be fixed upstream in BDK. What do you think is the best approach here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my tests, it happened to be the last one, but looking at the implementation, direct_conflicts used to get the conflicts for a replaced txid, comes from BDK's tx_graph and doesn't guarantee any ordering. I think this needs to be fixed upstream in BDK. What do you think is the best approach here?

Hmm, mind opening an issue with them, maybe they can guarantee an ordering here, so that indeed the last Vec entry is always guaranteed to be the latest version that is kept in the mempool?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I will open one

let payment = self.create_payment_from_tx(
locked_wallet,
txid,
new_txid,
payment_id,
&tx,
PaymentStatus::Pending,
Expand Down Expand Up @@ -978,6 +1005,140 @@ impl Wallet {

None
}

#[allow(deprecated)]
pub(crate) fn bump_fee_rbf(&self, payment_id: PaymentId) -> Result<Txid, Error> {
let payment = self.payment_store.get(&payment_id).ok_or(Error::InvalidPaymentId)?;

let mut locked_wallet = self.inner.lock().unwrap();

if payment.direction != PaymentDirection::Outbound {
log_error!(self.logger, "Transaction {} is not an outbound payment", payment_id);
return Err(Error::InvalidPaymentId);
}

if let PaymentKind::Onchain { status, .. } = &payment.kind {
match status {
ConfirmationStatus::Confirmed { .. } => {
log_error!(
self.logger,
"Transaction {} is already confirmed and cannot be fee bumped",
payment_id
);
return Err(Error::InvalidPaymentId);
},
ConfirmationStatus::Unconfirmed => {},
}
}

let txid = match &payment.kind {
PaymentKind::Onchain { txid, .. } => *txid,
_ => return Err(Error::InvalidPaymentId),
};

let confirmation_target = ConfirmationTarget::OnchainPayment;
let estimated_fee_rate = self.fee_estimator.estimate_fee_rate(confirmation_target);

log_info!(self.logger, "Bumping fee to {}", estimated_fee_rate);

let mut psbt = {
let mut builder = locked_wallet.build_fee_bump(txid).map_err(|e| {
log_error!(self.logger, "BDK fee bump failed for {}: {:?}", txid, e);
match e {
BuildFeeBumpError::TransactionNotFound(_) => Error::InvalidPaymentId,
BuildFeeBumpError::TransactionConfirmed(_) => Error::InvalidPaymentId,
BuildFeeBumpError::IrreplaceableTransaction(_) => Error::InvalidPaymentId,
BuildFeeBumpError::FeeRateUnavailable => Error::InvalidPaymentId,
_ => Error::InvalidFeeRate,
}
})?;

builder.fee_rate(estimated_fee_rate);

match builder.finish() {
Ok(psbt) => Ok(psbt),
Err(CreateTxError::FeeRateTooLow { required }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we name this required_fee_rate or similar?

log_info!(self.logger, "BDK requires higher fee rate: {}", required);

// Safety check
const MAX_REASONABLE_FEE_RATE_SAT_VB: u64 = 1000;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did we get this limit from? Also, usually sat/vb is a float value and we tend to use sat/kwu in LDK. Would be great to keep it as consistent as possible and do such calculations in sat/kwu too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll use sat/kwu for consistency with LDK conventions.

The limit itself is a safety check to prevent setting an extremely high fee rate when bumping. Since BDK's build_fee_bump can return a required minimum fee that's higher than our estimated rate, I wanted to cap how high we'd go. I'm open to adjusting it or removing it entirely if you think the check isn't needed.

if required.to_sat_per_vb_ceil() > MAX_REASONABLE_FEE_RATE_SAT_VB {
log_error!(
self.logger,
"BDK requires unreasonably high fee rate: {} sat/vB",
required.to_sat_per_vb_ceil()
);
return Err(Error::InvalidFeeRate);
}

let mut builder = locked_wallet.build_fee_bump(txid).map_err(|e| {
log_error!(self.logger, "BDK fee bump retry failed for {}: {:?}", txid, e);
Error::InvalidFeeRate
})?;

builder.fee_rate(required);
builder.finish().map_err(|e| {
log_error!(
self.logger,
"Failed to finish PSBT with required fee rate: {:?}",
e
);
Error::InvalidFeeRate
})
},
Err(e) => {
log_error!(self.logger, "Failed to create fee bump PSBT: {:?}", e);
Err(Error::InvalidFeeRate)
},
}?
};

match locked_wallet.sign(&mut psbt, SignOptions::default()) {
Ok(finalized) => {
if !finalized {
return Err(Error::OnchainTxCreationFailed);
}
},
Err(err) => {
log_error!(self.logger, "Failed to create transaction: {}", err);
return Err(err.into());
},
}

let mut locked_persister = self.persister.lock().unwrap();
locked_wallet.persist(&mut locked_persister).map_err(|e| {
log_error!(self.logger, "Failed to persist wallet: {}", e);
Error::PersistenceFailed
})?;

let fee_bumped_tx = psbt.extract_tx().map_err(|e| {
log_error!(self.logger, "Failed to extract transaction: {}", e);
e
})?;

let new_txid = fee_bumped_tx.compute_txid();

self.broadcaster.broadcast_transactions(&[&fee_bumped_tx]);

let new_payment = self.create_payment_from_tx(
&locked_wallet,
new_txid,
payment.id,
&fee_bumped_tx,
PaymentStatus::Pending,
ConfirmationStatus::Unconfirmed,
);

let pending_payment_store =
self.create_pending_payment_from_tx(new_payment.clone(), Vec::new());

self.pending_payment_store.insert_or_update(pending_payment_store)?;
self.payment_store.insert_or_update(new_payment)?;

log_info!(self.logger, "RBF successful: replaced {} with {}", txid, new_txid);

Ok(new_txid)
}
}

impl Listen for Wallet {
Expand Down
Loading