-
Notifications
You must be signed in to change notification settings - Fork 113
Insert channel funding outputs into Wallet #726
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
31c12f9
1eca8a8
97f0ee8
4c3450e
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 |
|---|---|---|
|
|
@@ -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()); | ||
| 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) { | ||
|
Contributor
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. Hmmm... I guess we didn't really need the redeem script in
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. 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
Contributor
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. 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
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. 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!( | ||
|
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. In what case would we end up in this
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. According to the docs, the |
||
| self.logger, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -106,6 +106,22 @@ impl UniffiCustomTypeConverter for Address { | |
| } | ||
| } | ||
|
|
||
| impl UniffiCustomTypeConverter for ScriptBuf { | ||
|
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. nit: While this should be fine for now, we generally try to get away from the |
||
| 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 }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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); | ||
|
|
@@ -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
Contributor
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. @tnull Don't know what the convention is here. Should we replace the |
||
|
|
||
| self.channel_manager | ||
| .splice_channel( | ||
| &channel_details.channel_id, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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()); | ||
|
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. These test changes pass on
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. 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; | ||
|
|
@@ -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); | ||
|
|
||
|
|
||
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.
Doing this in
ChannelReadywouldn'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 realshared_inputwhen selecting UTXOs.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.
I dont think this is right, in the docs it says we get a ChannelReady for splices, also my test seems to confirm
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.
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
ChannelReadyevent for the initial funding.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.
ah i see for backward compat sake, added!