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
5 changes: 5 additions & 0 deletions bindings/ldk_node.udl
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,7 @@ enum NodeError {
"InvalidNodeAlias",
"InvalidDateTime",
"InvalidFeeRate",
"InvalidScriptPubKey",
"DuplicatePayment",
"UnsupportedCurrency",
"InsufficientFunds",
Expand Down Expand Up @@ -575,6 +576,7 @@ dictionary ChannelDetails {
ChannelId channel_id;
PublicKey counterparty_node_id;
OutPoint? funding_txo;
ScriptBuf? funding_redeem_script;
u64? short_channel_id;
u64? outbound_scid_alias;
u64? inbound_scid_alias;
Expand Down Expand Up @@ -901,3 +903,6 @@ typedef string LSPS1OrderId;

[Custom]
typedef string LSPSDateTime;

[Custom]
typedef string ScriptBuf;
3 changes: 3 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ pub enum Error {
InvalidDateTime,
/// The given fee rate is invalid.
InvalidFeeRate,
/// The given script public key is invalid.
InvalidScriptPubKey,
/// A payment with the given hash has already been initiated.
DuplicatePayment,
/// The provided offer was denonminated in an unsupported currency.
Expand Down Expand Up @@ -186,6 +188,7 @@ impl fmt::Display for Error {
Self::InvalidNodeAlias => write!(f, "The given node alias is invalid."),
Self::InvalidDateTime => write!(f, "The given date time is invalid."),
Self::InvalidFeeRate => write!(f, "The given fee rate is invalid."),
Self::InvalidScriptPubKey => write!(f, "The given script pubkey is invalid."),
Self::DuplicatePayment => {
write!(f, "A payment with the given hash has already been initiated.")
},
Expand Down
27 changes: 27 additions & 0 deletions src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1458,6 +1458,33 @@ where
counterparty_node_id,
funding_txo,
);

let chans =
self.channel_manager.list_channels_with_counterparty(&counterparty_node_id);
let chan_output = chans
.iter()
.find(|c| c.user_channel_id == user_channel_id)
.and_then(|c| c.get_funding_output());
Comment on lines +1462 to +1467
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing this in ChannelReady wouldn't work for splicing preexisting channels. Wonder if we should instead do this when initiating a splice? There we already look up the channel. It would also let us use the real shared_input when selecting UTXOs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing this in ChannelReady wouldn't work for splicing preexisting channels.

I dont think this is right, in the docs it says we get a ChannelReady for splices, also my test seems to confirm

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, we do get it for spliced channels, but we need the initial funding output because that is the one being spent. And for channels existing before this change, we would have already processed the ChannelReady event for the initial funding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah i see for backward compat sake, added!

match chan_output {
None => {
log_error!(
self.logger,
"Failed to find channel info for pending channel {channel_id} with counterparty {counterparty_node_id}"
);
debug_assert!(false,
"Failed to find channel info for pending channel {channel_id} with counterparty {counterparty_node_id}"
);
},
Some(output) => {
if let Err(e) = self.wallet.insert_txo(funding_txo, output) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... I guess we didn't really need the redeem script in ChannelPending and SplicePending since we only need the previous funding output in the wallet for the next splice. Might have been better in ChannelReady. We'd be able to avoid the look-up, although it wouldn't remove the Option check.

Copy link
Contributor Author

@benthecarman benthecarman Dec 9, 2025

Choose a reason for hiding this comment

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

Yeah sadly I messed up here, i think we needed to add the redeem script along with the new channel size so we had the full utxo information

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, though if we insert the previous funding txo when initiating the splice as mentioned in the other comment, then I think we'll have all the necessary information in ChannelDetails? Essentially, we'd be lazily adding the previous funding output to the wallet when initiating the splice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made so we add it before a splice-in. I still think this one is good to keep so if you're doing any other wallet related action, it will have a more complete set of information and you don't need to initiate a splice for it to have the channel information

log_error!(
self.logger,
"Failed to insert funding TXO into wallet: {e}"
);
return Err(ReplayEvent());
}
},
}
} else {
log_info!(
Copy link
Collaborator

Choose a reason for hiding this comment

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

In what case would we end up in this else branch, and would we need to do something related to the redeemscript, too?

Copy link
Contributor Author

@benthecarman benthecarman Dec 4, 2025

Choose a reason for hiding this comment

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

According to the docs, the funding_txo "Will be None if the channel's funding transaction reached an acceptable depth prior to version 0.2". Since this was also added in 0.2, the redeem script would also be None so there shouldn't be anything to add. Either way, we need the funding txo to be able to insert so we can't add it.

self.logger,
Expand Down
18 changes: 17 additions & 1 deletion src/ffi/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ pub use bip39::Mnemonic;
use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::hashes::Hash;
use bitcoin::secp256k1::PublicKey;
pub use bitcoin::{Address, BlockHash, FeeRate, Network, OutPoint, Txid};
pub use bitcoin::{Address, BlockHash, FeeRate, Network, OutPoint, ScriptBuf, Txid};
pub use lightning::chain::channelmonitor::BalanceSource;
pub use lightning::events::{ClosureReason, PaymentFailureReason};
use lightning::ln::channelmanager::PaymentId;
Expand Down Expand Up @@ -106,6 +106,22 @@ impl UniffiCustomTypeConverter for Address {
}
}

impl UniffiCustomTypeConverter for ScriptBuf {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: While this should be fine for now, we generally try to get away from the UniffiCustomTypeConverter approach, and instead convert more and more objects to proper interfaces.

type Builtin = String;

fn into_custom(val: Self::Builtin) -> uniffi::Result<Self> {
if let Ok(key) = ScriptBuf::from_hex(&val) {
return Ok(key);
}

Err(Error::InvalidScriptPubKey.into())
}

fn from_custom(obj: Self) -> Self::Builtin {
obj.to_string()
}
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub enum OfferAmount {
Bitcoin { amount_msats: u64 },
Expand Down
31 changes: 19 additions & 12 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ use io::utils::write_node_metrics;
use lightning::chain::BestBlock;
use lightning::events::bump_transaction::{Input, Wallet as LdkWallet};
use lightning::impl_writeable_tlv_based;
use lightning::ln::chan_utils::{make_funding_redeemscript, FUNDING_TRANSACTION_WITNESS_WEIGHT};
use lightning::ln::chan_utils::FUNDING_TRANSACTION_WITNESS_WEIGHT;
use lightning::ln::channel_state::{ChannelDetails as LdkChannelDetails, ChannelShutdownState};
use lightning::ln::channelmanager::PaymentId;
use lightning::ln::funding::SpliceContribution;
Expand Down Expand Up @@ -1267,29 +1267,27 @@ impl Node {
const EMPTY_SCRIPT_SIG_WEIGHT: u64 =
1 /* empty script_sig */ * bitcoin::constants::WITNESS_SCALE_FACTOR as u64;

// Used for creating a redeem script for the previous funding txo and the new funding
// txo. Only needed when selecting which UTXOs to include in the funding tx that would
// be sufficient to pay for fees. Hence, the value does not matter.
let dummy_pubkey = PublicKey::from_slice(&[2; 33]).unwrap();

let funding_txo = channel_details.funding_txo.ok_or_else(|| {
log_error!(self.logger, "Failed to splice channel: channel not yet ready",);
Error::ChannelSplicingFailed
})?;

let funding_output = channel_details.get_funding_output().ok_or_else(|| {
log_error!(self.logger, "Failed to splice channel: channel not yet ready");
Error::ChannelSplicingFailed
})?;

let shared_input = Input {
outpoint: funding_txo.into_bitcoin_outpoint(),
previous_utxo: bitcoin::TxOut {
value: Amount::from_sat(channel_details.channel_value_satoshis),
script_pubkey: make_funding_redeemscript(&dummy_pubkey, &dummy_pubkey)
.to_p2wsh(),
},
previous_utxo: funding_output.clone(),
satisfaction_weight: EMPTY_SCRIPT_SIG_WEIGHT + FUNDING_TRANSACTION_WITNESS_WEIGHT,
};

let shared_output = bitcoin::TxOut {
value: shared_input.previous_utxo.value + Amount::from_sat(splice_amount_sats),
script_pubkey: make_funding_redeemscript(&dummy_pubkey, &dummy_pubkey).to_p2wsh(),
// will not actually be the exact same script pubkey after splice
// but it is the same size and good enough for coin selection purposes
script_pubkey: funding_output.script_pubkey.clone(),
};

let fee_rate = self.fee_estimator.estimate_fee_rate(ConfirmationTarget::ChannelFunding);
Expand Down Expand Up @@ -1321,6 +1319,15 @@ impl Node {
},
};

// insert channel's funding utxo into the wallet so we can later calculate fees
// correctly when viewing this splice-in.
self.wallet.insert_txo(funding_txo.into_bitcoin_outpoint(), funding_output).map_err(
|e| {
log_error!(self.logger, "Failed to splice channel: {:?}", e);
Error::ChannelSplicingFailed
},
)?;
Comment on lines +1324 to +1329
Copy link
Contributor

Choose a reason for hiding this comment

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

@tnull Don't know what the convention is here. Should we replace the Error::PersistenceFailed here with Error::ChannelSplicingFailed?


self.channel_manager
.splice_channel(
&channel_details.channel_id,
Expand Down
10 changes: 10 additions & 0 deletions src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,15 @@ pub struct ChannelDetails {
/// state until the splice transaction reaches sufficient confirmations to be locked (and we
/// exchange `splice_locked` messages with our peer).
pub funding_txo: Option<OutPoint>,
/// The witness script that is used to lock the channel's funding output to commitment transactions.
///
/// This field will be `None` if we have not negotiated the funding transaction with our
/// counterparty already.
///
/// When a channel is spliced, this continues to refer to the original pre-splice channel
/// state until the splice transaction reaches sufficient confirmations to be locked (and we
/// exchange `splice_locked` messages with our peer).
pub funding_redeem_script: Option<bitcoin::ScriptBuf>,
/// The position of the funding transaction in the chain. None if the funding transaction has
/// not yet been confirmed and the channel fully opened.
///
Expand Down Expand Up @@ -378,6 +387,7 @@ impl From<LdkChannelDetails> for ChannelDetails {
channel_id: value.channel_id,
counterparty_node_id: value.counterparty.node_id,
funding_txo: value.funding_txo.map(|o| o.into_bitcoin_outpoint()),
funding_redeem_script: value.funding_redeem_script,
short_channel_id: value.short_channel_id,
outbound_scid_alias: value.outbound_scid_alias,
inbound_scid_alias: value.inbound_scid_alias,
Expand Down
15 changes: 14 additions & 1 deletion src/wallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use bitcoin::secp256k1::ecdh::SharedSecret;
use bitcoin::secp256k1::ecdsa::{RecoverableSignature, Signature};
use bitcoin::secp256k1::{All, PublicKey, Scalar, Secp256k1, SecretKey};
use bitcoin::{
Address, Amount, FeeRate, ScriptBuf, Transaction, TxOut, Txid, WPubkeyHash, Weight,
Address, Amount, FeeRate, OutPoint, ScriptBuf, Transaction, TxOut, Txid, WPubkeyHash, Weight,
WitnessProgram, WitnessVersion,
};
use lightning::chain::chaininterface::BroadcasterInterface;
Expand Down Expand Up @@ -153,6 +153,19 @@ impl Wallet {
Ok(())
}

pub(crate) fn insert_txo(&self, outpoint: OutPoint, txout: TxOut) -> Result<(), Error> {
let mut locked_wallet = self.inner.lock().unwrap();
locked_wallet.insert_txout(outpoint, txout);

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
})?;

Ok(())
}

fn update_payment_store<'a>(
&self, locked_wallet: &'a mut PersistedWallet<KVStoreWalletPersister>,
) -> Result<(), Error> {
Expand Down
11 changes: 8 additions & 3 deletions tests/integration_tests_rust.rs
Original file line number Diff line number Diff line change
Expand Up @@ -995,7 +995,7 @@ async fn splice_channel() {
// Splice-in funds for Node B so that it has outbound liquidity to make a payment
node_b.splice_in(&user_channel_id_b, node_a.node_id(), 4_000_000).unwrap();

expect_splice_pending_event!(node_a, node_b.node_id());
let txo = expect_splice_pending_event!(node_a, node_b.node_id());
Copy link
Collaborator

Choose a reason for hiding this comment

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

These test changes pass on main. Can you add coverage for the cases where the previous approach would lead to inaccurate fee estimations?

Copy link
Contributor Author

@benthecarman benthecarman Dec 4, 2025

Choose a reason for hiding this comment

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

The bug/issue only exists with bitcoind syncing so to do so, we'd have to change the test to sync with bitcoind rpc. Is that wanted?

expect_splice_pending_event!(node_b, node_a.node_id());

generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6).await;
Expand All @@ -1006,11 +1006,16 @@ async fn splice_channel() {
expect_channel_ready_event!(node_a, node_b.node_id());
expect_channel_ready_event!(node_b, node_a.node_id());

let splice_in_fee_sat = 252;
let expected_splice_in_fee_sat = 252;

let payments = node_b.list_payments();
let payment =
payments.into_iter().find(|p| p.id == PaymentId(txo.txid.to_byte_array())).unwrap();
assert_eq!(payment.fee_paid_msat, Some(expected_splice_in_fee_sat * 1_000));

assert_eq!(
node_b.list_balances().total_onchain_balance_sats,
premine_amount_sat - 4_000_000 - splice_in_fee_sat
premine_amount_sat - 4_000_000 - expected_splice_in_fee_sat
);
assert_eq!(node_b.list_balances().total_lightning_balance_sats, 4_000_000);

Expand Down