Skip to content

fix and make scripts smaller#1

Merged
Roasbeef merged 1 commit intoRoasbeef:simple-taproot-chansfrom
antonilol:taproot-channel-fixes
Jun 30, 2022
Merged

fix and make scripts smaller#1
Roasbeef merged 1 commit intoRoasbeef:simple-taproot-chansfrom
antonilol:taproot-channel-fixes

Conversation

@antonilol
Copy link
Copy Markdown

@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
Comment thread bolt-simple-taproot.md Outdated
@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
Copy Markdown
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
Copy Markdown
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?).

Comment thread 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
Copy Markdown
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 ;)

Comment thread bolt-simple-taproot.md
<local_delayedpubkey>
OP_CHECKSIGVERIFY
<local_delayedpubkey> OP_CHECKSIG
<to_self_delay> OP_CHECKSEQUENCEVERIFY OP_DROP
Copy link
Copy Markdown
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.

👍

Comment thread 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
Copy Markdown
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).

Comment thread bolt-simple-taproot.md
```
<remotepubkey> OP_CHECKSIGVERIFY 1 OP_CHECKSEQUENCEVERIFY
<remotepubkey> OP_CHECKSIG
OP_CHECKSEQUENCEVERIFY
Copy link
Copy Markdown
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
Copy Markdown
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)

Comment thread bolt-simple-taproot.md
(output_key_y_parity | 0xc0) || anchor_internal_key
```

`nSequence` needs to be set to 16.
Copy link
Copy Markdown
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 👍

Comment thread 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
Copy Markdown
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.

Comment thread bolt-simple-taproot.md
<remote_htlcpubkey> OP_CHECKSIG
```

Note that there is no `OP_CHECKSEQUENCEVERIFY` in the offered HTLC's timeout path
Copy link
Copy Markdown
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
Copy Markdown
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