test: fix MiniWallet script-path spend (missing parity bit in leaf version)

[theStack]

This PR fixes a dormant bug in MiniWallet that exists since support for P2TR was initially added in #23371 (see commit 041abfe).

In the course of spending the output, the leaf version byte of the control block in the witness stack doesn't set the parity bit, i.e. we were so far just lucky that the used combinations of relevant data (internal pubkey, leaf script / version) didn't result in a tweaked pubkey with odd y-parity. If that was the case, we'd get the following validation error:

mandatory-script-verify-flag-failed (Witness program hash mismatch) (-26)

Since MiniWallets can now optionally be tagged (#29939), resulting in different internal pubkeys, the issue is more prevalent now. Fix it by passing the parity bit, as specified in BIP341.

Can be tested with the following patch (fails on master, succeeds on PR):

diff --git a/test/functional/test_framework/mempool_util.py b/test/functional/test_framework/mempool_util.py
index 148cc935ed..7ebe858681 100644
--- a/test/functional/test_framework/mempool_util.py
+++ b/test/functional/test_framework/mempool_util.py
@@ -42,7 +42,7 @@ def fill_mempool(test_framework, node):
     # Generate UTXOs to flood the mempool
     # 1 to create a tx initially that will be evicted from the mempool later
     # 75 transactions each with a fee rate higher than the previous one
-    ephemeral_miniwallet = MiniWallet(node, tag_name="fill_mempool_ephemeral_wallet")
+    ephemeral_miniwallet = MiniWallet(node, tag_name="fill_mempool_ephemeral_wallet3")
     test_framework.generate(ephemeral_miniwallet, 1 + num_of_batches * tx_batch_size)
 
     # Mine enough blocks so that the UTXOs are allowed to be spent

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	rkrux

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29371 (test: Add leaf_version parameter to taproot_tree_helper() by Christewart)
• #29221 (Implement 64 bit arithmetic op codes in the Script interpreter by Christewart)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[rkrux]

tACK e4abac2

Reviewing #29939 previously made it easy for me to review this one!

Make successful and all functional tests pass on this branch. Using the shared patch, I can see these 2 tests fails in master branch.

feature_versionbits_warning.py                         | ✖ Failed  | 3 s
wallet_fundrawtransaction.py --descriptors             | ✖ Failed  | 17 s

ALL                                                    | ✖ Failed  | 2752 s (accumulated)

[rkrux]

I feel we should tie this only-path here to taproot_construct call in address.py, either by passing it as a parameter or creating it as a constant. Otherwise, it makes you wonder here at this line where did only-path come from.

[theStack]

Good point. I think what we could do here is first asserting that there is only a single entry in the _taproot_info.leaves dictionary and then using that entry, so we don't even need the "only-path" magic string. Will look into it.

[rkrux]

Yes, this is also good.

[rkrux]

Would love to see the corresponding cpp reference code, share if you know?

[theStack]

The corresponding C++ code in Core for creating the control block (second item on the witness stack) can be found in the signingprovider module, see function TaprootBuilder::GetSpendData():

bitcoin/src/script/signingprovider.cpp

Lines 414 to 417 in 2cedb42

	std::vector<unsigned char> control_block;
	control_block.resize(TAPROOT_CONTROL_BASE_SIZE + TAPROOT_CONTROL_NODE_SIZE * leaf.merkle_branch.size());
	control_block[0] = leaf.leaf_version | (m_parity ? 1 : 0);
	std::copy(m_internal_key.begin(), m_internal_key.end(), control_block.begin() + 1);

The actual witness stack, consisting of script and control block, is assembled in the SignTaproot function:

bitcoin/src/script/sign.cpp

Lines 379 to 380 in 2cedb42

	result_stack.emplace_back(std::begin(script), std::end(script)); // Push the script
	result_stack.push_back(*control_blocks.begin()); // Push the smallest control block

The validation counter-part for taproot script-path spends is found here:

bitcoin/src/script/interpreter.cpp

Lines 1923 to 1932 in 2cedb42

	// Script path spending (stack size is >1 after removing optional annex)
	const valtype& control = SpanPopBack(stack);
	const valtype& script = SpanPopBack(stack);
	if (control.size() < TAPROOT_CONTROL_BASE_SIZE || control.size() > TAPROOT_CONTROL_MAX_SIZE || ((control.size() - TAPROOT_CONTROL_BASE_SIZE) % TAPROOT_CONTROL_NODE_SIZE) != 0) {
	return set_error(serror, SCRIPT_ERR_TAPROOT_WRONG_CONTROL_SIZE);
	}
	execdata.m_tapleaf_hash = ComputeTapleafHash(control[0] & TAPROOT_LEAF_MASK, script);
	if (!VerifyTaprootCommitment(control, program, execdata.m_tapleaf_hash)) {
	return set_error(serror, SCRIPT_ERR_WITNESS_PROGRAM_MISMATCH);
	}

[rkrux]

Great, thanks for sharing, I'm digging into this!

[theStack]

Thanks for the review @rkrux! Addressed your comment #30076 (comment); the single taproot leaf is now accessed directly rather than by name, so there is no magic string involved anymore. Note that the leaf name (second parameter to taproot_construct) still had to be changed from None to some string, as otherwise no entry in the taproot_info.leaves dictionary would be created.

[rkrux]

@theStack Thanks for the quick update on this, reACK 57eb590