refactor: Add helper for Taproot control block size check #34139

pull billymcbip wants to merge 2 commits into bitcoin:master from billymcbip:control changing 5 files +36 −7

billymcbip commented at 5:24 PM on December 22, 2025: contributor

Add a helper for the Taproot control block size check to improve readability and reduce repetition. We currently have three instances of this check.

Build & unit tests succeed on my end.
refactor: Add helper for Taproot control block size check d7bbd8de23
DrahtBot added the label Refactoring on Dec 22, 2025
DrahtBot commented at 5:24 PM on December 22, 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/34139.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.
rkrux commented at 12:41 PM on December 23, 2025: contributor

Concept ~0 (leaning towards n-a-c-k) d7bbd8de23cdef0d844e359cf3e86b7980510cdb

While this refactor does reduce repetition and also allows reusability in the signingprovider as well, I don't think it's worth the code churn imho.

Let's see what other reviewers have to say on this.
billymcbip commented at 12:50 PM on December 23, 2025: contributor

@rkrux thank you for your feedback. Could you have a look at #34142 as well? That'd be a fourth instance of this check.
test: Add unit tests for Taproot control block size check 2c53c4d342
in src/script/interpreter.cpp:1872 in d7bbd8de23 outdated
```
1868 | @@ -1869,6 +1869,11 @@ static bool ExecuteWitnessScript(const std::span<const valtype>& stack_span, con
1869 |      return true;
1870 |  }
1871 |  
1872 | +bool VerifyTaprootControlBlockSize(std::span<const unsigned char> control)
```
janb84 commented at 11:34 AM on December 24, 2025:

With the introduction of this function I would expect that there also would be some additional tests

billymcbip commented at 12:01 PM on December 24, 2025:

Added! 2c53c4d3428518b22bd9f372ea70bd546bbd65c3

I don't want to commit a test case for max size because that would require a 8260-character string, but I did test that locally as well: 21031934357fc5f97a4dfa3c13ed9f3b77219d42
rkrux commented at 8:51 AM on December 26, 2025: contributor

@rkrux thank you for your feedback. Could you have a look at #34142 as well? That'd be a fourth instance of this check.

Both these PRs contain similar changes with minor differences, I was initially confused but later got the intent.

Suggest to keep only 1 PR that contains all the changes, will be easier for the reviewers imho to get the whole context.
billymcbip commented at 2:23 PM on December 27, 2025: contributor

@rkrux OK.

#34142 now includes all changes proposed in this PR. I will close this PR.
billymcbip closed this on Dec 27, 2025
billymcbip deleted the branch on Jan 23, 2026

Contributors

Labels

refactor: Add helper for Taproot control block size check #34139

Code Coverage & Benchmarks

Reviews