[Splicing] Tx negotiation during splicing #3736
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
optout21
force-pushed
the
splice-dual-tx4
branch
from
7f6dfbd to
c3778bc
Compare
1 similar comment
1 similar comment
1 similar comment
1 similar comment
1 similar comment
1 similar comment
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3736 +/- ##
==========================================
+ Coverage 89.73% 90.28% +0.55%
==========================================
Files 159 160 +1
Lines 128910 132032 +3122
Branches 128910 132032 +3122
==========================================
+ Hits 115676 119207 +3531
+ Misses 10536 10172 -364
+ Partials 2698 2653 -45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
optout21
force-pushed
the
splice-dual-tx4
branch
3 times, most recently
from
88d2e83 to
866368d
Compare
|
Ready for a new round of review. I have addressed the comments, applied most of them. There is still one to-do (update channel reserve values), that I will do, but the rest is ready for review. |
|
Ready for a new round of review. All pending and newly raised comments addressed. |
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
Getting closer :) As I see the changes needed:
In this PR or another one? Probably best would be to include these into this PR (causing some further processing time). |
Yeah, I think we need to do it in this PR. |
There was a problem hiding this comment.
When using fixup commits, please keep each one grouped under the relevant commit it should be applied to. You already have the context as to what commit the fix belongs to, and you won't have to reload that context when it's time to squash them (it also makes review easier).
|
I'm working on shared input support, can be seen in draft PR #3842 . WIP. |
optout21
force-pushed
the
splice-dual-tx4
branch
4 times, most recently
from
de1b796 to
367593b
Compare
|
Comments addressed, summary of pending changes:
|
|
Let us know when this is ready for another round, no need to wait on #3842 to get more review on this. |
optout21
force-pushed
the
splice-dual-tx4
branch
from
20233de to
e14f4c9
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
This is a simple rename, DualFundingContext to FundingNegotiationContext, to suggest that this is use not only in dual-funded channel open. Also rename the field dual_funding_context to funding_negotiation_context.
optout21
force-pushed
the
splice-dual-tx4
branch
from
e14f4c9 to
3eca72f
Compare
|
Commits squashed, rebased (post #3741), fmt. Ready for a new round of review. |
optout21
force-pushed
the
splice-dual-tx4
branch
2 times, most recently
from
be09f9a to
59e9b23
Compare
The begin_interactive_funding_tx_construction() method is extended with `is_initiator` parameter (splice initiator), and `prev_funding_input` optional parameter, containing the previous funding transaction, which will be added to the negotiation as an input by the initiator.
Introduce struct NegotiatingChannelView to perform transaction negotiation logic, on top of either PendingV2Channel (dual-funded channel open) or FundedChannel (splicing).
Fill the logic for including transaction negotiation during splicing, implement the functions: splice_channel, splice_init, splice_ack, funding_tx_constructed. Also extend the test case test_v1_splice_in with the steps for funding negotiation during splicing.
optout21
force-pushed
the
splice-dual-tx4
branch
from
59e9b23 to
3577418
Compare
| fn begin_interactive_funding_tx_construction<ES: Deref>( | ||
| &mut self, signer_provider: &SP, entropy_source: &ES, holder_node_id: PublicKey, | ||
| change_destination_opt: Option<ScriptBuf>, | ||
| is_initiator: bool, change_destination_opt: Option<ScriptBuf>, | ||
| prev_funding_input: Option<(TxIn, TransactionU16LenLimited)>, |
There was a problem hiding this comment.
Should we just drop this commit and revisit once #3842 lands? Or we can just drop the prev_funding_input part for now.
| let logger = WithChannelContext::from(logger, &chan.context, None); | ||
| chan.funding_tx_constructed(signing_session, &&logger) | ||
| let logger = WithChannelContext::from(logger, self.context(), None); | ||
| if let Ok(mut negotiating_channel) = self.as_negotiating_channel() { |
There was a problem hiding this comment.
Nit: looks like we don't need the else branch since as_negotiating_channel already returns a similar error
| /// Can be produced by: | ||
| /// - [`PendingV2Channel`], at V2 channel open, and | ||
| /// - [`FundedChannel`], when splicing. | ||
| pub struct NegotiatingChannelView<'a, SP: Deref> |
There was a problem hiding this comment.
This should be pub(crate/super)
lightning/src/ln/channelmanager.rs
Outdated
| @@ -8884,7 +8884,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/ | |||
| } | |||
|
|
|||
| #[rustfmt::skip] | |||
| fn internal_tx_msg<HandleTxMsgFn: Fn(&mut Channel<SP>) -> Result<MessageSendEvent, &'static str>>( | |||
| fn internal_tx_msg<HandleTxMsgFn: Fn(&mut Channel<SP>) -> Result<MessageSendEvent, ChannelError>>( | |||
There was a problem hiding this comment.
It doesn't look like HandleTxMsgFn needs to return a result at all, just the MessageSendEvent
| let post_value_to_self_msat_signed = (prev_funding.value_to_self_msat as i64) | ||
| .saturating_add(our_funding_contribution_sats * 1000); | ||
| if post_value_to_self_msat_signed < 0 { |
There was a problem hiding this comment.
This should already have been validated in splice_init/splice_ack when we check we still meet the reserve post-splice, right?
| let prev_funding_txid = prev_funding | ||
| .channel_transaction_parameters | ||
| .funding_outpoint | ||
| .map(|outpoint| outpoint.txid); |
There was a problem hiding this comment.
Can use prev_funding.funding_txid()
| holder_selected_contest_delay: prev_funding | ||
| .channel_transaction_parameters | ||
| .holder_selected_contest_delay, |
There was a problem hiding this comment.
Nit: could define an intermediate variable channel_parameters to make these fit into a single line
| funded_channel.splice_init( | ||
| msg, our_funding_contribution, &self.signer_provider, &self.entropy_source, | ||
| &self.get_our_node_id(), &self.logger | ||
| ), chan_entry); |
There was a problem hiding this comment.
Extract the splice_init (and splice_ack below) function call to its own variable
| interactive_tx_constructor: &mut pending_splice.interactive_tx_constructor, | ||
| interactive_tx_signing_session: &mut pending_splice.interactive_tx_signing_session, | ||
| holder_commitment_transaction_number: self.holder_commitment_point.transaction_number(), | ||
| is_splice: true, |
There was a problem hiding this comment.
Still don't think we should track this again and should just rely on splice_parent_funding_txid
Implementation of transaction negotiation during splicing.
Builds on 3407 and 3443.
No new phase,Funded(FundedChannel)is used throughout splicingFundedChannelandPendingV2Channelcan act as a transaction constructorPendingV2Channellogic is put behind a trait --FundingTxConstructorV2RenegotiatingScopeis used to store extra state during splicingFundingChannelcan act as aFundingTxConstructorV2, using the state fromRenegotiatingScope(if present)Since bothFundedChannelandFundingTxConstructorhas context(), context accessors are extracted into a common base trait,ChannelContextProvider(it is also shared byInitialRemoteCommitmentReceiver).(Also relevant: #3444)