Batch funding #2486

wvanlint · 2023-08-09T19:23:29Z

Implements batch funding:

A batch funding transaction can be provided to ChannelManager, together with the temporary channel IDs it is funding.
The state of the batch will be maintained by the ChannelManager, tracking whether channels have received funding_signed and whether the monitor is persisted.
Channels will have an additional state flag to indicate that funding_signed has been received but broadcasting the funding transaction is held until all channels in the batch have received funding_signed and have their monitors persisted. This state is cleared by ChannelManager at the appropriate time.
No persistence is necessary as funding nodes can forget channels if they have not broadcast the corresponding funding transaction.
When a channel in a batch closes or a peer disconnects, this propagates to the other channels in the batch.
To avoid deadlocks, locks are acquired in the same order. The newly added locks are held at the innermost level, with locks around the state of the batch being acquired before locks around batches being closed.

TheBlueMatt

This wasn't as bad as I thought it would be, but is definitely gonna require re-reading most of channel and channelmanager to convince myself its correct :)

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2023-08-11T18:53:12Z

lightning/src/ln/channel.rs

+	/// Flag which is set on `FundingSent` to indicate this channel is funded in a batch and the
+	/// broadcasting of the funding transaction is being held until all channels in the batch
+	/// have received funding_signed and have their monitors persisted.
+	WaitingForBatch = 1 << 13,
 }
 const BOTH_SIDES_SHUTDOWN_MASK: u32 = ChannelState::LocalShutdownSent as u32 | ChannelState::RemoteShutdownSent as u32;
 const MULTI_STATE_FLAGS: u32 = BOTH_SIDES_SHUTDOWN_MASK | ChannelState::PeerDisconnected as u32 | ChannelState::MonitorUpdateInProgress as u32;


I think the new state should also be added here, but we'll need test coverage - can you add a test that checks for receiving our peer's channel_ready on a channel which is currently WaitingForBatch? This is basically the peer accepting the channel 0conf.

The WaitingForBatch flag is not a MULTI_STATE_FLAG I believe as it is only set on FundingSent? I did a pass through all the uses of FundingSent in channel.rs and added tests for the channel_ready behavior.

Hmm, yea, the MULTI_STATE_FLAGS thing is a bit of a misnomer, I think, it maybe should just be NON_STATE_FLAGS - the channel_state is really the OR of a state (the initial states and then ChannelReady) and some flags. MULTI_STATE_FLAGS lists the flags. But, eg, is monitor_updating_restored correct - below the funding_broadcastable setting we set it back to None if channel_state & !MULTI_STATE_FLAGS >= ChannelReady (which is true if WaitingForBatch is set even if we're not ChannelReady). Similar in do_best_block_updated. Still, to your point, if we do add it to MULTI_STATE_FLAGS I think we'll probably break check_get_channel_ready (we'll need to also skip the send if we are in WaitingForBatch.

It seems there are a couple of use cases:

A mask of flags that can be applied to multiple states and should be inherited when transitioning between states. MULTI_STATE_FLAGS seems to fulfill that purpose for the *ShutdownSent, PeerDisconnected, MonitorUpdateInProgress flags.

A mask of all flags that can be applied to only look at the sequential states and compare them. I added a superset of MULTI_STATE_FLAGS called STATE_FLAGS here.

I went through all uses of Channel::channel_state, verified the usage and adjusted any < or > comparisons to occur against self.channel_state & !STATE_FLAGS.

lightning/src/ln/channelmanager.rs

codecov-commenter · 2023-08-15T00:16:16Z

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (20e1c27) 88.86% compared to head (7b013b5) 88.91%.
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2486      +/-   ##
==========================================
+ Coverage   88.86%   88.91%   +0.05%     
==========================================
  Files         113      113              
  Lines       84517    85131     +614     
  Branches    84517    85131     +614     
==========================================
+ Hits        75103    75696     +593     
- Misses       7211     7222      +11     
- Partials     2203     2213      +10

Files	Coverage Δ
lightning/src/ln/reload_tests.rs	`96.24% <100.00%> (+0.19%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`91.43% <98.43%> (+0.27%)`	⬆️
lightning/src/events/mod.rs	`29.98% <0.00%> (+0.24%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.30% <98.22%> (-0.03%)`	⬇️
lightning/src/ln/channel.rs	`88.32% <95.69%> (+0.27%)`	⬆️
lightning/src/ln/channelmanager.rs	`81.60% <81.93%> (+0.04%)`	⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ariard

I think the current approach of dropping the whole batch is the safest one from a malleability viewpoint, yet once the transaction is counter-signed by all counterparties and broadcast, malleability is no more an issue if we can confirm the transaction.

I think there are intersections here with dual-funding/splicing and per-counterparty negotiated option_zeroconf, at least in term of API.

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2023-08-16T20:20:04Z

I think the current approach of dropping the whole batch is the safest one from a malleability viewpoint, yet once the transaction is counter-signed by all counterparties and broadcast, malleability is no more an issue if we can confirm the transaction.

Not sure what you're referring to? Are you referring to some specific above discussion or just stating that we need to drop the batch if some output doesn't get a counter-signature?

TheBlueMatt

Okay, thanks for all the work here, I really like the overall patch, but a handful of comments on minor tweaks for readability. I'd also love to see more test coverage of some edge cases, eg what happens if a peer sends us a channel_ready before we've broadcasted while we're waiting on a batch, a test of the initial monitor update(s) completing async, and a test for shutting down with one of the two channels having had their initial monitor update persisted but not the other one.

lightning/src/ln/channel.rs

TheBlueMatt · 2023-08-22T04:51:24Z

lightning/src/ln/channel.rs

+	/// Flag which is set on `FundingSent` to indicate this channel is funded in a batch and the
+	/// broadcasting of the funding transaction is being held until all channels in the batch
+	/// have received funding_signed and have their monitors persisted.
+	WaitingForBatch = 1 << 13,


I do kinda wonder if we shouldn't put this up further and renumber later flags. We'd have to juggle it a bunch in the serialization logic so maybe not worth it, but there's a few places it'd simplify the code cause we could keep >= kinda things.

Hmm, right, it would make the serialization logic more complex. I do wonder if this can be refactored at some point to model the ordered states and flags without bitwise logic.

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

wvanlint · 2023-08-24T05:35:49Z

Expanded testing coverage which covers:

Asynchronous monitor persistence in ln::functional_tests::test_batch_channel_open
Peer disconnection in ln::functional_tests::test_disconnect_in_funding_batch
Early channel ready in ln::channel::tests::test_waiting_for_batch
Reloading the node with a partially succeeded funding batch in ln::reload_tests::test_reload_partial_funding_batch

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/functional_tests.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/functional_tests.rs

lightning/src/ln/reload_tests.rs

TheBlueMatt

Ugh, I'd like to land this monday for 0.0.117 (later probably we can't make the release 😭), and am happy with the channel.rs changes and most of channelmanager, just want to see the lockorder inverted so we can simplify the potential for races.

lightning/src/ln/functional_test_utils.rs

lightning/src/ln/channelmanager.rs

wvanlint · 2023-09-26T06:43:37Z

In the latest change, the lock order is now reversed, acquiring funding_batch_states locks before per_peer_state locks. This improves the locking behavior when making changes across the batch instead of minimizing the number of code changes.

It does require a central place to propagate closures of channels to the remaining channels in the batch. Previously, this was done in update_maps_on_chan_removal, but that macro assumes per_peer_state locks are held. Instead, ChannelManager::finish_force_close_channel was extended as a method to handle all channel closures as it enforces no per_peer_state locks are held after #2597. ShutdownResult was extended as well, and is now sometimes constructed in ChannelManager, but provides a step towards further unification of channel close handling and can be moved into Channel methods that move to Shutdown states as well.

TheBlueMatt

A few minor cleanups and assertions, still digging into test coverage.

lightning/src/ln/channelmanager.rs

lightning/src/ln/functional_tests.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/functional_tests.rs

TheBlueMatt

LGTM, please squash the fixup.

lightning/src/ln/channelmanager.rs

This is a step towards more unified closing of channels, and provides a place where the per_peer_state lock is not held.

wvanlint · 2023-09-26T20:42:32Z

Squashed.

wpaulino

Glad we were able to get rid of the additional tracking map!

wpaulino · 2023-09-26T21:13:48Z

lightning/src/ln/channelmanager.rs

@@ -2632,7 +2632,7 @@ where
 		self.close_channel_internal(channel_id, counterparty_node_id, target_feerate_sats_per_1000_weight, shutdown_script)
 	}

-	fn finish_force_close_channel(&self, shutdown_res: ShutdownResult) {
+	fn finish_close_channel(&self, shutdown_res: ShutdownResult) {


Nit: also update the Finishing force-closure of channel... log below

Done in follow-up.

wpaulino · 2023-09-26T21:26:05Z

lightning/src/ln/functional_tests.rs

+	let events = nodes[0].node.get_and_clear_pending_events();
+	assert_eq!(events.len(), 1);
+	match events[0] {
+		Event::ChannelClosed { channel_id, .. } => {


Nit: use check_closed_event?

Unfortunately, that function does not match exactly as there are multiple closure reasons (the root cause one and the batch closure). Tried to refactor this in the follow-up.

wpaulino · 2023-09-26T21:33:41Z

lightning/src/ln/channel.rs

+/// - An optional (counterparty_node_id, funding_txo, [`ChannelMonitorUpdate`]) tuple
+/// - A list of HTLCs to fail back in the form of the (source, payment hash, and this channel's
+/// counterparty_node_id and channel_id).
+/// - An optional transaction id identifying a corresponding batch funding transaction.
 pub(crate) type ShutdownResult = (


Probably a good time to make this a struct now, but not a blocker.

Done in follow-up.

wpaulino · 2023-09-26T21:59:35Z

lightning/src/ln/reload_tests.rs

@@ -25,6 +25,7 @@ use crate::util::ser::{Writeable, ReadableArgs};
 use crate::util::config::UserConfig;
 use crate::util::string::UntrustedString;

+use bitcoin::{PackedLockTime, Transaction, TxOut};


Nit: these are all unused now

Removed in follow-up.

wpaulino · 2023-09-26T22:05:31Z

lightning/src/ln/channelmanager.rs

+				btree_map::Entry::Vacant(vacant) => Some(vacant.insert(Vec::new())),
+			}
+		});
+		for (channel_idx, &(temporary_channel_id, counterparty_node_id)) in temporary_channels.iter().enumerate() {


Nit: channel_idx is unused now

Removed in follow-up.

TheBlueMatt

Lets address any remaining feedback in a followup so we can land this.

TheBlueMatt · 2023-09-26T22:27:02Z

lightning/src/ln/channelmanager.rs

+	/// Return values are identical to [`Self::funding_transaction_generated`], respective to
+	/// each individual channel and transaction output.
+	///
+	/// Do NOT broadcast the funding transaction yourself. This batch funding transcaction


transcaction 😂

Fixed in follow-up 😅

wvanlint · 2023-09-28T04:33:47Z

Following up in #2613.

wvanlint force-pushed the batch_channel_opens branch 2 times, most recently from cc00689 to ac5c8c5 Compare August 9, 2023 20:33

wvanlint marked this pull request as ready for review August 9, 2023 23:07

TheBlueMatt reviewed Aug 11, 2023

View reviewed changes

wvanlint force-pushed the batch_channel_opens branch from ac5c8c5 to 6ce3713 Compare August 15, 2023 00:16

wvanlint force-pushed the batch_channel_opens branch from 6ce3713 to e26ba06 Compare August 15, 2023 18:00

wvanlint requested a review from TheBlueMatt August 15, 2023 18:10

ariard reviewed Aug 16, 2023

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

wvanlint force-pushed the batch_channel_opens branch 3 times, most recently from bceda6a to 6761bc6 Compare August 17, 2023 23:54

TheBlueMatt reviewed Aug 22, 2023

View reviewed changes

TheBlueMatt mentioned this pull request Aug 23, 2023

Use a single peer state map for all channel phases in peer state #2495

Merged

wvanlint force-pushed the batch_channel_opens branch 3 times, most recently from bd1ad7e to fc880f2 Compare August 24, 2023 05:19

wvanlint requested a review from TheBlueMatt August 24, 2023 18:01

wpaulino reviewed Aug 29, 2023

View reviewed changes

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

wvanlint force-pushed the batch_channel_opens branch 3 times, most recently from 3baad70 to 54010cd Compare August 30, 2023 23:53

wvanlint requested a review from wpaulino August 31, 2023 00:22

wpaulino reviewed Sep 5, 2023

View reviewed changes

wvanlint force-pushed the batch_channel_opens branch from 54010cd to 3cd0b4d Compare September 12, 2023 06:36

wvanlint requested a review from wpaulino September 12, 2023 18:42

wpaulino reviewed Sep 12, 2023

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

wvanlint requested a review from wpaulino September 12, 2023 23:36

wvanlint force-pushed the batch_channel_opens branch 2 times, most recently from 25d5504 to 6d40a8f Compare September 19, 2023 00:16

wvanlint requested a review from TheBlueMatt September 19, 2023 00:23

wpaulino reviewed Sep 20, 2023

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

wvanlint force-pushed the batch_channel_opens branch from 6d40a8f to 570a1c7 Compare September 20, 2023 21:32

wvanlint requested a review from wpaulino September 20, 2023 21:32

wpaulino previously approved these changes Sep 22, 2023

View reviewed changes

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/functional_tests.rs Outdated Show resolved Hide resolved

lightning/src/ln/reload_tests.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed Sep 23, 2023

View reviewed changes

lightning/src/ln/functional_test_utils.rs Outdated Show resolved Hide resolved

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

wvanlint dismissed wpaulino’s stale review via 535c19e September 26, 2023 01:01

wvanlint force-pushed the batch_channel_opens branch 2 times, most recently from 535c19e to 7816cf6 Compare September 26, 2023 06:22

wvanlint requested review from TheBlueMatt and wpaulino September 26, 2023 06:47

TheBlueMatt reviewed Sep 26, 2023

View reviewed changes

lightning/src/ln/functional_tests.rs Show resolved Hide resolved

wvanlint force-pushed the batch_channel_opens branch from 9886bba to 7b013b5 Compare September 26, 2023 19:21

TheBlueMatt reviewed Sep 26, 2023

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

wvanlint added 3 commits September 26, 2023 13:37

Always call finish_force_close_channel on closure

cd4b38d

This is a step towards more unified closing of channels, and provides a place where the per_peer_state lock is not held.

Rename finish_force_close_channel to finish_close_channel

c55a507

Batch funding for v1 channel establishments

8234199

wvanlint force-pushed the batch_channel_opens branch from 7b013b5 to 8234199 Compare September 26, 2023 20:40

wpaulino approved these changes Sep 26, 2023

View reviewed changes

TheBlueMatt approved these changes Sep 26, 2023

View reviewed changes

TheBlueMatt merged commit 3e1e393 into lightningdevkit:main Sep 26, 2023

wvanlint deleted the batch_channel_opens branch September 26, 2023 23:08

wvanlint mentioned this pull request Sep 28, 2023

Refactor ShutdownResult type and construction #2613

Merged

wvanlint changed the title ~~Batch funding for v1 channel establishments~~ Batch funding Oct 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch funding #2486

Batch funding #2486

wvanlint commented Aug 9, 2023 •

edited

Loading

TheBlueMatt left a comment •

edited

Loading

TheBlueMatt Aug 11, 2023

wvanlint Aug 15, 2023

TheBlueMatt Aug 16, 2023

wvanlint Aug 17, 2023

codecov-commenter commented Aug 15, 2023 •

edited

Loading

ariard left a comment

TheBlueMatt commented Aug 16, 2023

TheBlueMatt left a comment

TheBlueMatt Aug 22, 2023

wvanlint Aug 24, 2023

wvanlint commented Aug 24, 2023

TheBlueMatt left a comment

wvanlint commented Sep 26, 2023 •

edited

Loading

TheBlueMatt left a comment

TheBlueMatt left a comment

wvanlint commented Sep 26, 2023

wpaulino left a comment

wpaulino Sep 26, 2023

wvanlint Sep 28, 2023

wpaulino Sep 26, 2023

wvanlint Sep 28, 2023 •

edited

Loading

wpaulino Sep 26, 2023

wvanlint Sep 28, 2023

wpaulino Sep 26, 2023

wvanlint Sep 28, 2023

wpaulino Sep 26, 2023

wvanlint Sep 28, 2023

TheBlueMatt left a comment

TheBlueMatt Sep 26, 2023

wvanlint Sep 28, 2023

wvanlint commented Sep 28, 2023

Batch funding #2486

Batch funding #2486

Conversation

wvanlint commented Aug 9, 2023 • edited Loading

TheBlueMatt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 15, 2023 • edited Loading

Codecov Report

ariard left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 16, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvanlint commented Aug 24, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

wvanlint commented Sep 26, 2023 • edited Loading

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

wvanlint commented Sep 26, 2023

wpaulino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvanlint Sep 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wvanlint commented Sep 28, 2023

wvanlint commented Aug 9, 2023 •

edited

Loading

TheBlueMatt left a comment •

edited

Loading

codecov-commenter commented Aug 15, 2023 •

edited

Loading

wvanlint commented Sep 26, 2023 •

edited

Loading

wvanlint Sep 28, 2023 •

edited

Loading