Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
11 changes: 9 additions & 2 deletions src/policy/policy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
// Check policy limits for Taproot spends:
// - MAX_STANDARD_TAPSCRIPT_STACK_ITEM_SIZE limit for stack item size
// - No annexes
// ELEMENTS: allow annexes for simplicity transactions
if (witnessversion == 1 && witnessprogram.size() == WITNESS_V1_TAPROOT_SIZE && !p2sh) {
// Missing witness; invalid by consensus rules
if (i >= tx.witness.vtxinwit.size()) {
Expand All @@ -315,8 +316,14 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
// Taproot spend (non-P2SH-wrapped, version 1, witness program size 32; see BIP 341)
Span stack{tx.witness.vtxinwit[i].scriptWitness.stack};
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;
SpanPopBack(stack); // drop the annex
const auto& control_block = SpanPopBack(stack);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In bfde7b7:

This looks like a double-pop. A few lines below this change there is another if-block, if (stack.size() >= 2) (which will trigger if this block triggers) that also has

const auto& control_block = SpanPopBack(stack);

Found by Qwen 3.8-Flash-Next.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The bot also gave me this unit test file

// Temporary review probe for Elements PR #1539 (NOT for commit):
// annexed Simplicity spend where the 32-byte CMR starts with 0xc0/0xc1.
// IsWitnessStandard's annex branch pops annex AND control block, then the
// generic script-path branch pops the CMR as "control block"; if cmr[0] & 0xfe
// == 0xc0 (Tapscript leaf), MAX_STANDARD_TAPSCRIPT_STACK_ITEM_SIZE (80) is
// applied to the remaining witness items. Probe compares a Simplicity annexed
// spend with a 100-byte witness across cmr[0] in {0x11, 0xc0, 0xc1, 0xbe}.
#include <test/util/setup_common.h>

#include <coins.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
#include <script/script.h>
#include <arith_uint256.h>
#include <uint256.h>

#include <boost/test/unit_test.hpp>

#include <map>
#include <vector>

namespace {
class MapCoinsView : public CCoinsView
{
    std::map<COutPoint, Coin> m_coins;
public:
    void Add(const COutPoint& out, Coin coin) { m_coins[out] = std::move(coin); }
    std::optional<Coin> GetCoin(const COutPoint& outpoint) const override
    {
        auto it = m_coins.find(outpoint);
        if (it == m_coins.end()) return std::nullopt;
        return it->second;
    }
};
} // namespace

BOOST_FIXTURE_TEST_SUITE(annex_padding_probe, BasicTestingSetup)

static CTransactionRef MakeAnnexedSpend(uint8_t cmr_first, size_t witness_len)
{
    CMutableTransaction tx;
    tx.vin.resize(1);
    tx.vin[0].prevout = COutPoint(Txid::FromUint256(ArithToUint256(1)), 0);
    tx.vout.resize(1);
    tx.vout[0].nValue.SetToAmount(1000);

    CTxInWitness win;
    std::vector<uint8_t> cmr(32, 0x22);
    cmr[0] = cmr_first;
    std::vector<uint8_t> control(33, 0xcc);
    control[0] = 0xbe; // TAPROOT_LEAF_TAPSIMPLICITY, depth 0
    std::vector<uint8_t> annex(12, 0);
    annex[0] = 0x50;   // ANNEX_TAG + zero padding
    win.scriptWitness.stack = {
        std::vector<uint8_t>(witness_len, 0xaa), // "simplicity witness"
        std::vector<uint8_t>(40, 0x11),          // "simplicity program"
        cmr,                                     // script CMR (32 bytes)
        control,                                 // control block
        annex,
    };
    tx.witness.vtxinwit.push_back(win);
    return MakeTransactionRef(std::move(tx));
}

BOOST_AUTO_TEST_CASE(annex_padding_double_pop_probe)
{
    CScript taproot_spk;
    std::vector<uint8_t> prog(32, 0xab);
    taproot_spk << OP_1 << prog;

    MapCoinsView view;
    CTxOut prevout;
    prevout.nValue.SetToAmount(100000);
    prevout.scriptPubKey = taproot_spk;
    view.Add({Txid::FromUint256(ArithToUint256(1)), 0}, Coin{std::move(prevout), 1, false});
    CCoinsViewCache coins(&view);

    const bool a = IsWitnessStandard(*MakeAnnexedSpend(0x11, 100), coins);
    const bool b = IsWitnessStandard(*MakeAnnexedSpend(0xc4, 100), coins);
    const bool c = IsWitnessStandard(*MakeAnnexedSpend(0xc5, 100), coins);
    const bool d = IsWitnessStandard(*MakeAnnexedSpend(0xbe, 100), coins);
    const bool e = IsWitnessStandard(*MakeAnnexedSpend(0xc4, 80), coins); // <= limit
    BOOST_TEST_MESSAGE("100-byte witness: cmr[0]=0x11 -> " << a << "; 0xc4 -> " << b
                       << "; 0xc5 -> " << c << "; 0xbe -> " << d << "   | 80-byte witness, 0xc4 -> " << e);
    BOOST_CHECK(a);
    BOOST_CHECK(d);
    BOOST_CHECK(e);
    BOOST_CHECK_EQUAL(b, a);
    BOOST_CHECK_EQUAL(c, a);
}

BOOST_AUTO_TEST_SUITE_END()

// Annexes are allowed for Simplicity spends only
// checks for zero padding and exact size are done in CheckSimplicity
if (control_block.empty() || (control_block[0] & TAPROOT_LEAF_MASK) != TAPROOT_LEAF_TAPSIMPLICITY) {
// Annexes are nonstandard as long as no semantics are defined for them.
return false;
}
}
if (stack.size() >= 2) {
// Script path spend (2 or more stack elements after removing optional annex)
Expand Down
4 changes: 3 additions & 1 deletion src/policy/policy.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,9 @@ static constexpr unsigned int STANDARD_SCRIPT_VERIFY_FLAGS{MANDATORY_SCRIPT_VERI
SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION |
SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS |
SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE |
SCRIPT_VERIFY_SIMPLICITY};
SCRIPT_VERIFY_SIMPLICITY |
SCRIPT_VERIFY_ANNEX_PADDING};


/** 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
30 changes: 26 additions & 4 deletions src/script/interpreter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3094,14 +3094,14 @@ uint32_t GenericTransactionSignatureChecker<T>::GetnIn() const
}

template <class T>
bool GenericTransactionSignatureChecker<T>::CheckSimplicity(const valtype& program, const valtype& witness, const rawElementsTapEnv& simplicityRawTap, int64_t budget, ScriptError* serror) const
bool GenericTransactionSignatureChecker<T>::CheckSimplicity(const valtype& program, const valtype& witness, const rawElementsTapEnv& simplicityRawTap, int64_t minCost, int64_t budget, ScriptError* serror) const
{
simplicity_err error;
elementsTapEnv* simplicityTapEnv = simplicity_elements_mallocTapEnv(&simplicityRawTap);

assert(txdata->m_simplicity_tx_data);
assert(simplicityTapEnv);
if (!simplicity_elements_execSimplicity(&error, nullptr, txdata->m_simplicity_tx_data.get(), nIn, simplicityTapEnv, txdata->m_hash_genesis_block.data(), 0, budget, nullptr, program.data(), program.size(), witness.data(), witness.size())) {
if (!simplicity_elements_execSimplicity(&error, nullptr, txdata->m_simplicity_tx_data.get(), nIn, simplicityTapEnv, txdata->m_hash_genesis_block.data(), minCost, budget, nullptr, program.data(), program.size(), witness.data(), witness.size())) {
assert(!"simplicity_elements_execSimplicity internal error");
}
simplicity_elements_freeTapEnv(simplicityTapEnv);
Expand Down Expand Up @@ -3262,9 +3262,11 @@ static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion,
// BIP341 Taproot: 32-byte non-P2SH witness v1 program (which encodes a P2C-tweaked pubkey)
if (!(flags & SCRIPT_VERIFY_TAPROOT)) return set_success(serror);
if (stack.size() == 0) return set_error(serror, SCRIPT_ERR_WITNESS_PROGRAM_WITNESS_EMPTY);
valtype annex;
if (stack.size() >= 2 && !stack.back().empty() && stack.back()[0] == ANNEX_TAG) {
// Drop annex (this is non-standard; see IsWitnessStandard)
const valtype& annex = SpanPopBack(stack);
// ELEMENTS: store the annex for CheckSimplicity
annex = SpanPopBack(stack);
execdata.m_annex_hash = (HashWriter{} << annex).GetSHA256();
execdata.m_annex_present = true;
} else {
Expand Down Expand Up @@ -3306,7 +3308,27 @@ static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion,
simplicityRawTap.controlBlock = control.data();
simplicityRawTap.pathLen = (control.size() - TAPROOT_CONTROL_BASE_SIZE) / TAPROOT_CONTROL_NODE_SIZE;
simplicityRawTap.scriptCMR = script.data();
return checker.CheckSimplicity(simplicity_program, simplicity_witness, simplicityRawTap, budget, serror);
// If there is no annex, or we are in consensus checking mode, minCost is set to 0 which effectively disables any overweight cost checks.
int64_t minCost = 0;
if ((flags & SCRIPT_VERIFY_ANNEX_PADDING) && annex.size() > 0) {
valtype zero_padding(annex.size(), 0);
zero_padding[0] = ANNEX_TAG;
if (annex != zero_padding) {
return set_error(serror, SCRIPT_ERR_SIMPLICITY_PADDING_NONZERO);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Feel free to push back on this idea, but I think it would be marginally better to move this zero-padding check into policy.cpp. That is to say that IsWitnessStandard says that an annex is standard only when it is both

  1. all zeros and
  2. is part of a Simplicity redemption.

The all zero check can be validated without parsing of the Simplicity program, and moving the code there removes the need for an explicit SCRIPT_ERR_SIMPLICITY_PADDING_NONZERO error. It just simply becomes non-standard to have a non-zero annex.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm of two minds about this, I initially did have it in IsWitnessStandard, but I mildly prefer this approach since it returns a specific error to the client instead of just "non-standard" with no additional info. I don't really mind either way, and do see your point not having to parse the Simplicity program or needing a new error variant.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, let's leave it as is unless an Element's reviewer suggests otherwise.

}
// Compute what the budget would have been without the padding.
// budget includes the padding cost, so subtracting this stack item won't underflow.
minCost = budget - ::GetSerializeSize(annex);
// If the annex exists and is empty (i.e. its size is 1), then the only way the annex could be smaller is by eliminating it entirely.
// So we use the above computed value for minCost. Note: in that case the minCost is 2 WU less than the budget.

// If the annex exists and is non-empty, then we add to minCost the value of an annex that contains one fewer byte.
if (zero_padding.size() > 1) {
zero_padding.pop_back();
minCost += ::GetSerializeSize(zero_padding);
}
}
return checker.CheckSimplicity(simplicity_program, simplicity_witness, simplicityRawTap, minCost, budget, serror);
}
if (flags & SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION) {
return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION);
Expand Down
8 changes: 6 additions & 2 deletions src/script/interpreter.h
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,10 @@ enum : uint32_t {
//
SCRIPT_VERIFY_SIMPLICITY = (1U << 23),

// Check exact annex padding policy for simplicity spends
//
SCRIPT_VERIFY_ANNEX_PADDING = (1U << 24),

// Constants to point to the highest flag in use. Add new flags above this line.
//
SCRIPT_VERIFY_END_MARKER
Expand Down Expand Up @@ -345,7 +349,7 @@ class BaseSignatureChecker
return std::numeric_limits<uint32_t>::max();
}

virtual bool CheckSimplicity(const std::vector<unsigned char>& witness, const std::vector<unsigned char>& program, const rawElementsTapEnv& simplicityRawTap, int64_t budget, ScriptError* serror) const
virtual bool CheckSimplicity(const std::vector<unsigned char>& witness, const std::vector<unsigned char>& program, const rawElementsTapEnv& simplicityRawTap, int64_t minCost, int64_t budget, ScriptError* serror) const
{
return false;
}
Expand Down Expand Up @@ -393,7 +397,7 @@ class GenericTransactionSignatureChecker : public BaseSignatureChecker

const PrecomputedTransactionData* GetPrecomputedTransactionData() const override;
uint32_t GetnIn() const override;
bool CheckSimplicity(const std::vector<unsigned char>& program, const std::vector<unsigned char>& witness, const rawElementsTapEnv& simplicityRawTap, int64_t budget, ScriptError* serror) const override;
bool CheckSimplicity(const std::vector<unsigned char>& program, const std::vector<unsigned char>& witness, const rawElementsTapEnv& simplicityRawTap, int64_t minCost, int64_t budget, ScriptError* serror) const override;
};

using TransactionSignatureChecker = GenericTransactionSignatureChecker<CTransaction>;
Expand Down
2 changes: 2 additions & 0 deletions src/script/script_error.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@ std::string ScriptErrorString(const ScriptError serror)
return "EC scalar mult verify fail";
case SCRIPT_ERR_SIMPLICITY_WRONG_LENGTH:
return "Simplicity witness has incorrect length";
case SCRIPT_ERR_SIMPLICITY_PADDING_NONZERO:
return "Simplicity annex padding must be all zeros";
case SCRIPT_ERR_SIMPLICITY_DATA_OUT_OF_RANGE:
return SIMPLICITY_ERR_MSG(SIMPLICITY_ERR_DATA_OUT_OF_RANGE);
case SCRIPT_ERR_SIMPLICITY_DATA_OUT_OF_ORDER:
Expand Down
1 change: 1 addition & 0 deletions src/script/script_error.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ typedef enum ScriptError_t

/* Elements: Simplicity related errors */
SCRIPT_ERR_SIMPLICITY_WRONG_LENGTH,
SCRIPT_ERR_SIMPLICITY_PADDING_NONZERO,
SCRIPT_ERR_SIMPLICITY_NOT_YET_IMPLEMENTED,
SCRIPT_ERR_SIMPLICITY_DATA_OUT_OF_RANGE,
SCRIPT_ERR_SIMPLICITY_DATA_OUT_OF_ORDER,
Expand Down
1 change: 1 addition & 0 deletions src/test/transaction_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ static std::map<std::string, unsigned int> mapFlagNames = {
{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},
{std::string("SIMPLICITY"), (unsigned int)SCRIPT_VERIFY_SIMPLICITY},
{std::string("ANNEX_PADDING"), (unsigned int)SCRIPT_VERIFY_ANNEX_PADDING},
};

unsigned int ParseScriptFlags(std::string strFlags)
Expand Down