-
Notifications
You must be signed in to change notification settings - Fork 122
Enhance onchain transaction management #628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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}; | ||
|
|
@@ -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() | ||
| ); | ||
| } | ||
| } | ||
| }, | ||
|
|
@@ -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); | ||
| let payment = self.create_payment_from_tx( | ||
| locked_wallet, | ||
| txid, | ||
| new_txid, | ||
| payment_id, | ||
| &tx, | ||
| PaymentStatus::Pending, | ||
|
|
@@ -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 }) => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we name this |
||
| log_info!(self.logger, "BDK requires higher fee rate: {}", required); | ||
|
|
||
| // Safety check | ||
| const MAX_REASONABLE_FEE_RATE_SAT_VB: u64 = 1000; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
|
||
There was a problem hiding this comment.
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
Txidthat will be our original Txid? Can we lean on that or do we need more explicit APIs from BDK here?There was a problem hiding this comment.
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_conflictsused to get the conflicts for a replaced txid, comes from BDK'stx_graphand doesn't guarantee any ordering. I think this needs to be fixed upstream in BDK. What do you think is the best approach here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, mind opening an issue with them, maybe they can guarantee an ordering here, so that indeed the last
Vecentry is always guaranteed to be the latest version that is kept in the mempool?There was a problem hiding this comment.
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