-
Notifications
You must be signed in to change notification settings - Fork 435
Add FundingNeeded event for splicing
#4290
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?
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
f5933e5 to
854e9ca
Compare
|
@TheBlueMatt @wpaulino Looking for some high-level feedback on the API introduced in the last commit. In summary:
The same mechanism can be used later for contributing inputs for counterparty-initiated splices or v2 channel opens since Test code still needs to be fixed up, and |
wpaulino
left a comment
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.
The API design LGTM, though there's one issue with WalletSource. One thing users need to keep in mind now is that from the moment they receive FundingNeeded, they need to act quickly to ensure the counterparty doesn't disconnect due to quiescence taking too long.
| fn list_confirmed_utxos(&self) -> Result<Vec<Utxo>, ()>; | ||
|
|
||
| /// | ||
| fn select_confirmed_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.
Adding this here now requires implementers to satisfy this method when, in the context of anchor channels, WalletSource is only intended to be used such that we perform coin selection on behalf of the user. Ideally, we also give users the option between choosing WalletSource/CoinSelectionSource when funding channels.
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.
Right, I guess I'm a bit confused why we can't use select_confirmed_utxos as-is? Indeed the claim_id is annoying, but we can make that either an enum across a ClaimId and some unit value describing a splice or just make it an Option. Aside from that it seems to be basically what we want.
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.
Adding this here now requires implementers to satisfy this method when, in the context of anchor channels,
WalletSourceis only intended to be used such that we perform coin selection on behalf of the user. Ideally, we also give users the option between choosingWalletSource/CoinSelectionSourcewhen funding channels.
Hmm... I see. Would a separate trait be desirable? Also, see my reply to @TheBlueMatt below.
Right, I guess I'm a bit confused why we can't use
select_confirmed_utxosas-is? Indeed theclaim_idis annoying, but we can make that either anenumacross aClaimIdand some unit value describing a splice or just make it anOption. Aside from that it seems to be basically what we want.
The return value also isn't compatible. It contains Utxos but we also need the previous tx and sequence number as part of each FundingTxInput. Though its constructor will give a default sequence number.
We could change CoinSelection to use FundingTxInput instead of Utxo, but that would be odd for use with the anchor context.
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.
Honestly that seems fine to me? We expect ~all of our users to want to use splicing, which implies they need to support the "return coin selection with full transactions" interface. So what if anchors throw away some of that data?
If we feel strongly about it we can add a new trait method that does return the full transactions and provide a default implementation for the current method so that those that really want to avoid always fetching the transaction data can.
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.
Right, I don't have a strong opinion, but note that Wallet's implementation of CoinSelectionSource::select_confirmed_utxos delegates to WalletSource::list_confirmed_utxos. So it might be expensive to use that abstraction. @wpaulino WDYT?
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 was thinking of keeping
CoinSelectionSourcethe same (with the full transaction data in the response, or with the default-impl-method described above) but changingWalletSourceso that we don't have to fetch all previous-transactions at the start.
Yeah, Wallet wraps a WalletSource and implements CoinSelectionSource by listing all the UTXOs and selecting from them. So WalletSource's interface would remain unchanged while CoinSelection would use FundingTxInput instead of Utxo. Which I guess means FundingTemplate::build should actually take a CoinSelectionSource.
Seems reasonable to require a sequence number in the response for that as well, even for anchors?
Hmm... in WalletSource::list_confirmed_utxos by adding a field to Utxto (and removing it from FundingTxInput)? Or by having the CoinSelectionSource implementation fill it in on FundingTxInput?
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.
So WalletSource's interface would remain unchanged
Wouldn't we need a WalletSource::get_previous_transaction_for_utxo method to fetch the full tx data for the UTXOs we selected?
Hmm... in WalletSource::list_confirmed_utxos by adding a field to Utxto (and removing it from FundingTxInput)? Or by having the CoinSelectionSource implementation fill it in on FundingTxInput?
ISTM we should replace Utxo with FundingTxInput since FundingTxInput has strictly more fields (it contains a Utxo!) and we'd move to returning FundingTxInput from the trait.
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.
Wouldn't we need a
WalletSource::get_previous_transaction_for_utxomethod to fetch the full tx data for the UTXOs we selected?
Right, we need another method for that.
ISTM we should replace
UtxowithFundingTxInputsinceFundingTxInputhas strictly more fields (it contains aUtxo!) and we'd move to returningFundingTxInputfrom the trait.
The question is more what should be setting Sequence? Either:
(1) Move it to Utxo and have WalletSource::list_confirmed_utxos set it since it returns Vec<Utxo>.
(2) Have CoinSelectionSource::select_confirmed_utxos set it since CoinSelection would now contain Vec<FundingTxInput>
We just can't replace Utxo with FundingTxInput in WalletSource::list_confirmed_utxos since we don't want to return the previous tx there.
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.
(1) Move it to Utxo and have WalletSource::list_confirmed_utxos set it since it returns Vec.
Presumably this. No reason to want it to not be possible in WalletSource.
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.
Done as discussed here and offline. I'm in the middle of updating the tests, but I've pushed an update for now.
TheBlueMatt
left a comment
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.
Aside from the above which-interface question I think the API is good.
854e9ca to
6d78c3f
Compare
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
6d78c3f to
94b1aa9
Compare
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 2nd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 3rd Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
A forthcoming commit will change CoinSelection to include FundingTxInput instead of Utxo, though the former will probably be renamed. This is so CoinSelectionSource can be used when funding a splice. Further updating WalletSource to use FundingTxInput is not desirable, however, as it would result in looking up each confirmed UTXOs previous transaction even if it is not selected. See Wallet's implementation of CoinSelectionSource, which delegates to WalletSource for listing all confirmed UTXOs. This commit moves FundingTxInput::sequence to Utxo, and thus the responsibility for setting it to WalletSource implementations. Doing so will allow Wallet's CoinSelectionSource implementation to delegate looking up previous transactions to WalletSource without having to explicitly set the sequence on any FundingTxInput.
In order to reuse CoinSelectionSource for splicing, the previous transaction of each UTXO is needed. Update CoinSelection to use FundingTxInput (renamed to ConfirmedUtxo) so that it is available. This requires adding a method to WalletSource to look up a previous transaction for a UTXO. Otherwise, Wallet's implementation of CoinSelectionSource would need WalletSource to include the previous transactions when listing confirmed UTXOs to select from. But this would be inefficient since only some UTXOs are selected.
94b1aa9 to
c3f3453
Compare
lightning/src/ln/funding.rs
Outdated
| // FIXME: Should claim_id be an Option? | ||
| let claim_id = ClaimId([0; 32]); |
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.
Regarding the CoinSelectionSource API, do we want to make claim_id an Option?
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.
IMO yes
lightning/src/ln/splicing_tests.rs
Outdated
| Amount::from_sat(383) | ||
| Amount::from_sat(385) | ||
| } else { | ||
| Amount::from_sat(384) | ||
| Amount::from_sat(386) |
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.
Seems select_confirmed_utxos_internal might be off on the change calculation because it's using the weight of the change output to compute additional fees instead of re-computing the total fees using the total weight when including a change output.
c3f3453 to
3253a99
Compare
|
🔔 4th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
1 similar comment
|
🔔 4th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
f2d9fa5 to
fcccbac
Compare
|
🔔 5th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
|
We discussed this a bunch offline on friday and I believe there are some further changes here. Let us know when you're ready for another look. |
fcccbac to
12eee3d
Compare
Pushed changes to move Additionally, updated Working on separating contributed inputs and outputs from |
lightning/src/ln/channel.rs
Outdated
| .map_err(|e| APIError::APIMisuseError { err: e.to_owned() }) | ||
| // Don't use propose_quiescence as we don't want to initiate quiescence until FundingNeeded | ||
| // has been processed. This prevents initiating other quiescent actions in the meanwhile. | ||
| self.quiescent_action = Some(QuiescentAction::AwaitingFunding); |
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.
This changes the semantics of QuiescentAction from an action to take after becoming quiescent to also include actions before becoming quiescent. Can we instead rely on a FundingNeeded event still needing to be handled to fail early 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.
Yeah, that was essentially what this was standing in for. Alternatively, IIUC, we'd need some other state to track that, right? Something like adding another flag to ChannelReadyFlags?
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.
Discussed offline. We'll eventually want to queue up QuiescentActions, but it might be fine to let a second action jump ahead if the user hasn't processed FundingNeeded. The recent push checks if the FundingNeeded event is present to determine if a second splice attempt should be rejected. PTAL.
2e44da8 to
1c8e7e9
Compare
|
Squashed |
CoinSelectionSource is used for anchor bumping where a ClaimId is passed in to avoid double spending other claims. To re-use this trait for funding a splice, the ClaimId must be optional. And, if None, then any locked UTXOs may be considered ineligible by an implementation.
Rather than requiring the user to pass FundingTxInputs when initiating a splice, generate a FundingNeeded event once the channel has become quiescent. This simplifies error handling and UTXO / change address clean-up by consolidating it in SpliceFailed event handling. Later, this event will be used for opportunistic contributions (i.e., when the counterparty wins quiescence or initiates), dual-funding, and RBF.
Now that CoinSelection is used to fund a splice funding transaction, use that for determining of a change output should be used. Previously, the initiator could either provide a change script upfront or let LDK generate one using SignerProvider::get_destination_script. Since older versions may have serialized a SpliceInstruction without a change script while waiting on quiescence, LDK must still generate a change output in this case.
Wallet-related types were tightly coupled to bump_transaction, making them less accessible for other use cases like channel funding and splicing. Extract these utilities to a dedicated module for improved code organization and reusability across the codebase. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Synchronous wallet utilities were coupled to bump_transaction::sync, limiting their reusability for other features like channel funding and splicing which need synchronous wallet operations. Consolidate all wallet utilities in a single module for consistency and improved code organization. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
FundingTxInput was originally designed for channel funding but is now used more broadly for coin selection and splicing. The name ConfirmedUtxo better reflects its general-purpose nature as a confirmed UTXO with previous transaction data. Make ConfirmedUtxo the real struct in wallet_utils and alias FundingTxInput to it for backward compatibility. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1c8e7e9 to
78b4f3c
Compare
jkczyz
left a comment
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 can split out the wallet_utils refactor into a separate PR, if preferred.
lightning/src/events/mod.rs
Outdated
| /// Indicates that funding is needed for a channel splice or a dual-funded channel open. | ||
| /// | ||
| /// The client should build a [`FundingContribution`] from the provided [`FundingTemplate`] and | ||
| /// pass it to [`ChannelManager::funding_contributed`]. |
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.
Now that the presence of this event in the pending events queue determines whether or not a splice can be initiated, no action is needed from the user if they no longer want to splice.
| ) -> Result<msgs::SpliceInit, SpliceFundingFailed> | ||
| where | ||
| L::Target: Logger, | ||
| { |
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.
Based on offline discussion, we are no longer waiting to enter quiescence to generate FundingNeeded. It will instead be initiated when funding_contributed is called.
| } | ||
|
|
||
| /// An unspent transaction output with at least one confirmation. | ||
| pub type ConfirmedUtxo = FundingTxInput; |
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.
Moved to wallet_utils courtesy of claude. I reversed the type alias for now. I could drop it if you prefer. Just seems like FundingTxInput reads a little better in funding contexts.
| if self.context.channel_state.is_quiescent() { | ||
| return Err(APIError::APIMisuseError { | ||
| err: format!( | ||
| "Channel {} cannot be spliced as it is already quiescent", |
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'll leave this for a follow-up PR.
|
🔔 1st Reminder Hey @TheBlueMatt @wpaulino! This PR has been waiting for your review. |
| } | ||
| } | ||
| Ok(tx) | ||
| // FIXME: Why does handle_channel_close override the witness? |
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.
Not sure i understand this comment
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, this is related to two commits later where test_op_return_under_funds is updated to not use a hardcoded value for the outpoint's txid. But then I realized the calling code handle_channel_close overrides the witness.
So my question is why are we setting the witnesses in TestCoinSelectionSource::sign_pstb if not necessary for the test? Maybe that test should be checking its TestBroadcaster?
| // TODO: Do we care about cleaning this up once the UTXOs have a confirmed spend? We can do so | ||
| // by checking whether any UTXOs that exist in the map are no longer returned in | ||
| // `list_confirmed_utxos`. | ||
| locked_utxos: Mutex<HashMap<OutPoint, Option<ClaimId>>>, |
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.
Do we want to handle DiscardFunding now so that we can drop entries if a splice RBFs?
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.
As in add a method on Wallet? Yeah, though I'll likely do that in a follow-up when refactoring DiscardFunding from SpliceFailed events.
For RBF, did we want to produce DiscardFunding before FundingNeeded so that the UTXOs can be reused in the RBF? Though an earlier splice / RBF attempt may still get confirmed...
IIUC, we already have DiscardFunding events generated by the ChannelMonitor once the splice is promoted. But ISTM that wouldn't let re-use the UTXOs in the RBF tx unless we generalize ClaimId to be used for splicing / dual-funding? Though, at least in the Wallet implementation`, we'll fall back to reusing UTXOs if we don't have any others to use.
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
Rather than requiring the user to pass
FundingTxInputs when initiating a splice, generate aFundingNeededevent once the channel has become quiescent. This simplifies error handling and UTXO / change address clean-up by consolidating it inSpliceFailedevent handling.Later, this event will be used for opportunistic contributions (i.e., when the counterparty wins quiescence or initiates), dual-funding, and RBF.
Based on #4261.
This is still fairly rough. It does not yet include any code for creating aFundingNegotiationContextfrom aFundingContribution. The former may need to a dedicated struct instead so that any data needed fromChannelManagerorChannelContextcan be produced internally. Alternatively, that data could be included inFundingContribution, but it would need to be serializable.