Implement option_upfront_shutdown_script on both sides #348

ariard · 2019-07-09T19:51:22Z

We take care of not enforcing the option if remote peer send us an open_channel/accept_channel messages including an shutdown_scriptpubkey but without asking for in feature flags.
On the user-side, we rely on the config to decide to commit.

Close #239

TheBlueMatt · 2019-07-10T21:00:15Z

This could probably use some tests, no?

TheBlueMatt · 2019-07-10T21:03:40Z

Technically, I think you can drop the entire "Provide peer local_features to handle_open_channel/accept_channel" commit - the way I read the spec, if the peer sends an upfront_shutdown_script but didn't bother signaling for it, we should still honor and enforce it. Further, we should be allowed to send our own upfront_shutdown_script whether they signal for it or not - they should ignore any unknown parts at the end of a message.

ariard · 2019-07-10T21:06:21Z

Yes, was waiting for your interpretation of the spec before to write tests, specially on the question of peer need to opt-in for it, the way I read the spec is the reverse "if both nodes advertised the option_upfront_shutdown_script feature" seems to mean both peers need to signal for it.

ariard · 2019-07-10T21:09:28Z

For sending our own upfront_shutdown_script, I think I've implemented with simple opt-in of user (via config), as spec always let us "MAY include ashutdown_scriptpubkey."

TheBlueMatt · 2019-07-10T21:10:12Z

No, the section you're quoting only talks about when to include the serialized script (or a zero-length one to opt-out if we signal). This implies that we should just always signal, but send either a zero-length script or one specified by the user, but never None. The shutdown discussion talks only about whether a non-zero-length shutdown script was sent, not whether the flag was set.

ariard · 2019-07-10T21:16:10Z

From the shutdown discussion : "if both nodes advertised the option_upfront_shutdown_script feature, and the receiving node received a non-zero-length shutdown_scriptpubkey in open_channel or accept_channel, and that shutdown_scriptpubkey is not equal to scriptpubkey:

MUST fail the connection." First sentence isn't about flag being set ?

You're right on user-side we should send zero-length script to opt-out not None if we signal.

TheBlueMatt · 2019-07-10T21:17:10Z

Ah, you're right, I missed the difference between the sending side of shutdown and the receiving side of shutdown, alright, looks good.

ariard · 2019-07-10T21:18:20Z

Cool will join tests now we agree on interpretation :)

ariard · 2019-07-11T20:21:17Z

Test on 8b245b2. Larger diff is about modification of test utilities to pass LocalFeatures and UserConfig, it should become more used with the bunch of 1.1 features being flag-gated.

TheBlueMatt · 2019-07-17T18:02:58Z

src/ln/channelmanager.rs

 	}

-	fn internal_open_channel(&self, their_node_id: &PublicKey, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {
+	fn internal_open_channel(&self, their_node_id: &PublicKey, their_local_features: &Option<LocalFeatures>, msg: &msgs::OpenChannel) -> Result<(), MsgHandleErrInternal> {


Hmm, why Optional? Protocol should enforce that its always set, no?

Yes, after verification it's always set, corrected

TheBlueMatt · 2019-07-17T18:11:43Z

src/ln/channel.rs

+			if features.supports_upfront_shutdown_script() {
+				match &msg.shutdown_scriptpubkey {
+					&OptionalField::Present(ref script) => {
+						if script.is_p2pkh() || script.is_p2sh() || script.is_v0_p2wsh() || script.is_v0_p2wpkh() {


Hmm, shouldn't we reject channels with a shutdown script set to something other than these (or 0-length)?

Reworked to reject channels with a shutdown script different from accepted formats if peer is signaling option. BOLT2 : "if the scriptpubkey is not in one of the above forms:
SHOULD fail the connection.
"

TheBlueMatt · 2019-07-17T18:12:33Z

src/ln/peer_handler.rs

 													if self.initial_syncs_sent.load(Ordering::Acquire) < INITIAL_SYNCS_TO_SEND {
 														self.initial_syncs_sent.fetch_add(1, Ordering::AcqRel);
 														local_features.set_initial_routing_sync();
+														local_features.set_upfront_shutdown_script();


Huh? We should set this on all peers, not just the first 5.

Also, technically this hunk should go in a later commit, when we set ourselves to always send (at least) a 0-length script.

Maybe add a test that connects a bunch of peers, too, while you're at it?

Oh yes confused myself on this one! Move to right commit

src/ln/functional_test_utils.rs

TheBlueMatt · 2019-07-19T19:01:41Z

src/ln/channel.rs

 		channel_monitor.set_their_base_keys(&msg.htlc_basepoint, &msg.delayed_payment_basepoint);
 		channel_monitor.set_their_to_self_delay(msg.to_self_delay);

+		let their_shutdown_scriptpubkey = if their_local_features.supports_upfront_shutdown_script() {


Shouldn't we use the shutdown_scriptpubkey if it is Present (ad not-0-length) even if they have't set their local features? We just reject the message if they did set local features (and we did too) and they didnt send a shutdown_scriptpubkey?

On first point, spec said "otherwise:

MAY include ashutdown_scriptpubkey."

But that's a option offered for sending node, not a requirement for the receiving node to enforce upfront_shutdown it sending doesn't signal?

Second point, well I think 0-length is the correct opt-out mechanism, which is exactly to avoid ambiguity. IMO, a node signaling upfront_shutdown should opt-out or we may assume it's a buggy one ?

Oops, you're right, sorry, still reading the wrong side of the spec.

Peer may send us a shutdown_scriptpubkey in open_channel or accept_channel messages. Before to enforce this policy on channel closing, we want to be sure that our peer has opt-in to it. Extend LocalFeatures new method visibilty from crate to public for fuzz tests

src/ln/channel.rs

TheBlueMatt · 2019-07-22T18:29:16Z

src/ln/channel.rs

+			match &msg.shutdown_scriptpubkey {
+				&OptionalField::Present(ref script) => {
+					// Peer is signaling upfront_shutdown and has provided a non-accepted scriptpubkey format. We enforce it while receiving shutdown msg
+					if script.is_p2pkh() || script.is_p2sh() || script.is_v0_p2wsh() || script.is_v0_p2wpkh() || script.len() == 0 {


the script.len()== 0 check at the end breaks the next if.

Are you on the latest tip ? I've pushed a version with this bug but it was corrected at least on b603d94

Looks like its still there on the latest?

Sorry, I've forgot to correct on both open_channel/accept_channel case, extend test_option_upfront_shutdown_script to cover it

src/ln/msgs.rs

TheBlueMatt · 2019-07-22T18:35:22Z

src/ln/functional_test_utils.rs

 }

-pub fn create_network(node_count: usize) -> Vec<Node> {
+pub fn create_network(node_count: usize, node_config: &[Option<UserConfig>]) -> Vec<Node> {


nit: If we're just gonna call UserConfig::new() here, why not just do that everywhere instead of None, None? Its not much different and keeps things obvious.

Hmmm but if we pass a new UserConfig, then we set default flags in create_network but for some specific tests I want to deactivate them ?

Ehh, I meant just drop the Option<>

Wait if I pass create_network(2, UserConfig::new(), UserConfig::new()) create_network is going to set default flags, a behavior I wish to opt-out?

You can provide custom ones instead where needed?

I can, but if create_network override them with the default ones, how I signal to do it with a per-node granularity ?

Keep the array? Anyway, not worth fixing if its just this left to fix.

Track shutdown_pubkey of peer at open/accept_channel messages Fix encoding_init test

We use user config to decide to commit to closing script in open_channel/accept_channel messages. We don't check that other peer supporting the option as including script without other peer public support is borne by the protocol. If user opt-out, following protocol and due to the fact we always signal, we provide a zero-length script

We may want more granularity on the set of features activated at a given time, specially with new 1.1 spec features

We way want more granularity on the set of user opt-in features at a given time, specially with new 1.1 spec features

TheBlueMatt reviewed Jul 17, 2019

View reviewed changes

TheBlueMatt reviewed Jul 19, 2019

View reviewed changes

TheBlueMatt reviewed Jul 22, 2019

View reviewed changes

Antoine Riard added 5 commits July 22, 2019 17:03

Support option_upfront_shutdown_script for local peer

675cf4a

Track shutdown_pubkey of peer at open/accept_channel messages Fix encoding_init test

Pass LocalFeatures flag to channel creation test utilities

d2647e0

We may want more granularity on the set of features activated at a given time, specially with new 1.1 spec features

Pass UserConfig to node creation test utilities

a4620af

We way want more granularity on the set of user opt-in features at a given time, specially with new 1.1 spec features

Test option_upfront_shutdown_script

504d9f5

TheBlueMatt merged commit ef3e9dd into lightningdevkit:master Jul 22, 2019

Implement option_upfront_shutdown_script on both sides #348

Implement option_upfront_shutdown_script on both sides #348

Uh oh!

Conversation

ariard commented Jul 9, 2019

Uh oh!

TheBlueMatt commented Jul 10, 2019

Uh oh!

TheBlueMatt commented Jul 10, 2019

Uh oh!

ariard commented Jul 10, 2019

Uh oh!

ariard commented Jul 10, 2019

Uh oh!

TheBlueMatt commented Jul 10, 2019

Uh oh!

ariard commented Jul 10, 2019

Uh oh!

TheBlueMatt commented Jul 10, 2019

Uh oh!

ariard commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariard commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ariard commented Jul 10, 2019 •

edited

Loading

ariard commented Jul 11, 2019 •

edited

Loading