Skip to content

Discourage unsigned / uncommitted annexes #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: 2025-std-unstructured-annex
Choose a base branch
from
Open

Discourage unsigned / uncommitted annexes #10

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
07961a6
make unsigned / uncommitted annex non-standard
joshdoman May 2, 2025
3eeeed5
renamed unsigned annex
joshdoman Jun 4, 2025
6480293
renamed m_annex_unsigned
joshdoman Jun 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/policy/policy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ static constexpr unsigned int STANDARD_SCRIPT_VERIFY_FLAGS{MANDATORY_SCRIPT_VERI
SCRIPT_VERIFY_CONST_SCRIPTCODE |
SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION |
SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS |
SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE};
SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE |
SCRIPT_VERIFY_DISCOURAGE_UNCOMMITTED_ANNEX};

/** For convenience, standard but not mandatory verify flags. */
static constexpr unsigned int STANDARD_NOT_MANDATORY_VERIFY_FLAGS{STANDARD_SCRIPT_VERIFY_FLAGS & ~MANDATORY_SCRIPT_VERIFY_FLAGS};
Expand Down
6 changes: 6 additions & 0 deletions src/script/interpreter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link

@instagibbs instagibbs May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah, that's even better

Copy link
Owner

@petertodd petertodd Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@joshdoman joshdoman Jun 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

@petertodd petertodd Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@joshdoman joshdoman Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that suggestion. Calling the variable m_annex_unsigned might be the cleanest semantically.

I updated the PR to reflect that change.

return true;
}

Expand Down Expand Up @@ -1819,6 +1820,11 @@ static bool ExecuteWitnessScript(const Span<const valtype>& stack_span, const CS
// Run the script interpreter.
if (!EvalScript(stack, exec_script, flags, checker, sigversion, execdata, serror)) return false;

// Discourage uncommitted / unsigned annexes
if (execdata.m_annex_present && !execdata.m_annex_committed && (flags & SCRIPT_VERIFY_DISCOURAGE_UNCOMMITTED_ANNEX)) {
return set_error(serror, SCRIPT_ERR_DISCOURAGE_UNCOMMITTED_ANNEX);
}

// Scripts inside witness implicitly require cleanstack behaviour
if (stack.size() != 1) return set_error(serror, SCRIPT_ERR_CLEANSTACK);
if (!CastToBool(stack.back())) return set_error(serror, SCRIPT_ERR_EVAL_FALSE);
Expand Down
6 changes: 6 additions & 0 deletions src/script/interpreter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 uncommitted annexes non-standard
//
SCRIPT_VERIFY_DISCOURAGE_UNCOMMITTED_ANNEX = (1U << 21),

// Constants to point to the highest flag in use. Add new flags above this line.
//
SCRIPT_VERIFY_END_MARKER
Expand Down Expand Up @@ -213,6 +217,8 @@ struct ScriptExecutionData
bool m_annex_present;
//! Hash of the annex data.
uint256 m_annex_hash;
//! Whether the annex has been committed to.
bool m_annex_committed = false;

//! Whether m_validation_weight_left is initialized.
bool m_validation_weight_left_init = false;
Expand Down
2 changes: 2 additions & 0 deletions src/script/script_error.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ std::string ScriptErrorString(const ScriptError serror)
return "OP_SUCCESSx reserved for soft-fork upgrades";
case SCRIPT_ERR_DISCOURAGE_UPGRADABLE_PUBKEYTYPE:
return "Public key version reserved for soft-fork upgrades";
case SCRIPT_ERR_DISCOURAGE_UNCOMMITTED_ANNEX:
return "Uncommitted annex";
case SCRIPT_ERR_PUBKEYTYPE:
return "Public key is neither compressed or uncompressed";
case SCRIPT_ERR_CLEANSTACK:
Expand Down
1 change: 1 addition & 0 deletions src/script/script_error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ typedef enum ScriptError_t
SCRIPT_ERR_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION,
SCRIPT_ERR_DISCOURAGE_OP_SUCCESS,
SCRIPT_ERR_DISCOURAGE_UPGRADABLE_PUBKEYTYPE,
SCRIPT_ERR_DISCOURAGE_UNCOMMITTED_ANNEX,

/* segregated witness */
SCRIPT_ERR_WITNESS_PROGRAM_WRONG_LENGTH,
Expand Down
1 change: 1 addition & 0 deletions src/test/transaction_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ static std::map<std::string, unsigned int> mapFlagNames = {
{std::string("CONST_SCRIPTCODE"), (unsigned int)SCRIPT_VERIFY_CONST_SCRIPTCODE},
{std::string("TAPROOT"), (unsigned int)SCRIPT_VERIFY_TAPROOT},
{std::string("DISCOURAGE_UPGRADABLE_PUBKEYTYPE"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE},
{std::string("DISCOURAGE_UNCOMMITTED_ANNEX"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_UNCOMMITTED_ANNEX},
{std::string("DISCOURAGE_OP_SUCCESS"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS},
{std::string("DISCOURAGE_UPGRADABLE_TAPROOT_VERSION"), (unsigned int)SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION},
};
Expand Down