Funding signed event

[jbesraa]

resolves #3023

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 80.95238% with 32 lines in your changes missing coverage. Please review.

Project coverage is 89.82%. Comparing base (bfc20f8) to head (8403755).

Files	Patch %	Lines
lightning/src/events/mod.rs	0.00%	20 Missing :warning:
lightning/src/ln/channelmanager.rs	87.17%	6 Missing and 4 partials :warning:
lightning/src/ln/functional_tests.rs	95.65%	2 Missing :warning:

:exclamation: 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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jbesraa]

Done few things:

1. CherryP #3056 here
2. Removed FUNDING_SIGNED channel state
3. Removed config manually_broadcast_.. argument

Yet to do:

1. Complete the docs
2. Fix CI
3. Improve/fix naming around the new property in ChannelContext

[jbesraa]

@TheBlueMatt @benthecarman this is ready for review

[jbesraa]

Thanks for the reviews, addressed all the comments.

[jbesraa]

Thanks @tnull Fixed-up the following:

diff --git a/lightning/src/events/mod.rs b/lightning/src/events/mod.rs
index 38077c8b..a2403c3e 100644
--- a/lightning/src/events/mod.rs
+++ b/lightning/src/events/mod.rs
@@ -535,7 +535,7 @@ pub enum Event {
        /// Used to indicate that the counterparty node has provided the signature(s) required to
        /// recover our funds in case they go offline.
        ///
-       /// It is safe(and your responsibility) to broadcast the funding transaction upon receiving this
+       /// It is safe (and your responsibility) to broadcast the funding transaction upon receiving this
        /// event.
        ///
        /// This event is only emitted if you called
@@ -1489,7 +1489,7 @@ impl Writeable for Event {
                                        (2, user_channel_id, required),
                                        (4, funding_tx, required),
                                        (6, counterparty_node_id, required),
-                                       (8, former_temporary_channel_id, required),
+                                       (8, former_temporary_channel_id, option),
                                });
                        },
                        // Note that, going forward, all new events must only write data inside of
@@ -1939,7 +1939,7 @@ impl MaybeReadable for Event {
                                                (2, user_channel_id, required),
                                                (4, funding_tx, required),
                                                (6, counterparty_node_id, required),
-                                               (8, former_temporary_channel_id, required)
+                                               (8, former_temporary_channel_id, option)
                                        });
                                        Ok(Some(Event::FundingTxBroadcastSafe {
                                                channel_id: channel_id.0.unwrap(),
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 9d39ca72..ac316d45 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -9352,7 +9352,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
                                // Later in the ChannelManager deserialization phase we scan for channels and assign scid aliases if its missing
                                outbound_scid_alias: outbound_scid_alias.unwrap_or(0),

-                               funding_tx_broadcast_safe_event_emitted: funding_tx_broadcast_safe_event_emitted.unwrap_or(true),
+                               funding_tx_broadcast_safe_event_emitted: funding_tx_broadcast_safe_event_emitted.unwrap_or(false),
                                channel_pending_event_emitted: channel_pending_event_emitted.unwrap_or(true),
                                channel_ready_event_emitted: channel_ready_event_emitted.unwrap_or(true),

@@ -9365,7 +9365,7 @@ impl<'a, 'b, 'c, ES: Deref, SP: Deref> ReadableArgs<(&'a ES, &'b SP, u32, &'c Ch
                                local_initiated_shutdown,

                                blocked_monitor_updates: blocked_monitor_updates.unwrap(),
-                               is_manual_broadcast: is_manual_broadcast.unwrap_or(true),
+                               is_manual_broadcast: is_manual_broadcast.unwrap_or(false),
                        },
                        #[cfg(any(dual_funding, splicing))]
                        dual_funding_channel_context: None,
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index f6816708..d1b773b9 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -4339,27 +4339,23 @@ where
        }


-       /// Unsafe: This method does not check if the funding transaction is signed ie. if the witness data
+       /// Unsafe: This method does not check if the funding transaction is signed, i.e., if the witness data
        /// is empty or not. It is the caller's responsibility to ensure that the funding transaction
        /// is final.
        ///
-       /// If you wish to use a safer method, use [`ChannelManager::funding_transaction_generated`]
+       /// If you wish to use a safer method, use [`ChannelManager::funding_transaction_generated`].
        ///
        /// Call this upon creation of a funding transaction for the given channel.
        ///
        /// Note that if this method is called successfully and the counterparty sent us
-       /// `funding_signed`, the funding transaction wont be broadcasted and you are expected to
-       /// broadcast it manually when receiving the `FundingTxBroadcastSafe` event.
+       /// `funding_signed` msg, the funding transaction won't be broadcasted and you are expected to
+       /// broadcast it manually when receiving the [`FundingTxBroadcastSafe`] event.
        ///
-       /// In contrary to the original `funding_transaction_generated` method, the `ChannelPending`
-       /// event is never emitted. Once the funding transaction is confirmed by both parties, the
-       /// next event will be `ChannelReady`.
-       ///
-       /// Returns an [`APIError::APIMisuseError`] if the funding_transaction spent non-SegWit outputs
+       /// Returns an [`APIError::APIMisuseError`] if the `funding_transaction` spent non-SegWit outputs
        /// or if no output was found which matches the parameters in [`Event::FundingGenerationReady`].
        ///
        /// Returns [`APIError::APIMisuseError`] if the funding transaction is not final for propagation
-       /// across the p2p network.
+       /// across the peer-to-peer network.
        ///
        /// 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`].
@@ -4368,7 +4364,7 @@ where
        /// channel (note that this should be trivially prevented by using unique funding transaction
        /// keys per-channel).
        ///
-       /// Note that this includes RBF or similar transaction replacement strategies - lightning does
+       /// Note that this includes RBF or similar transaction replacement strategies - Lightning does
        /// not currently support replacing a funding transaction on an existing channel. Instead,
        /// create a new channel with a conflicting funding transaction.
        ///
@@ -4378,6 +4374,7 @@ where
        /// for more details.
        ///
        /// [`Event::FundingGenerationReady`]: crate::events::Event::FundingGenerationReady
+       /// [`Event::FundingTxBroadcastSafe`]: crate::events::Event::FundingTxBroadcastSafe
        /// [`Event::ChannelClosed`]: crate::events::Event::ChannelClosed
        /// [`ChannelManager::funding_transaction_generated`]: crate::ln::channelmanager::ChannelManager::funding_transaction_generated
        pub fn unsafe_manual_funding_transaction_generated(&self, temporary_channel_id: &ChannelId, counterparty_node_id: &PublicKey, funding_transaction: Transaction) -> Result<(), APIError> {

[jbesraa]

A usage example of this PR can be seen in this commit: https://github.com/lightningdevkit/ldk-node/pull/301/commits/d137df1d81fdd6b52d8d68c9876a3f5e3434e406

specifically, channel_manager::unsafe_manual_funiding_.. is called here https://github.com/lightningdevkit/ldk-node/commit/d137df1d81fdd6b52d8d68c9876a3f5e3434e406#diff-03a28135b3f471a98f1c40eb8a4dcc7c4759f4bf9aabc83c12c71f14d68e00c1R228

and FundingTxBroadcastSafe is handled here https://github.com/lightningdevkit/ldk-node/commit/d137df1d81fdd6b52d8d68c9876a3f5e3434e406#diff-14da7c350de5742eaa248de110fefeae9b4b639c0358e26024d6040ccf174a41R937

[TheBlueMatt]

Sadly needs a simple rebase.

[jbesraa]

@TheBlueMatt Cherry-picked your commit here. It does indeed improve readability and provide more flexibility but does make most of whats returned from FundingGenerationReady event redundant. I am cool with either.

[TheBlueMatt]

I personally like it more, thanks. I'll let @tnull chime in but I think this is basically there.

[jbesraa]

@TheBlueMatt please see last commit for changes.

~~I am not sure now we still need funding_transaction_generated_intern and funding_transaction_generated ? wdyt?~~

[jbesraa]

@TheBlueMatt Addressed all comments

[tnull]

Excuse the delay here, landing this.