LNHANCE inquisition (CSFS, INTERNALKEY) #45
Conversation
reardencode
force-pushed
the
lnhance-inquisition
branch
2 times, most recently
from
f4a2f3f
to
bde4edd
Compare
IIUC it would have to be:
Because functional tests won't be possible until regtest deployment. This makes sense on this PR, but less so on my core one (from whence this was ported) because there it would have involved splitting the CTV functional testing commits from the rest of the CTV changes. For CSFS and IKEY I only did cpp unit tests, which may or may not be sufficient (especially here where they're split from CTV's deployment testing). |
reardencode
force-pushed
the
lnhance-inquisition
branch
from
bde4edd
to
c6d78c8
Compare
|
Addressed all of your comments except fully splitting out the deployment. Happy to do that too if you think it makes sense with the other changes in place. Thanks much for the review! |
reardencode
force-pushed
the
lnhance-inquisition
branch
from
c6d78c8
to
d1c9e9a
Compare
|
Updated for BINANA and to remove ECDSA verification from CHECKSIGFROMSTACKVERIFY on legacy/witnessv0 (BIN-2024-0003 rev1). |
| .revision = 1, | ||
| .start = 1705046400, // 2024-01-12 | ||
| .timeout = 2020665600, // 2034-01-12 | ||
| }; |
There was a problem hiding this comment.
I was under the impression that LNHANCE is a proposal to do CTV+CSFS+IKEY as a unit. CTV is already on inquisition, but if we want to stay in the spirit of the proposal, might it make more sense to have CSFS and IKEY be deployed as a single unit?
There was a problem hiding this comment.
Yeah, IDK about what @ajtowns wants to do here for inquisition purposes. For core I'd probably want to do something with fewer activations and fewer flags.
| { | ||
| assert(sigbytes.size() == 64); | ||
| secp256k1_xonly_pubkey pubkey; | ||
| if (!secp256k1_xonly_pubkey_parse(secp256k1_context_static, &pubkey, m_keydata.data())) return false; | ||
| return secp256k1_schnorrsig_verify(secp256k1_context_static, sigbytes.data(), msg.begin(), 32, &pubkey); | ||
| return secp256k1_schnorrsig_verify(secp256k1_context_static, sigbytes.data(), msg.data(), msg.size(), &pubkey); |
There was a problem hiding this comment.
To allow data lengths other than 32. BIP340 explicitly allows it, but Taproot sighashes are all exactly 32-bytes so it wasn't needed previously.
|
Did you want to continue with this on 25.x or skip straight to 27.x? |
|
@reardencode could you rebase this? I would love to test CSFS in combination with CAT |
reardencode
force-pushed
the
lnhance-inquisition
branch
from
d1c9e9a
to
455c56b
Compare
|
Rebased on 27.x. |
Testing is minimal, but so is the code.
Activates OP_CHECKSIGFROMSTACK, OP_CHECKSIGFROMSTACKVERIFY, and OP_INTERNALKEY on regtest and signet. always active on regtest active from 2024-01-12 until 2034-01-12 on signet never active on testnet, mainnet
reardencode
force-pushed
the
lnhance-inquisition
branch
from
455c56b
to
2938646
Compare
| @@ -953,7 +953,7 @@ def taproot_construct(pubkey, scripts=None, *, keyver=None, treat_internal_as_in | |||
| return TaprootInfo(CScript([OP_1, tweaked]), pubkey, negated + 0, tweak, leaves, h, tweaked, keyver) | |||
|
|
|||
| def is_op_success(o): | |||
| # assume OP_CAT is activated in tests | |||
| if o == OP_CAT: | |||
There was a problem hiding this comment.
test/functional/test_framework/script.py:957:22: F821 undefined name 'OP_INTERNALKEY'
test/functional/test_framework/script.py:957:38: F821 undefined name 'OP_CHECKSIGFROMSTACK'
This adds CHECKSIGFROMSTACK and INTERNALKEY to bitcoin inquisition for review and testing.
(Related to bitcoin#29198)