Fetch shutdown script based on `commit_upfront_shutdown_pubkey` #1019

jkczyz · 2021-07-28T20:39:18Z

Rather than fetching a channel's shutdown script from KeysInterface at creation time, do so based on ChannelConfig::commit_upfront_shutdown_pubkey (i.e., either creation or shutdown time).

Additionally, support any acceptable shutdown script pubkey as defined by BOLT 2 while maintaining backwards compatibility with the legacy KeysInterface::get_shutdown_pubkey, which is replaced by KeysInterface::get_shutdown_scriptpubkey.

This change requires storing an optional script as a TLV field for both Channel and ChannelMonitor. Additionally, since the field is now optional, a new ChannelMonitorUpdateStep is needed for when the script is generated at shutdown time.

Fixes #994.

codecov · 2021-07-28T20:42:23Z

Codecov Report

Merging #1019 (b24b4e9) into main (69ee486) will increase coverage by 0.12%.
The diff coverage is 92.15%.

❗ Current head b24b4e9 differs from pull request most recent head 1d3861e. Consider uploading reports for the commit 1d3861e to get more accurate results

@@            Coverage Diff             @@
##             main    #1019      +/-   ##
==========================================
+ Coverage   90.79%   90.92%   +0.12%     
==========================================
  Files          61       62       +1     
  Lines       31578    32253     +675     
==========================================
+ Hits        28672    29326     +654     
- Misses       2906     2927      +21

Impacted Files	Coverage Δ
lightning/src/ln/mod.rs	`90.00% <ø> (ø)`
lightning/src/util/errors.rs	`67.30% <0.00%> (-4.13%)`	⬇️
lightning/src/util/test_utils.rs	`82.29% <76.00%> (-0.26%)`	⬇️
lightning/src/chain/channelmonitor.rs	`90.43% <83.33%> (-0.15%)`	⬇️
lightning/src/ln/channelmanager.rs	`86.82% <85.45%> (+1.12%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.11% <91.66%> (-0.07%)`	⬇️
lightning/src/ln/script.rs	`93.75% <93.75%> (ø)`
lightning/src/ln/functional_tests.rs	`97.23% <93.87%> (-0.05%)`	⬇️
lightning/src/ln/channel.rs	`89.34% <95.20%> (+0.11%)`	⬆️
lightning-background-processor/src/lib.rs	`95.48% <100.00%> (-0.10%)`	⬇️
... and 7 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 69ee486...1d3861e. Read the comment docs.

lightning/src/chain/channelmonitor.rs

lightning/src/ln/script.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt · 2021-07-31T14:42:55Z

Oops, this needs a similar test fix in the background-processor as well - https://git.bitcoin.ninja/index.cgi?p=rust-lightning;a=commit;h=9218054becc3dcf690bdd8fcf819952ee783365a

TheBlueMatt · 2021-07-31T14:50:00Z

lightning/src/ln/script.rs

+	/// # Panics
+	///
+	/// This function may panic if given a segwit program with an invalid length.
+	pub fn new_witness_program(version: NonZeroU8, program: &[u8]) -> Self {


I think we should return an Result here - version needs to be <= 16 so users can totally make it fail, we shouldn't panic in that case, no? Also, why do the conversion to u5 and then call new_witness_program? That seems like a very roundabout way to get there, just push the version followed by the program as done at https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#280-290 (but do the push using push_int which does the right thing here, don't have to resort to manual opcode calculation https://docs.rs/bitcoin/0.27.0/src/bitcoin/blockdata/script.rs.html#706-720 )

Also this will change soon upstream, not sure how that would impact this. rust-bitcoin/rust-bitcoin#617

Yeah, must cleaner like that. Will need to swap NonZeroU8 for WitnessVersion when it is available upstream.

lightning/src/ln/script.rs

lightning/src/chain/channelmonitor.rs

lightning/src/ln/channel.rs

TheBlueMatt · 2021-07-31T22:02:32Z

lightning/src/ln/channelmanager.rs

 						msg: shutdown_msg
 					});
+					if let Some(monitor_update) = monitor_update {
+						if let Err(_) = self.chain_monitor.update_channel(chan_entry.get().get_funding_txo().unwrap(), monitor_update) {
+							// TODO: How should this be handled?


Should be handled like any other error - call handle_monitor_err to convert the error into a MsgHandleErrInternal, then store it in a variable outside the lock (like failed_htlcs and chan_option) and handle_error the error outside of the lock.

Hmmm... in the case of an error should we still send the shutdown message above? Similarly, should an error short-circuit any remaining code (e.g., failing back HTLCs, broadcasting channel update)?

in the case of an error should we still send the shutdown message above?

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message, for temporary, we should still send the shutdown.

failing back HTLCs,

We must never, ever forget to fail back htlcs we were told to fail back.

broadcasting channel update

This is only if we force-closed the channel due to an error, I think, so we shouldn't get to this point today if there's a monitor update, but a monitor update could cause us to get there now (if its a permanent one).

Ok, this turned out to be a bit more tricky than I imagined. Confirming my understanding below.

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

There are two errors - permanent ones and temporary ones. In the case of permenent, yes, we should force-close instead and send an error message,

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

for temporary, we should still send the shutdown.

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

Rebased and added for internal_shutdown, too: a024930

Change is in 49614d8. Let me know if that is what you had in mind and if there's a simpler way of going about this. Need to do the same for internal_shutdown.

Nope, that's about right. These things are tricky to handle. I left a few specific comments on that commit.

These are handled by handle_monitor_err and handle_error, respectively, IIUC.

That's the MsgHandleErrInternal vs monitor-error distinction, not the permanent-vs-temporary distinction, both of which are handled by handle_monitor_err (and friends)

What I don't quite understand is why we would send shutdown if we aren't certain that our ChannelMonitor has been updated with the shutdown script yet. Is it because if TemporaryError ever becomes a PermanentError, we wouldn't care about the shutdown script since we are force closing in that case?

The Temporary->Permanent thing isn't a big deal, as you note. The real question is what happens if someone gives a Temporary update, and then takes longer to update their ChannelMonitor than we do doing the full closing_signed negotiation (because only after closing_signed negotiation could a transaction ever appear on chain with the script contained in the monitor update). Unlike most other monitor updates (at least RevokeAndACK), sending a Shutdown message isn't an immediate lock-in to a new states and the monitor doesn't need to know about it immediately, only before the closing transaction appears on chain.

If we wait for the update to complete here, then we have to add Shutdown resending, which I really don't want to do. An alternative, which is much easier, is to just wait for the monitor update to complete before ever sending a closing_signed, something I can shove in #985

lightning/src/ln/script.rs

lightning/src/ln/channelmanager.rs

TheBlueMatt

The ChannelMonitorUpdate changes look good to me, will re-review the rest a bit later but the rest was already basically good IMO.

lightning/src/ln/channelmanager.rs

lightning/src/ln/channel.rs

TheBlueMatt

Basically LGTM

lightning/src/ln/channel.rs

lightning/src/util/errors.rs

valentinewallace

Some of the new logic in ChannelManager is still sinking in for me, but overall this is looking good! Finished a first complete pass

lightning/src/ln/functional_tests.rs

jkczyz

Thanks for all the good feedback! Will see what I can do about the missing test coverage. It's a matter how easy it is to induce some error conditions (e.g., monitor update failing, keys provider giving an incompatible script).

lightning/src/ln/channel.rs

lightning/src/ln/channelmanager.rs

lightning/src/ln/functional_tests.rs

valentinewallace · 2021-08-06T19:02:15Z

lightning/src/ln/channelmanager.rs

 						msg: shutdown_msg
 					});
+
 					if chan_entry.get().is_shutdown() {


Is it the intended behavior to only broadcast a channel update if the channel was < ChannelState::FundingSent? Because I think this line will only return true if that's the situation 🤔 .

I think coverage is missing here too. I get that we're in "crunch time" though, so I'm pretty fine on punting tests if it comes to it

I believe it may also occur if we already handled a shutdown from the counterparty but didn't yet send our own shutdown.

Ah, actually now that I think of it, you're right. Since we lock channel_state, the situation that I mentioned can't occur.

Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

I believe this code block is actually now unreachable. We return an Err in case we're ready to shut down here instead of getting into this block.

Removed code block from internal_shutdown rather than here as discussed on Slack.

valentinewallace

I can't really find anything to comment on, this is shaping up from my PoV!

valentinewallace · 2021-08-06T19:42:13Z

lightning/src/ln/channel.rs

@@ -843,6 +854,16 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 		} else { None };


Prob not worth checking to see if they set a script here (which would make them a buggy peer), right?

They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

lightning/src/ln/channel.rs

TheBlueMatt · 2021-08-08T20:12:18Z

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

jkczyz

Added the requested tests. For testing IncompatibleShutdownScript errors, I needed to add InitFeatures to NodeCfg and pass it when connecting peers (see separate commit 2e03385) since ChannelManager::close_channel pulls the features from per_peer_state. We could probably use this in some of the test utilities instead of passing &InitFeatures::known() everywhere, but that's a more involved follow-up.

It looks like this was rebased without any conflict, was there a reason for the rebase I missed?

I was probably proactively rebasing just in case. 🙂

jkczyz · 2021-08-09T04:47:22Z

lightning/src/ln/channel.rs

@@ -843,6 +854,16 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 		} else { None };


They are allowed to support the feature but not necessarily set the shutdown script upfront, if I understand BOLT 2 correctly.

lightning/src/ln/channel.rs

TheBlueMatt · 2021-08-09T15:17:28Z

I was probably proactively rebasing just in case. slightly_smiling_face

Oops, in general is it possible to avoid this? It makes it hard to diff-tree between revisions to see what changed.

valentinewallace

ACK mod one last clarification. Thanks for the additional test coverage added!

valentinewallace · 2021-08-09T17:18:09Z

lightning/src/ln/channel.rs

@@ -843,6 +854,16 @@ impl<Signer: Sign> Channel<Signer> {
 			}
 		} else { None };


They are allowed to support the feature but not necessarily set the shutdown script upfront,

IIUC, this is allowed unless both peers advertise support for the feature, in which case setting a script is required: from BOLT 2

I was concerned about the case where a peer didn't advertise the feature but still set a shutdown script up front, but checked the spec and that seems to be allowed 🤷

valentinewallace · 2021-08-09T17:47:22Z

lightning/src/ln/channelmanager.rs

 						msg: shutdown_msg
 					});
+
 					if chan_entry.get().is_shutdown() {


Yeah, I guess I just wanna get clear that this code block (and similar) are necessary, and what situation they would run in? Double checked and couldn't find any test coverage by adding prints

TheBlueMatt

ACK post-squash.

BOLT 2 enumerates the script formats that may be used for a shutdown script. KeysInterface::get_shutdown_pubkey returns a PublicKey used to form one of the acceptable formats (P2WPKH). Add a ShutdownScript abstraction to encapsulate all accept formats and be backwards compatible with P2WPKH scripts serialized as the corresponding PublicKey.

KeysInterface::get_shutdown_pubkey is used to form P2WPKH shutdown scripts. However, BOLT 2 allows for a wider variety of scripts. Refactor KeysInterface to allow any supported script while still maintaining serialization backwards compatibility with P2WPKH script pubkeys stored simply as the PublicKey. Add an optional TLV field to Channel and ChannelMonitor to support the new format, but continue to serialize the legacy PublicKey format.

Similar to 2745bd5, this ensures that ChannelManager knows about the features its peers.

When a shutdown script is omitted from open_channel or accept_channel, it must be provided when sending shutdown. Generate the shutdown script at channel closing time in this case rather at channel opening. This requires producing a ChannelMonitorUpdate with the shutdown script since it is no longer known at ChannelMonitor creation.

When handling shutdown messages, Channel cannot move to ChannelState::ShutdownComplete. Remove the code in ChannelManager that adds a MessageSendEvent::BroadcastChannelUpdate in this case since it is unreachable.

jkczyz force-pushed the 2021-07-shutdown-pubkey branch from e75d748 to c0bf4fd Compare July 28, 2021 23:19

jkczyz mentioned this pull request Jul 29, 2021

Require feature option_shutdown_anysegwit #1018

Closed

jkczyz force-pushed the 2021-07-shutdown-pubkey branch 2 times, most recently from 05d73ed to 83f3245 Compare July 30, 2021 04:23

TheBlueMatt reviewed Jul 30, 2021

View reviewed changes

jkczyz mentioned this pull request Jul 30, 2021

Connect to channel peers before creating a channel in tests. #971

Closed

TheBlueMatt added this to the 0.0.100 milestone Jul 31, 2021

jkczyz force-pushed the 2021-07-shutdown-pubkey branch from 83f3245 to ade99f8 Compare July 31, 2021 05:14

jkczyz force-pushed the 2021-07-shutdown-pubkey branch from ade99f8 to 260f269 Compare July 31, 2021 15:41

TheBlueMatt reviewed Jul 31, 2021

View reviewed changes

jkczyz force-pushed the 2021-07-shutdown-pubkey branch from 260f269 to c8ffc05 Compare July 31, 2021 18:31

TheBlueMatt reviewed Jul 31, 2021

View reviewed changes

jkczyz force-pushed the 2021-07-shutdown-pubkey branch 10 times, most recently from 9924683 to 621e31e Compare August 4, 2021 19:20

TheBlueMatt reviewed Aug 4, 2021

View reviewed changes

lightning/src/ln/script.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

jkczyz force-pushed the 2021-07-shutdown-pubkey branch from 621e31e to d808581 Compare August 4, 2021 20:44

TheBlueMatt reviewed Aug 4, 2021

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 Show resolved Hide resolved

valentinewallace reviewed Aug 4, 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/channel.rs Show resolved Hide resolved

jkczyz force-pushed the 2021-07-shutdown-pubkey branch 2 times, most recently from 46075b3 to 79eb7da Compare August 4, 2021 23:30

TheBlueMatt reviewed Aug 5, 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/util/errors.rs Show resolved Hide resolved

valentinewallace reviewed Aug 5, 2021

View reviewed changes

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

jkczyz commented Aug 6, 2021

View reviewed changes

jkczyz force-pushed the 2021-07-shutdown-pubkey branch from 3f87283 to d6aec00 Compare August 6, 2021 04:18

valentinewallace reviewed Aug 6, 2021

View reviewed changes

TheBlueMatt approved these changes Aug 8, 2021

View reviewed changes

jkczyz force-pushed the 2021-07-shutdown-pubkey branch from d6aec00 to fbf624e Compare August 9, 2021 05:04

jkczyz commented Aug 9, 2021

View reviewed changes

jkczyz force-pushed the 2021-07-shutdown-pubkey branch from fbf624e to 76307ce Compare August 9, 2021 12:58

valentinewallace reviewed Aug 9, 2021

View reviewed changes

TheBlueMatt mentioned this pull request Aug 9, 2021

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

Merged

jkczyz force-pushed the 2021-07-shutdown-pubkey branch from 76307ce to b24b4e9 Compare August 9, 2021 20:33

valentinewallace approved these changes Aug 9, 2021

View reviewed changes

TheBlueMatt approved these changes Aug 9, 2021

View reviewed changes

jkczyz added 11 commits August 9, 2021 15:55

Clean up and add shutdown script functional tests

2833786

Use ShutdownScript to check scripts from peers

1ab2c7c

Connect nodes in remaining tests

89fa274

Similar to 2745bd5, this ensures that ChannelManager knows about the features its peers.

Add features to NodeCfg and use in create_network

1c449a3

Remove unreachable BroadcastChannelUpdate

f1c07b5

When handling shutdown messages, Channel cannot move to ChannelState::ShutdownComplete. Remove the code in ChannelManager that adds a MessageSendEvent::BroadcastChannelUpdate in this case since it is unreachable.

Pass InitFeatures by reference to Channel

e1989ad

Implement Display for ShutdownScript

4a44bfa

Add APIError::IncompatibleShutdownScript

1d3861e

jkczyz force-pushed the 2021-07-shutdown-pubkey branch from b24b4e9 to 1d3861e Compare August 9, 2021 21:00

TheBlueMatt merged commit 767f120 into lightningdevkit:main Aug 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch shutdown script based on `commit_upfront_shutdown_pubkey` #1019

Fetch shutdown script based on `commit_upfront_shutdown_pubkey` #1019

jkczyz commented Jul 28, 2021 •

edited

codecov bot commented Jul 28, 2021 •

edited

TheBlueMatt commented Jul 31, 2021

TheBlueMatt Jul 31, 2021

TheBlueMatt Jul 31, 2021

jkczyz Jul 31, 2021

TheBlueMatt Jul 31, 2021

jkczyz Aug 2, 2021

TheBlueMatt Aug 2, 2021

jkczyz Aug 4, 2021

jkczyz Aug 4, 2021

TheBlueMatt Aug 4, 2021

TheBlueMatt left a comment

TheBlueMatt left a comment

valentinewallace left a comment

jkczyz left a comment

valentinewallace Aug 6, 2021 •

edited

jkczyz Aug 9, 2021

jkczyz Aug 9, 2021

valentinewallace Aug 9, 2021

TheBlueMatt Aug 9, 2021

jkczyz Aug 9, 2021

valentinewallace left a comment

valentinewallace Aug 6, 2021

jkczyz Aug 9, 2021

valentinewallace Aug 9, 2021

TheBlueMatt commented Aug 8, 2021

jkczyz left a comment

jkczyz Aug 9, 2021

TheBlueMatt commented Aug 9, 2021

valentinewallace left a comment •

edited

valentinewallace Aug 9, 2021

valentinewallace Aug 9, 2021

TheBlueMatt left a comment

		@@ -843,6 +854,16 @@ impl<Signer: Sign> Channel<Signer> {
		}
		} else { None };

Fetch shutdown script based on commit_upfront_shutdown_pubkey #1019

Fetch shutdown script based on commit_upfront_shutdown_pubkey #1019

Conversation

jkczyz commented Jul 28, 2021 • edited

codecov bot commented Jul 28, 2021 • edited

Codecov Report

TheBlueMatt commented Jul 31, 2021

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 left a comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

valentinewallace left a comment

Choose a reason for hiding this comment

jkczyz left a comment

Choose a reason for hiding this comment

valentinewallace Aug 6, 2021 • edited

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

valentinewallace 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

TheBlueMatt commented Aug 8, 2021

jkczyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Aug 9, 2021

valentinewallace left a comment • edited

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

Fetch shutdown script based on `commit_upfront_shutdown_pubkey` #1019

Fetch shutdown script based on `commit_upfront_shutdown_pubkey` #1019

jkczyz commented Jul 28, 2021 •

edited

codecov bot commented Jul 28, 2021 •

edited

valentinewallace Aug 6, 2021 •

edited

valentinewallace left a comment •

edited