Skip to content
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 BIP 349: OP_INTERNALKEY #1534

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

reardencode
Copy link

OP_INTERNALKEY is a new BIP342 tapscript only opcode (upgraded using OP_SUCCESS semantics) that takes bytes 1-32 (0-index, inclusive) from the BIP341 taproot control block and places them on the stack. This BIP describes that behavior.


When building taproot outputs, especially those secured by an aggregate key
representing more than 1 signer; the parties may wish to collaborate on signing
with the taproot internal key, but only with additional script restrictions. In
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you give an example where using the key path with an aggregate key and having every participant enforce the additional restrictions themselves (i.e. only signing if they're met) wouldn't suffice?

Copy link
Author

Choose a reason for hiding this comment

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

The most obvious case would be lightning symmetry where the parties want to pre-sign with CTV+CSFS to create a rebindable update transaction.

bip-internalkey.mediawiki Outdated Show resolved Hide resolved
@reardencode reardencode force-pushed the internalkey branch 2 times, most recently from c9842c4 to 729276c Compare April 24, 2024 21:34
@reardencode
Copy link
Author

Updated to match the BIN.

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

Has this proposal been discussed on the mailing list?

bip-internalkey.md Outdated Show resolved Hide resolved
@murchandamus murchandamus added the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label May 8, 2024
[BIP 342]), `OP_INTERNALKEY` replaces `OP_SUCCESS203` (0xcb). `OP_INTERNALKEY`
pushes the taproot internal key, as defined in [BIP 341], to the stack.

## Motivation
Copy link

Choose a reason for hiding this comment

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

Additional useful feature: ability to inspect the tweak to a key (in combination with some OP_TWEAKVERIFY).

Copy link
Author

Choose a reason for hiding this comment

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

That would require additional introspection beyond just INTERNALKEY too, wouldn't it?

Copy link

Choose a reason for hiding this comment

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

Yes, something like TWEAKADD or TWEAKVERIFY.


When verifying taproot script spends having leaf version `0xc0` (as defined in
[BIP 342]), `OP_INTERNALKEY` replaces `OP_SUCCESS203` (0xcb). `OP_INTERNALKEY`
pushes the taproot internal key, as defined in [BIP 341], to the stack.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be nice to mark "taproot internal key" as a formal term, either by italicizing, backticks or the like. I find that marking terms that invoke a suite of definitions and properties is really helpful. We do it a lot in the BOLTs and I'd recommend doing it in the BIPs as much as we can.

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

This seems close to ready for a merge, although I am wondering whether it has gotten enough review from the community. I noticed that both this PR and the mailing list thread were not that active.


## Specification

When verifying taproot script spends having leaf version `0xc0` (as defined in
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When verifying taproot script spends having leaf version `0xc0` (as defined in
When verifying taproot script path spends having leaf version `0xc0` (as defined in

When building taproot outputs, especially those secured by an aggregate key
representing more than one signer, the parties may wish to collaborate on
signing with the taproot internal key, but only with additional script
restrictions. In this case, `OP_INTERNALKEY` saves 8 vBytes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you could elaborate how OP_INTERNALKEY would save 8 vB, for example in a footnote. (I assume it’s an x-only key of 32 bytes, and otherwise you’d need a PUSH and the 32 bytes, but with OP_INTERNALKEY you only need only that one byte instead. But if I had to think about that, maybe others would also appreciate an explanation.


### Mitigated control block overhead for scripts using hash locks

In cases where script path spending is not desired, the internal key may be set
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean:

Suggested change
In cases where script path spending is not desired, the internal key may be set
In cases where key path spending is not desired, the internal key may be set


When verifying taproot script spends having leaf version `0xc0` (as defined in
[BIP 342]), `OP_INTERNALKEY` replaces `OP_SUCCESS203` (0xcb). `OP_INTERNALKEY`
pushes the taproot internal key, as defined in [BIP 341], to the stack.
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that this should be perhaps a bit more specific what exactly is being pushed to the stack. I assume this is referring to

Let p = c[1:33] and let P = lift_x(int(p)) where lift_x and [:] are defined as in BIP340. Fail if this point is not on the curve. […] q is referred to as taproot output key and p as taproot internal key.

but it would not hurt to clarify.

Choose a reason for hiding this comment

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

"the taproot internal key denoted as P in [BIP 341]"?

Copy link
Contributor

@murchandamus murchandamus Nov 14, 2024

Choose a reason for hiding this comment

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

My point is that the "taproot internal key" refers to an abstract object in BIP 341 that is denoted as p:

"q is referred to as taproot output key and p as taproot internal key."

and it wouldn’t hurt to simply describe what you are referring to, instead of sending people to parse another BIP to get clued in.

How about:

"pushes the 32-byte x-only representation P of the taproot internal key p, as defined in [BIP 341], to the stack."

Choose a reason for hiding this comment

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

How about?

When verifying taproot script path spends having leaf version 0xc0 (as defined in [BIP 342]), OP_INTERNALKEY replaces OP_SUCCESS203 (0xcb). OP_INTERNALKEY pushes the 32-byte x-only representation (referred to as P) of the taproot internal key (referred to as p), as defined in [BIP 341], to the stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that BIP341 already defines the taproot internal key p (note the lowercase) as a 32-byte array (as opposed to, say, a point on the secp256k1 curve), so it's not necessary to reinterpret it in any way before pushing it to the stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

If others feel that this passage is sufficiently clear, I don’t feel strongly about it.

Choose a reason for hiding this comment

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

So then:

When verifying taproot script path spends having leaf version 0xc0 (as defined in [BIP 342]), OP_INTERNALKEY replaces OP_SUCCESS203 (0xcb). OP_INTERNALKEY pushes the 32-byte taproot internal key (referred to as p), as defined in [BIP 341], to the stack.

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we can add the "32-byte" qualifier for additional clarity. I would use slightly different phrasing, but that's just a suggestion:

When verifying taproot script path spends having leaf version 0xc0 (as defined in [BIP 342]), OP_INTERNALKEY replaces OP_SUCCESS203 (0xcb). OP_INTERNALKEY pushes the 32-byte taproot internal key p, as defined in [BIP 341], to the stack.

Choose a reason for hiding this comment

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

I find it weird that both taproot internal key and p are in italic and together like that. No hard preference just feels off somehow!

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that, but it's not unprecedented, for example in BIP341 you'll find "annex a". Also notice that they're in separate italic blocks, even though you can't tell from the rendered version.

@moonsettler
Copy link

So what exactly needs to be done to get this BIP unstuck?

@murchandamus
Copy link
Contributor

There do seem to be several unaddressed review comments. Happy to review again when the open review comments have been addressed.

@murchandamus
Copy link
Contributor

Let’s call this BIP 349. Could you please rename the file, add the number to the preamble, and update the README table accordingly?

@murchandamus murchandamus changed the title Add bip-internalkey Add BIP 349: OP_INTERNALKEY Nov 14, 2024
Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

CI isn’t passing, I think the problem are two little issues with the Preamble.

bip-0349.md Outdated
Comment on lines 1 to 11
```
BIP: 349
Layer: Consensus (soft fork)
Title: OP_INTERNALKEY
Author: Brandon Black <[email protected]>, Jeremy Rubin <[email protected]>
Comments-URI: https://github.com/bitcoin/bips/wiki/Comments:BIP-0349
Status: Draft
Type: Standards Track
Created: 2023-12-22
License: BSD-3-Clause
```
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these changes should make the CI pass:

  • use <pre> and </pre> instead of ```
  • put each author on a separate line

I also suggest updating the date in the Created header to the day that the proposal was assigned a number as that’s the meaning of the field according to BIP 2.

Suggested change
```
BIP: 349
Layer: Consensus (soft fork)
Title: OP_INTERNALKEY
Author: Brandon Black <[email protected]>, Jeremy Rubin <[email protected]>
Comments-URI: https://github.com/bitcoin/bips/wiki/Comments:BIP-0349
Status: Draft
Type: Standards Track
Created: 2023-12-22
License: BSD-3-Clause
```
<pre>
BIP: 349
Layer: Consensus (soft fork)
Title: OP_INTERNALKEY
Author: Brandon Black <[email protected]>
Jeremy Rubin <[email protected]>
Comments-URI: https://github.com/bitcoin/bips/wiki/Comments:BIP-0349
Status: Draft
Type: Standards Track
Created: 2024-11-14
License: BSD-3-Clause
</pre>

@murchandamus murchandamus removed the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label Nov 14, 2024
@reardencode
Copy link
Author

Thanks @murchandamus !

@murchandamus
Copy link
Contributor

I updated the Preamble formatting:

  • Move authors to separate lines
  • Use <pre> formatting
  • Correct the background color in README table
  • Updated the "Created" header to use the date of the number assignment as prescribed by BIP 2.

This should now be good to go unless you wanted to make more changes before it is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants