-
Notifications
You must be signed in to change notification settings - Fork 374
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
Add support for opt_shutdown_anysegwit
feature #780
#794
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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 |
There was a problem hiding this 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
infunctional_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 betweenaccept/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 :)
lightning/src/ln/channel.rs
Outdated
@@ -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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
lightning/src/ln/channel.rs
Outdated
@@ -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. |
There was a problem hiding this comment.
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, ..."
lightning/src/ln/features.rs
Outdated
@@ -101,6 +101,8 @@ mod sealed { | |||
StaticRemoteKey, | |||
// Byte 2 | |||
, | |||
// Byte 3 | |||
ShutdownAnySegwit, |
There was a problem hiding this comment.
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.
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:
Side note: I feel it'd be cleaner if each of the situations tested inside |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good!
lightning/src/ln/channel.rs
Outdated
@@ -386,6 +386,8 @@ pub(super) struct Channel<ChanSigner: ChannelKeys> { | |||
commitment_secrets: CounterpartyCommitmentSecrets, | |||
|
|||
network_sync: UpdateStatus, | |||
|
|||
supports_shutdown_anysegwit: bool, |
There was a problem hiding this comment.
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.
lightning/src/ln/channel.rs
Outdated
@@ -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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 😅
lightning/src/ln/channel.rs
Outdated
@@ -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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this 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?
Please update the issue/PR when you find out :)
Yeah, I'll probably create 2 separate tests, one for each different scenario in the next PR revision. |
I've pushed some updates to address the latest round of feedback. The include:
|
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. |
CI failure was due to my changes. Updating C bindings... |
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). |
lightning/src/ln/msgs.rs
Outdated
@@ -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); |
There was a problem hiding this comment.
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.
lightning/src/ln/channel.rs
Outdated
@@ -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) { |
There was a problem hiding this comment.
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.
lightning/src/ln/channel.rs
Outdated
// 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 { |
There was a problem hiding this comment.
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.
Updated the PR with 3 commits to handle the latest feedback. |
Added a new commit to tidy up things. |
There was a problem hiding this 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"
lightning/src/ln/channel.rs
Outdated
// 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()) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
lightning/src/ln/channel.rs
Outdated
// 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; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :\
There was a problem hiding this comment.
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.
lightning/src/ln/channel.rs
Outdated
@@ -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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
lightning/src/ln/channel.rs
Outdated
// 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()) |
There was a problem hiding this comment.
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.
lightning/src/ln/channel.rs
Outdated
// 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(); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
lightning/src/ln/features.rs
Outdated
@@ -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], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
a286007
to
479070d
Compare
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:
|
lightning/src/ln/channel.rs
Outdated
|
||
// Providing an empty is supported | ||
let script_lenght = script.len(); | ||
if script_lenght == 0 { |
There was a problem hiding this comment.
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.
lightning/src/ln/channel.rs
Outdated
// 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() |
There was a problem hiding this comment.
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.
lightning/src/ln/functional_tests.rs
Outdated
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 |
There was a problem hiding this comment.
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.
Addressed latest round of feedback:
|
There was a problem hiding this 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.
Code Review ACK 6746a16 modulo squash! |
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.
6746a16
to
24ed1dc
Compare
Commits squashed 😊 |
This is a draft PR for #780.
So far it includes:
The following is a checklist of doubts / todos I'd like to resolve:
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 inchannel.rs
or a functional test infunctional_tests.rs
?scriptpubkey
supports a witness program? I've added checks for it in the PR but I'd like to make sure they work.new_from_req
andaccept_channel
that affects this PR. Should that be refactored? Wouldchannel.rs
be the best place for it?KeysInterface.get_shutdown_pubkey
API (as suggested by Matt on here.p.s. This is fun :)