policy: Check Taproot control block size correctly in IsWitnessStandard #34142

1. 
   billymcbip commented at 12:43 pm on December 23, 2025: contributor

   I’d like to improve the Taproot control size check in IsWitnessStandard. Rather than just checking emptiness, we should perform all three checks implemented in VerifyTaprootControlBlockSize (see below). This way we can invalidate the transaction earlier when it’s clear that the control size is incompatible with consensus rules (saving compute).

   Old checks:

   • control.size() > 0

   New checks:

   • control.size() >= TAPROOT_CONTROL_BASE_SIZE
   • control.size() <= TAPROOT_CONTROL_MAX_SIZE
   • (control.size() - TAPROOT_CONTROL_BASE_SIZE) % TAPROOT_CONTROL_NODE_SIZE == 0

   For example, a single byte control block wouldn’t be empty, but it would be invalid because it’s shorter than TAPROOT_CONTROL_BASE_SIZE (33). IsWitnessStandard should return false in that case.

   This PR also adds a helper function for the control block size check to reduce repetition and to improve readability of the (rather heavy) functions that call it. We have four instances of the same check.

   I ran: cmake -B build -DENABLE_WALLET=OFF && cmake --build build -j 8 && ctest --test-dir build -j 8

   Looking forward to your feedback.

2. DrahtBot added the label TX fees and policy on Dec 23, 2025
3. 
   DrahtBot commented at 12:43 pm on December 23, 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/34142.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	bensig
   Concept NACK	darosior

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

4. 
   sedited commented at 1:21 pm on December 23, 2025: contributor

   Can you expand in the desccription what this actually improves and why this change should be done?
5. 
   billymcbip commented at 1:59 pm on December 23, 2025: contributor

   @sedited done (the new checks are those in VerifyTaprootControlBlockSize).
6. 
   billymcbip commented at 2:05 pm on December 23, 2025: contributor

   To be clear, this does not change which transactions are policy-valid. It just adds the control size check to IsWitnessStandard because it’s a light-weight check we can run with the context available at that point.
7. refactor: Add helper for Taproot control block size check 9a5554f644
8. policy: Improve Taproot control block size precheck 3cb5f8138b
9. test: Add unit tests for Taproot control block size check c567abf338
10. billymcbip force-pushed on Dec 27, 2025
11. billymcbip renamed this:
    policy: Improve Taproot control block size precheck
    policy: Check Taproot control block size correctly in `IsWitnessStandard`
    on Dec 27, 2025
12. 
    bensig commented at 6:30 pm on January 5, 2026: contributor

    ACK c567abf33821263e68b66dcccca0bca057a0ab09

    Tested on macOS - build and script_tests pass.

    Clean refactor - consolidates 4 instances of the same control block size check into VerifyTaprootControlBlockSize() - so the improved policy check in IsWitnessStandard does now correctly rejects invalid control blocks early, not just empty ones. Good test coverage added.

13. in src/policy/policy.cpp:321 in c567abf338

    317@@ -318,7 +318,7 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
    318                 // Script path spend (2 or more stack elements after removing optional annex)
    319                 const auto& control_block = SpanPopBack(stack);
    320                 SpanPopBack(stack); // Ignore script
    321-                if (control_block.empty()) return false; // Empty control block is invalid
    322+                if (!VerifyTaprootControlBlockSize(control_block)) return false;
    

    ---

    ---

    

    darosior commented at 8:02 pm on January 13, 2026:

    This check is not meant as an early sanity check of the control block, but to make sure that accessing the first byte of the control block on the following line is not going to read out of bounds.

    ---

    

    billymcbip commented at 12:53 pm on January 14, 2026:

    This check is not meant as an early sanity check of the control block

    The // 0 stack elements; this is already invalid by consensus rules clause below is not related to the two policy limits either (no annex, max stack size), so that argument is inconsistent in my opinion.

14. 
    darosior commented at 8:11 pm on January 13, 2026: member

    Concept NACK. I realize you invested time working on this PR but unfortunately i do not think this change is worth it (3cb5f8138ba5ca6a31961f627d6a661887912af1), nor that the stated goal meets the threshold for refactoring critical code (9a5554f644a9bda9ef8978b600e9ed517aefa327).

    It is important to not waste computing resources on known-invalid transactions with no PoW attached. But this change does not meaningfully improve that since the taproot control block size will be checked before Script execution, and there are many other ways an adversary can make a node waste a lot more compute.

Contributors
billymcbip DrahtBot sedited bensig darosior

Labels
TX fees and policy