-
Notifications
You must be signed in to change notification settings - Fork 374
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
Follow-ups to PR 3137 #3423
base: main
Are you sure you want to change the base?
Follow-ups to PR 3137 #3423
Conversation
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.
Could you expand upon the commit messages? They should include both the what and the why (when not obvious) without relying in Github PRs or issues.
let channel_phase = chan_phase_entry.get_mut(); | ||
match channel_phase { | ||
ChannelPhase::UnfundedOutboundV2(chan) => { | ||
if chan.interactive_tx_signing_session.is_some() { | ||
let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry(); | ||
match channel_phase { | ||
ChannelPhase::UnfundedOutboundV2(chan) => { | ||
(channel_id, chan.into_channel()) | ||
} | ||
_ => { | ||
debug_assert!(false, "The channel phase was not UnfundedOutboundV2"); | ||
let err = ChannelError::close( | ||
"Closing due to unexpected sender error".into()); | ||
return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase, | ||
&channel_id).1) | ||
} | ||
} | ||
} else { | ||
let logger = WithChannelContext::from(&self.logger, &chan.context, None); | ||
log_error!(logger, "Persisting initial ChannelMonitor failed, implying the funding outpoint was duplicated"); | ||
try_chan_phase_entry!(self, peer_state, Err(ChannelError::Close( | ||
( | ||
"Channel funding outpoint was a duplicate".to_owned(), | ||
ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }, | ||
) | ||
)), chan_phase_entry) | ||
return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( | ||
"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry); | ||
} | ||
} else { | ||
}, | ||
ChannelPhase::UnfundedInboundV2(chan) => { | ||
// TODO(dual_funding): This should be somewhat DRYable when #3418 is merged. | ||
if chan.interactive_tx_signing_session.is_some() { | ||
let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry(); | ||
match channel_phase { | ||
ChannelPhase::UnfundedInboundV2(chan) => { | ||
(channel_id, chan.into_channel()) | ||
} | ||
_ => { | ||
debug_assert!(false, "The channel phase was not UnfundedInboundV2"); | ||
let err = ChannelError::close( | ||
"Closing due to unexpected sender error".into()); | ||
return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase, | ||
&channel_id).1) | ||
} | ||
} | ||
} else { | ||
return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( | ||
"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry); | ||
} | ||
}, |
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.
Is it the case that only one of interactive_tx_signing_session
or interactive_tx_constructor
will be Some
at a given time?
More and more it seems having phases for FundingTxConstruction
and FundingTxSigning
(with a property for inbound/outbound) may be more appropriate than phases for inbound/outbound. Probably not worth changing here, but we may want to consider it for post #3418.
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.
Oh, I see we use interactive_tx_signing_session
in the Funded
phase but only for v2 channels. Would moving to the construction suggested above prevent needing interactive_tx_signing_session
in Channel
? That seems ideal given it isn't applicable for v1 channels.
@TheBlueMatt Do you see a problem with that? IIUC, we would transition to the Funded
phase later for v2 channels. I'm not sure what implications that may have for #3137 (comment) and #3137 (comment). It seems we would need to persist both in the new FundingTxSigning
phase and the Funded
phase?
Also, just noticing when we read a Channel
we unconditionally set interactive_tx_signing_session
to None
when reading. Won't this be a problem for v2 channels as currently written?
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.
Yes that could prevent interactive_tx_signing_sesion
from appearing in Channel
. I'll need to look at the splicing work, but it seems we could reuse that phase for a new splice and for interactive RBF attempts.
Regarding, signing session persistence, yes that's needed in this PR... I believe we at least persist the funding_tx
already, but I'll look at what else might need persistence and do that in a separate commit.
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 think we can already drop the signing session after this PR because we move to Funded
later.
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.
We still rely on the signing session when receiving a remote's tx_signatures
(e.g. InteractiveTxSigningSession::received_tx_signatures
which depends on `ConstructedTransaction::add_remote_witnesses, otherwise I'd just lift it up to
Channel` but not sure that's the right place for it). The other issues in this PR depend on figuring out what to do about this.
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, yea, damn. Indeed, I agree with @jkczyz that redoing the phases to have some "ChannelFundedAwaitingSignatures" phase (and maybe not have an outbound/inbound distinction) makes sense, but certainly not for this PR.
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.
Cleaning up commits and adding explanations, and then we can discuss ac3fdbf#r1857415099 further.
let channel_phase = chan_phase_entry.get_mut(); | ||
match channel_phase { | ||
ChannelPhase::UnfundedOutboundV2(chan) => { | ||
if chan.interactive_tx_signing_session.is_some() { | ||
let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry(); | ||
match channel_phase { | ||
ChannelPhase::UnfundedOutboundV2(chan) => { | ||
(channel_id, chan.into_channel()) | ||
} | ||
_ => { | ||
debug_assert!(false, "The channel phase was not UnfundedOutboundV2"); | ||
let err = ChannelError::close( | ||
"Closing due to unexpected sender error".into()); | ||
return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase, | ||
&channel_id).1) | ||
} | ||
} | ||
} else { | ||
let logger = WithChannelContext::from(&self.logger, &chan.context, None); | ||
log_error!(logger, "Persisting initial ChannelMonitor failed, implying the funding outpoint was duplicated"); | ||
try_chan_phase_entry!(self, peer_state, Err(ChannelError::Close( | ||
( | ||
"Channel funding outpoint was a duplicate".to_owned(), | ||
ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }, | ||
) | ||
)), chan_phase_entry) | ||
return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( | ||
"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry); | ||
} | ||
} else { | ||
}, | ||
ChannelPhase::UnfundedInboundV2(chan) => { | ||
// TODO(dual_funding): This should be somewhat DRYable when #3418 is merged. | ||
if chan.interactive_tx_signing_session.is_some() { | ||
let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry(); | ||
match channel_phase { | ||
ChannelPhase::UnfundedInboundV2(chan) => { | ||
(channel_id, chan.into_channel()) | ||
} | ||
_ => { | ||
debug_assert!(false, "The channel phase was not UnfundedInboundV2"); | ||
let err = ChannelError::close( | ||
"Closing due to unexpected sender error".into()); | ||
return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase, | ||
&channel_id).1) | ||
} | ||
} | ||
} else { | ||
return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( | ||
"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry); | ||
} | ||
}, |
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.
Yes that could prevent interactive_tx_signing_sesion
from appearing in Channel
. I'll need to look at the splicing work, but it seems we could reuse that phase for a new splice and for interactive RBF attempts.
Regarding, signing session persistence, yes that's needed in this PR... I believe we at least persist the funding_tx
already, but I'll look at what else might need persistence and do that in a separate commit.
ac3fdbf
to
0707e53
Compare
Still needs:
|
e29bc7d
to
26fb59f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3423 +/- ##
==========================================
- Coverage 89.70% 89.64% -0.06%
==========================================
Files 130 130
Lines 107422 107488 +66
Branches 107422 107488 +66
==========================================
Hits 96362 96362
- Misses 8669 8723 +54
- Partials 2391 2403 +12 ☔ View full report in Codecov by Sentry. |
let channel_phase = chan_phase_entry.get_mut(); | ||
match channel_phase { | ||
ChannelPhase::UnfundedOutboundV2(chan) => { | ||
if chan.interactive_tx_signing_session.is_some() { | ||
let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry(); | ||
match channel_phase { | ||
ChannelPhase::UnfundedOutboundV2(chan) => { | ||
(channel_id, chan.into_channel()) | ||
} | ||
_ => { | ||
debug_assert!(false, "The channel phase was not UnfundedOutboundV2"); | ||
let err = ChannelError::close( | ||
"Closing due to unexpected sender error".into()); | ||
return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase, | ||
&channel_id).1) | ||
} | ||
} | ||
} else { | ||
let logger = WithChannelContext::from(&self.logger, &chan.context, None); | ||
log_error!(logger, "Persisting initial ChannelMonitor failed, implying the funding outpoint was duplicated"); | ||
try_chan_phase_entry!(self, peer_state, Err(ChannelError::Close( | ||
( | ||
"Channel funding outpoint was a duplicate".to_owned(), | ||
ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) }, | ||
) | ||
)), chan_phase_entry) | ||
return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( | ||
"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry); | ||
} | ||
} else { | ||
}, | ||
ChannelPhase::UnfundedInboundV2(chan) => { | ||
// TODO(dual_funding): This should be somewhat DRYable when #3418 is merged. | ||
if chan.interactive_tx_signing_session.is_some() { | ||
let (channel_id, mut channel_phase) = chan_phase_entry.remove_entry(); | ||
match channel_phase { | ||
ChannelPhase::UnfundedInboundV2(chan) => { | ||
(channel_id, chan.into_channel()) | ||
} | ||
_ => { | ||
debug_assert!(false, "The channel phase was not UnfundedInboundV2"); | ||
let err = ChannelError::close( | ||
"Closing due to unexpected sender error".into()); | ||
return Err(convert_chan_phase_err!(self, peer_state, err, &mut channel_phase, | ||
&channel_id).1) | ||
} | ||
} | ||
} else { | ||
return try_chan_phase_entry!(self, peer_state, Err(ChannelError::close( | ||
"Got a commitment_signed message for a V2 channel before funding transaction constructed!".into())), chan_phase_entry); | ||
} | ||
}, |
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 think we can already drop the signing session after this PR because we move to Funded
later.
let mut occupied_entry = peer_state.channel_by_id.entry(channel_id).insert(ChannelPhase::Funded(chan)); | ||
let channel_phase_entry = occupied_entry.get_mut(); | ||
match channel_phase_entry { | ||
ChannelPhase::Funded(chan) => { handle_new_monitor_update!(self, persist_state, peer_state_lock, peer_state, |
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.
While we can definitely transition to Funded
later (and IMO probably should, for consistency and to enable us to remove the interactive_tx_signing_session
from Channel
as discussed above), once we start adding our own inputs, we have to have the ChannelMonitor
on disk before we can send our funding signatures, so this probably needs to move earlier.
lightning/src/ln/channel.rs
Outdated
(None, None, Some(msgs::TxAbort { channel_id: self.context.channel_id(), data: vec![] })) | ||
} | ||
} else { | ||
// Counterparty set `next_funding_txid` at incorrect state. |
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.
Did they? It could be that we sent our tx_signatures
but they didn't receive them, no?
lightning/src/ln/channel.rs
Outdated
@@ -9898,6 +9908,28 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch | |||
let mut next_holder_commitment_point_opt: Option<PublicKey> = None; | |||
let mut is_manual_broadcast = None; | |||
|
|||
let interactive_tx_signing_session = if matches!(channel_state, ChannelState::FundingNegotiated) { |
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.
Its weird to me to recreate an empty interactive_tx_signing_session
on startup just to trick some later code into thinking we have one. That probably indicates that we need to stop looking for the presence of interactive_tx_signing_session
to determine the state the channels in and instead look at the existing state (channel_state
and the commitment numbers).
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.
Yes this is icky. I feel like we might need an intermediate state which may only make sense for dual-funded channels. I will try to avoid introducing that as much as possible, though.
How's progress here looking? As we move towards an 0.1-beta hopefully in a week we should really make sure to get this landed! |
Yeah it's pretty important. I should be able to get the latest fixes up by Saturday. Hopefully Friday, though, so it can get a final look before next week. |
8f5d100
to
38e43b2
Compare
This replaces the hard panic from this case.
By moving the txid check to the start of `tx_signatures` handling, we can avoid spurious witness count mismatch errors. Also, it just makes more sense to check the txid earlier.
We actually don't need to check if the counterparty had already sent their `tx_signatures` in `InteractiveTxSigningSession::received_tx_signatures`. Further, we can get rid of the clone of `funding_tx_opt` in `Channel::tx_signatures` when setting the `Context::funding_transaction` as we don't actually need to propagate it through to `ChannelManager::internal_tx_complete` as we can use `ChannelContext::unbroadcasted_funding()` which clones the `ChannelContext::funding_transaction` anyway.
In a following commit we change the state at which a V2 channel is promoted to a `Channel` (after monitor persistence) to avoid a crash upon reading a `Channel` with no corresponding monitor persisted. This means that we will have no need for an optional (based on signing order) `TxSignatures` being returned when `provide_holder_witnesses` is called. The above is also the reason for removing the `received_commitment_signed` and signing order checks in the body of `provide_holder_witnesses`. These checks will only be important when we actually contribute inputs. Currently, we don't so we always send `tx_signatures` first in accordance with the Interactive Transaction Construction specification: https://github.com/lightning/bolts/blob/aa5207aea/02-peer-protocol.md?plain=1#L404
We don't yet support contibuting inputs to V2 channels, so we need to debug_assert and return an error if our signing session somehow has local inputs. We also return an error if for some mystical reason, in the no input contributions case, the input count does not equal the witness count of zero.
Before this commit, unfunded V2 channels were promoted to `Channel`s in the `Funded` channel phase in `funding_tx_constructed`. Since a monitor is only created upon receipt of an initial `commitment_signed`, this would cause a crash if the channel was read from persisted data between those two events. Consequently, we also need to hold an `interactive_tx_signing_session` for both of our unfunded V2 channel structs.
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
We allow persisting `Channel`s in a `ChannelState::FundingNegotiated` only if the holder commitment number has advanced to the second commitment number. Currently we don't need new persistence, but we will need to persist some information such as `tx_signatures` when we allow contributing funds to dual-funded channels.
Needs rebase. I think |
What are your thoughts here? We actually do need to keep the signing session around as its methods for adding signatures use the instance of ConstructedTransaction to figure out which signatures correspond to which witnesses (as they are in order but could be interleaved with the counterparty so serial id is still needed) |
38e43b2
to
b729f2c
Compare
Ah, sorry missed that. IMO we should just leave this PR as-is then and consider it later. |
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'm very skeptical of the reestablish logic without any test coverage, honestly. I'm kinda wondering if we shouldn't try to cfg-flag the v2 establish again for 0.1 :/
debug_assert!(false); | ||
return Err(ChannelError::Close(("Tried to get an initial commitment_signed messsage at a time other than \ | ||
immediately after initial handshake completion (or tried to get funding_created twice)".to_string(), | ||
ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(false) } |
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.
Hmm, we should broadcast, right? May be the channel isn't funded, but we may be funded (hence why we hit the "wrong state" case), in which case we need to broadcast.
if let Some(tx_signatures) = tx_signatures_opt { | ||
peer_state.pending_msg_events.push(events::MessageSendEvent::SendTxSignatures { | ||
node_id: *counterparty_node_id, | ||
msg: tx_signatures, | ||
}); | ||
} | ||
if let Some(ref funding_tx) = funding_tx_opt { | ||
if let Some(ref funding_tx) = chan.context.unbroadcasted_funding() { |
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 have test coverage for ensuring we don't broadcast before the initial monitor is done for v2? This looks somewhat brittle (cc #3411).
@@ -1801,15 +1783,19 @@ pub(super) trait InteractivelyFunded<SP: Deref> where SP::Target: SignerProvider | |||
}, | |||
}; | |||
|
|||
let funding_ready_for_sig_event = None; | |||
if signing_session.local_inputs_count() == 0 { | |||
let funding_ready_for_sig_event = if signing_session.local_inputs_count() == 0 { |
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 is never set to Some
.
ChannelPhase::UnfundedOutboundV2(chan) => chan.interactive_tx_signing_session = Some(signing_session), | ||
ChannelPhase::UnfundedInboundV2(chan) => chan.interactive_tx_signing_session = Some(signing_session), |
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.
We can inline these into the above match, no? Why bother doing two matches?
(channel_id, chan.into_channel()) | ||
} | ||
_ => { | ||
debug_assert!(false, "The channel phase was not UnfundedOutboundV2"); |
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.
Heh, I mean in general we shouldn't panic, but when we just checked it five lines earlier I'm not sure handling the unreachable case is worth it :)
None | ||
} else { | ||
// ...and we received a `tx_signatures` from the counterparty. | ||
Some(self.funding_outpoint().txid) |
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.
...and we MUST NOT set it if we have?
if session.unsigned_tx.compute_txid() == next_funding_txid { | ||
// if it has not received tx_signatures for that funding transaction: | ||
if !session.counterparty_sent_tx_signatures { | ||
// MUST retransmit its commitment_signed for that funding transaction. |
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 think the spec is wrong here lightning/bolts#1214. Also note that we need to add another special case to the special case of handling next_local_commitment_number == 0
at the top to check that next_funding_txid
is not set.
// if it has already received commitment_signed and it should sign first, as specified in the tx_signatures requirements: | ||
if session.received_commitment_signed && session.holder_sends_tx_signatures_first { | ||
// MUST send its tx_signatures for that funding transaction. | ||
(commitment_update, session.holder_tx_signatures.clone(), None) |
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.
Sometimes we may not get here when we need to send them. If we sent our TxSignatures
first, then received theirs we'll be in AwaitingChannelReady
, even if they didn't receive our TxSignatures
and we need to resend them. We handle the AwaitingChannelReady
case separately above, so won't even get this far.
pub counterparty_sent_tx_signatures: bool, | ||
pub holder_sends_tx_signatures_first: bool, | ||
pub received_commitment_signed: bool, | ||
pub holder_tx_signatures: Option<TxSignatures>, |
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'm kinda meh on making all these internal state fields pub
. They should mostly only be updated internally as we see the relevant message, so should probably get an accessor instead.
channel_state.set_peer_disconnected(); | ||
} else if matches!(channel_state, ChannelState::FundingNegotiated) && | ||
self.context.holder_commitment_point.transaction_number() == INITIAL_COMMITMENT_NUMBER - 1 { | ||
// This is a V2 session which has an active signing session |
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.
Can we get test coverage for this?
Closes #3416.