Add FundingNeeded event for splicing#4290
Add FundingNeeded event for splicing#4290jkczyz wants to merge 16 commits intolightningdevkit:mainfrom
FundingNeeded event for splicing#4290Conversation
|
👋 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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
(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.
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.
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. |
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.
Regarding the CoinSelectionSource API, do we want to make claim_id an Option?
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.
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4290 +/- ##
==========================================
- Coverage 86.01% 85.87% -0.15%
==========================================
Files 156 156
Lines 102857 103211 +354
Branches 102857 103211 +354
==========================================
+ Hits 88476 88633 +157
- Misses 11871 12059 +188
- Partials 2510 2519 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
TheBlueMatt
left a comment
There was a problem hiding this comment.
API looks good. some misc comments i noted while skimming it
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.
Needs a call-out to what to do if you actually don't want to splice anymore (ie on failure)
There was a problem hiding this comment.
Also maybe a note that the channel is hung waiting on our response, so we need to respond quickly.
There was a problem hiding this comment.
Should they simply not handle the event? We'll be in quiescence and timeout after DISCONNECT_PEER_AWAITING_RESPONSE_TICKS. Though it seems this now inadvertently (but maybe expectedly) now applies to us sending splice_init. So maybe some renaming is in order?
Or should we expose ChannelManager::exit_quiescence?
There was a problem hiding this comment.
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.
lightning/src/ln/funding.rs
Outdated
| // FIXME: Should claim_id be an Option? | ||
| let claim_id = ClaimId([0; 32]); |
lightning/src/ln/funding.rs
Outdated
|
|
||
| /// Creates a `FundingContribution` from the template by using `wallet` to perform coin | ||
| /// selection with the given fee rate. | ||
| pub fn build_sync<W: Deref>( |
There was a problem hiding this comment.
Can we also have a method for building with a provided set of inputs rather than going through the trait?
There was a problem hiding this comment.
We could... or probably a CoinSelection so that a change output can be given.
lightning/src/ln/channel.rs
Outdated
| 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.
Don't we want to support queuing up the splice to do afterwards?
There was a problem hiding this comment.
Hmmm... this was to handle the time after we've generated FundingNeeded when we no longer have a quiescent_action but are still quiescent waiting on the user to call funding_contributed. But then we can't differentiate this from counterparty-initiated quiescence. Maybe we need to make a placeholder QuiescentAction for when we are waiting on the user to respond? Something like AwaitingFundingContribution, which could also be checked when funding_contributed is called.
There was a problem hiding this comment.
Regarding error handling, since the FundingContribution is taken by value we need to generate a SpliceFailed event to allow the user to unlock the UTXOs. However, some failure may be result of misuse (e.g., wrong channel / counterparty, unexpected funding) or bad timing (e.g., peer already disconnected timing out quiescence).
In those cases, it doesn't make sense to generate SpliceFailed. Maybe we need to use DiscardFunding for some of these cases? We'd need another FundingInfo variant, though.
Otherwise, we'd need to return the UTXOs back to the caller, which we wanted to avoid.
Thoughts?
There was a problem hiding this comment.
I'll leave this for a follow-up PR.
There was a problem hiding this comment.
Seems like the motivation for this is no longer relevant now that FundingNeeded comes before quiescence. We should definitely allow queueing the splice when quiescent, but only if we're not the initiator and/or we're attempting a different quiescent protocol (not possible yet). If we're already splicing, we should suggest doing an RBF once that's supported.
There was a problem hiding this comment.
Seems like the motivation for this is no longer relevant now that
FundingNeededcomes before quiescence.
The misuse cases still hold when calling funding_contributed. Maybe they are less likely though given it would essentially be a programmer error? Although, is it still possible when a channel is closed before calling funding_contributed?
We should definitely allow queueing the splice when quiescent, but only if we're not the initiator and/or we're attempting a different quiescent protocol (not possible yet). If we're already splicing, we should suggest doing an RBF once that's supported.
Ok, that should work, but note that once the splice is pending and we are no longer quiescent, we've lost whether we had been the initiator. So we'd need to allow queueing another splice then, which I think should be fine.
When implementing RBF, either side could include the contributions from the enqueued action.
lightning/src/ln/channel.rs
Outdated
| ) -> Result<msgs::SpliceInit, SpliceFundingFailed> | ||
| where | ||
| L::Target: Logger, | ||
| { |
There was a problem hiding this comment.
presumably we need to check that we're quiescent and its our turn to talk?
There was a problem hiding this comment.
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.
|
Given that the new API is to be reused for RBF, I wrote up a proposal to consider with this PR. @TheBlueMatt @wpaulino Looking for feedback / alternative ideas before continuing that work. Observations
Proposal
Questions
|
22958ef to
449268d
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. |
| if let Some(utxo_claim_id) = locked_utxos.get(&utxo.outpoint) { | ||
| if *utxo_claim_id != claim_id && !force_conflicting_utxo_spend { | ||
| if (utxo_claim_id.is_none() || *utxo_claim_id != claim_id) | ||
| && !force_conflicting_utxo_spend |
There was a problem hiding this comment.
We probably shouldn't ever call select_confirmed_utxos_internal with force_conflicting_utxo_spend set when the claim_id isn't set.
There was a problem hiding this comment.
Right... though it would be nice for RBF to allow forcing, but we's need something like a claim id for the splices. See #4290 (comment). Updated.
|
|
||
| // If the acceptor had a pending QuiescentAction, store the stfu message so that it can be used | ||
| // later in complete_splice_handshake. | ||
| let node_b_stfu = msg_events |
There was a problem hiding this comment.
Why not just return it? Also, isn't this being sent a bit too early? We should wait until splice_locked is exchanged to start another splice (and quiescence).
There was a problem hiding this comment.
splice_locked is being exchanged above. Returning doesn't help as we want to use complete_splice_handshake which expects the message to be there.
| is_splice: bool, | ||
| } | ||
|
|
||
| impl_writeable_tlv_based!(FundingContribution, { |
There was a problem hiding this comment.
I guess we still need this until we also get rid of QuiescentAction serialization?
|
|
||
| return Err(ChannelError::WarnAndDisconnect( | ||
| format!( | ||
| "Channel {} cannot be spliced as it already has a splice pending", |
There was a problem hiding this comment.
This is a local error, no need to bother the counterparty with it in the warning message
There was a problem hiding this comment.
Hmm... same for LegacySplice? What sort of phrasing are you looking for? FWIW, this gets logged in peer_handler.rs.
Note that we have a similar message when the action is None, but we debug_assert there. Should we do that here as well?
| self.propose_quiescence(logger, QuiescentAction::Splice { contribution, locktime }).map_err( | ||
| |(e, action)| { | ||
| log_error!(logger, "{}", e); | ||
| // FIXME: Any better way to do this? |
There was a problem hiding this comment.
I pushed a WIP commit the splits DiscardFunding out of SpliceFailed along the lines of what is mentioned in my earlier comment.
Is this in a separate PR? I don't see it here
TheBlueMatt
left a comment
There was a problem hiding this comment.
a few comments i ran out of time to finish looking at the actually-important stuff sadly
lightning/src/ln/funding.rs
Outdated
| impl_writeable_tlv_based!(FundingTxInput, { | ||
| (1, utxo, required), | ||
| (3, sequence, required), | ||
| (3, _sequence, (legacy, Sequence, |input: &FundingTxInput| Some(input.utxo.sequence))), |
There was a problem hiding this comment.
If we do read a sequence here and it differs from utxo.sequence doesn't that mean we read the default and should update? You might need/want the custom TLV read/write variant commit from #4373.
There was a problem hiding this comment.
May need a change to support this since FundingTxInput::sequence isn't a field anymore. See #4373 (comment).
| /// which UTXOs to double spend is left to the implementation, but it must strive to keep the | ||
| /// set of other claims being double spent to a minimum. | ||
| /// | ||
| /// If `claim_id` is not set, then the selection should be treated as if it were for a unique |
There was a problem hiding this comment.
This might want to be a bit stronger - if a claim fails we just RBF it later, but if a splice fails the user is gonna be confused and annoyed.
There was a problem hiding this comment.
Did you have something specific in mind? Is this with regards to unlocking the UTXOs?
| /// | ||
| /// The client should build a [`FundingContribution`] from the provided [`FundingTemplate`] and | ||
| /// pass it to [`ChannelManager::funding_contributed`]. If the method is not called while | ||
| /// handling the event, it will have the effect of canceling the splice. |
There was a problem hiding this comment.
I mean this isn't quite true, right? I can call the method after handling the event, it just has to happen sooner rather than later?
There was a problem hiding this comment.
Ah, I had the thought of checking the event queue when calling ChannelManager::funding_contributed similar to what we are doing in splice_channel, which would prevent calling it later. But we don't need to be that strict. Any feelings on that?
At various points we've been stuck in our TLV read/write variants but just want to break out and write some damn code to initialize a field and some more code to decide what to write for a TLV. We added the write-side part of this with the `legacy` TLV read/write variant, but its useful to also be able to specify a function which is called on the read side. Here we add a `custom` TLV read/write variant which calls a method both on read and write to either decide what to write or to map a read value (if any) to the final field.
449268d to
7155b4b
Compare
7155b4b to
6b4d229
Compare
Update the `legacy` TLV read/write variant signature from `(legacy, $fieldty, $write)` to `(legacy, $fieldty, $read, $write)`, adding a read closure parameter matching the `custom` variant's signature. The read closure is applied in `_check_missing_tlv!` after all TLV fields are read but before `static_value` fields consume legacy values. This preserves backwards compatibility with `static_value` and `default_value` expressions that reference legacy field variables as `Option<$fieldty>` during TLV reading. The read closure signature matches `custom`: `FnOnce(Option<$fieldty>) -> Result<Option<$fieldty>, DecodeError>`. All existing usage sites use `Ok` as their read closure (identity/ no-op). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
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.
Instead of logging both inside propose_quiescence and at the call site, only log inside it. This simplifies the return type.
6b4d229 to
f16d632
Compare
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 #4390.
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.