[Policy] Discourage Unsigned Annexes #32453

JeremyRubin · 2025-05-08T22:46:54Z

This patch adds a "redundant" rule with the annex discouragement, to discourage Annexes that are present but do not get signed.

This PR does not include tests, as the code paths are entirely redundant (and small!) with existing annex discouragement. If desired, tests could be made.

This new policy would come into play if someone somewhere for some reason decided that they were going to modify standard policy on their node to allow annexes even though it is abundantly clear that annex policy should not change until there is a semantic meaning for them (see the Taproot BIPs for more context), they can leave this lighter policy intact (or add it as a backport if they have already started accepting annexes).

Given recent discussion, it seems clear that certain nodes will remove such limitations, and having this code present in core provides an encouragement to retain this safety oriented policy, even if Annexes are desired prematurely.

Annexes should be signed because if they are not, certain output types (e.g. and OP_TRUE script in a taproot output, used only as an anchor) would become malleable to third parties, introducing griefing and transaction pinning vectors. If the annexes will come anyways, they must at least be specifically requested under this rule.

DrahtBot · 2025-05-08T22:46:57Z

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.

Type	Reviewers
Concept ACK	1440000bytes, Christewart

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.

rot13maxi · 2025-05-08T23:14:09Z

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.

DrahtBot · 2025-05-08T23:46:22Z

🚧 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.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

Sjors · 2025-05-09T10:03:24Z

If desired, tests could be made.

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

fixme: Improve comment to make grammatically more clear that m_annex_present requires initialization FIXME: Add Flag to tests

JeremyRubin · 2025-05-09T19:38:49Z

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.

JeremyRubin · 2025-05-10T14:48:12Z

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

instagibbs · 2025-05-12T14:04:59Z

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;

JeremyRubin · 2025-05-12T15:34:21Z

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).

joshdoman · 2025-05-12T18:39:07Z

@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).

Christewart · 2025-06-04T16:29:37Z

Concept ACK

existing annex discouragement.

Could you link to these?

Else tests would be nice.

JeremyRubin · 2025-06-04T21:43:01Z

bitcoin/src/policy/policy.cpp

Lines 283 to 286 in a5e98dc

    
           if (stack.size() >= 2 && !stack.back().empty() && stack.back()[0] == ANNEX_TAG) { 
        
               // Annexes are nonstandard as long as no semantics are defined for them. 
        
               return false; 
        
           }

DrahtBot added the CI failed label May 8, 2025

DrahtBot mentioned this pull request May 9, 2025

OP_CHECKCONTRACTVERIFY #32080

Draft

DrahtBot removed the CI failed label May 9, 2025

[Policy] Discourage Unsigned Annexes

9d7a558

fixme: Improve comment to make grammatically more clear that m_annex_present requires initialization FIXME: Add Flag to tests

JeremyRubin force-pushed the unsigned_annex branch from 45fe85d to 9d7a558 Compare May 9, 2025 19:36

DrahtBot mentioned this pull request May 9, 2025

CAT in Tapscript (BIP-347) #29247

Draft

This comment was marked as abuse.

Sign in to view

EthnTuttle mentioned this pull request May 12, 2025

DC BitDevs 015 Topic Suggestion dcbitdevs/dcbitdevs.github.io#28

Closed

This was referenced May 13, 2025

BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only) #32247

Open

BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) #31989

Open

Randy808 mentioned this pull request Jun 9, 2025

Socratic Seminar 42 (June 2025) Bitcoin-Grove/miamibitdevs.org#20

Closed

ajtowns mentioned this pull request Jul 16, 2025

Bump SCRIPT_VERIFY flags to 64 bit #32998

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Policy] Discourage Unsigned Annexes #32453

[Policy] Discourage Unsigned Annexes #32453

Uh oh!

JeremyRubin commented May 8, 2025

Uh oh!

DrahtBot commented May 8, 2025 •

edited

Loading

Uh oh!

rot13maxi commented May 8, 2025

Uh oh!

DrahtBot commented May 8, 2025

Uh oh!

Sjors commented May 9, 2025

Uh oh!

JeremyRubin commented May 9, 2025

Uh oh!

JeremyRubin commented May 10, 2025 •

edited

Loading

Uh oh!

This comment was marked as abuse.

instagibbs commented May 12, 2025

Uh oh!

JeremyRubin commented May 12, 2025

Uh oh!

joshdoman commented May 12, 2025 •

edited

Loading

Uh oh!

Christewart commented Jun 4, 2025

Uh oh!

JeremyRubin commented Jun 4, 2025

Uh oh!

Uh oh!

[Policy] Discourage Unsigned Annexes #32453

Are you sure you want to change the base?

[Policy] Discourage Unsigned Annexes #32453

Uh oh!

Conversation

JeremyRubin commented May 8, 2025

Uh oh!

DrahtBot commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

rot13maxi commented May 8, 2025

Uh oh!

DrahtBot commented May 8, 2025

Uh oh!

Sjors commented May 9, 2025

Uh oh!

JeremyRubin commented May 9, 2025

Uh oh!

JeremyRubin commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as abuse.

instagibbs commented May 12, 2025

Uh oh!

JeremyRubin commented May 12, 2025

Uh oh!

joshdoman commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Christewart commented Jun 4, 2025

Uh oh!

JeremyRubin commented Jun 4, 2025

Uh oh!

Uh oh!

DrahtBot commented May 8, 2025 •

edited

Loading

JeremyRubin commented May 10, 2025 •

edited

Loading

joshdoman commented May 12, 2025 •

edited

Loading