Add Event::DiscardFunding generation

[1nF0rmed]

Fixes #953

Generates a new Event::DiscardFunding when we issue an Event::ChannelClosed as a result of the following Closure reasons:

1. OutdatedChannelManager

2. ProcessingError

3. HolderForcedOutdatedChannelManagerClosed

4. CounterpartyForceClosed

5. DisconnectedPeer

During the event of a channel close, if the channel is in a state of FundingCreated then an Event::DiscardFunding is issued along with the event for the channel close.

[TheBlueMatt]

We shouldn't generate a DiscardFunding always based on the closure reason, but instead need to consider the channel's current state. I believe something like this new function in Channel should gate the creation of such an event:

	/// Returns true if we have a pending funding transaction to be broadcasted (and haven't yet
	/// done so).
	pub fn funding_pending(&self) -> bool {
		self.funding_transaction.is_some() &&
			self.channel_state & (ChannelState::FundingCreated as u32) != 0
	}

More generally, you may want to consider this here as well #1086 (comment). No pressure if you don't want to rewrite the PR to do so, but refactoring the event generation on close into a common function would make lots of code more readable (including the new code here).

[1nF0rmed]

We shouldn't generate a DiscardFunding always based on the closure reason, but instead need to consider the channel's current state.

My bad, I missed the check for the channel state. Will add a check similar to funding_pending that also checks if the state consists of PeerDisconnected.

If I'm not wrong then I should refactor the code that issues the Event::ChannelClosed and Event::DiscardFunding into a helper function that is similar to #1086 (comment) and calls the above-mentioned function.

Also, with regard to the order of event dispatch. I'm assuming we would want to discard the funding before closing the channel.

[TheBlueMatt]

If I'm not wrong then I should refactor the code that issues the Event::ChannelClosed and Event::DiscardFunding into a helper function that is similar to #1086 (comment) and calls the above-mentioned function.

Yep, sounds great!

Also, with regard to the order of event dispatch. I'm assuming we would want to discard the funding before closing the channel.

I don't think it matters - the Event is just a "the channel has now been closed" and is provided to the user asynchronously anyway, so there's no ordering that matters.

[1nF0rmed]

I made an assumption that we would want to discard the funding only when the other party doesn't send in their funding_signed message and they leave the channel.

Based on that for every time we attempted to close a channel, it runs a check for that condition before issuing the event to discard funding.

[codecov]

Codecov Report

Merging #1098 (14256ea) into main (367a2cc) will decrease coverage by 0.06%.
The diff coverage is 85.18%.

❗ Current head 14256ea differs from pull request most recent head 1955008. Consider uploading reports for the commit 1955008 to get more accurate results

@@            Coverage Diff             @@
##             main    #1098      +/-   ##
==========================================
- Coverage   90.73%   90.66%   -0.07%     
==========================================
  Files          65       65              
  Lines       34318    34559     +241     
==========================================
+ Hits        31137    31332     +195     
- Misses       3181     3227      +46     

Impacted Files	Coverage Δ
lightning/src/ln/functional_test_utils.rs	95.08% <ø> (ø)
lightning/src/ln/functional_tests.rs	97.41% <ø> (-0.04%)	⬇️
lightning/src/util/events.rs	32.19% <33.33%> (+0.19%)	⬆️
lightning/src/ln/channel.rs	88.32% <100.00%> (-0.32%)	⬇️
lightning/src/ln/channelmanager.rs	85.11% <100.00%> (+0.18%)	⬆️
lightning-block-sync/src/lib.rs	93.49% <0.00%> (-1.69%)	⬇️
lightning/src/ln/script.rs	92.64% <0.00%> (-1.11%)	⬇️
lightning/src/routing/network_graph.rs	91.53% <0.00%> (-0.61%)	⬇️
lightning-net-tokio/src/lib.rs	76.69% <0.00%> (-0.59%)	⬇️
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 367a2cc...1955008. Read the comment docs.

[TheBlueMatt]

I'm not sure why this needs to be an option - is_funding_pending is gated on funding_transaction.is_some(), so we could combine it and funding_transaction into one function that checks the channel_state is as expected, and then returns the transaction if there is one, otherwise returns None.

[1nF0rmed]

The initial idea was that funding transaction must be added if it exists, but now given that we are checking for it via funding_transaction.is_some() I will update the same to be Transaction

[TheBlueMatt]

Tagged for 0.0.102 since we had a user request for exactly this.

[TheBlueMatt]

Basically LGTM.

[valentinewallace]

Thanks, very nice cleanup as well!

[TheBlueMatt]

Can you address the two pending comments and then squash the changes here to cleanup the git history prior to merge?

[1nF0rmed]

@TheBlueMatt @valentinewallace Will address the pending comments and squash the changes. Thank you for all the pointers!

[TheBlueMatt]

Diff from my/Val's last review:

$ git diff-tree -U1 bfe1f39e1 1955008d6
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index 062319e64..936bbab7c 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -1904,5 +1904,4 @@ impl<Signer: Sign> Channel<Signer> {
 	/// Returns transaction if there is pending funding transaction that is yet to broadcast
-	pub fn has_funding_pending(&self) -> Option<Transaction> {
-		 if self.funding_transaction.is_some() &&
-				self.channel_state & (ChannelState::FundingCreated as u32) != 0 {
+	pub fn unbroadcasted_funding(&self) -> Option<Transaction> {
+		 if self.channel_state & (ChannelState::FundingCreated as u32) != 0 {
 			 self.funding_transaction.clone()
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 800486fcc..5e43158b1 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1331,3 +1331,3 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		let mut pending_events_lock = self.pending_events.lock().unwrap();
-		match channel.has_funding_pending() {
+		match channel.unbroadcasted_funding() {
 			Some(transaction) => {
$