Skip to content

fix and make scripts smaller #1

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 30, 2022
Merged

fix and make scripts smaller #1

merged 1 commit into from
Jun 30, 2022

Conversation

@antonilol
Copy link

@antonilol antonilol commented Jun 1, 2022

lightning#995

fixed some tapscripts, made some smaller and fixed witness stack order (bip 341)

@antonilol antonilol force-pushed the taproot-channel-fixes branch from 06328cb to 43f45e8 Compare June 1, 2022 07:33
@antonilol antonilol marked this pull request as ready for review June 1, 2022 07:33
antonilol
antonilol commented Jun 1, 2022
View reviewed changes
@antonilol antonilol mentioned this pull request Jun 1, 2022
Open
@antonilol antonilol force-pushed the taproot-channel-fixes branch 4 times, most recently from 577ab05 to fe5a289 Compare June 1, 2022 15:03
@antonilol antonilol force-pushed the taproot-channel-fixes branch from fe5a289 to c696608 Compare June 13, 2022 12:06
@antonilol
bolt-simple-taproot: fix and make scripts smaller
dba17e7 
+ script layout and readability
@antonilol antonilol force-pushed the taproot-channel-fixes branch from c696608 to dba17e7 Compare June 13, 2022 12:17
@Roasbeef
Copy link
Owner

Roasbeef commented Jun 30, 2022 •
edited
Loading

It's unspendable based on the context. With the new script change from @antonilol, the 0 doesn't get dropped as it previously would, so execution terminates with it and fails.

I wrote a simple program to confirm this: https://go.dev/play/p/ZOo7zkr0Dew. The output is:

execution failed:  false stack entry at end of script execution
stepping 01:0000: OP_0
Stack: [[]]AltStack: []stepping 01:0001: OP_DATA_32 0x3cb15966ae87e58da30fe11b2399b2ac528a58bca879035a07df65b901d76944
Stack: [[]]AltStack: []stepping 02:0000: OP_DATA_33 0x0327ab03537d1865881beec62124af1826af5c66026be1870df6c479f71dc5f832
Stack: [[] [3 39 171 3 83 125 24 101 136 27 238 198 33 36 175 24 38 175 92 102 2 107 225 135 13 246 196 121 247 29 197 248 50]]AltStack: []stepping 02:0001: OP_CHECKSIG
Stack: [[]]AltStack: []stepping 02:0002: OP_CHECKSEQUENCEVERIFY
Stack: [[]]AltStack: []
we not gucci

As expected, things fail due to the clean stack consensus rule. It makes things a bit harder to understand at a glance, but I guess saving that extra byte is worth it (?).

Roasbeef
Roasbeef approved these changes Jun 30, 2022
View reviewed changes
Copy link
Owner

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Solid PR, thanks for all the heavy lifting!

I wrote a small program to confirm the new (sorta hacky/clever) sequence usage and things check out. I think we might still want to run things thru mini script to further optimize (tho maybe that's what you did in the first place?).

bolt-simple-taproot.md
The final witness to spend a script path output is:
```
control_block || leaf_script || witness
<witness1> ... <witnessN> <leaf_script> <control_block>
Copy link
Owner

@Roasbeef Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

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

I guess we visualize the stack differently ;)

bolt-simple-taproot.md
<local_delayedpubkey>
OP_CHECKSIGVERIFY
<local_delayedpubkey> OP_CHECKSIG
<to_self_delay> OP_CHECKSEQUENCEVERIFY OP_DROP
Copy link
Owner

@Roasbeef Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

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

👍

bolt-simple-taproot.md
* where:
* `taproot_nums_point = 0245b18183a06ee58228f07d9716f0f121cd194e4d924b037522503a7160432f15`
* `to_remote_output_key = taproot_nums_point + tagged_hash("TapTweak", taproot_nums_point|| to_remote_script_root`
* `to_remote_output_key = taproot_nums_point + tagged_hash("TapTweak", taproot_nums_point || to_remote_script_root)`
Copy link
Owner

@Roasbeef Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

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

FWIW, the NUMs point here will soon be removed (will just be the multi-sig script).

bolt-simple-taproot.md
```
<remotepubkey> OP_CHECKSIGVERIFY 1 OP_CHECKSEQUENCEVERIFY
<remotepubkey> OP_CHECKSIG
OP_CHECKSEQUENCEVERIFY
Copy link
Owner

@Roasbeef Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

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

Clever...but will need to double check the semantics here to ensure this doesn't become an any-one-can-spend...

Copy link
Owner

@Roasbeef Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

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

Confirmed that it all checks out: #1 (comment)

bolt-simple-taproot.md
(output_key_y_parity | 0xc0) || anchor_internal_key
```
`nSequence` needs to be set to 16.
Copy link
Owner

@Roasbeef Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

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

Nice addition 👍

bolt-simple-taproot.md
<local_htlcpubkey> OP_CHECKSIGADD <remote_htlcpubkey> OP_CHECKSIGADD 2 OP_EQUALVERIFY
1 OP_CHECKSEQUENCEVERIFY
<local_htlcpubkey> OP_CHECKSIGVERIFY
<remote_htlcpubkey> OP_CHECKSIG
Copy link
Owner

@Roasbeef Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

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

This isn't correct, the sequence is still need here as we want to make sure all spends need to wait a block.

bolt-simple-taproot.md
<remote_htlcpubkey> OP_CHECKSIG
```
Note that there is no `OP_CHECKSEQUENCEVERIFY` in the offered HTLC's timeout path
Copy link
Owner

@Roasbeef Roasbeef Jun 30, 2022

Choose a reason for hiding this comment

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

Thanks for this note, makes sense, and isn't too big of a departure w.r.t how things work today.

@Roasbeef Roasbeef merged commit bf65824 into Roasbeef:simple-taproot-chans Jun 30, 2022
@antonilol
Copy link
Author

antonilol commented Jun 30, 2022

Solid PR, thanks for all the heavy lifting!

no problem :)

I wrote a small program to confirm the new (sorta hacky/clever) sequence usage and things check out. I think we might still want to run things thru mini script to further optimize (tho maybe that's what you did in the first place?).

havent used miniscript
it is actually really simple if you keep in mind that OP_CSV used to be a nop, so it does the same as just <pubkey> OP_CHECKSIG, plus when checksig gives a 1, sequence must be 1

@antonilol antonilol deleted the taproot-channel-fixes branch June 30, 2022 09:27
Roasbeef pushed a commit that referenced this pull request Mar 28, 2024
@jrakibi @rustyrussell
Update Signet port to correct hex value in BOLT #1 spec
ec525cc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Roasbeef Roasbeef Roasbeef approved these changes

+2 more reviewers

@wpaulino wpaulino wpaulino left review comments

@Crypt-iQ Crypt-iQ Crypt-iQ left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@antonilol @Roasbeef @wpaulino @Crypt-iQ