Discourage unsigned / uncommitted annexes #10
Conversation
src/script/interpreter.cpp
Outdated
| @@ -1694,6 +1694,7 @@ bool GenericTransactionSignatureChecker<T>::CheckSchnorrSignature(Span<const uns | |||
| return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_HASHTYPE); | |||
| } | |||
| if (!VerifySchnorrSignature(sig, pubkey, sighash)) return set_error(serror, SCRIPT_ERR_SCHNORR_SIG); | |||
| execdata.m_annex_committed = execdata.m_annex_present; | |||
There was a problem hiding this comment.
I'm not sure this is actually correct.
IIUC schnorr signatures always sign the presence or absence of a annex. If the annex is not present, they're still committing to the absence of the annex. While the current standardness rule in Libre Relay requires annexes to either be all present or not present at all. What we actually want is for annexes to be not present only in input types that can't commit to an annex. Which is specifically taproot inputs that don't have any check sig operations.
There was a problem hiding this comment.
It's a matter of semantics and how we want to define m_annex_signed. Namely, should m_annex_signed = true if and only if an annex has been signed, or if and only if there exists a signature that would commit to the annex, if present?
As far as discouraging unsigned annexes is concerned, it shouldn't matter, since we only return an error if execdata.m_annex_present && !execdata.m_annex_signed.
Personally, I'm indifferent, and I'm happy to go with whichever folks prefer. My only concern is that it would be confusing for m_annex_signed to be true if no annex is present.
There was a problem hiding this comment.
It's unfortunate that we decided to make it possible to have both an empty annex, and no annex at all.
Maybe we can think about this in terms of an unwanted annex: an annex that exists. But without a clear signature authorizing its existence. Or alternatively, an unsigned annex.
There was a problem hiding this comment.
I like that suggestion. Calling the variable m_annex_unsigned might be the cleanest semantically.
I updated the PR to reflect that change.
|
In principal this seems reasonable. One nit: how about we called it an "unsigned" annex rather than "uncommitted"? That's probably going to be less confusing for people, as at the moment the only way to commit to an annex is to sign it; no-one is proposing actual annex commitments in any soft-fork. |
joshdoman
force-pushed
the
2025-std-unstructured-annex-improvements
branch
from
22f5ca3 to
3eeeed5
Compare
Sounds good. I updated the PR. |
| @@ -143,6 +143,10 @@ enum : uint32_t { | |||
| // Making unknown public key versions (in BIP 342 scripts) non-standard | |||
| SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE = (1U << 20), | |||
|
|
|||
| // Making unsigned annexes non-standard | |||
| // | |||
| SCRIPT_VERIFY_DISCOURAGE_UNSIGNED_ANNEX = (1U << 21), | |||
There was a problem hiding this comment.
Oh, since Bitcoin Core does not have this patch, let's use a totally different bit number for this to avoid merge conflicts in the future.
Maybe the highest bit?
I've been thinking about the possibility of witness malleability if the annex becomes standard. As it stands, any script path spend that lacks a CHECKSIG can have an arbitrary amount of data placed in the annex by a third party.
A simple solution would be to make unsigned annexes non-standard. I implemented this by adding a
m_annex_committedfield toScriptExecutionData, which is only set totrueifCheckSchnorrSignatureexecutes successfully and an annex is present. I called this variablem_annex_committedrather thanm_annex_signedin case there is a future opcode that commits to the annex explicitly.@petertodd Do you think this is a good idea, if the annex is made standard? I'm not sure how common non-CHECKSIG scripts are today, but I imagine they could become more common if opcodes like CTV or CSFS are adopted in the future.