-
Notifications
You must be signed in to change notification settings - Fork 7
Implement BIP 349 validation (OP_INTERNALKEY) #86
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
Implement BIP 349 validation (OP_INTERNALKEY) #86
Conversation
f7e77ab to
24fb948
Compare
|
|
||
| case OP_INTERNALKEY: { | ||
| // OP_INTERNALKEY is only available in Tapscript | ||
| if (sigversion == SigVersion::BASE || sigversion == SigVersion::WITNESS_V0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert(sigversion != BASE && sigversion != WITNESS_V0) ?
assert(flags & SCRIPT_VERIFY_INTERNALKEY) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert(sigversion != BASE && sigversion != WITNESS_V0)
I don't believe this is correct; added test that fails with this change.
assert(flags & SCRIPT_VERIFY_INTERNALKEY)
This ends up being true post-activation, but I don't think would be prior?
We could add OP_INTERNALKEY to the disabled opcode area ala OP_CAT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe this is correct; added test that fails with this change.
Right; disabled opcodes are checked beforehand so can have an assert, but everything needs to duplicate the set_error from the default: section. 👍
assert(flags & SCRIPT_VERIFY_INTERNALKEY)
This ends up being true post-activation, but I don't think would be prior?
Prior to activation, in the taproot case, it should be treated as OP_SUCCESS and this code shouldn't be reachable (CheckTapscriptOpSuccess will hit op_success_check via INQ_SUCCESS_OPCODES and either flags & discourage will force an error, or !(flags & enable) will force a success).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right, if I leave in the script context gating, it will be fine!
| return set_error(serror, SCRIPT_ERR_BAD_OPCODE); | ||
| } | ||
| // Always present in Tapscript | ||
| assert(execdata.m_internal_key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert(sigversion == TAPSCRIPT) in that case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
|
|
||
| def is_op_success(o): | ||
| if o in [OP_INTERNALKEY]: | ||
| return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess binana.json should be generating these too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue, #93. Wasn't obvious to me how to sensibly make these functions depend on info in ../data/binana.json (or if that should get changed to binana.py like invalid_txs.py, or something else).
ed3c785 to
ab73a28
Compare
|
note to self to add pre-activation tests ala 76f9913 once other PR has settled |
ab73a28 to
df38a9f
Compare
|
Looks like we're running out of script flags for this PR. edit: will review bitcoin#32998 |
df38a9f to
4287a3a
Compare
|
rebased on 64 bit flags work, added pre-activation test for IK |
benthecarman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack 4287a3a
|
|
||
| def spenders_internalkey_active(): | ||
|
|
||
| secs = [generate_privkey() for _ in range(8)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit you're generating 8 keys here but only using [0] and [1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will touch if I do anything else
No description provided.