test: Add `leaf_version` parameter to `taproot_tree_helper()` #29371

Christewart · 2024-02-02T16:46:23Z

This PR adds a leaf_version parameter to taproot_tree_helper(). Previously the leaf version was hard coded, because we only currently support 1 leaf version.

Proposed soft forks such as #29221 require new leaf versions to be allocated. This PR allows the test framework to accomodate new leaf versions. This commit is also included in #29221, but in the policy of trying to avoid mega-PRs, I carved this out into a separate PR that can be merged.

This PR does not alter any test functionality, just adds a parameter and uses it across the codebase rather than hard coding the value inside of taproot_tree_helper().

DrahtBot · 2024-02-02T16:46:26Z

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/29371.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theStack, achow101, scgbckbone
Stale ACK	edilmedeiros, itornaza

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

edilmedeiros · 2024-02-05T18:19:49Z

Tested ACK fe53feb

I've run test/functional/feature_taproot.py and test/functional/wallet_taproot.py.

The changes make sense in preparation for future expansions, as said.
They don't alter current test logic.

test/functional/test_framework/script.py

scgbckbone

utACK

test/functional/test_framework/script.py

Christewart · 2024-02-06T15:53:35Z

I believe this CI failure is spurious: https://github.com/bitcoin/bitcoin/pull/29371/checks?check_run_id=21277712706

Could someone restart the job ( 🙏 ) or I can push an empty commit to restart everything.

theStack · 2024-03-12T16:34:21Z

Concept ACK

In order to avoid changes in functional tests and keep the patch small and simple, it probably would makes sense to set LEAF_VERSION_TAPSCRIPT as default value for parameter leaf_version for taproot_construct()?

Christewart · 2024-03-12T19:26:13Z

In order to avoid changes in functional tests and keep the patch small and simple, it probably would makes sense to set LEAF_VERSION_TAPSCRIPT as default value for parameter leaf_version for taproot_construct()?

This is more philsophical, but I really prefer not to provide default parameters to critical test code like this. It is SO easy to just forget to pass a parameter and have your test cases not test all possible branches of code! In this case, when dealing with consensus critical test cases, I think it should be required that the developer of the unit test pass in the tapscript version they are intending to test rather than us just assuming they want to test LEAF_VERSION_TAPSCRIPT.

Please see #29221 for examples of this. For instance if I forgot to pass LEAF_VERSION_TAPSCRIPT_64BIT in a specific spot I believe my tests would trivially pass because it is a soft fork!

Reasonable minds can differ of course, for future readers of the PR leave a comment or give me a 👎 if you disagree and would like to see the default parameter used in this case. If it is clear that I'm in the minority, I will change it.

itornaza · 2024-04-10T17:29:04Z

tested ACK 1ccd751

Did a code review and everything lgtm
Built the PR with --with-incompatible-bdb and --enable-surpress-external-warnings
Run unit tests with make check and alll tests pass
Run functional tests with no extra flags and all tests pass
Run extended functional tests with --extended all tests pass

Reasonable minds can differ of course, for future readers of the PR leave a comment or give me a 👎 if you disagree and would like to see the default parameter used in this case. If it is clear that I'm in the minority, I will change it.

Who am I to say, but I philosophically agree with your reasoning on not passing default parameters in critical test code.

scgbckbone

re-ACK (I'm pro default function parameter)

achow101

Concept ACK

Please cleanup your commit message, it contains message fragments from squashed commits.

test/functional/test_framework/script.py

Christewart · 2024-11-05T14:29:25Z

Concept ACK

Please cleanup your commit message, it contains message fragments from squashed commits.

Done in 6ffdabd

itornaza

re ACK 6ffdabd

scgbckbone

ACK

Christewart · 2025-02-08T14:52:03Z

Rebased

itornaza

rtACK fca0367

Christewart · 2025-04-15T22:10:11Z

Rebased

scgbckbone

ACK

DrahtBot added the Tests label Feb 2, 2024

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from 778c65e to fe53feb Compare February 2, 2024 16:55

Christewart marked this pull request as ready for review February 2, 2024 18:54

DrahtBot mentioned this pull request Feb 2, 2024

Covenant tools softfork #28550

Closed

Christewart commented Feb 5, 2024

View reviewed changes

test/functional/test_framework/script.py Show resolved Hide resolved

edilmedeiros approved these changes Feb 5, 2024

View reviewed changes

scgbckbone approved these changes Feb 6, 2024

View reviewed changes

test/functional/test_framework/script.py Outdated Show resolved Hide resolved

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from fe53feb to 25dc427 Compare February 6, 2024 15:07

DrahtBot added CI failed and removed CI failed labels Feb 6, 2024

DrahtBot mentioned this pull request Feb 6, 2024

Implement 64 bit arithmetic op codes in the Script interpreter #29221

Closed

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from 25dc427 to 00f5d15 Compare February 22, 2024 14:08

DrahtBot added the CI failed label Feb 22, 2024

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from 00f5d15 to accc172 Compare March 6, 2024 13:55

DrahtBot removed the CI failed label Mar 6, 2024

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from accc172 to 1ccd751 Compare March 9, 2024 12:15

Christewart mentioned this pull request Mar 11, 2024

test: fix accurate multisig sigop count (BIP16), add unit test #29615

Merged

DrahtBot requested review from edilmedeiros, scgbckbone and theStack April 10, 2024 18:46

DrahtBot mentioned this pull request Apr 23, 2024

test: add MiniWallet tagging support to avoid UTXO mixing, use in fill_mempool #29939

Merged

scgbckbone reviewed Apr 27, 2024

View reviewed changes

DrahtBot added the Needs rebase label May 13, 2024

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from 1ccd751 to ac5d855 Compare May 13, 2024 18:28

DrahtBot removed the CI failed label Oct 25, 2024

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from ed4febd to 6a0406b Compare October 29, 2024 14:28

achow101 reviewed Nov 4, 2024

View reviewed changes

test/functional/test_framework/script.py Outdated Show resolved Hide resolved

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from 6a0406b to 6ffdabd Compare November 5, 2024 14:28

itornaza approved these changes Nov 18, 2024

View reviewed changes

DrahtBot requested review from achow101, edilmedeiros and scgbckbone November 18, 2024 17:40

scgbckbone approved these changes Nov 27, 2024

View reviewed changes

MainS2020 mentioned this pull request Nov 27, 2024

ACK MorpheusAIs/Docs#394

Open

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from 6ffdabd to fca0367 Compare February 8, 2025 14:51

itornaza reviewed Feb 25, 2025

View reviewed changes

DrahtBot requested a review from scgbckbone February 25, 2025 09:45

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from fca0367 to e6c1026 Compare March 17, 2025 18:00

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from e6c1026 to 4560189 Compare April 15, 2025 22:03

DrahtBot added the Needs rebase label Apr 15, 2025

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from 4560189 to 2eacaa2 Compare April 15, 2025 22:09

DrahtBot added the CI failed label Apr 15, 2025

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from 2eacaa2 to 4194b44 Compare April 15, 2025 23:21

DrahtBot removed Needs rebase CI failed labels Apr 16, 2025

DrahtBot mentioned this pull request May 3, 2025

scripted-diff: adapt script error constant names in feature_taproot.py #32415

Merged

DrahtBot added the Needs rebase label May 6, 2025

test: Add leaf_version parameter to taproot_tree_helper()

3b8b612

Christewart force-pushed the 2024-02-01-scriptpy-leafver branch from 4194b44 to 3b8b612 Compare May 9, 2025 16:42

DrahtBot removed the Needs rebase label May 9, 2025

scgbckbone reviewed Jun 27, 2025

View reviewed changes

test: Add leaf_version parameter to taproot_tree_helper() #29371

Are you sure you want to change the base?

test: Add leaf_version parameter to taproot_tree_helper() #29371

Uh oh!

Conversation

Christewart commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

edilmedeiros commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

scgbckbone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Christewart commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack commented Mar 12, 2024

Uh oh!

Christewart commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itornaza commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scgbckbone left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Christewart commented Nov 5, 2024

Uh oh!

itornaza left a comment

Choose a reason for hiding this comment

Uh oh!

scgbckbone left a comment

Choose a reason for hiding this comment

Uh oh!

Christewart commented Feb 8, 2025

Uh oh!

itornaza left a comment

Choose a reason for hiding this comment

Uh oh!

Christewart commented Apr 15, 2025

Uh oh!

scgbckbone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

test: Add `leaf_version` parameter to `taproot_tree_helper()` #29371

test: Add `leaf_version` parameter to `taproot_tree_helper()` #29371

Christewart commented Feb 2, 2024 •

edited

Loading

DrahtBot commented Feb 2, 2024 •

edited

Loading

edilmedeiros commented Feb 5, 2024 •

edited

Loading

Christewart commented Feb 6, 2024 •

edited

Loading

Christewart commented Mar 12, 2024 •

edited

Loading

itornaza commented Apr 10, 2024 •

edited

Loading