[Policy] Discourage Unsigned Annexes

JeremyRubin commented at 10:46 pm on May 8, 2025: contributor

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 commented at 10:46 pm on May 8, 2025: contributor

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

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:

#32247 (BIP-348 (OP_CHECKSIGFROMSTACK) (regtest only) by jamesob)
#31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) by jamesob)
#29247 (CAT in Tapscript (BIP-347) by 0xBEEFCAF3)

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 commented at 11:14 pm on May 8, 2025: none

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 added the label CI failed on May 8, 2025

DrahtBot commented at 11:46 pm on May 8, 2025: contributor

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

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.

DrahtBot removed the label CI failed on May 9, 2025

Sjors commented at 10:03 am on May 9, 2025: member

If desired, tests could be made.

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

[Policy] Discourage Unsigned Annexes

fixme: Improve comment to make grammatically more clear that m_annex_present requires initialization

FIXME: Add Flag to tests

9d7a55828f

JeremyRubin force-pushed on May 9, 2025

JeremyRubin commented at 7:38 pm on May 9, 2025: contributor

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 commented at 2:48 pm on May 10, 2025: contributor

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

1440000bytes commented at 9:49 pm on May 11, 2025: none

Concept ACK

I couldn’t think of any legit use case that would require unsigned annex.

instagibbs commented at 2:04 pm on May 12, 2025: member

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 https://github.com/petertodd/bitcoin/commit/04c8e449a34e74e048bf5751d13592a22763ff7e . 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):

 0diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
 1index 15239e182a..c0f91b8a6d 100644
 2--- a/src/script/interpreter.cpp
 3+++ b/src/script/interpreter.cpp
 4@@ -378,10 +378,13 @@ static bool EvalChecksigTapscript(const valtype& sig, const valtype& pubkey, Scr
 5         if ((flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE) != 0) {
 6             return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_PUBKEYTYPE);
 7         }
 8     }
 9 
10+    assert(execdata.m_annex_init);
11+    execdata.m_annex_committed = true;
12+
13     return true;
14 }
15 
16 /** Helper for OP_CHECKSIG, OP_CHECKSIGVERIFY, and (in Tapscript) OP_CHECKSIGADD.
17  *
18@@ -431,16 +434,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
19     bool fRequireMinimal = (flags & SCRIPT_VERIFY_MINIMALDATA) != 0;
20     uint32_t opcode_pos = 0;
21     execdata.m_codeseparator_pos = 0xFFFFFFFFUL;
22     execdata.m_codeseparator_pos_init = true;
23 
24-    bool unsigned_annex_discouragement_needed = false;
25-    if (execdata.m_annex_init && execdata.m_annex_present) {
26-        if (flags & SCRIPT_VERIFY_DISCOURAGE_UNSIGNED_ANNEX) {
27-            unsigned_annex_discouragement_needed = true;
28-        }
29-    }
30     try
31     {
32         for (; pc < pend; ++opcode_pos) {
33             bool fExec = vfExec.all_true();
34 
35@@ -1074,12 +1071,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
36                     bool fSuccess = true;
37                     if (!EvalChecksig(vchSig, vchPubKey, pbegincodehash, pend, execdata, flags, checker, sigversion, serror, fSuccess)) return false;
38                     popstack(stack);
39                     popstack(stack);
40                     stack.push_back(fSuccess ? vchTrue : vchFalse);
41-                    if (fSuccess)
42-                        unsigned_annex_discouragement_needed = false;
43                     if (opcode == OP_CHECKSIGVERIFY)
44                     {
45                         if (fSuccess)
46                             popstack(stack);
47                         else
48@@ -1104,12 +1099,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
49                     if (!EvalChecksig(sig, pubkey, pbegincodehash, pend, execdata, flags, checker, sigversion, serror, success)) return false;
50                     popstack(stack);
51                     popstack(stack);
52                     popstack(stack);
53                     stack.push_back((num + (success ? 1 : 0)).getvch());
54-                    if (success)
55-                        unsigned_annex_discouragement_needed = false;
56                 }
57                 break;
58 
59                 case OP_CHECKMULTISIG:
60                 case OP_CHECKMULTISIGVERIFY:
61@@ -1235,11 +1228,13 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
62     catch (...)
63     {
64         return set_error(serror, SCRIPT_ERR_UNKNOWN_ERROR);
65     }
66 
67-    if (unsigned_annex_discouragement_needed) {
68+    if (flags & SCRIPT_VERIFY_DISCOURAGE_UNSIGNED_ANNEX &&
69+        execdata.m_annex_present &&
70+        !execdata.m_annex_committed) {
71         return set_error(serror, SCRIPT_ERR_TAPSCRIPT_UNSIGNED_ANNEX);
72     }
73     if (!vfExec.empty())
74         return set_error(serror, SCRIPT_ERR_UNBALANCED_CONDITIONAL);
75 
76diff --git a/src/script/interpreter.h b/src/script/interpreter.h
77index e6206f06fa..5a859d1dce 100644
78--- a/src/script/interpreter.h
79+++ b/src/script/interpreter.h
80@@ -212,10 +212,12 @@ struct ScriptExecutionData
81 
82     //! Whether m_annex_present is initialized and (only when needed) whether m_annex_hash is initialized.
83     bool m_annex_init = false;
84     //! Whether an annex is present.
85     bool m_annex_present;
86+    //! Whether the annex was commited to by a signature
87+    bool m_annex_committed = false;
88     //! Hash of the annex data.
89     uint256 m_annex_hash;
90 
91     //! Whether m_validation_weight_left is initialized.
92     bool m_validation_weight_left_init = false;

JeremyRubin commented at 3:34 pm on May 12, 2025: contributor

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 commented at 6:39 pm on May 12, 2025: none

@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 (https://github.com/petertodd/bitcoin/pull/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).

[Policy] Discourage Unsigned Annexes #32453

Code Coverage & Benchmarks

Reviews

Conflicts