Merkleize / bitcoin

Bitcoin Core integration/staging tree
https://bitcoincore.org/en/download
MIT License
4 stars 0 forks source link

Need to check pubkey length for CHECKCONTRACTVERIFY #3

Closed dgpv closed 11 months ago

dgpv commented 11 months ago

https://github.com/Merkleize/bitcoin/commit/5a993dcc75082d52001861ca2a811497096110f3#r128986858

It seems that current implementation of CHECKCONTRACTVERIFY does not check if the pubkey is of length 32 when it is not empty and use_current_pubkey is false. At least I did not find such checks. I think it should have the same checks as taptree.

bigspider commented 11 months ago

Fixed in the new version, by leaving by making all undefined values behave like OP_SUCCESS, in order to keep it possible to add new behaviors with sofr-forks.

The only caveat is that if people use a pubkey passed from the witness stack without checking that the length is 32, they would inadvertently turn the script into an ANYONECANSPEND; probably not a comon issue in practice, as the pubkey is usually hardcoded in the Script; keeping the soft-forkability of future pubkey lengths/values open seems better to me.

Thanks, @dgpv!

dgpv commented 11 months ago

I'd like to add a to note that currently, no enabled opcode calls set_success() directly. In case any opcode in the script is not defined, EvalScript is not called, and return set_success(serror) is done instead. There is behavior like 'upgradable pubkeys' where signature check is deemed successful if the pubkey is not of length 32 in tapscript, but this is not the same.

The behavior of CHECKCONTRACTVERIFY where it calls set_success() for particular arguments is "novel", not found in the script interpreter before. This might add concerns in the discussion about if this opcode should be included in bitcoin. I myself don't have enough knowledge to say if such behavior of calling set_success() from within the opcode implementation might have problems or not, though. (what I know is that bsst would need to add code to handle this behavior, https://github.com/dgpv/bsst/issues/10).

bigspider commented 11 months ago

It seems to me that is the only reasonable way to keep opcodes upgradeable, but indeed some concerns were raised in the discussion of the BIP proposal for TXHASH on a similar approach.

At this time, the code didn't receive enough expert reviews (and I'm definitely not an expert!), so it's only best-effort and for demonstrative purposes.