Clean up existing and add range-based closing_signed negotiation #1011

TheBlueMatt · 2021-07-20T14:19:36Z

This adds the new range-based closing_signed negotiation specified
in lightning/bolts#847 as
well as cleans up the existing closing_signed negotiation to unify
the new codepaths and the old ones.

Note that because the new range-based closing_signed negotiation
allows the channel fundee to ultimately select the fee out of a
range specified by the funder, which we, of course, always select
the highest allowed amount from. Thus, we've added an extra round
of closing_signed in the common case as we will not simply accept
the first fee we see, always preferring to make the funder pay as
much as they're willing to.

Probably needs a few additional tests, but the existing shutdown coverage isn't bad either. May need to wait on resolution of a few issues in the spec PR.

codecov · 2021-07-20T14:37:41Z

Codecov Report

Merging #1011 (db6c69d) into main (a369f9e) will increase coverage by 0.05%.
The diff coverage is 89.87%.

❗ Current head db6c69d differs from pull request most recent head 82e7df1. Consider uploading reports for the commit 82e7df1 to get more accurate results

@@            Coverage Diff             @@
##             main    #1011      +/-   ##
==========================================
+ Coverage   90.96%   91.02%   +0.05%     
==========================================
  Files          64       65       +1     
  Lines       32393    33856    +1463     
==========================================
+ Hits        29467    30818    +1351     
- Misses       2926     3038     +112

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`90.00% <ø> (ø)`
lightning/src/util/ser_macros.rs	`87.71% <0.00%> (-8.61%)`	⬇️
lightning/src/util/config.rs	`46.66% <50.00%> (+0.15%)`	⬆️
lightning/src/ln/channelmanager.rs	`85.84% <70.88%> (-0.27%)`	⬇️
lightning/src/ln/channel.rs	`88.70% <72.78%> (-0.67%)`	⬇️
lightning/src/ln/shutdown_tests.rs	`95.58% <95.58%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.89% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.13% <100.00%> (+0.01%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.50% <100.00%> (+0.17%)`	⬆️
lightning/src/ln/msgs.rs	`89.10% <100.00%> (+0.24%)`	⬆️
... 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 a369f9e...82e7df1. Read the comment docs.

TheBlueMatt · 2021-07-22T16:09:12Z

Tagged 0.0.100 given this fixes some current issues users have commented on where we're too restrictive in the existing negotiation. Also may depend on #1013 depending on how the BOLT changes go.

TheBlueMatt · 2021-07-26T22:51:23Z

Updated to latest BOLT PR version, now based on #985.

ariard

Mostly nits so far, thinking if there are more economically optimal strategies for funder or flexibility desirered (e,g if you're a LSP and you want to cache pre-signed cooperative close, attaching a CPFP at latter broadcast). Greedy fundee sounds a good one.

lightning/src/ln/channel.rs

ariard · 2021-07-30T23:16:37Z

lightning/src/ln/channel.rs

+			};
+
+		// The spec requires that (when the channel does not have anchors) we only send absolute
+		// channel fees no greater than the absolute channel fee on the current commitment


I think the spec could be change here and prepare to splice-out support, where the closing transaction size might be bigger than a commitment one ? Let's avoid to multiply closing pipeline in function of channel type.

All of the channel close stuff is based on fees, not feerates. The old code based on feerates being so complicated and brittle I think is good enough reason to move to something fee-based with calculation at start. We'll probably want separate storage for splice negotiation anyway.

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/msgs.rs

ariard · 2021-07-30T23:44:15Z

lightning/src/util/config.rs

+	/// When we are not the funder, we require the closing transaction fee pay at least our
+	/// [`Background`] fee estimate, but allow our counterparty to pay as much fee as they like.
+	///
+	/// Default value: 1000 satoshis.


Well a smarter value setting could be based on to_self_delay length and channel value size? Hmmm...

Sure, there's lots of things we could do, but adding more knobs does not better enable users to control things, usually the opposite, knobs are just confusing. If users want to do really fine-grained control based on the channel details they can set the target when they call close (or update this value, when we implement that).

TheBlueMatt · 2021-08-09T19:02:24Z

Rebased on #1019.

TheBlueMatt · 2021-08-10T00:01:13Z

Rebased after #1019 was merged and fixed the outstanding issues (I believe).

jkczyz

Made it through most commits but need to review the second half of the PR still.

lightning/src/ln/channelmanager.rs

jkczyz · 2021-08-11T17:22:12Z

lightning/src/ln/channel.rs

+/// HTLCs for days we may need this to suffice for feerate increases across days, but that may
+/// leave the channel less usable as we hold a bigger reserve.
+#[cfg(fuzzing)]
+pub const FEE_SPIKE_BUFFER_FEE_INCREASE_MULTIPLE: u64 = 2;


Is this the same constant 2 used in ChannelManager::update_channel_fee or related in any way?

No, this is only at issue if the fee is increasing, the 2 constant in update_channel_fee is "minimum the feerate needs to decrease before we bother updating the channel feerate".

lightning/src/ln/channel.rs

jkczyz · 2021-08-11T17:50:43Z

lightning/src/ln/channel.rs

@@ -733,7 +733,7 @@ impl<Signer: Sign> Channel<Signer> {
 		if feerate_per_kw < lower_limit {
 			return Err(ChannelError::Close(format!("Peer's feerate much too low. Actual: {}. Our expected lower limit: {}", feerate_per_kw, lower_limit)));
 		}
-		let upper_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64  * 2;
+		let upper_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64 * 4;


Perhaps a documented constant is in order if this needs a lengthy explanation in the commit message?

This was rewritten in #985.

jkczyz · 2021-08-11T18:31:36Z

Is this also based on #985? I just realized my comments overlap with that PR 😕

jkczyz

Nice clean-up! I have a couple more commits to dig into a bit later, but wanted to send out comments in the meanwhile.

jkczyz · 2021-08-11T21:46:44Z

lightning/src/util/ser_macros.rs

@@ -26,7 +26,7 @@ macro_rules! encode_tlv {
 }

 macro_rules! encode_tlv_stream {
-	($stream: expr, {$(($type: expr, $field: expr, $fieldty: ident)),*}) => { {
+	($stream: expr, {$(($type: expr, $field: expr, $fieldty: ident)),* $(,)*}) => { {


Should this be ? for zero or one instead of *?

I believe we support versions of rust prior to the introduction of ?.

Looks like it made it into 1.32.

https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1320-2019-01-17

Ah, maybe it would work now and my information is out of date? In any case, we use * in a number of places and this particular commit has already landed upstream on another PR.

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

jkczyz · 2021-08-11T22:38:48Z

lightning/src/util/ser_macros.rs

@@ -8,6 +8,9 @@
 // licenses.

 macro_rules! encode_tlv {
+	($stream: expr, $type: expr, $field: expr, (default_value, $default: expr)) => {


How about (default: $default: expr)?

Sadly this already landed upstream via a different PR, but I agree this would be a nice cleanup.

lightning/src/ln/channel.rs

lightning/src/ln/functional_tests.rs

lightning/src/ln/channelmanager.rs

jkczyz · 2021-08-11T23:43:07Z

lightning/src/ln/channel.rs

+	pub fn timer_check_closing_negotiation_progress(&mut self) -> Result<(), ChannelError> {
+		if self.closing_negotiation_ready() {
+			if self.closing_signed_in_flight {
+				return Err(ChannelError::Close("closing_signed negotiation failed to finish within one minute".to_owned()));


The "within one minute" wording here assumes the user is using BackgroundProcessor and that negotiation began at the last tick. Maybe something less precise?

I made it "two timer ticks", though note that the ChannelManager docs strongly suggest minutely calls.

TheBlueMatt · 2021-08-12T16:37:47Z

Is this also based on #985? I just realized my comments overlap with that PR confused

Oops, yes, sorry, it still is. I addressed a number of your comments here on that PR.

TheBlueMatt · 2021-08-12T18:41:12Z

Rebased on latest #985 (and upstream).

jkczyz

Hitting a bit of a wall tonight, so will take a closer look at the test and coverage in the morning.

lightning/src/util/config.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

valentinewallace · 2021-08-13T14:53:18Z

lightning/src/ln/channel.rs

@@ -2943,6 +3007,8 @@ impl<Signer: Sign> Channel<Signer> {
 		// Upon reconnect we have to start the closing_signed dance over, but shutdown messages
 		// will be retransmitted.
 		self.last_sent_closing_fee = None;
+		self.pending_counterparty_closing_signed = None;
+		self.closing_fee_limits = None;



Should we also set closing_signed_in_flight to false here?

I don't think so - if we started the close negotiation and our peer disconnects, we should still force close after a little bit.

lightning/src/ln/channel.rs

jkczyz

Yes, looking good implementation-wise, coverage concerns aside.

lightning/src/ln/channelmanager.rs

jkczyz · 2021-08-13T21:48:58Z

lightning/src/ln/shutdown_tests.rs

+	}
+
+	let mut node_0_closing_signed = get_event_msg!(nodes[0], MessageSendEvent::SendClosingSigned, nodes[1].node.get_our_node_id());
+	assert!(node_0_closing_signed.fee_satoshis <= 500);


Decent middle ground to indicate "less than the 10 sat/vbyte nodes[1] wants". added a comment.

lightning/src/ln/shutdown_tests.rs

jkczyz · 2021-08-13T22:24:31Z

lightning/src/ln/channel.rs

+			if let Some((last_fee, _)) = self.last_sent_closing_fee {
+				if msg.fee_satoshis > last_fee {
+					if msg.fee_satoshis < our_max_fee {
+						propose_fee!(msg.fee_satoshis);
+					} else if last_fee < our_max_fee {
+						propose_fee!(our_max_fee);
+					} else {
+						return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wants something ({} sat) higher than our max fee ({} sat)", msg.fee_satoshis, our_max_fee)));
+					}
+				} else {
+					if msg.fee_satoshis > our_min_fee {
+						propose_fee!(msg.fee_satoshis);
+					} else if last_fee > our_min_fee {
+						propose_fee!(our_min_fee);
+					} else {
+						return Err(ChannelError::Close(format!("Unable to come to consensus about closing feerate, remote wants something ({} sat) lower than our min fee ({} sat)", msg.fee_satoshis, our_min_fee)));
+					}
+				}
+			} else {
+				if msg.fee_satoshis < our_min_fee {
+					propose_fee!(our_min_fee);
+				} else if msg.fee_satoshis > our_max_fee {
+					propose_fee!(our_max_fee);
+				} else {
+					propose_fee!(msg.fee_satoshis);
 				}
 			}


Some rate limiting is preventing me from viewing the code coverage report. Are these cases largely covered by test_closing_signed_reinit_timeout?

The legacy feerate negotiation stuff is probably pretty under-tested after this PR (not that it was super well-tested before this PR, given it had a handful of bugs), I added a pretty simple tests of it in a new commit. At least the logic that is legacy-specific is pretty trivial.

We don't actually yet support `warning` messages as there are issues left to resolve in the spec PR, but there's nothing to stop us adding an internal enum variant for sending a warning message before we actually support doing so.

This allows decode_tlv_stream!() to be called with either a mutable reference to a stream or a stream itself and allows encode_tlv_stream!() to be called with an excess , at the end of the parameter list.

This adds the serialization and structures for the new fee range specifiers in closing_signed as added upstream at lightning/bolts#847

TheBlueMatt · 2021-08-13T23:03:08Z

Rebased on latest git after merge of #985 and squashed fixup commits down.

We're supposed to write `Channel` to disk as if `remove_uncommitted_htlcs_and_mark_paused` had just run, however we were writing `last_sent_closing_fee` to disk (if it is not-None), whereas `remove_uncommitted_htlcs_and_mark_paused` clears it. Indeed, the BOLTs say fee "... negotiation restarts on reconnection."

valentinewallace

Do we wanna add the test coverage or punt it? I'm ACK

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2021-08-16T22:50:32Z

Do we wanna add the test coverage or punt it? I'm ACK

Ooops! Yes, I definitely want to add test coverage for the new target-based feerate stuff. I added a simple test of it, at least, in a new commit.

jkczyz · 2021-08-17T00:35:22Z

ACK db6c69d

Good to go after squash.

When we added the support for external signing, many of the signing functions were allowed to return an error, closing the channel in such a case. `sign_closing_transaction` is one such function which can now return an error, except instead of handling it properly we'd simply never send a `closing_signed` message, hanging the channel until users intervene and force-close it. Piping the channel-closing error back through the various callsites (several of which already have pending results by the time they call `maybe_propose_first_closing_signed`) may be rather complicated, so instead we simply attempt to propose the initial `closing_signed` in `get_and_clear_pending_msg_events` like we do for holding-cell freeing. Further, since we now (possibly) generate a `ChannelMonitorUpdate` on `shutdown`, we may need to wait for monitor updating to complete before we can send a `closing_signed`, meaning we need to handle the send asynchronously anyway. This simplifies a few function interfaces and has no impact on behavior, aside from a few message-ordering edge-cases, as seen in the two small test changes required.

This adds the new range-based closing_signed negotiation specified in lightning/bolts#847 as well as cleans up the existing closing_signed negotiation to unify the new codepaths and the old ones. Note that because the new range-based closing_signed negotiation allows the channel fundee to ultimately select the fee out of a range specified by the funder, which we, of course, always select the highest allowed amount from. Thus, we've added an extra round of closing_signed in the common case as we will not simply accept the first fee we see, always preferring to make the funder pay as much as they're willing to.

Because ln::functional_tests if over 9000 LoC long, its useful to move tests into new modules as we can. Here we move all cooperative shutdown related tests into a new module entitled `shutdown_tests`

This doesn't exhaustively test closing fee negotiation at all, but ensures that it is at least basically able to come to consensus and sign cooperative closing transactions.

TheBlueMatt · 2021-08-17T02:17:05Z

Squashed without diff (db6c69d8...82e7df1c), diff from Val's ACK-with-comments above is:

$ git diff-tree -U1  2141a3a 82e7df1c
diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs
index ca25cb19..03968118 100644
--- a/lightning/src/ln/channel.rs
+++ b/lightning/src/ln/channel.rs
@@ -456,3 +456,3 @@ pub(super) struct Channel<Signer: Sign> {
 
-	/// If our counterparty sent us a closing_signed while we were waiting for a channel monitor
+	/// If our counterparty sent us a closing_signed while we were waiting for a `ChannelMonitor`
 	/// update, we need to delay processing it until later. We do that here by simply storing the
@@ -3378,2 +3378,5 @@ impl<Signer: Sign> Channel<Signer> {
 
+	/// Calculates and returns our minimum and maximum closing transaction fee amounts, in whole
+	/// satoshis. The amounts remain consistent unless a peer disconnects/reconnects or we restart,
+	/// at which point they will be recalculated.
 	fn calculate_closing_fee_limits<F: Deref>(&mut self, fee_estimator: &F) -> (u64, u64)
@@ -3387,7 +3390,4 @@ impl<Signer: Sign> Channel<Signer> {
 		let mut proposed_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
-		let mut proposed_max_feerate = if self.is_outbound() {
-				fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal)
-			} else {
-				u32::max_value()
-			};
+		let normal_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Normal);
+		let mut proposed_max_feerate = if self.is_outbound() { normal_feerate } else { u32::max_value() };
 
@@ -3415,3 +3415,7 @@ impl<Signer: Sign> Channel<Signer> {
 		let proposed_max_total_fee_satoshis = if self.is_outbound() {
-				proposed_max_feerate as u64 * tx_weight / 1000 + self.config.force_close_avoidance_max_fee_satoshis
+				// We always add force_close_avoidance_max_fee_satoshis to our normal
+				// feerate-calculated fee, but allow the max to be overridden if we're using a
+				// target feerate-calculated fee.
+				cmp::max(normal_feerate as u64 * tx_weight / 1000 + self.config.force_close_avoidance_max_fee_satoshis,
+					proposed_max_feerate as u64 * tx_weight / 1000)
 			} else {
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 67c316d6..cff31467 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -1355,3 +1355,3 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 	///  * If our counterparty is the channel initiator, we will require a channel closing
-	///    transaction feerate of our [`Background`] feerate or the feerate which
+	///    transaction feerate of at least our [`Background`] feerate or the feerate which
 	///    would appear on a force-closure transaction, whichever is lower. We will allow our
diff --git a/lightning/src/ln/shutdown_tests.rs b/lightning/src/ln/shutdown_tests.rs
index f4cc3d40..a40ad237 100644
--- a/lightning/src/ln/shutdown_tests.rs
+++ b/lightning/src/ln/shutdown_tests.rs
@@ -881 +881,48 @@ fn simple_legacy_shutdown_test() {
 }
+
+#[test]
+fn simple_target_feerate_shutdown() {
+	// Simple test of target in `close_channel_with_target_feerate`.
+	let chanmon_cfgs = create_chanmon_cfgs(2);
+	let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
+	let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+	let chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
+	let chan_id = OutPoint { txid: chan.3.txid(), index: 0 }.to_channel_id();
+
+	nodes[0].node.close_channel_with_target_feerate(&chan_id, 253 * 10).unwrap();
+	let node_0_shutdown = get_event_msg!(nodes[0], MessageSendEvent::SendShutdown, nodes[1].node.get_our_node_id());
+	nodes[1].node.close_channel_with_target_feerate(&chan_id, 253 * 5).unwrap();
+	let node_1_shutdown = get_event_msg!(nodes[1], MessageSendEvent::SendShutdown, nodes[0].node.get_our_node_id());
+
+	nodes[1].node.handle_shutdown(&nodes[0].node.get_our_node_id(), &InitFeatures::known(), &node_0_shutdown);
+	nodes[0].node.handle_shutdown(&nodes[1].node.get_our_node_id(), &InitFeatures::known(), &node_1_shutdown);
+
+	let node_0_closing_signed = get_event_msg!(nodes[0], MessageSendEvent::SendClosingSigned, nodes[1].node.get_our_node_id());
+	nodes[1].node.handle_closing_signed(&nodes[0].node.get_our_node_id(), &node_0_closing_signed);
+	let (_, node_1_closing_signed_opt) = get_closing_signed_broadcast!(nodes[1].node, nodes[0].node.get_our_node_id());
+	let node_1_closing_signed = node_1_closing_signed_opt.unwrap();
+
+	// nodes[1] was passed a target which was larger than the current channel feerate, which it
+	// should ignore in favor of the channel fee, as there is no use demanding a minimum higher
+	// than what will be paid on a force-close transaction. Note that we have to consider rounding,
+	// so only check that we're within 10 sats.
+	assert!(node_0_closing_signed.fee_range.as_ref().unwrap().min_fee_satoshis >=
+	        node_1_closing_signed.fee_range.as_ref().unwrap().min_fee_satoshis * 10 - 5);
+	assert!(node_0_closing_signed.fee_range.as_ref().unwrap().min_fee_satoshis <=
+	        node_1_closing_signed.fee_range.as_ref().unwrap().min_fee_satoshis * 10 + 5);
+
+	// Further, because nodes[0]'s target fee is larger than the `Normal` fee estimation plus our
+	// force-closure-avoidance buffer, min should equal max, and the nodes[1]-selected fee should
+	// be the nodes[0] only available fee.
+	assert_eq!(node_0_closing_signed.fee_range.as_ref().unwrap().min_fee_satoshis,
+	           node_0_closing_signed.fee_range.as_ref().unwrap().max_fee_satoshis);
+	assert_eq!(node_0_closing_signed.fee_range.as_ref().unwrap().min_fee_satoshis,
+	           node_0_closing_signed.fee_satoshis);
+	assert_eq!(node_0_closing_signed.fee_satoshis, node_1_closing_signed.fee_satoshis);
+
+	nodes[0].node.handle_closing_signed(&nodes[1].node.get_our_node_id(), &node_1_closing_signed);
+	let (_, node_0_none) = get_closing_signed_broadcast!(nodes[0].node, nodes[1].node.get_our_node_id());
+	assert!(node_0_none.is_none());
+}
$

ariard

Code Review ACK 82e7df1.

I had a minimal last look, I don't think it's breaking more a part of the codebase which was already buggy and non-compliant. We can improve further once the spec has made progress on solving the dust_limit_satoshis issue at closing and we have CPFP support post-anchor output.

ariard · 2021-08-17T17:23:48Z

lightning/src/ln/channel.rs

+		}
+
+		// Note that technically we could end up with a lower minimum fee if one sides' balance is
+		// below our dust limit, causing the output to disappear. We don't bother handling this


As observed yesterday during the meeting, I think enforcing the dust_limit_satoshis on closing transactions is a bit dubious as it would re-introduce a negotiation among counterparties on the "honest" dust_limit_satoshis at ongoing feerate.

If the output type is known, let's just enforced compared to known Core values and otherwise take the non-safety risk of non-propagating transactions. We can always add checks when new outputs types will be deployed with a corresponding policy. Though spec discussion.

Right, I think we'll want to use the "each side can unilaterally remove its output" thing, but its somewhat broken in the current spec. Will work with t-bast on it.

ariard · 2021-08-17T17:34:46Z

lightning/src/ln/channel.rs


+		// Propose a range from our current Background feerate to our Normal feerate plus our
+		// force_close_avoidance_max_fee_satoshis.
+		// If we fail to come to consensus, we'll have to force-close.
 		let mut proposed_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);


One hardening we could introduce in the future would be to hardcode an upper bound in case of compromise of our fee-estimator. The closing transaction max size is known and in the worst-case scenario the confirmation of a closing transaction is not a matter of safety.

Hmm, yea, we've talked a few times about bounding the fee estimator results to "reasonable" values. We should probably do it sometime, but for now I'd say its fine.

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from 58a5baa to 02af55e Compare July 20, 2021 14:35

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch 3 times, most recently from ac65c47 to d986933 Compare July 21, 2021 15:33

TheBlueMatt added this to the 0.0.100 milestone Jul 22, 2021

jkczyz self-requested a review July 26, 2021 19:15

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from cafce2b to 9e1028c Compare July 26, 2021 19:43

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch 2 times, most recently from 498c66c to 11be074 Compare July 29, 2021 14:14

ariard reviewed Jul 30, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from 11be074 to 81e98c0 Compare July 31, 2021 17:28

TheBlueMatt mentioned this pull request Aug 4, 2021

Fetch shutdown script based on commit_upfront_shutdown_pubkey #1019

Merged

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from 81e98c0 to 9e0b281 Compare August 9, 2021 19:02

jkczyz requested a review from valentinewallace August 9, 2021 19:12

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from 9e0b281 to aab9709 Compare August 10, 2021 00:00

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch 2 times, most recently from 2ef0139 to 46d1f6e Compare August 10, 2021 22:25

jkczyz reviewed Aug 11, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from 46d1f6e to d18d119 Compare August 12, 2021 18:41

jkczyz reviewed Aug 13, 2021

View reviewed changes

valentinewallace reviewed Aug 13, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from 92feba8 to 4b31519 Compare August 13, 2021 22:11

jkczyz reviewed Aug 13, 2021

View reviewed changes

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch 2 times, most recently from 313b773 to 54b70fa Compare August 13, 2021 22:50

TheBlueMatt added 4 commits August 13, 2021 23:02

Add #[allow(unused_mut)] on reorg_test as older rustc requires mut

be9ffb4

Add new ChannelError category to send warning messages

bd14069

We don't actually yet support `warning` messages as there are issues left to resolve in the spec PR, but there's nothing to stop us adding an internal enum variant for sending a warning message before we actually support doing so.

Improve TLV serialization macro callability very slightly

769eea8

This allows decode_tlv_stream!() to be called with either a mutable reference to a stream or a stream itself and allows encode_tlv_stream!() to be called with an excess , at the end of the parameter list.

Implement the closing_signed TLV suffix with allowed fee ranges

6dc9076

This adds the serialization and structures for the new fee range specifiers in closing_signed as added upstream at lightning/bolts#847

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from 54b70fa to 3b706a1 Compare August 13, 2021 23:02

TheBlueMatt added 2 commits August 13, 2021 23:07

Log shutdown including which side of the channel initiated shutdown

267053f

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from 3b706a1 to 2141a3a Compare August 13, 2021 23:08

TheBlueMatt mentioned this pull request Aug 14, 2021

Cut 0.0.100 #1044

Merged

valentinewallace reviewed Aug 16, 2021

View reviewed changes

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

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

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

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from 2141a3a to db6c69d Compare August 16, 2021 22:50

TheBlueMatt added 7 commits August 17, 2021 02:16

Move shutdown-related tests into a new module

5712de2

Because ln::functional_tests if over 9000 LoC long, its useful to move tests into new modules as we can. Here we move all cooperative shutdown related tests into a new module entitled `shutdown_tests`

Force-close if finish closing_signed negotiation takes a full minute

d63b024

Add a test for shutdown negotiaion funder restart and timeout

dcb0832

Slightly clarify the closing_signed error msg on fee mismatch

0b481e9

Add relatively simple tests of the legacy and target closing fee

82e7df1

This doesn't exhaustively test closing fee negotiation at all, but ensures that it is at least basically able to come to consensus and sign cooperative closing transactions.

TheBlueMatt force-pushed the 2021-07-new-closing-fee branch from db6c69d to 82e7df1 Compare August 17, 2021 02:16

valentinewallace approved these changes Aug 17, 2021

View reviewed changes

ariard reviewed Aug 17, 2021

View reviewed changes

TheBlueMatt merged commit 6f16453 into lightningdevkit:main Aug 17, 2021

Clean up existing and add range-based closing_signed negotiation #1011

Clean up existing and add range-based closing_signed negotiation #1011

Conversation

TheBlueMatt commented Jul 20, 2021

codecov bot commented Jul 20, 2021 • edited Loading

Codecov Report

TheBlueMatt commented Jul 22, 2021

TheBlueMatt commented Jul 26, 2021

ariard 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

Choose a reason for hiding this comment

TheBlueMatt commented Aug 9, 2021

TheBlueMatt commented Aug 10, 2021

jkczyz 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

Choose a reason for hiding this comment

jkczyz commented Aug 11, 2021

jkczyz 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

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 commented Aug 12, 2021

TheBlueMatt commented Aug 12, 2021

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz 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

Choose a reason for hiding this comment

TheBlueMatt commented Aug 13, 2021 • edited Loading

valentinewallace left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 16, 2021

jkczyz commented Aug 17, 2021

TheBlueMatt commented Aug 17, 2021

ariard 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

Choose a reason for hiding this comment

codecov bot commented Jul 20, 2021 •

edited

Loading

TheBlueMatt commented Aug 13, 2021 •

edited

Loading