smite: add is_standard_shutdown_script helper - #186
Conversation
f33051b to
22217d9
Compare
NishantBansal2003
left a comment
NishantBansal2003
left a comment
There was a problem hiding this comment.
Thanks! I was about to add this as a follow-up to #185, but it looks like I don’t have to now
| /// Feature bits that widen the set of standard `shutdown` scriptpubkeys. | ||
| #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] | ||
| pub struct ShutdownScriptFeatures { | ||
| /// Additionally permits witness program versions 1..=16 with a 2..=40 byte | ||
| /// program. | ||
| pub option_shutdown_anysegwit: bool, | ||
| /// Additionally permits a single-push `OP_RETURN` script. | ||
| pub option_simple_close: bool, | ||
| } |
There was a problem hiding this comment.
I think this can be removed/simplified once #192 gets merged, so we can just use just use: negotiated_features.supports_feature(Features::OPTION_SHUTDOWN_ANYSEGWIT) or negotiated_features.supports_feature(Features::OPTION_SIMPLE_CLOSE)
There was a problem hiding this comment.
Sounds good, I will review #192 and then rebase this PR after merge
22217d9 to
220b1bf
Compare
BOLT-02 specifies sender requirements for shutdown scripts. They must be witness v0 (P2WPKH, P2WSH) or following features must be negotiated: * `option_shutdown_anysegwit`: witness v1-v16 with a 2..=40 byte program * `option_simple_close`: `OP_RETURN` with a single minimal data push of 6..=80 bytes Receivers may accept legacy scripts (P2PKH, P2SH), but we reject them since we're judging the sender's output. This applies to the `shutdown` and `closing_complete` messages, and the `upfront_shutdown_script` TLV in the `open_channel`, `open_channel2`, `accept_channel` and `accept_channel2` messages. This commit adds a helper to catch targets that don't comply with the spec.
220b1bf to
847c2f1
Compare
NishantBansal2003
left a comment
NishantBansal2003
left a comment
There was a problem hiding this comment.
Mostly, it looks good. Just a few small comments. I'm not sure how this follows the merge with #192, but if it's merged before, I'll add the updated feature check there
| /// Returns `true` if `spk` is a standard `shutdown` scriptpubkey per BOLT 2: P2WPKH or P2WSH. | ||
| /// Negotiated `features` widen the accepted set. | ||
| /// | ||
| /// Legacy P2PKH/P2SH are rejected. A receiver may accept them for backward compatibility, but this |
There was a problem hiding this comment.
In the oracle, we verify what the peer accepted (and whether it was valid), and then we check our side to ensure that the target is actually sending that.
So, when verifying a target-accepted open_channel, I need it to pass even if it contains the legacy script. However, for our received accept_channel, I need to ensure that it does not contain legacy scripts
|
|
||
| #[test] | ||
| #[allow(clippy::similar_names)] | ||
| fn is_standard_shutdown_script_rejects_legacy_accepts_witness_v0() { |
There was a problem hiding this comment.
nit: I think for testing semantic script types, we should use the bitcoin crate to create the scripts, eg: ScriptBuf::new_p2wpkh(&WPubkeyHash::all_zeros()).into_bytes();, but for testing non-standard cases, we can construct the raw bytes manually
| } | ||
|
|
||
| #[test] | ||
| fn is_standard_shutdown_script_accepts_anysegwit() { |
There was a problem hiding this comment.
nit: spec allows 2..=40 bytes, so we can test the boundary cases as well
| assert!(!is_standard_shutdown_script(&too_long, ALL)); | ||
|
|
||
| // Non-minimal push: OP_PUSHDATA1 used for less than 76 bytes. | ||
| let mut non_minimal = vec![OP_RETURN.to_u8(), OP_PUSHDATA1.to_u8(), 10]; |
There was a problem hiding this comment.
nit: could test it at the boundary, i.e. 75, to be explicit about the boundary
| // OP_RETURN with a 1-byte push: below the simple_close minimum of 6. | ||
| let op_return_spk = vec![OP_RETURN.to_u8(), OP_PUSHBYTES_1.to_u8(), 0xff]; | ||
| assert!(!is_standard_shutdown_script(&op_return_spk, ALL)); |
There was a problem hiding this comment.
nit: this case is similar to one of the assertions added in is_standard_shutdown_script_rejects_invalid_simple_close_op_return. I think we can follow the same pattern here, remove this OP_RETURN assertion, and rename the test to something like is_standard_shutdown_script_rejects_invalid_segwit or a better name, since this test mostly contains those cases
2 participants
From the commit message:
As per the note I added to the code, I'm not sure if the fuzzer should also reject legacy scripts, since a target must not send them.update: decided to reject them, see discussionI haven't wired this into existing code or #163 yet, but I thought the introduction of the helper might be worthwile to review itself, especially considering the question wrt legacy scripts.