Implement BIP 349 validation (OP_INTERNALKEY)#86
Implement BIP 349 validation (OP_INTERNALKEY)#86ajtowns merged 3 commits intobitcoin-inquisition:29.xfrom
Conversation
f7e77ab to
24fb948
Compare
src/script/interpreter.cpp
Outdated
There was a problem hiding this comment.
assert(sigversion != BASE && sigversion != WITNESS_V0) ?
assert(flags & SCRIPT_VERIFY_INTERNALKEY) ?
There was a problem hiding this comment.
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.
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.
ah right, if I leave in the script context gating, it will be fine!
src/script/interpreter.cpp
Outdated
There was a problem hiding this comment.
assert(sigversion == TAPSCRIPT) in that case?
There was a problem hiding this comment.
I guess binana.json should be generating these too.
There was a problem hiding this comment.
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 |
|
|
||
| def spenders_internalkey_active(): | ||
|
|
||
| secs = [generate_privkey() for _ in range(8)] |
There was a problem hiding this comment.
nit you're generating 8 keys here but only using [0] and [1]
There was a problem hiding this comment.
will touch if I do anything else
c40dbbb test: Move `script_assets_tests` into its own suite (Hennadii Stepanov) Pull request description: This PR ensures that the `script_assets_tests` test case is explicitly reported as "Skipped" when it is not run, making it clearer when running the test suite with `ctest`: - on the master branch @ 9355578: ``` $ env -u DIR_UNIT_TEST_DATA ctest --test-dir build -j 16 -R "^script_" Internal ctest changing into directory: /home/hebasto/git/bitcoin/build Test project /home/hebasto/git/bitcoin/build Start 87: script_tests Start 83: script_p2sh_tests Start 85: script_segwit_tests Start 86: script_standard_tests Start 84: script_parse_tests 1/5 Test bitcoin-inquisition#84: script_parse_tests ............... Passed 0.11 sec 2/5 Test bitcoin-inquisition#86: script_standard_tests ............ Passed 0.11 sec 3/5 Test bitcoin-inquisition#85: script_segwit_tests .............. Passed 0.12 sec 4/5 Test bitcoin-inquisition#83: script_p2sh_tests ................ Passed 0.12 sec 5/5 Test bitcoin-inquisition#87: script_tests ..................... Passed 0.36 sec 100% tests passed, 0 tests failed out of 5 Total Test time (real) = 0.37 sec ``` - with this PR: ``` $ env -u DIR_UNIT_TEST_DATA ctest --test-dir build -j 16 -R "^script_" Internal ctest changing into directory: /home/hebasto/git/bitcoin/build Test project /home/hebasto/git/bitcoin/build Start 83: script_assets_tests Start 88: script_tests Start 84: script_p2sh_tests Start 86: script_segwit_tests Start 87: script_standard_tests Start 85: script_parse_tests 1/6 Test bitcoin-inquisition#85: script_parse_tests ............... Passed 0.11 sec 2/6 Test bitcoin-inquisition#83: script_assets_tests ..............***Skipped 0.12 sec 3/6 Test bitcoin-inquisition#86: script_segwit_tests .............. Passed 0.11 sec 4/6 Test bitcoin-inquisition#87: script_standard_tests ............ Passed 0.11 sec 5/6 Test bitcoin-inquisition#84: script_p2sh_tests ................ Passed 0.12 sec 6/6 Test bitcoin-inquisition#88: script_tests ..................... Passed 0.36 sec 100% tests passed, 0 tests failed out of 6 Total Test time (real) = 0.37 sec The following tests did not run: 83 - script_assets_tests (Skipped) $ env DIR_UNIT_TEST_DATA=/home/hebasto/git/bitcoin/qa-assets/unit_test_data ctest --test-dir build -j 16 -R "^script_" Internal ctest changing into directory: /home/hebasto/git/bitcoin/build Test project /home/hebasto/git/bitcoin/build Start 83: script_assets_tests Start 88: script_tests Start 84: script_p2sh_tests Start 86: script_segwit_tests Start 87: script_standard_tests Start 85: script_parse_tests 1/6 Test bitcoin-inquisition#85: script_parse_tests ............... Passed 0.11 sec 2/6 Test bitcoin-inquisition#87: script_standard_tests ............ Passed 0.11 sec 3/6 Test bitcoin-inquisition#86: script_segwit_tests .............. Passed 0.11 sec 4/6 Test bitcoin-inquisition#84: script_p2sh_tests ................ Passed 0.12 sec 5/6 Test bitcoin-inquisition#88: script_tests ..................... Passed 0.35 sec 6/6 Test bitcoin-inquisition#83: script_assets_tests .............. Passed 1.58 sec 100% tests passed, 0 tests failed out of 6 Total Test time (real) = 1.58 sec ``` ACKs for top commit: maflcko: re-ACK c40dbbb 👈 ajtowns: ACK c40dbbb achow101: ACK c40dbbb Tree-SHA512: 25713e1c3b507b6f2a5fecc7b1ea285a6642b906c248769238a58fc0df48489ac5f7606778f9e3653b407b7f1d06563e1554d04321303b350c80eb888500cc5d
No description provided.