Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for opt_shutdown_anysegwit feature #780 #794

Merged

Conversation

galderz
Copy link
Contributor

@galderz galderz commented Feb 5, 2021

This is a draft PR for #780.

So far it includes:

  • Implemented protocol.
  • Made feature required for all.

The following is a checklist of doubts / todos I'd like to resolve:

  • At the very least, I'd like to add one more test, where the scriptpubkey on shutdown is a witness program, in order to verify that it's handled as correctly. Do you have any pointers for how to test this? Would it be another unit test in channel.rs or a functional test in functional_tests.rs?
  • Should there be another test for verifying that the counterparty shutdown scriptpubkey supports a witness program? I've added checks for it in the PR but I'd like to make sure they work.
  • There seems to be a fair bit of code duplication between new_from_req and accept_channel that affects this PR. Should that be refactored? Would channel.rs be the best place for it?
  • Improve the KeysInterface.get_shutdown_pubkey API (as suggested by Matt on here.

p.s. This is fun :)

@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #794 (4e7f1da) into main (d6f41d3) will decrease coverage by 0.02%.
The diff coverage is 92.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #794      +/-   ##
==========================================
- Coverage   90.85%   90.82%   -0.03%     
==========================================
  Files          45       45              
  Lines       24547    24660     +113     
==========================================
+ Hits        22301    22398      +97     
- Misses       2246     2262      +16     
Impacted Files Coverage Δ
lightning-net-tokio/src/lib.rs 76.70% <0.00%> (ø)
lightning/src/ln/msgs.rs 88.62% <ø> (ø)
lightning/src/ln/peer_handler.rs 49.72% <0.00%> (ø)
lightning/src/util/test_utils.rs 83.16% <0.00%> (ø)
lightning/src/ln/channel.rs 87.80% <88.23%> (+0.09%) ⬆️
lightning/src/ln/functional_tests.rs 96.94% <93.91%> (-0.26%) ⬇️
lightning/src/ln/channelmanager.rs 85.24% <100.00%> (ø)
lightning/src/ln/features.rs 98.84% <100.00%> (+0.04%) ⬆️
lightning/src/ln/functional_test_utils.rs 94.97% <100.00%> (ø)

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 d6f41d3...24ed1dc. Read the comment docs.

@galderz
Copy link
Contributor Author

galderz commented Feb 5, 2021

Added a further item to the list above. I had a chat with @TheBlueMatt on Slack to dig further into what he meant in the (c) section of his comment here. I'll try to add this as a further commit on this branch.

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to implement this, see my points about making it non-mandatory.

  • Have a look on test_upfront_shutdown_scripts in functional_tests.rs as an inspiration source for a functional test enforcing new feature at shutdown
  • IIUC, you want to craft a witness program v1 and feed it ? In what it differs from the test you're want to write above ?
  • Are you proposing a method check_counterparty_channel_policy to wrap code dup between accept/new_from_req ? Why not, but I guess you will still have some checks context-dependent. See Channel internal refactoring and cleanups #648 on further channel internal refactoring

Staying available on slack/irc if you have any questions :)

@@ -691,11 +691,17 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
return Err(ChannelError::Close("Insufficient funding amount for initial commitment".to_owned()));
}

// TODO duplicate code new_from_req / accept_channel
if !their_features.requires_shutdown_anysegwit() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check makes it mandatory for our peers to support opt_shutdown_anysegwit ? I would keep it optional for now, we don't higher versions of segwit deployed yet.

Copy link
Contributor Author

@galderz galderz Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm. That's indeed a doubt I had while I was working on this. I used @TheBlueMatt's comment here as guide:

(b) we should require the feature for all peers

I understood that that he meant that this would be mandatory. Happy to make it optional if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, sorry, as I'd mentioned on slack the other day, I think it may be a bit too early for this. Likely we can do it in a few months, at least once a few other implementations get it shipped, but its likely preferrable to have (b) and (c) sitting in a yet-unmerged PR for now.

Copy link
Contributor Author

@galderz galderz Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I missed the bit where you said it was too early ☺️ I'll rework the PR and let you know when it's ready.

@@ -2906,13 +2918,13 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
assert_eq!(self.channel_state & ChannelState::ShutdownComplete as u32, 0);

// BOLT 2 says we must only send a scriptpubkey of certain standard forms, which are up to
// 34 bytes in length, so don't let the remote peer feed us some super fee-heavy script.
if self.is_outbound() && msg.scriptpubkey.len() > 34 {
// 42 bytes in length, so don't let the remote peer feed us some super fee-heavy script.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe precise bip ref "A BIP-141-compliant witness program is at max 42 bytes in length, ..."

@@ -101,6 +101,8 @@ mod sealed {
StaticRemoteKey,
// Byte 2
,
// Byte 3
ShutdownAnySegwit,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my point above but I think for now it's better to keep this new feature optional.

@galderz
Copy link
Contributor Author

galderz commented Feb 11, 2021

Updated the PR with a new commit which hopefully handles all the feedback so far. Here's what I've done in this new commit:

  • Made the feature optional.
  • Verify that the default value is true.
  • Add a supports_shutdown_anysegwit field to Channel, based on the features of the remote peer.
  • Verify that on shutdown, if Channel.supports_shutdown_anysegwit is enabled, the script can be a witness program.
  • Added a test that verifies that a scriptpubkey for an unreleased segwit version is handled successfully.
  • Added a test that verifies that if node has op_shutdown_anysegwit disabled, a scriptpubkey with an unreleased segwit version on shutdown throws an error.

Side note: I feel it'd be cleaner if each of the situations tested inside test_upfront_shutdown_script would be a separate test itself. Any reason to keep them all inside there?

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good!

@@ -386,6 +386,8 @@ pub(super) struct Channel<ChanSigner: ChannelKeys> {
commitment_secrets: CounterpartyCommitmentSecrets,

network_sync: UpdateStatus,

supports_shutdown_anysegwit: bool,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to cache this in the Channel - see my comment at lightning/bolts#672 (review) but I think we should be passing the InitFeatures object through the shutdown message handler (this may require a change to ChannelManager, PeerHandler, and the ln::msgs::ChannelMessageHandler trait, but it should be as easy as adding a new parameter.

@@ -1392,7 +1399,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
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() {
let is_witness_program_allowed = their_features.supports_shutdown_anysegwit() && script.is_witness_program();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be nice to abstract this out into a helper function.

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my point, I think the spec is currently fuzzy on what is meant by "if only opt_shutdown_anysegwit is negotiated". Asking clarifications on IRC.

Side note: I feel it'd be cleaner if each of the situations tested inside test_upfront_shutdown_script would be a separate test itself. Any reason to keep them all inside there?

For sure, at least this new option deserves its own test. Zooming out, our functional test would be worthy to be split up and better documented to ease its understanding by newcomers and also what's actually covered by contributors. I've few branches doing it but never pushed it forward 😅

@@ -695,7 +699,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
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() {
let is_witness_program_allowed = their_features.supports_shutdown_anysegwit() && script.is_witness_program();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this PR checks holder node supports shutdown_anysegwit at shutdown message reception only.

It has the following pitfall. If our peer provide us a anysegwit scriptpubkey at channel opening through the upfront_shutdown feature we'll accept it but reject it later during the cooperative closure attempt, thus forcing us to fallback on a costly, privacy-damaging unilateral closure.

If we don't support shutdown_anysegwit, I think we should reject channel opening at all. It's up to our peer to read our public features correctly and propose us an upfront witness suiting us.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, so if I'm reading this right, you're suggesting that if they say they don't support shutdown anysegwit, but the script is witness, we fail right away?

@galderz
Copy link
Contributor Author

galderz commented Feb 12, 2021

See my point, I think the spec is currently fuzzy on what is meant by "if only opt_shutdown_anysegwit is negotiated". Asking clarifications on IRC.

Please update the issue/PR when you find out :)

For sure, at least this new option deserves its own test. Zooming out, our functional test would be worthy to be split up and better documented to ease its understanding by newcomers and also what's actually covered by contributors. I've few branches doing it but never pushed it forward 😅

Yeah, I'll probably create 2 separate tests, one for each different scenario in the next PR revision.

@galderz
Copy link
Contributor Author

galderz commented Feb 16, 2021

I've pushed some updates to address the latest round of feedback. The include:

  • Remove added field from Channel, and instead pass peer features via the handle_shutdown callback.
  • Created a helper function to indicate whether a shutdown script that's a segwit program is unsupported. Refactored code to use that helper function.
  • Use that function to check the validity of upfront shutdown script. Created a test that specifically tests the situation when the upfront shutdown script is segwit unsupported and fails quickly.
  • Finally, I've separated each test case into a separate test function for better encapsulation and improved readability.

@galderz
Copy link
Contributor Author

galderz commented Feb 16, 2021

By the way, I'm planning to squash the commits once the feature is ready, but for now I keep them separate for easier understanding of feedback improvements.

@galderz
Copy link
Contributor Author

galderz commented Feb 16, 2021

CI failure was due to my changes. Updating C bindings...

@TheBlueMatt
Copy link
Collaborator

Don't worry about the C bindings, we generally update those asynchronously (and are probably going to pull them out into a separate repo soon).

@@ -761,7 +761,7 @@ pub trait ChannelMessageHandler : MessageSendEventsProvider + Send + Sync {

// Channl close:
/// Handle an incoming shutdown message from the given peer.
fn handle_shutdown(&self, their_node_id: &PublicKey, msg: &Shutdown);
fn handle_shutdown(&self, their_node_id: &PublicKey, their_features: InitFeatures, msg: &Shutdown);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably pass their_features as a &InitFeatures instead, the Channel doesn't need ownership so good to avoid cloning.

@@ -1391,6 +1395,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
let counterparty_shutdown_scriptpubkey = if their_features.supports_upfront_shutdown_script() {
match &msg.shutdown_scriptpubkey {
&OptionalField::Present(ref script) => {
if unsupported_witness_shutdown_script(their_features, script) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just drop the next if block and do the pre-anysegwit check in unsupported_witness_script as well? It's a change in behavior to reject a new channel if the script is invalid, but that's ok, we probably should have been doing that initially anyway.

// BOLT 2 says we must only send a scriptpubkey of certain standard forms,
// which for a a BIP-141-compliant witness program is at max 42 bytes in length.
// So don't let the remote peer feed us some super fee-heavy script.
if self.is_outbound() && msg.scriptpubkey.len() > 42 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I dont see why this check had an is_outbound() on it - it didn't matter because the script had to be a more concrete (and small) type in the next check, but now it does. This check should probably also move into unsupported_witness_shutdown_script IMO.

@galderz
Copy link
Contributor Author

galderz commented Feb 18, 2021

Updated the PR with 3 commits to handle the latest feedback.

@galderz
Copy link
Contributor Author

galderz commented Feb 18, 2021

Added a new commit to tidy up things. is_unsupported_witness_shutdown_script does not have much use any more since all checks are covered inside is_unsupported_shutdown_script.

Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, so if I'm reading this right, you're suggesting that if they say they don't support shutdown anysegwit, but the script is witness, we fail right away?

After last LN devs meeting, we concluded this behavior can happen but was likely to be that much an edge-case that it wasn't worthy to mention even in the spec. If we or our peer forget we support opt_shutdown_anysegwit between channel opening and shutdown let's reject the witness and go for an unilateral close.

fn is_unsupported_shutdown_script(their_features: &InitFeatures, scriptpubkey: &Script, is_outbound: bool) -> bool {
// BOLT 2 says we must only send a scriptpubkey of certain standard forms,
// which for a a BIP-141-compliant witness program is at max 42 bytes in length.
// So don't let the remote peer feed us some super fee-heavy script.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We restrain shutdown scripts for at least different reasons :
a) to avoid fee-heavy script costing us a lot if we're funder (fees are only paid by funder for now)
b) to avoid transactions not propagating on the p2p tx-relay network (e.g sending you a 20-of-20 CHECKMULTISIG scriptpubkey, too big)

I think the first one is clear enough but would be better to underscore b), standardness is a more restrictive notion than smallness

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, this comment was already in place before this PR (see here). All I have done here is move it as a result of moving the validation code, and add a very small modification that you mentioned in this PR before.

If you want something different, can you be specific on the exact comment you want in the code? I can copy/paste what you said 👆 but not sure that's exactly what you want.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the risk of non-standard transactions broadcasts should be mentioned, it's the reason why those checks should be applied independently of funder/fundee role.

"We restrain shutdown scripts to standards forms to avoid transactions not propagating on the p2p tx-relay network"

// So don't let the remote peer feed us some super fee-heavy script.
let is_script_too_long = is_outbound && scriptpubkey.len() > 42;
let not_valid_witness = !their_features.supports_shutdown_anysegwit() && scriptpubkey.is_witness_program();
return is_script_too_long || (not_valid_witness && !scriptpubkey.is_p2pkh() && !scriptpubkey.is_p2sh() && !scriptpubkey.is_v0_p2wpkh() && !scriptpubkey.is_v0_p2wsh())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to check script length at all. P2PKH, P2SH and segwit scriptpubkeys by defintion MUST be inferior to 42 bytes.

For not_valid_witness, I think you need to pass our features support to check that our node is supporting shutdown_anysegwit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The is_script_too_long thing is also applied for anysegwit, so I think its fine?

As for checkout our own features - we always use Features::supported() so there's nothing to check against. We can revisit if we build a way for users to configure different features themselves.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! is_witness_program enforces size already https://docs.rs/bitcoin/0.26.0/src/bitcoin/blockdata/script.rs.html#366. I don't mind (kinda like) enforcing it double, but could be commented to note that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmmm, I would keep the check because the check inside is_witness_program should only be done if they support shutdown any segwit (I'll comment on the condition in the other comment).

// BOLT 2 says we must only send a scriptpubkey of certain standard forms,
// which for a a BIP-141-compliant witness program is at max 42 bytes in length.
// So don't let the remote peer feed us some super fee-heavy script.
let is_script_too_long = is_outbound && scriptpubkey.len() > 42;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is_outbound isn't relevant here - see first half of #794 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmm, I don't understand. I thought you meant that the check had to be moved into the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what the second part suggested. I didn't fully understand what you really were after with the first part of the comment :\

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to clarify on slack, sorry for the confusion. My point there was that I think we should do the script_too_long check in both the outbound and inbound channels, so no need for an is_outbound check here. This is a change against master, but not in practice, since the list of standard output types all come with a strict length check.

@@ -773,7 +774,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
channel_transaction_parameters: ChannelTransactionParameters {
holder_pubkeys: pubkeys,
holder_selected_contest_delay: config.own_channel_config.our_to_self_delay,
is_outbound_from_holder: false,
is_outbound_from_holder: is_outbound,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in this context (new_from_req) we're handling an incoming channel_open message so it is always inbound. I think the outbound parameter can be dropped from is_unsupported_shutdown_script anyway, so should just drop the is_outbound variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the outbound parameter check is dropped from is_unsupported_shutdown_script, is it kept anywhere in the context of handling shutdown?

// So don't let the remote peer feed us some super fee-heavy script.
let is_script_too_long = is_outbound && scriptpubkey.len() > 42;
let not_valid_witness = !their_features.supports_shutdown_anysegwit() && scriptpubkey.is_witness_program();
return is_script_too_long || (not_valid_witness && !scriptpubkey.is_p2pkh() && !scriptpubkey.is_p2sh() && !scriptpubkey.is_v0_p2wpkh() && !scriptpubkey.is_v0_p2wsh())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The is_script_too_long thing is also applied for anysegwit, so I think its fine?

As for checkout our own features - we always use Features::supported() so there's nothing to check against. We can revisit if we build a way for users to configure different features themselves.

// which for a a BIP-141-compliant witness program is at max 42 bytes in length.
// So don't let the remote peer feed us some super fee-heavy script.
let is_script_too_long = is_outbound && scriptpubkey.len() > 42;
let not_valid_witness = !their_features.supports_shutdown_anysegwit() && scriptpubkey.is_witness_program();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be !their_features.supports_shutdown_anysegwit() || !scriptpubkey.is_witness_program()? That would mean the script is invalid if the script is too long, or ((its not a witness or they didn't say they wanted any witness) and its not an otherwise-known type).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this doesn't seem right. their_features.supports_shutdown_anysegwit() && !scriptpubkey.is_witness_program() sounds better in my head...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this the witness check is also incorrect - witness version 0 where the push size is not 20 or 32 is non-standard so would result in us signing a non-standard transaction. The BOLT PR captures this, noting that anysegwit enables " OP_1 through OP_16 inclusive, followed by a single push of 2 to 40 bytes" but not OP_0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a specific test to make sure OP_0 starting script fails and will adjust other test to make sure OP_1 or any following are allowed.

@@ -253,6 +261,8 @@ mod sealed {
"Feature flags for `payment_secret`.");
define_feature!(17, BasicMPP, [InitContext, NodeContext],
"Feature flags for `basic_mpp`.");
define_feature!(25, ShutdownAnySegwit, [InitContext, NodeContext],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec PR has changed to 27.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx for heads up, I'll rebase and readjust as the last step.

@galderz
Copy link
Contributor Author

galderz commented Feb 23, 2021

I've squashed all the work so far (except the spec number update) so that I could easily rebase with the latest changes in main.

Before squashing I added the following changes that should handle all the latest comments:

  • Do unsupported shutdown scripts in both input and output channels, so removed the is_outbound parameter.
  • An empty shutdown script (len == 0) is valid, so added a check for that. This is necessary to avoid existing tests failing.
  • Modified anysegwit check to make sure it corresponds with the spec. We can't rely on the is_witness_program call.
  • Added a test to check that if a segwit script with version 0 is provided, the updated anysegwit check detects it and returns unsupported.
  • Added some extra comments to the is_unsupported_shutdown_script method.


// Providing an empty is supported
let script_lenght = script.len();
if script_lenght == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means we'll accept an empty script in shutdown, which we definitely cannot do - an empty script will be nonstandard. I think instead just check the 0-length case directly in the accept_channel/open_channel handling.

// PUSHNUM_1-PUSHNUM_16
&& (script_bytes[0] >= min_vernum && script_bytes[0] <= max_vernum)
// Second byte push opcode 2-40 bytes
&& script_bytes[1] >= opcodes::all::OP_PUSHBYTES_2.into_u8()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not allow PUSHBYTES_1? More generally, instead of adding a bunch of script parsing code, can we not just switch to script.is_witness_script() && script.as_bytes()[0] != OP_PUSHNUM_1? That should be less error-prone.

let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &user_cfgs);
let nodes = create_network(3, &node_cfgs, &node_chanmgrs);

//// We test that if the remote peer does not accept opt_shutdown_anysegwit, the witness program cannot be used on shutdown
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is wrong - we're using anysegwit here, but checking that a nonstandard-size version-0 script fails.

@galderz
Copy link
Contributor Author

galderz commented Feb 23, 2021

Addressed latest round of feedback:

  • An empty script is only allowed when sent as upfront shutdown script, so make sure that check is only done for accept/open_channel situations.
  • Instead of reimplementing a variant of is_witness_script, just call it and verify that the witness version is not 0.
  • Fixed an outdated comment after a test copy/paste.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a squash before merge (in general, its best if we avoid commits that exist just to fix code in a previous commit unless it helps with reviewability of the commits), but code looks good.

@ariard
Copy link

ariard commented Feb 24, 2021

Code Review ACK 6746a16 modulo squash!

@galderz
Copy link
Contributor Author

galderz commented Feb 25, 2021

Needs a squash before merge (in general, its best if we avoid commits that exist just to fix code in a previous commit unless it helps with reviewability of the commits), but code looks good.

Indeed I think the same. It's just that sometimes when I'm working on a PR I like to keep the commits separate so that it's easier for the reviewer what feedback has just been handled. This means that the reviewer can focus a subsequent review in just the last commit. Indeed at the end I normally squash things unless there's a good reason to keep them separate (e.g. make backporting easier or clear distinct functionality). We're on the same page :)

* Implemented protocol.
* Made feature optional.
* Verify that the default value is true.
* Verify that on shutdown,
if Channel.supports_shutdown_anysegwit is enabled,
the script can be a witness program.
* Added a test that verifies that a scriptpubkey
for an unreleased segwit version is handled successfully.
* Added a test that verifies that
if node has op_shutdown_anysegwit disabled,
a scriptpubkey with an unreleased segwit version on shutdown
throws an error.
* Added peer InitFeatures to handle_shutdown
* Check if shutdown script is valid when given upfront.
* Added a test to verify that an invalid test results in error.
* Added a test to check that if a segwit script with version 0 is provided,
the updated anysegwit check detects it and returns unsupported.
* An empty script is only allowed when sent as upfront shutdown script,
so make sure that check is only done for accept/open_channel situations.
* Instead of reimplementing a variant of is_witness_script,
just call it and verify that the witness version is not 0.
@galderz
Copy link
Contributor Author

galderz commented Feb 25, 2021

Commits squashed 😊

@TheBlueMatt TheBlueMatt merged commit ad09a2f into lightningdevkit:main Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants