Introduce interactive signing state flags for funded states. #3637
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
2 times, most recently
from
4c6b6ab to
c1f430a
Compare
dunxen
changed the title
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
c1f430a to
e89ba58
Compare
Not yet. Tracked in #3636. Will need to be able to contribute inputs first to test a useful order of message exchange + restart. |
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
e89ba58 to
3b2ac55
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3637 +/- ##
==========================================
+ Coverage 89.12% 90.34% +1.21%
==========================================
Files 156 158 +2
Lines 123514 134233 +10719
Branches 123514 134233 +10719
==========================================
+ Hits 110086 121268 +11182
+ Misses 10749 10429 -320
+ Partials 2679 2536 -143 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
3b2ac55 to
5110ecc
Compare
|
@dunxen re-request when this is ready for review again, feel free to squash as well |
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
5110ecc to
1d96044
Compare
|
🔔 1st Reminder Hey @TheBlueMatt @jkczyz @wpaulino! This PR has been waiting for your review. |
| @@ -6924,11 +6933,72 @@ impl<SP: Deref> FundedChannel<SP> where | |||
| log_debug!(logger, "Reconnected channel {} with no loss", &self.context.channel_id()); | |||
| } | |||
|
|
|||
| // if next_funding_txid is set: | |||
There was a problem hiding this comment.
If possible, would be nice to get some test coverage in now for this
There was a problem hiding this comment.
Might be tricky at the moment, but makes sense to try do that now 👍
There was a problem hiding this comment.
Still tricky, but working on this now currently. I'll let you know if we'll have to defer this to a PR after "create outbound dual-funded channel" is complete.
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
1d96044 to
55e5f6f
Compare
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
aec4e43 to
4f6192f
Compare
lightning/src/ln/channel.rs
Outdated
| @@ -5809,7 +5848,6 @@ impl<SP: Deref> FundedChannel<SP> where | |||
| log_info!(logger, "Received initial commitment_signed from peer for channel {}", &self.context.channel_id()); | |||
|
|
|||
| let need_channel_ready = self.check_get_channel_ready(0, logger).is_some(); | |||
| self.context.channel_state = ChannelState::AwaitingChannelReady(AwaitingChannelReadyFlags::new()); | |||
| self.monitor_updating_paused(false, false, need_channel_ready, Vec::new(), Vec::new(), Vec::new()); | |||
There was a problem hiding this comment.
I don't think we need to check for channel ready here because it should be impossible to send splice_locked if tx_signatures has not been exchanged. It does mean we'll have to check it once we receive tx_signatures though.
There was a problem hiding this comment.
Will remove the check here.
We don't return a channel monitor update in tx_signatures so it doesn't make sense to pause. We'd check on next block at least. Note we do not support 0conf for dual-funded channels yet.
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
4f6192f to
21a25fb
Compare
|
Changes: git diff-tree -U1 4f6192f 21a25fb3f
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index b43e47e68..a60f3f293 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -5849,4 +5849,3 @@ impl<SP: Deref> FundedChannel<SP> where
- let need_channel_ready = self.check_get_channel_ready(0, logger).is_some();
- self.monitor_updating_paused(false, false, need_channel_ready, Vec::new(), Vec::new(), Vec::new());
+ self.monitor_updating_paused(false, false, false, Vec::new(), Vec::new(), Vec::new());
@@ -6496,3 +6495,3 @@ impl<SP: Deref> FundedChannel<SP> where
if !matches!(self.context.channel_state, ChannelState::FundingNegotiated(flags) if flags.is_interactive_signing()) {
- return Err(ChannelError::close("Received tx_signatures in strange state!".to_owned()));
+ return Err(ChannelError::Ignore("Ignoring tx_signatures received outside of interactive signing".to_owned()));
} |
Instead of having an explicit `ChannelContext::next_funding_txid` to set and read, we can get this value on the fly when it is appropriate to do so.
This follows the the specification closely in branching without being too verbose, so that it should be easy to follow the logic. See: https://github.com/lightning/bolts/blob/aa5207a/02-peer-protocol.md?plain=1#L2520-L2531
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
21a25fb to
7d19a76
Compare
lightning/src/ln/channelmanager.rs
Outdated
| @@ -11987,6 +11987,12 @@ where | |||
| } | |||
|
|
|||
| fn handle_open_channel_v2(&self, counterparty_node_id: PublicKey, msg: &msgs::OpenChannelV2) { | |||
| if !self.node_features().supports_dual_fund() { | |||
There was a problem hiding this comment.
Looks like we use init_features to check for quiescence support. Though maybe we could check the UserConfig instead of constructing the features?
There was a problem hiding this comment.
I'll just match quiescence by checking init_features for now, as I imagine we'll eventually have it enabled by default.
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
7d19a76 to
21d3c78
Compare
|
Changes: git diff-tree -U1 7d19a76 21d3c78
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index ecec47387..93dd16e7d 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -11989,3 +11989,3 @@ where
fn handle_open_channel_v2(&self, counterparty_node_id: PublicKey, msg: &msgs::OpenChannelV2) {
- if !self.node_features().supports_dual_fund() {
+ if !self.init_features().supports_dual_fund() {
let _: Result<(), _> = handle_error!(self, Err(MsgHandleErrInternal::send_err_msg_no_close( |
|
🔔 2nd Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @wpaulino! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @wpaulino! This PR has been waiting for your review. |
lightning/src/ln/channel.rs
Outdated
| // Since we have a signing_session, this implies we've sent an initial `commitment_signed`... | ||
| if !signing_session.counterparty_sent_tx_signatures { | ||
| // ...but we didn't receive a `tx_signatures` from the counterparty yet. | ||
| Some(self.funding_outpoint().txid) |
There was a problem hiding this comment.
This works for now, but the moment we try a splice/RBF this will no longer be correct. Any reason we're not getting it from the signing session?
There was a problem hiding this comment.
Right. I'll get this from the signing session.
| @@ -5842,7 +5895,7 @@ impl<SP: Deref> FundedChannel<SP> where | |||
| ) -> Result<ChannelMonitor<<SP::Target as SignerProvider>::EcdsaSigner>, ChannelError> | |||
| where L::Target: Logger | |||
| { | |||
| if !matches!(self.context.channel_state, ChannelState::FundingNegotiated) { | |||
| if !matches!(self.context.channel_state, ChannelState::FundingNegotiated(flags) if flags.is_interactive_signing()) { | |||
There was a problem hiding this comment.
Just double checking my thinking here -- it seems we don't need to handle them re-sending this as part of the reestablish flow, as it should only be possible if we haven't advanced our holder_commitment_point in initial_commitment_signed.
There was a problem hiding this comment.
Yes, that's also my understanding. We don't need to handle it in the reestablish flow even though a receiving node of a next_funding_txid MUST retransmit its commitment_signed for that funding transaction.
lightning/src/ln/channel.rs
Outdated
| where L::Target: Logger | ||
| { | ||
| if !matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(_)) { | ||
| return Err(ChannelError::close("Received tx_signatures in strange state!".to_owned())); | ||
| if !matches!(self.context.channel_state, ChannelState::FundingNegotiated(flags) if flags.is_interactive_signing()) { |
There was a problem hiding this comment.
This should also check for is_their_tx_signatures_sent, no? We don't want to re-run the logic below if they resent the message.
There was a problem hiding this comment.
Yeah, it's not terrible if we do, but I'll add the extra condition in the check.
lightning/src/ln/channel.rs
Outdated
| @@ -6823,6 +6882,9 @@ impl<SP: Deref> FundedChannel<SP> where | |||
| // MonitorUpdateInProgress (and we assume the user will never directly broadcast the funding | |||
| // transaction and waits for us to do it). | |||
| let tx_signatures = self.context.monitor_pending_tx_signatures.take(); | |||
| if tx_signatures.is_some() { | |||
| self.context.channel_state.set_our_tx_signatures_ready(); | |||
There was a problem hiding this comment.
If we already received theirs, and now we're sending ours, shouldn't we transition to AwaitingChannelReady?
This intoduces the INTERACTIVE_SIGNING, THEIR_TX_SIGNATURES_SENT, and OUR_TX_SIGNATURES_SENT funded state flags. A top-level state flag for INTERACTIVE_SIGNING was avoided so that this work is compatible with splicing as well as V2 channel establishment (dual-funding). This commit also ensures that `ChannelPending` is only emitted after peers exchange `tx_signatures`.
…eived tx_signatures and queue ours
We fully persist `InteractiveTxSigningSession` as it provides the full context of the constructed transaction which is still needed for signing.
When this config field is enabled, the dual_fund feature bit will be set which determines support when receiving `open_channel2` messages.
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
21d3c78 to
692d59b
Compare
This PR includes some deferred follow-ups extracted from #3423 and introduces new state flags to track interactive signing along with persistence of the minimum information needed from a signing session to reconstruct it.
A top-level state flag was avoided so that this work is compatible with splicing as well as V2 channel establishment (dual-funding).