The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32453.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #32998 (Bump SCRIPT_VERIFY flags to 64 bit by ajtowns)
• #31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) by jamesob)
• #29247 (CAT in Tapscript (BIP-347) by arminsabouri)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Im surprised we haven’t seen any weird annex-use yet. This seems like a good safety policy to have in here until the annex’s use gets consensus. Could someone remove this? Sure, but at least theres sort of a warning sign that they might not want to.

🚧 At least one of the CI tasks failed.
_{Task CentOS, depends, gui: https://github.com/bitcoin/bitcoin/runs/41906525804
LLM reason (✨ experimental): The CI failure is due to the test_bitcoin-qt test failing and bench_sanity_check being aborted.}

If desired, tests could be made.

I don't understand annexes at all, so having some tests to illustrate the issue would be handy.

squashed the CI fixmes to get "test each commit" to pass.

@Sjors no one understands the annex, so you're not alone. You can see https://groups.google.com/g/bitcoindev/c/Q5j2Kb6XeHI?pli=1 for some context on efforts to "standardize" the annex currently ongoing.

if anyone is looking for an annex in the wild, here is the first one on mainnet:

https://mempool.space/tx/b78748f2ee3222d9bf4b23ab917136c745531ccc5ef8097235d11e16483e466f?mode=details

seems to be keypath, so it's signed.

edit: I was mistaken, I missed the OP_TRUE, it's unsigned annex -- https://scriptpath.info/tx/b78748f2ee3222d9bf4b23ab917136c745531ccc5ef8097235d11e16483e466f

I don't love the script-time check, but this level of granularity may make sense.

I've been mulling over future interactions with things like CTV, where enabling CTV (which doesn't commit to annex) is vulnerable to witness inflation in the taproot context. It's not immediately clear to me if that means the annex should be committed or not. This patchset would at least be a minimal relaxation of annex policy, if paired with another annex relay PR such as petertodd@04c8e44 . Future "keyless" relaxations can be done later.

I have a suggestion for the code that feels a bit less hide and go seek (that may be broken, no tests):

diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 15239e182a..c0f91b8a6d 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -378,10 +378,13 @@ static bool EvalChecksigTapscript(const valtype& sig, const valtype& pubkey, Scr
         if ((flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE) != 0) {
             return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_PUBKEYTYPE);
         }
     }
 
+    assert(execdata.m_annex_init);
+    execdata.m_annex_committed = true;
+
     return true;
 }
 
 /** Helper for OP_CHECKSIG, OP_CHECKSIGVERIFY, and (in Tapscript) OP_CHECKSIGADD.
  *
@@ -431,16 +434,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
     bool fRequireMinimal = (flags & SCRIPT_VERIFY_MINIMALDATA) != 0;
     uint32_t opcode_pos = 0;
     execdata.m_codeseparator_pos = 0xFFFFFFFFUL;
     execdata.m_codeseparator_pos_init = true;
 
-    bool unsigned_annex_discouragement_needed = false;
-    if (execdata.m_annex_init && execdata.m_annex_present) {
-        if (flags & SCRIPT_VERIFY_DISCOURAGE_UNSIGNED_ANNEX) {
-            unsigned_annex_discouragement_needed = true;
-        }
-    }
     try
     {
         for (; pc < pend; ++opcode_pos) {
             bool fExec = vfExec.all_true();
 
@@ -1074,12 +1071,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
                     bool fSuccess = true;
                     if (!EvalChecksig(vchSig, vchPubKey, pbegincodehash, pend, execdata, flags, checker, sigversion, serror, fSuccess)) return false;
                     popstack(stack);
                     popstack(stack);
                     stack.push_back(fSuccess ? vchTrue : vchFalse);
-                    if (fSuccess)
-                        unsigned_annex_discouragement_needed = false;
                     if (opcode == OP_CHECKSIGVERIFY)
                     {
                         if (fSuccess)
                             popstack(stack);
                         else
@@ -1104,12 +1099,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
                     if (!EvalChecksig(sig, pubkey, pbegincodehash, pend, execdata, flags, checker, sigversion, serror, success)) return false;
                     popstack(stack);
                     popstack(stack);
                     popstack(stack);
                     stack.push_back((num + (success ? 1 : 0)).getvch());
-                    if (success)
-                        unsigned_annex_discouragement_needed = false;
                 }
                 break;
 
                 case OP_CHECKMULTISIG:
                 case OP_CHECKMULTISIGVERIFY:
@@ -1235,11 +1228,13 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
     catch (...)
     {
         return set_error(serror, SCRIPT_ERR_UNKNOWN_ERROR);
     }
 
-    if (unsigned_annex_discouragement_needed) {
+    if (flags & SCRIPT_VERIFY_DISCOURAGE_UNSIGNED_ANNEX &&
+        execdata.m_annex_present &&
+        !execdata.m_annex_committed) {
         return set_error(serror, SCRIPT_ERR_TAPSCRIPT_UNSIGNED_ANNEX);
     }
     if (!vfExec.empty())
         return set_error(serror, SCRIPT_ERR_UNBALANCED_CONDITIONAL);
 
diff --git a/src/script/interpreter.h b/src/script/interpreter.h
index e6206f06fa..5a859d1dce 100644
--- a/src/script/interpreter.h
+++ b/src/script/interpreter.h
@@ -212,10 +212,12 @@ struct ScriptExecutionData
 
     //! Whether m_annex_present is initialized and (only when needed) whether m_annex_hash is initialized.
     bool m_annex_init = false;
     //! Whether an annex is present.
     bool m_annex_present;
+    //! Whether the annex was commited to by a signature
+    bool m_annex_committed = false;
     //! Hash of the annex data.
     uint256 m_annex_hash;
 
     //! Whether m_validation_weight_left is initialized.
     bool m_validation_weight_left_init = false;

good alternative approach -- I can change to that, after there's a clear preference from a couple devs who look. I don't know if people like altering execdata if there isn't a clear API reason to want to propagate that data (I think of it as only relevant when we really need to pass the information across, whereas the change as written is fully local to script exec).

@JeremyRubin I think this is a great idea. I made a PR into Peter Todd's repo doing something similar, which may or may not be useful (petertodd#10).

Primary difference is I modified execdata like @instagibbs described and add a single line execdata.m_annex_committed = execdata.m_annex_present; to the end of CheckSchnorrSignature. This eliminates ~6 lines of code and doesn't assume that future public key versions commit to the annex (though that's probably a safe assumption to make).

Concept ACK

existing annex discouragement.

Could you link to these?

Else tests would be nice.

🐙 This pull request conflicts with the target branch and needs rebase.

(since I was tagged)

I don't think this PR makes sense stand-alone, but should definitely be considered if any sort of annex relay is to be considered standard in some future.

So I guess it can be closed for now?

I don't think this PR makes sense stand-alone, but should definitely be considered if any sort of annex relay is to be considered standard in some future.

For me, I think there's two blockers: (a) we shouldn't add dead code that will only take effect if someone forks the code; since we won't relay any annex content, this is just dead code. if there was some option to enable relaying of txs with some annex content, maybe that would be different (b) before relaying any txs with annexes, we should have a BIP that documents the way those annexes will be interpreted; even if that's only "we'll relay annexes beginning with \x00 of length up to 520 bytes, and apply no interpretation to that content". Personally I don't think "we'll relay this data and ignore it entirely" is a useful enough upgrade over existing approaches with inscriptions or OP_RETURN to be worth prioritising.