option_simple_close: Add feature, messaging types, shutdown script construction
#3861
Conversation
|
👋 Thanks for assigning @tankyleo as a reviewer! |
tnull
force-pushed
the
2025-06-simple-close-1
branch
2 times, most recently
from
5b737bd to
dfcd328
Compare
tnull
requested review from
wpaulino and
tankyleo
and removed request for
tankyleo
| // | ||
| // - The first byte of the OP_RETURN data (interpreted as u8 int) is not equal to the | ||
| // remaining number of bytes (i.e., `[5; 6]` would succeed here). | ||
| let op_return = ScriptBuf::new_op_return(&[5; 5]); |
There was a problem hiding this comment.
Could also add one for the OP_PUSHDATA1 case below 80 bytes
There was a problem hiding this comment.
Now added test coverage for OP_PUSHDATA1 scripts with 6..=75 bytes. While the spec says we should use OP_PUSHBYTES for these, I think we should be lenient when we parse them, i.e., should still accept them if our counterparty messes up.
There was a problem hiding this comment.
But let me know if you think we should enforce minimal pushes, which would be a one-line change.
There was a problem hiding this comment.
It does save a byte, but the spec doesn't seem to have a minimal push requirement, just that it's a single push.
| // `Instruction::PushBytes`, having us land here in this case, too. | ||
| // | ||
| // * `76` followed by `76` to `80` followed by exactly that many bytes | ||
| if (76..=80).contains(&bytes.len()) { |
There was a problem hiding this comment.
The spec does not mention this range is inclusive unlike the one above
There was a problem hiding this comment.
Hmm, '76 to 80' bytes reads as inclusive to me, no? The spec also mentions:
`OP_RETURN` is only standard if followed by PUSH opcodes, and the total script
is 83 bytes or less. We are slightly stricter, to only allow a single PUSH, but
there are two forms in script: one which pushes up to 75 bytes, and a longer
one (`OP_PUSHDATA1`) which is needed for 76-80 bytes.
Making it pretty clear that OP_PUSHDATA1 will be used for any script length of 76 to (including) 80 bytes?
There was a problem hiding this comment.
It's clear enough, I was referring to this though:
OP_RETURNfollowed by one of:
6to75inclusive followed by exactly that many bytes76followed by76to80followed by exactly that many bytes
Just seemed weird to include "inclusive" in one but not the other if both are intended to be.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
lightning/src/ln/script.rs
Outdated
| /// Note this only supports creating a script with data of up to 76 bytes length via | ||
| /// [`PushBytes`]. |
There was a problem hiding this comment.
Hmm this passes though. From the PushBytes docs: "The encoding of Bitcoin script restricts data pushes to be less than 2^32 bytes long. This type represents slices that are guaranteed to be within the limit so they can be put in the script safely."
use bitcoin::script::PushBytes;
use bitcoin::script::ScriptBuf;
fn main() {
let big_buffer = &[0xfau8; 1000][..];
let push_bytes: &PushBytes = big_buffer.try_into().unwrap();
let script_buf = ScriptBuf::new_op_return(push_bytes);
assert_eq!(script_buf.len(), 1004);
}
There was a problem hiding this comment.
Hmm, that's indeed true.
For 6 up to 75 bytes there are specific OP_PUSHBYTES_X variants, and rust-bitcoin only implements corresponding code to convert of static-sized arrays only for these lengths (https://github.com/rust-bitcoin/rust-bitcoin/blob/f034367bbcdca92b1dee4f86dcd23fcd7f90a880/bitcoin/src/blockdata/script/push_bytes.rs#L137-L189) but indeed it seems you can construct them as you sketched above, not sure how I overlooked this.
Now added a fixup that will check BOLT2-compat in new_op_return, and added a test case to make sure the above example fails to parse.
tnull
force-pushed
the
2025-06-simple-close-1
branch
2 times, most recently
from
892b169 to
16d6f83
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
tnull
force-pushed
the
2025-06-simple-close-1
branch
3 times, most recently
from
d66ec30 to
3c32e33
Compare
|
Good to squash |
We define the feature bits that will be set in `Init` and `Node` contexts.
.. although behind a `cfg` gate for now
We add a new `ShutdownScript::new_op_return` constructor and extend the `is_bolt_compliant` check to enforce the new rules introduced by `option_simple_close`.
tnull
force-pushed
the
2025-06-simple-close-1
branch
from
3c32e33 to
76971d8
Compare
Squashed without further changes. |
This is the initial PR in a series aiming to add support for the new mutual close protocol
option_simple_close(lightning/bolts#1205).In this PR we add the feature bits, message types (including serialization and fuzz targets), as well as support to construct
ShutdownScripts from anOP_RETURNpayload as per spec.We also include some commits dropping
rustfmt::skips from methods related to channel closing, as we're going to touch them going forward.Reviewers can find an outline of further planned steps/PRs over at #3862