I think we should probably require spent_outputs in the constructor. That gets us a compile-time error rather than a runtime surprise, if any code is missing an update.

Aside from tests (which have a mere 4 uses), it's only used in libbitcoinconsensus, where the omission is arguably a bug (impossible to use existing APIs for Taproot verification).

I would very much like that, but I think we inevitably need a few states in it at least.

The PrecomputedTransactionData objects used for block validation are constructed once up front, and then initialized simultaneously with other checks in CheckInputScripts. This means they need some sort of "uninitialized" state which can detect incorrect use before initialization. A cleaner solution would be to move the construction of these objects out of CheckInputScripts and do it up front, so they can be constructed with all available information once and for all. I think that makes sense, but it's a bigger change to validation logic that I'd like to avoid in this PR.

The other reason is indeed libconsensus, which has no API for taproot validation. There is also no problem, as it doesn't expose a taproot validation flag (yet). A future extension to it would add a new call that lets you pass in the spent UTXOs, and let you enable taproot validation. But as of right now, changing things to require the UTXOs whenever initializing the PrecomputedTransactionData would mean that libconsensus would need to construct mock UTXOs and pass those in... which I think is worse than having an explicit uninitialized state that'd trigger an error on misuse.

(Review TODO: Check that this is actually true, even when no signature checking occurs.)

If no signature checking occurs none of the code in interpreter.cpp matters.

Should this be non-experimental before merging?

I don't think so: It probably shouldn't be marked non-experimental until after it deployed for activation in Bitcoin because it wouldn't be good to encourage third party users of it while it is still easy to make incompatible changes in Bitcoin (e.g. as was just done. :) )

Or, maybe more likely: the API in libsecp256k1 may change still significantly in the near future (e.g. support for variable-length messages, batch validation, ...) even after we treat the scheme itself final.

Prefer assert(false); for unreachable conditions

This case will cause the assert(false) below to be triggered. Is that sufficient?

All existing disabled opcodes trigger SCRIPT_ERR_DISABLED_OPCODE unconditionally, even if their branch isn't taken. Do we want to preserve that for CHECKMULTISIG? If not, do we still want to use the same error code?

Nice catch, I didn't realize there was a discrepancy here. As BIP342 explicitly states that CMS/CMSV behave as OP_RETURN, namely failing the script if executed, I'm not going to change semantics, but I've changed it to have a separate dedicated error code.

I kept it out so that invalid control size can be its own error code. It's sanity checked before this function is invoked.

Good point.

Why can't we outlaw witness-with-annex from our policy ?

EDIT: This is part of it as 7548827. Maybe add a reference that's is outlawed in IsWitnessStandard. A side-question, why OP_SUCESSx and unknown public key types aren't rejected there too to gather all witness sanitization check in one place ?

There was some earlier discussion about this (github's webinterface is starting to suffer from the obesity of the discussion here, so I can't find the link).

I'm not sure what the best strategy is here:

• Keeping things solely in the interpreter avoids duplicate work/complexity of detecting what type of spending is used, but means extra script validation flags.
• Putting things in IsWitnessStandard may be more efficient, as it runs before any script validation is executed (and can be bypassed on testnet, which may or may not be desirable).

I'm currrently following a "things that don't need significant duplication of work go in IsWitnessStandard, the rest is done using script validation flags" rule, but I admit it's fairly arbitrary.

Going to add a comment here to note that annexes are nonstandard through other code.

I personally lean towards a clean separation between policy and consensus. You need to duplicate detection anyway both at relay and validation, what we may avoid I hope is having twice the same parsing logic. Parsing can be unique and interpretation according to consensus or policy ?

Anyway, it sounds costly in refactoring, I think that's a wider discussion to have so opened #19875 (pinning previous PR discussion on this matter)

@ariard Yes, I agree conceptually with better separation between consensus and policy implementation of script (I myself suggested splitting CScript in a minimal consensus implementation, and a more featureful separate one for everything else - policy and more), but I don't think that's relevant to the discussion here.

Even if we have a separate script interpreter for consensus and one for policy, we'll still want a separate IsStandard* function, which can do policy checks before any script interpreter (which may be expensive) is run.

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 don't like the idea of changing script validation flags based on context. It might be ok here, but in general that gets complex very quickly - things could even start interacting.

I've just added a comment to explain this. What do you think?

It depends on how you define context, does it include sigversion parameter ? BIP342 introduction presents well the script rules hierarchy and that would be better if we could encode it in clean code paths, a "flat" flag matrix is obviously harder to reason on.

I think added comment is good enough for now, I guess this conversation belong to the previous, wider one on refactoring/duplicating interpreter before post-taproot script softforks.

nit: SCRIPT_VERIFY_MINIMALIF you forgot the IF in new comment.

nit: SCRIPT_VERIFY_MINIMALIF you forgot the IF in new comment.

Fixed.

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 ?

Nice catch, this one was harder to address. I've added support in the test framework for specifying that a particular spending test requires a mismatching output (so the number of outputs <= input position), shuffling inputs around to accomplish that.