I don't get `feature_taproot.py` failure for this : ``` diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 053df47a2..eb3847250 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1528,7 +1528,7 @@ bool SignatureHashSchnorr(uint256& hash_out, const ScriptExecutionData& execdata // Data about the output(s) if (output_type == SIGHASH_SINGLE) { - if (in_pos >= tx_to.vout.size()) return false; + if (in_pos >= tx_to.vout.size()) return true; CHashWriter sha_single_output(SER_GETHASH, 0); sha_single_output << tx_to.vout[in_pos]; ss << sha_single_output.GetSHA256(); ``` Also I think this is check can be moved above at `hash_type` validation as every predicate is already known and we would save on hashing by failing faster ?

I think melting a consensus check with a policy one in the same conditional is likely source to confusion for future reviewers. I'm quite sure that's already something done elsewhere but maybe we could do better by adding a `GetScriptVersionFlags(SigVersion)` called at the top of `EvalScript`. This new function should set each consensus-compliant interpreter flags per-type of script evaluated thus avoiding setting flags for old scripts types if we would do this in `GetBlockScriptFlags` while easily overloading flag setting for future types.

I'm getting some strange mutation results: ``` --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1497,7 +1497,7 @@ bool SignatureHashSchnorr(uint256& hash_out, const ScriptExecutionData& execdata // Hash type const uint8_t output_type = (hash_type == SIGHASH_DEFAULT) ? SIGHASH_ALL : (hash_type & SIGHASH_OUTPUT_MASK); // Default (no sighash byte) is equivalent to SIGHASH_ALL const uint8_t input_type = hash_type & SIGHASH_INPUT_MASK; - if (!(hash_type <= 0x03 || (hash_type >= 0x80 && hash_type <= 0x83))) return false; + if (!(hash_type <= 0x04 || (hash_type >= 0x80 && hash_type <= 0x84))) return false; ss << hash_type; ``` passes even though it's definitely running the 0x04/0x84 cases. Anything beyond that seems to fail.

Adding the `!is_p2sh` here means that any transaction input spending a P2SH-wrapped segwit v1 output will be valid, but will fail standardness with`SCRIPT_ERR_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM` ("Witness version reserved for soft-fork upgrades") which seems like the wrong error message. We're not expecting P2SH-wrapped segwit v1 outputs to reinterpreted by a future soft-fork, we're simply leaving them unencumbered for safety reasons. I think for correctness we should return a new error type in that case.

I don't get `feature_taproot.py` failure for : ``` diff --git a/src/pubkey.cpp b/src/pubkey.cpp index 111f9c10e..0163f61a2 100644 --- a/src/pubkey.cpp +++ b/src/pubkey.cpp @@ -174,7 +174,7 @@ XOnlyPubKey::XOnlyPubKey(Span<const unsigned char> in) } bool XOnlyPubKey::VerifySchnorr(const uint256& msg, Span<const unsigned char> sigbytes) const { - if (sigbytes.size() != 64) return false; + if (sigbytes.size() != 64) return true; if (msg.size() != 32) return false; secp256k1_xonly_pubkey pubkey; if (!secp256k1_xonly_pubkey_parse(secp256k1_context_verify, & ``` And same for next check on message size.

Again don't get failure in `feature_taproot.py` ? What should be the prefix something like `sighash/keypath_*` ? ``` diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 053df47a2..24ba2ac41 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1662,7 +1662,7 @@ bool GenericTransactionSignatureChecker<T>::CheckSchnorrSignature(Span<const uns hashtype = SpanPopBack(sig); if (hashtype == SIGHASH_DEFAULT) return false; } - if (sig.size() != 64) return false; + if (sig.size() != 64) return true; uint256 sighash; if (!SignatureHashSchnorr(sighash, execdata, *txTo, nIn, hashtype, sigversion, this->txdata)) return false; return VerifySchnorrSignature(sig, pubkey, sighash); ```

Idem, is this part of the code tested non-deterministically ? ``` diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 053df47a2..fcaacc152 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1653,7 +1653,7 @@ template <class T> bool GenericTransactionSignatureChecker<T>::CheckSchnorrSignature(Span<const unsigned char> sig, Span<const unsigned char> pubkey_in, SigVersion sigversion, const ScriptExecutionData& execdata) const { if (sig.empty()) return false; - if (pubkey_in.size() != 32) return false; + if (pubkey_in.size() != 32) return true; XOnlyPubKey pubkey{pubkey_in}; ```

I don't get a `feature_taproot.py` failure for this ? ``` diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 053df47a2..02de07bc5 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1652,7 +1652,7 @@ bool GenericTransactionSignatureChecker<T>::CheckECDSASignature(const std::vecto template <class T> bool GenericTransactionSignatureChecker<T>::CheckSchnorrSignature(Span<const unsigned char> sig, Span<const unsigned char> pubkey_in, SigVersion sigversion, const ScriptExecutionData& execdata) const { - if (sig.empty()) return false; + if (sig.empty()) return true; if (pubkey_in.size() != 32) return false; XOnlyPubKey pubkey{pubkey_in}; ```

nit if you retouch ```suggestion const int path_len = (control.size() - TAPROOT_CONTROL_BASE_SIZE) / TAPROOT_CONTROL_NODE_SIZE; const XOnlyPubKey p{uint256(std::vector<unsigned char>(control.begin() + 1, control.begin() + TAPROOT_CONTROL_BASE_SIZE))}; const XOnlyPubKey q{uint256(program)}; uint256 k = (CHashWriter(HASHER_TAPLEAF) << uint8_t(control[0] & TAPROOT_LEAF_MASK) << script).GetSHA256(); for (int i = 0; i < path_len; ++i) { CHashWriter ss_branch{HASHER_TAPBRANCH}; ```

41d08f5d77f52bec0e31bb081d85fff2d67d0467: let's document that `uint256 GetPrevoutHash` and `GetSequenceHash` (now) return a single SHA256 hash. Perhaps we should rename them to GetPrevout**s**Hash and GetSequence**s**Hash, consistent with `GetSpentAmountsHash` and `GetSpentScriptsHash`. In BIP-341 the phrasing is slightly ambiguous: "sha_prevouts (32): the SHA256 of the serialization of all input outpoints." It could be more clear that this means concatenate all previous and then hash them, rather than to concatenate the hashes. Changing the variable name helps with that clarity too.

I'm generally not a fan of these Data interfaces which have all the bool x_init fields. As an alternative, we could use an optional type wrapper. However, optional types depend on boost until c++17, which we shouldn't introduce consensus dependencies on. My proposal would be to copy the definition of c++17 optional into https://github.com/bitcoin/bitcoin/blob/99813a9745fe10a58bedd7a4cb721faf14f907a4/src/optional.h and drop the boost dependency, and then use this here. This can be done as a PR separate from Taproot so as not to further burden this PR. Here, optionals express both the nothing set and not initialized state (hence dual-wrapped options) but perhaps we'd be OK in collapsing the wrapped ones to represent (not initialized or not set) OR value. ```c++ struct ScriptExecutionData { //! The tapleaf hash. Option<uint256> m_tapleaf_hash; Option<Option<uint32_t>> m_codeseparator_pos; //! Hash of the annex data, if any Option<Option<uint256>> m_annex_hash; /** How much validation weight is left (decremented for every successful signature check). */ Option<int64_t> m_validation_weight_left; }; ``` This would seem, to me, to make the interface quite a bit safer and eliminates a bunch of initialization logic & prevents UB from cropping up. It would also be, in my opinion, a larger project (perhaps worthwhile) to generally rethink this state object to be accessed through an interface that guarantees the fields are accessed correctly -- e.g., once a m_validation_weight_left is set it can only be read or decreased, and the function to decrease can set error if the condition isn't met on underflow. Maybe not worth that large of a refactor though, I think optionals would already be an improvement.

```suggestion from test_framework.script import ( ANNEX_TAG, CScript, CScriptOp, LEAF_VERSION_TAPSCRIPT, LOCKTIME_THRESHOLD, MAX_SCRIPT_ELEMENT_SIZE, OP_0, OP_1, OP_1SUB, OP_1NEGATE, OP_2DROP, OP_2DUP, OP_CHECKMULTISIG, OP_CHECKMULTISIGVERIFY, OP_CHECKSIG, OP_CHECKSIGADD, OP_CHECKSIGVERIFY, OP_CODESEPARATOR, OP_DROP, OP_DUP, OP_ELSE, OP_ENDIF, OP_EQUAL, OP_IF, OP_NOT, OP_NOTIF, OP_RETURN, OP_SWAP, OP_VERIF, SIGHASH_SINGLE, TaprootSignatureHash, is_op_success, taproot_construct, ) ```

constcast-glitch

why nested conditional here, but not nested above (line 240)? both branches have "stack_size >= 2" as well. without understanding what any of this means, would it be clearer to write like this, removing the duplication and nesting? ``` // precondition for taproot (?) if (stack_size < 2) { return } if (!stack[stack_size - 1].empty() && stack[stack_size - 1][0] == ANNEX_TAG) { // whatever this is } else if ((stack[stack_size - 1][0] & 0xfe) == 0xc0)) { // script path spend } ```