Conversation
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3024 +/- ##
========================================
Coverage 89.81% 89.82%
========================================
Files 121 121
Lines 99515 99657 +142
Branches 99515 99657 +142
========================================
+ Hits 89381 89518 +137
- Misses 7519 7530 +11
+ Partials 2615 2609 -6 ☔ View full report in Codecov by Sentry. |
lightning/src/events/mod.rs
Outdated
| /// [`ChannelManager::create_channel`]: crate::ln::channelmanager::ChannelManager::create_channel | ||
| /// [`ChannelManager::accept_inbound_channel`]: crate::ln::channelmanager::ChannelManager::accept_inbound_channel | ||
| /// [`UserConfig::manually_accept_inbound_channels`]: crate::util::config::UserConfig::manually_accept_inbound_channels | ||
| user_channel_id: u128, |
There was a problem hiding this comment.
You should give the funding tx to broadcast
e9ffefc to
d48453a
Compare
cb5e32f to
20313c8
Compare
lightning/src/util/config.rs
Outdated
| /// [`ChannelManager`]: crate::ln::channelmanager::ChannelManager | ||
| pub accept_mpp_keysend: bool, | ||
| /// broadcast funding transaction manually | ||
| pub manually_broadcast_outbound_channels: bool, |
There was a problem hiding this comment.
Hmm, is it best to do this via a config or via a different method to pass LDK the funding transaction? A config seems to imply people might want to just turn this on, but really its a totally differnet flow, and maybe thus makes sense as a different method since it'll change the programming entirely?
There was a problem hiding this comment.
Ok ill give that a try to see how that works.
What do you think about the next event after FundingSigned? In the current implementation its going from FundingSigned to ChannelReady ie skips the ChannelPending. I did that because in the code ChannelPending = "funding is ready and signed, just wait for confirmations", but that is not accurate for all cases. in payjoin scenario we never really know that the funding is fully signed and broadcasted.
There was a problem hiding this comment.
Hmm, we should probably keep ChannelPending, I think - its pending from the lightning POV, just something else is dealing with broadcasting.
5f7cf6c to
1aa96df
Compare
|
Done few things:
Yet to do:
|
bed2ba6 to
b3d033d
Compare
|
@TheBlueMatt @benthecarman this is ready for review |
lightning/src/events/mod.rs
Outdated
| /// | ||
| /// Upon receiving this event, it is your responsibility to broadcast the funding transaction. | ||
| /// | ||
| /// [`ChannelManager::unsafe_manual_funding_transaction_generated`]: |
There was a problem hiding this comment.
nit: These references need to live on a single line each, I believe.
lightning/src/events/mod.rs
Outdated
| /// crate::ln::channelmanager::ChannelManager::unsafe_manual_funding_transaction_generated | ||
| /// [`ChannelManager::funding_transaction_generated`]: | ||
| /// crate::ln::channelmanager::ChannelManager::funding_transaction_generated | ||
| FundingSigned { |
There was a problem hiding this comment.
Can we also include the counterparty_node_id and former_temporary_channel_id here?
lightning/src/events/mod.rs
Outdated
| /// network. For 0conf channels it will be immediately followed by the corresponding | ||
| /// [`Event::ChannelReady`] event. | ||
| /// | ||
| /// [`ChannelManager::unsafe_manual_funding_transaction_generated`]: |
There was a problem hiding this comment.
nit: Same as above: move to a single line.
lightning/src/events/mod.rs
Outdated
| f() | ||
| }, | ||
| 41u8 => { | ||
| let mut channel_id = ChannelId::new_zero(); |
There was a problem hiding this comment.
Let's use RequiredWrapper(None) instead of initializing these defaults.
lightning/src/ln/channel.rs
Outdated
| /// The funding transaction can be accessed through the [`Event::FundingSigned`] event. | ||
| /// | ||
| /// [`Event::FundingSigned`]: crate::events::Event::FundingSigned | ||
| manually_broadcast_outbound_channels: Option<()>, |
There was a problem hiding this comment.
Why is this an Option<()> rather than a bool? Also, given this is a flag indicating whether we broadcast the transaction, should this start with did_ or is_?
There was a problem hiding this comment.
The Option<()> is mainly because other variables like is_batch_funding and local_initiated_shutdown are defined with Option so I was trying to follow that. I was sure it saves a byte or two but it seems it doesnt. happy to change it back to bool
lightning/src/ln/channel.rs
Outdated
| pub fn is_funding_broadcast(&self) -> bool { | ||
| !self.channel_state.is_pre_funded_state() && | ||
| !matches!(self.channel_state, ChannelState::AwaitingChannelReady(flags) if flags.is_set(AwaitingChannelReadyFlags::WAITING_FOR_BATCH)) | ||
| !self.channel_state.is_pre_funded_state() |
There was a problem hiding this comment.
nit: Remove the superfluous indent. Generally, since nothing seems to have actually changed here, might be preferable to not touch this code at all?
lightning/src/ln/channel.rs
Outdated
| (matches!(self.context.channel_state, ChannelState::AwaitingChannelReady(flags) if !flags.is_set(AwaitingChannelReadyFlags::WAITING_FOR_BATCH)) || | ||
| matches!(self.context.channel_state, ChannelState::ChannelReady(_))) | ||
| { | ||
| if self.context.manually_broadcast_outbound_channels.is_none() |
There was a problem hiding this comment.
Same here: if nothing actually changed, it would be preferable to not touch it, or do the reformatting in a separate commit.
lightning/src/ln/channelmanager.rs
Outdated
| } | ||
| if result != Ok(()) { return result; } | ||
|
|
||
| return self.batch_funding_transaction_generated_intern(temporary_channels, funding_transaction, false); |
There was a problem hiding this comment.
Could just avoid the above if and do
| return self.batch_funding_transaction_generated_intern(temporary_channels, funding_transaction, false); | |
| result.and(self.batch_funding_transaction_generated_intern(temporary_channels, funding_transaction, false)) |
here.
lightning/src/ln/channel.rs
Outdated
| // Checks whether we should emit a `ChannelPending` event. | ||
| pub(crate) fn should_emit_channel_pending_event(&mut self) -> bool { | ||
| self.is_funding_broadcast() && !self.channel_pending_event_emitted | ||
| self.is_funding_broadcast() && !self.channel_pending_event_emitted && self.manually_broadcast_outbound_channels.is_none() |
There was a problem hiding this comment.
Hmm, any way we can keep ChannelPending?
There was a problem hiding this comment.
I think we have two options here:
- Let the user tell us they broadcasted the transaction and as a result we emit
ChannelPending - Track the chain and check when the transaction is in the mempool and emit
ChannelPending
First option is not always doable from the user perspective, for example in payjoin scenario you dont really know when the tx will be broadcasted. wdyt?
There was a problem hiding this comment.
We could also just generate it when we generate the FundingSigned event, right?
There was a problem hiding this comment.
That would be the easiest if we are comfortable emitting both events simultaneously
There was a problem hiding this comment.
I assume we would, yes.
lightning/src/events/mod.rs
Outdated
| /// [`UserConfig::manually_accept_inbound_channels`]: crate::util::config::UserConfig::manually_accept_inbound_channels | ||
| user_channel_id: u128, | ||
| }, | ||
| /// Used to indicate that the counterparty node has sent us `funding_signed` message but we are |
There was a problem hiding this comment.
This is great for people who work on the lightning protocol, but we try to keep our docs understandable for those who do not as well. Instead, we should phrase simply as "the counterparty has provided the signature(s) required to recover our funds if they go offline and the funding transaction is now broadcastable" or so.
lightning/src/events/mod.rs
Outdated
| /// crate::ln::channelmanager::ChannelManager::unsafe_manual_funding_transaction_generated | ||
| /// [`ChannelManager::funding_transaction_generated`]: | ||
| /// crate::ln::channelmanager::ChannelManager::funding_transaction_generated | ||
| FundingSigned { |
There was a problem hiding this comment.
FundingSigned is a reference to a lightning-internal name, but isn't reflective of what has happened (the funding transaction itself was not signed, rather the first commitment transaction was) nor what has changed from the user's perspective (the funding transaction is now broadcastable).
lightning/src/ln/channelmanager.rs
Outdated
| } | ||
| macro_rules! emit_funding_signed_event { | ||
| ($locked_events: expr, $channel: expr) => { | ||
| let should_emit = $channel.context.should_emit_funding_signed_event(); |
There was a problem hiding this comment.
Sadly, like with actual broadcasting, we need to not emit the event until the channelmonitor update that has the new signature gets written to disk (as otherwise its not, actually, safe to broadcast yet). In general, there's a lot of machinery here to duplicate what we do with the funding transaction broadcast...ISTM we could better simplify this code by just checking if the channel is "manual broadcast" whenever we'd otherwise broadcast the funding tx and convert to an event (if relevant).
There was a problem hiding this comment.
Following your suggestion above with emitting both events simultaneously, could we have the same should_emit_..(roughly, excluding the respective variable is_emitted..) function for FundingSigned and ChannelPending events?
There was a problem hiding this comment.
Possibly, but it certainly seems to match up tighter with when LDK decides to broadcast, rather than when we decide to release the pending event.
0540f52 to
b21c02c
Compare
tnull
left a comment
There was a problem hiding this comment.
Basically LGTM, just a few nits.
lightning/src/events/mod.rs
Outdated
| channel_id: ChannelId, | ||
| /// The `user_channel_id` value passed in to [`ChannelManager::create_channel`] for outbound | ||
| /// channels, or to [`ChannelManager::accept_inbound_channel`] for inbound channels if | ||
| /// [`UserConfig::manually_accept_inbound_channels`] config flag is set to true. Otherwise |
There was a problem hiding this comment.
nit:
| /// [`UserConfig::manually_accept_inbound_channels`] config flag is set to true. Otherwise | |
| /// [`UserConfig::manually_accept_inbound_channels`] config flag is set to `true`. Otherwise |
lightning/src/ln/channel.rs
Outdated
| counterparty_forwarding_info: Option<CounterpartyForwardingInfo>, | ||
|
|
||
| pub(crate) channel_transaction_parameters: ChannelTransactionParameters, | ||
| /// The transaction which funds this channel. Note that for manually-funded channels(i.e, |
There was a problem hiding this comment.
nit:
| /// The transaction which funds this channel. Note that for manually-funded channels(i.e, | |
| /// The transaction which funds this channel. Note that for manually-funded channels (i.e., |
lightning/src/ln/channel.rs
Outdated
| /// This flag indicates that it is the user's responsibility to validated and broadcast the | ||
| /// funding transaction. | ||
| /// | ||
| /// [`Event::FundingTxBroadcastSafe`]: crate::events::Event::FundingTxBroadcastSafe |
There was a problem hiding this comment.
Drop unused doc link.
| /// [`Event::FundingTxBroadcastSafe`]: crate::events::Event::FundingTxBroadcastSafe |
lightning/src/ln/channel.rs
Outdated
| (45, cur_holder_commitment_point, option), | ||
| (47, next_holder_commitment_point, option), | ||
| (49, self.context.local_initiated_shutdown, option), // Added in 0.0.122 | ||
| (51, is_manual_broadcast, option), |
There was a problem hiding this comment.
Should we also add // Added in .. comments for these fields, like the ones above?
lightning/src/ln/channelmanager.rs
Outdated
| user_channel_id: $channel.context.get_user_id(), | ||
| funding_txo: $funding_txo, | ||
| counterparty_node_id: $channel.context.get_counterparty_node_id(), | ||
| former_temporary_channel_id: $channel.context.temporary_channel_id().expect("Unreachable: FundingTxBroadcastSafe event feature added to channel establishment process in LDK v0.124.0 where this should never be None."), |
There was a problem hiding this comment.
nit: Add newline before .expect
lightning/src/ln/channelmanager.rs
Outdated
| } | ||
|
|
||
|
|
||
| /// Unsafe: This method does not check the validatiy of the provided output. It is the |
There was a problem hiding this comment.
| /// Unsafe: This method does not check the validatiy of the provided output. It is the | |
| /// **Unsafe**: This method does not check the validity of the provided output. It is the |
dad9947 to
d0e779f
Compare
lightning/src/ln/channelmanager.rs
Outdated
| emit_funding_tx_broadcast_safe_event!(pending_events, channel, funding_txo.into_bitcoin_outpoint()) | ||
| }, | ||
| None => { | ||
| log_error!(logger, "Channel resumed without a funding txo, this should never happen!"); |
There was a problem hiding this comment.
I think we can safely debug_assert!(false); here :)
| internal_create_funding_transaction(node, expected_counterparty_node_id, expected_chan_value, expected_user_chan_id, true) | ||
| } | ||
|
|
||
| pub fn create_funding_tx_without_witness_data<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, |
There was a problem hiding this comment.
Given the method no longer even takes a full tx this seems like overkill :). It can now be deduced entirely from the API itself that the ChannelManager cannot possibly test for witnesses.
lightning/src/ln/channelmanager.rs
Outdated
| /// broadcasted and you are expected to broadcast it manually when receiving the | ||
| /// [`Event::FundingTxBroadcastSafe`] event. | ||
| /// | ||
| /// Returns an [`APIError::APIMisuseError`] if no output was found which matches the parameters |
There was a problem hiding this comment.
This can't happen anymore ;)
There was a problem hiding this comment.
This line still needs removing.
lightning/src/ln/channelmanager.rs
Outdated
| /// Returns [`APIError::ChannelUnavailable`] if a funding transaction has already been provided | ||
| /// for the channel or if the channel has been closed as indicated by [`Event::ChannelClosed`]. | ||
| /// | ||
| /// May panic if the output found in the funding transaction is duplicative with some other |
There was a problem hiding this comment.
| /// May panic if the output found in the funding transaction is duplicative with some other | |
| /// May panic if the funding output is duplicative with some other |
lightning/src/ln/channelmanager.rs
Outdated
| } | ||
|
|
||
|
|
||
| /// **Unsafe**: This method does not check the validity of the provided output. It is the |
There was a problem hiding this comment.
Ideally we should say what that validity requirement is. Specifically its very easy for a user to not check that inputs are segwit, so we should mention that explicitly imo.
e61909f to
d359dab
Compare
|
@TheBlueMatt please see last commit for changes.
|
TheBlueMatt
left a comment
There was a problem hiding this comment.
I am not sure now we still need funding_transaction_generated_intern and funding_transaction_generated ? wdyt?
Not quite sure what your question is?
lightning/src/ln/channelmanager.rs
Outdated
|
|
||
|
|
||
| /// **Unsafe**: This method does not check the validity of the provided output. It is the | ||
| /// caller's responsibility to ensure that. Its important to validate you are using SegWit |
There was a problem hiding this comment.
This reads as if you're supposed to check that the funding output is segwit, which is true but users shouldn't need to worry too much about it unless they screw up really bad. Instead, they need to check that all inputs are segwit.
a563480 to
06ba332
Compare
TheBlueMatt
left a comment
There was a problem hiding this comment.
LGTM, modulo a few nits.
lightning/src/ln/channelmanager.rs
Outdated
|
|
||
|
|
||
| /// **Unsafe**: This method does not validate the spent output. It is the caller's | ||
| /// responsibility to ensure the spent output are SegWit, as well as making sure the funding |
There was a problem hiding this comment.
| /// responsibility to ensure the spent output are SegWit, as well as making sure the funding | |
| /// responsibility to ensure the spent outputs are SegWit, as well as making sure the funding |
lightning/src/ln/channelmanager.rs
Outdated
| /// broadcasted and you are expected to broadcast it manually when receiving the | ||
| /// [`Event::FundingTxBroadcastSafe`] event. | ||
| /// | ||
| /// Returns an [`APIError::APIMisuseError`] if no output was found which matches the parameters |
There was a problem hiding this comment.
This line still needs removing.
lightning/src/ln/channelmanager.rs
Outdated
| err: "Transaction had more than 2^16 outputs, which is not supported".to_owned() | ||
| })); | ||
| } | ||
| { |
There was a problem hiding this comment.
nit: you can drop the unnecessary scope here if you want now.
06ba332 to
150e748
Compare
|
@TheBlueMatt Addressed all comments |
The `FundingTxBroadcastSafe` event indicates that we have received `funding_signed` message from our counterparty and that you should broadcast the funding transaction. This event is only emitted if upon generating the funding transaction you call `ChannelManager::unsafe_manual_funding_transaction_generated` that will emit this event instead of `ChannelPending` event. `ChannelManager::unsafe_manual_funding_transaction_generated` wont check if the funding transaction is signed, those its unsafe. It is manual because you are responsibile on broadcasting the transaction once the event is received.
150e748 to
8403755
Compare
TheBlueMatt
left a comment
There was a problem hiding this comment.
Oops, noticed one more issue, but we can address it in a followup in the same release cause this PR has gotten somewhat long in the tooth.
|
|
||
| pub(crate) channel_transaction_parameters: ChannelTransactionParameters, | ||
| /// The transaction which funds this channel. Note that for manually-funded channels (i.e., | ||
| /// is_manual_broadcast is true) this will be a dummy empty transaction. |
There was a problem hiding this comment.
Oops, one more issue here, the DiscardFunding event will now expose the dummy transaction here. We should hide that by adding an enum and including the funding OutPoint there instead.
|
Excuse the delay here, landing this. |
With [1], it's possible to specify `manual_broadcast` for the channel funding transaction. When `is_manual_broadcast` is set to true, the transaction in the `DiscardFunding` event is replaced with a dummy empty transaction. This commit checks if `is_manual_broadcast` is true and stores the funding OutPoint in the DiscardFunding event instead. [1] lightningdevkit#3024 Link: lightningdevkit#3164 Suggested-by: TheBlueMatt Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
With [1], it's possible to specify `manual_broadcast` for the channel funding transaction. When `is_manual_broadcast` is set to true, the transaction in the `DiscardFunding` event is replaced with a dummy empty transaction. This commit checks if `is_manual_broadcast` is true and stores the funding OutPoint in the DiscardFunding event instead. [1] lightningdevkit#3024 Link: lightningdevkit#3164 Suggested-by: TheBlueMatt Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
With [1], it's possible to specify `manual_broadcast` for the channel funding transaction. When `is_manual_broadcast` is set to true, the transaction in the `DiscardFunding` event is replaced with a dummy empty transaction. This commit checks if `is_manual_broadcast` is true and stores the funding OutPoint in the DiscardFunding event instead. [1] lightningdevkit#3024 Link: lightningdevkit#3164 Suggested-by: TheBlueMatt Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
With [1], it's possible to specify `manual_broadcast` for the channel funding transaction. When `is_manual_broadcast` is set to true, the transaction in the `DiscardFunding` event is replaced with a dummy empty transaction. This commit checks if `is_manual_broadcast` is true and stores the funding OutPoint in the DiscardFunding event instead. [1] lightningdevkit#3024 Link: lightningdevkit#3164 Suggested-by: TheBlueMatt Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
With [1], it's possible to specify `manual_broadcast` for the channel funding transaction. When `is_manual_broadcast` is set to true, the transaction in the `DiscardFunding` event is replaced with a dummy empty transaction. This commit checks if `is_manual_broadcast` is true and stores the funding OutPoint in the DiscardFunding event instead. [1] lightningdevkit#3024 Link: lightningdevkit#3164 Suggested-by: TheBlueMatt Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
With [1], it's possible to specify `manual_broadcast` for the channel funding transaction. When `is_manual_broadcast` is set to true, the transaction in the `DiscardFunding` event is replaced with a dummy empty transaction. This commit checks if `is_manual_broadcast` is true and stores the funding OutPoint in the DiscardFunding event instead. [1] lightningdevkit#3024 Link: lightningdevkit#3164 Suggested-by: TheBlueMatt Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
With [1], it's possible to specify `manual_broadcast` for the channel funding transaction. When `is_manual_broadcast` is set to true, the transaction in the `DiscardFunding` event is replaced with a dummy empty transaction. This commit checks if `is_manual_broadcast` is true and stores the funding OutPoint in the DiscardFunding event instead. [1] lightningdevkit#3024 Link: lightningdevkit#3164 Suggested-by: TheBlueMatt Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
With [1], it's possible to specify `manual_broadcast` for the channel funding transaction. When `is_manual_broadcast` is set to true, the transaction in the `DiscardFunding` event is replaced with a dummy empty transaction. This commit checks if `is_manual_broadcast` is true and stores the funding OutPoint in the DiscardFunding event instead. [1] lightningdevkit#3024 Link: lightningdevkit#3164 Suggested-by: TheBlueMatt Signed-off-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
resolves #3023 and #3022