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.37% 90.01% +0.63%
==========================================
Files 157 157
Lines 124095 129376 +5281
Branches 124095 129376 +5281
==========================================
+ Hits 110915 116452 +5537
+ Misses 10469 10187 -282
- Partials 2711 2737 +26 ☔ 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. |
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
1d96044 to
55e5f6f
Compare
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
692d59b to
9ca7d03
Compare
|
🔔 3rd Reminder Hey @wpaulino! This PR has been waiting for your review. |
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
9ca7d03 to
b3c4f18
Compare
|
Changes: git diff-tree -U1 9ca7d03 b3c4f18
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 1eeca4aee..5111a5060 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -6600,6 +6600,5 @@ impl<SP: Deref> FundedChannel<SP> where
{
- if !matches!(
- self.context.channel_state,
- ChannelState::FundingNegotiated(flags) if flags.is_interactive_signing() || !flags.is_their_tx_signatures_sent()
- ) {
+ if self.interactive_tx_signing_session.is_none() ||
+ matches!(self.context.channel_state, ChannelState::FundingNegotiated(flags) if flags.is_their_tx_signatures_sent())
+ {
return Err(ChannelError::Ignore("Ignoring tx_signatures received outside of interactive signing".to_owned())); |
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
b3c4f18 to
fb00cb6
Compare
|
Changes: git diff-tree -U1 b3c4f18 fb00cb6
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 5111a5060..66305c042 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -6600,4 +6600,4 @@ impl<SP: Deref> FundedChannel<SP> where
{
- if self.interactive_tx_signing_session.is_none() ||
- matches!(self.context.channel_state, ChannelState::FundingNegotiated(flags) if flags.is_their_tx_signatures_sent())
+ if !self.context.channel_state.is_interactive_signing()
+ || self.context.channel_state.is_their_tx_signatures_sent()
{ |
1 similar comment
1 similar comment
dunxen
force-pushed
the
2025-02-interactivesigningstate
branch
from
fb00cb6 to
3c0b810
Compare
|
@dunxen Was the latest push just a rebase? |
Oh yip. Sorry, I didn't put a |
lightning/src/ln/channel.rs
Outdated
| ).into_bytes() })) | ||
| } | ||
| } else { | ||
| return Err(ChannelError::Warn("No active signing session. The associated funding transaction may have already been broadcast.".into())); |
There was a problem hiding this comment.
Seems like we could just send TxAbort anyway and not close the channel?
| if !self.context.channel_state.is_interactive_signing() | ||
| || self.context.channel_state.is_their_tx_signatures_sent() | ||
| { | ||
| return Err(ChannelError::Ignore("Ignoring tx_signatures received outside of interactive signing".to_owned())); | ||
| } | ||
|
|
||
| if let Some(ref mut signing_session) = self.interactive_tx_signing_session { |
There was a problem hiding this comment.
We may need to close the channel if they haven't sent commitment_signed first?
There was a problem hiding this comment.
Yeah just to be safe I'll add the check, but we await initial monitor persist anyway before sending our tx_signatures. I think it would be a risk for a possible manual broadcast, but I haven't checked.
lightning/src/ln/channel.rs
Outdated
| @@ -5880,7 +5933,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.
Maybe also check for them not having already sent tx_signatures?
There was a problem hiding this comment.
Makes sense. Will add. I'll ensure there's coverage for the cases in tests in #3735.
|
🔔 4th Reminder Hey @jkczyz! This PR has been waiting for your review. |
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
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`.
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
3c0b810 to
74139d0
Compare
There was a problem hiding this comment.
Changes:
git diff-tree -U1 3c0b810 74139d0
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 66305c042..e6e9c8f3b 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -5942,6 +5942,6 @@ impl<SP: Deref> FundedChannel<SP> where
{
- if !matches!(self.context.channel_state, ChannelState::FundingNegotiated(flags) if flags.is_interactive_signing()) {
+ if !matches!(self.context.channel_state, ChannelState::FundingNegotiated(flags) if flags.is_interactive_signing() && !flags.is_their_tx_signatures_sent()) {
return Err(ChannelError::Close(
(
- "Received initial commitment_signed before funding transaction constructed!".to_owned(),
+ "Received initial commitment_signed before funding transaction constructed or after peer's tx_signatures received!".to_owned(),
ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
@@ -6615,2 +6615,13 @@ impl<SP: Deref> FundedChannel<SP> where
+ // We need to close the channel if our peer hasn't sent their commitment signed already.
+ // Technically we'd wait on having an initial monitor persisted, so we shouldn't be broadcasting
+ // the transaction, but this may risk losing funds for a manual broadcast if we continue.
+ if !signing_session.has_received_commitment_signed() {
+ return Err(ChannelError::Close(
+ (
+ "Received tx_signatures before initial commitment_signed".to_string(),
+ ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) },
+ )));
+ }
+
if msg.witnesses.len() != signing_session.remote_inputs_count() {
@@ -7458,3 +7469,9 @@ impl<SP: Deref> FundedChannel<SP> where
} else {
- return Err(ChannelError::close("Counterparty set `next_funding_txid` at incorrect state".into()));
+ // We'll just send a `tx_abort` here if we don't have a signing session for this channel
+ // on reestablish and tell our peer to just forget about it.
+ // Our peer is doing something strange, but it doesn't warrant closing the channel.
+ (None, None, Some(msgs::TxAbort {
+ channel_id: self.context.channel_id(),
+ data:
+ "No active signing session. The associated funding transaction may have already been broadcast.".as_bytes().to_vec() }))
}
lightning/src/ln/channel.rs
Outdated
| ).into_bytes() })) | ||
| } | ||
| } else { | ||
| return Err(ChannelError::Warn("No active signing session. The associated funding transaction may have already been broadcast.".into())); |
lightning/src/ln/channel.rs
Outdated
| @@ -5880,7 +5933,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.
Makes sense. Will add. I'll ensure there's coverage for the cases in tests in #3735.
| if !self.context.channel_state.is_interactive_signing() | ||
| || self.context.channel_state.is_their_tx_signatures_sent() | ||
| { | ||
| return Err(ChannelError::Ignore("Ignoring tx_signatures received outside of interactive signing".to_owned())); | ||
| } | ||
|
|
||
| if let Some(ref mut signing_session) = self.interactive_tx_signing_session { |
There was a problem hiding this comment.
Yeah just to be safe I'll add the check, but we await initial monitor persist anyway before sending our tx_signatures. I think it would be a risk for a possible manual broadcast, but I haven't checked.
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).