-
Notifications
You must be signed in to change notification settings - Fork 138
Safely migrate to FileSystemStoreV2 #872
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 |
|---|---|---|
|
|
@@ -256,6 +256,7 @@ impl UnifiedPayment { | |
| PaymentMethod::LightningBolt12(_) => 0, | ||
| PaymentMethod::LightningBolt11(_) => 1, | ||
| PaymentMethod::OnChain(_) => 2, | ||
| PaymentMethod::Cashu(_) => 3, | ||
|
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. Seems you also bumped |
||
| }); | ||
|
|
||
| for method in sorted_payment_methods { | ||
|
|
@@ -331,6 +332,10 @@ impl UnifiedPayment { | |
| let txid = self.onchain_payment.send_to_address(&address, amt_sats, None)?; | ||
| return Ok(UnifiedPaymentResult::Onchain { txid }); | ||
| }, | ||
| PaymentMethod::Cashu(_) => { | ||
| log_error!(self.logger, "Cashu payment methods are not supported. Skipping."); | ||
| continue; | ||
| }, | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -574,7 +574,7 @@ impl Wallet { | |
| witness_utxo: Some(input.previous_utxo.clone()), | ||
| ..Default::default() | ||
| }; | ||
| let weight = Weight::from_wu(input.satisfaction_weight); | ||
| let weight = ldk_to_bdk_satisfaction_weight(input.satisfaction_weight); | ||
| tx_builder.only_witness_utxo().exclude_unconfirmed(); | ||
| tx_builder.add_foreign_utxo(input.outpoint, psbt_input, weight).map_err(|e| { | ||
| log_error!(self.logger, "Failed to add shared input for fee estimation: {e}"); | ||
|
|
@@ -916,7 +916,7 @@ impl Wallet { | |
| witness_utxo: Some(input.previous_utxo.clone()), | ||
| ..Default::default() | ||
| }; | ||
| let weight = Weight::from_wu(input.satisfaction_weight); | ||
| let weight = ldk_to_bdk_satisfaction_weight(input.satisfaction_weight); | ||
| tx_builder.add_foreign_utxo(input.outpoint, psbt_input, weight).map_err(|_| ())?; | ||
| } | ||
|
|
||
|
|
@@ -955,11 +955,41 @@ impl Wallet { | |
| return Err(()); | ||
| } | ||
|
|
||
| let change_output = unsigned_tx | ||
| let mut change_output = unsigned_tx | ||
| .output | ||
| .into_iter() | ||
| .filter(|txout| must_pay_to.iter().all(|output| output != txout)) | ||
| .next(); | ||
| .find(|txout| must_pay_to.iter().all(|output| output != txout)); | ||
|
|
||
| // BDK's `create_tx` applies `ceil(fee_rate * weight)` separately for each | ||
| // fee component instead of once over the total weight, and LDK's | ||
| // `estimate_transaction_fee` does a single ceiling. Because LDK consumes | ||
| // our `change_output.value` verbatim and recomputes its own fee to size | ||
| // the new funding output, every sat BDK over-reserves gets funneled into | ||
| // the channel instead of staying in change. Absorb the worst-case | ||
| // over-reservation by bumping the change output up here. | ||
| // | ||
| // Two sources of rounding contribute: | ||
| // | ||
| // * Per-input ceilings — `bdk_wallet`'s `select_sorted_utxos` ceilings | ||
| // `fee_rate * (TxIn::default().segwit_weight() + satisfaction_weight)` | ||
| // once per input. With `N` inputs, the worst-case over-reservation is | ||
| // `N - 1` sats. TODO: drop once | ||
|
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. Please put this TODO on a dedicated line. (probably at the start or end of the comment). Also, can we create an issue for this so we don't forget to drop this? |
||
| // https://github.com/bitcoindevkit/bdk_wallet/pull/479 lands in a | ||
| // release. | ||
| // | ||
| // * Initial-tx and change ceilings — `create_tx` ceilings | ||
| // `fee_rate * tx.weight()` on the recipient-only tx, and `decide_change` | ||
| // ceilings `fee_rate * Weight::from_vb(drain_output_len)`. Two more | ||
| // independent ceilings, contributing up to 2 sats. | ||
| if let Some(ref mut change) = change_output { | ||
| let num_inputs = (confirmed_utxos.len() + must_spend.len()) as u64; | ||
| let per_input_rounding_surplus_sat = num_inputs.saturating_sub(1); | ||
|
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. Codex:
|
||
| let initial_and_change_rounding_surplus_sat = 2; | ||
| let bdk_rounding_surplus_sat = | ||
| per_input_rounding_surplus_sat + initial_and_change_rounding_surplus_sat; | ||
| change.value = | ||
| Amount::from_sat(change.value.to_sat().saturating_add(bdk_rounding_surplus_sat)); | ||
| } | ||
|
|
||
| if change_output.is_some() { | ||
| locked_wallet.persist(&mut locked_persister).map_err(|e| { | ||
|
|
@@ -1717,6 +1747,28 @@ impl ChangeDestinationSource for WalletKeysManager { | |
| } | ||
| } | ||
|
|
||
| /// Convert LDK's `Input::satisfaction_weight` to the value BDK's | ||
| /// [`bdk_wallet::TxBuilder::add_foreign_utxo`] expects. | ||
| /// | ||
| /// LDK and BDK disagree on what `satisfaction_weight` includes for a SegWit input. LDK | ||
| /// treats it as the full weight of the spent input's `script_sig` and `witness` *each | ||
| /// with their lengths included* — i.e., the empty `script_sig` length byte (4 WU) and | ||
| /// the witness-elements-count varint (1 WU) are part of the value. BDK adds | ||
| /// `TxIn::default().segwit_weight()` internally, which already accounts for those same | ||
| /// 5 WU (an empty TxIn has a 1-byte empty `script_sig` length and a 1-byte empty | ||
| /// witness-count varint). Passing LDK's value directly to BDK therefore double-counts | ||
| /// 5 WU per foreign input, which inflates BDK's fee estimate and ultimately funnels the | ||
| /// surplus into the new funding output during splice negotiation. | ||
| fn ldk_to_bdk_satisfaction_weight(ldk_satisfaction_weight: u64) -> Weight { | ||
| const EMPTY_SCRIPT_SIG_WEIGHT: u64 = | ||
| 1 /* empty script_sig length byte */ * WITNESS_SCALE_FACTOR as u64; | ||
| const EMPTY_WITNESS_COUNT_WEIGHT: u64 = 1 /* witness elements count varint */; | ||
| Weight::from_wu( | ||
| ldk_satisfaction_weight | ||
| .saturating_sub(EMPTY_SCRIPT_SIG_WEIGHT + EMPTY_WITNESS_COUNT_WEIGHT), | ||
| ) | ||
| } | ||
|
|
||
| // FIXME/TODO: This is copied-over from bdk_wallet and only used to generate `WalletEvent`s after | ||
| // applying mempool transactions. We should drop this when BDK offers to generate events for | ||
| // mempool transactions natively. | ||
|
|
||
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.
Codex:
Seems we still might want to double-check we survive an ill-timed crash?