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

theStack · 2024-05-09T22:52:38Z

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

In addition to that, another bug is fixed where the internal key derivation failed, as not every pseudorandom hash results in a valid x-only pubkey. Fix this by treating the hash result as private key and calculate the x-only public key out of that, to be used then as internal key.

Fixes #30528.

DrahtBot · 2024-05-09T22:52:41Z

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
ACK	glozow, rkrux, hodlinator

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 fail in master branch.

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

ALL                                                    | ✖ Failed  | 2752 s (accumulated)

test/functional/test_framework/wallet.py

theStack · 2024-05-11T16:51:28Z

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 · 2024-05-12T07:41:51Z

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

Rather than only returning the internal key from the P2TR anyone-can-spend address creation routine, provide the whole TaprootInfo object, which in turn contains a dictionary of TaprootLeafInfo object for named leaves. This data is used in MiniWallet for the default ADDRESS_OP_TRUE mode, in order to deduplicate the witness script and leaf version of the control block.

…rsion) This commit fixes a dormant bug in MiniWallet that exists since support for P2TR was initially added in bitcoin#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 (bitcoin#29939), resulting in different internal pubkeys, the issue is more prevalent now. Fix it by passing the parity bit, as specified in BIP341.

Not every pseudorandom hash result is a valid x-only public key, so the pubkey tweaking in the course of creating the output public key would fail about every second time. Fix this by treating the hash result as private key and calculate the x-only public key out of that, to be used then as internal key.

theStack · 2024-06-11T16:58:01Z

Rebased on master, after the merge of #30162. Adding a functional test for tagged wallets to the new feature_framework_miniwallet.py (see commit e4b0dab) revealed another bug w.r.t. the internal key derivation. The code on master wrongly assumes that every pseudorandom hash results in a valid x-only pubkey. Fix this by treating the hash result as private key and calculate the x-only public key out of that, to be used then as internal key (see commit 3162c91).

glozow

ACK e4b0dab

Not an expert but: Checked that test_wallet_tagging fails without the fix on "mandatory-script-verify-flag-failed (Witness program hash mismatch) (-26)". Reviewed the test by editing the parity check in VerifyTaprootCommitment to make it pass on master and examining bytes(version + negflag) while running.

rkrux

reACK e4b0dab

Applied the shared patch again and the following tests fail on master with the mentioned error. Though I've not been able to go through the corresponding control block C++ code yet :(

raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: mandatory-script-verify-flag-failed (Witness program hash mismatch) (-26)

mempool_limit.py                                         | ✖ Failed  | 1 s
p2p_1p1c_network.py                                      | ✖ Failed  | 5 s
p2p_opportunistic_1p1c.py                                | ✖ Failed  | 1 s
p2p_tx_download.py                                       | ✖ Failed  | 31 s
rpc_packages.py                                          | ✖ Failed  | 3 s

rkrux · 2024-06-17T20:30:51Z

test/functional/feature_framework_miniwallet.py

+        self.log.info(f"Test tagged wallet instances...")
+        node = self.nodes[0]
+        untagged_wallet = self.wallets[0][1]
+        for i in range(10):


Is the sole reason for creating 10 tagged wallets to be thorough? Or is there any other reason as well?

i.e. to test for the following assumption

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.

Is the sole reason for creating 10 tagged wallets to be thorough? Or is there any other reason as well?

Yes it's thoroughness, no other reason. Since the fixed bugs only occur with a certain probability, creating several random tagged wallets in a row increase the likelihood of catching the bugs in a single test run (nothing special about the number 10 though).

rkrux · 2024-06-18T05:36:40Z

Got it, nice.

…

On Tue, Jun 18, 2024, 06:43 Sebastian Falbesoner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/functional/feature_framework_miniwallet.py <#30076 (comment)>: > @@ -31,6 +34,20 @@ def test_tx_padding(self): assert_greater_than_or_equal(tx.get_weight(), target_weight) assert_greater_than_or_equal(target_weight + 3, tx.get_weight()) + def test_wallet_tagging(self): + """Verify that tagged wallet instances are able to send funds.""" + self.log.info(f"Test tagged wallet instances...") + node = self.nodes[0] + untagged_wallet = self.wallets[0][1] + for i in range(10): Is the sole reason for creating 10 tagged wallets to be thorough? Or is there any other reason as well? Yes it's thoroughness, no other reason. Since the fixed bugs only occur with a certain probability, creating several random tagged wallets in a row increase the likelihood of catching the bugs in a single test run (nothing special about the number 10 though). — Reply to this email directly, view it on GitHub <#30076 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABNPILUIX2ZRNELG5ECQDSLZH6CT3AVCNFSM6AAAAABHPTP73OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMRUGE2TAMBSGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

hodlinator

ACK e4b0dab

Confirmed to fix #30528 which I discovered independently. Rebased my test commit from there on top of this PR and verified that exceptions are no longer raised for the 2 cases I found.

Re-ran feature_framework_miniwallet.py without my changes on top and verified it passed. Also ran with --randomseed <X> to verify that tag names are generated deterministically.

Passed test_runner.py (other tests are using MiniWallet indirectly, and mempool_util.py specifically was already using a tag_name value that still works).

Not an expert on Taproot so haven't fully understood the context around the fixes, but they look small & safe and only touch functional test Python code.

Sorry again for not finding this PR before creating the issue. Eager to use this fix to fearlessly create multiple tagged wallets in upcoming tests. :)

DrahtBot added the Tests label May 9, 2024

rkrux approved these changes May 10, 2024

View reviewed changes

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

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

theStack force-pushed the 202405-MiniWallet-fix_script_path_spend_missing_sign_bit branch from e4abac2 to 57eb590 Compare May 11, 2024 16:45

This was referenced May 23, 2024

test: MiniWallet: respect passed feerate for padded txs (using target_weight) #30162

Merged

test: Add leaf_version parameter to taproot_tree_helper() #29371

Open

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

Closed

DrahtBot added the Needs rebase label Jun 11, 2024

theStack added 4 commits June 11, 2024 14:36

test: add functional test for tagged MiniWallet instances

e4b0dab

theStack force-pushed the 202405-MiniWallet-fix_script_path_spend_missing_sign_bit branch from 57eb590 to e4b0dab Compare June 11, 2024 16:54

DrahtBot removed the Needs rebase label Jun 11, 2024

glozow reviewed Jun 17, 2024

View reviewed changes

DrahtBot requested a review from rkrux June 17, 2024 16:28

rkrux approved these changes Jun 17, 2024

View reviewed changes

rkrux reviewed Jun 17, 2024

View reviewed changes

DrahtBot added CI failed and removed CI failed labels Jul 17, 2024

theStack mentioned this pull request Jul 26, 2024

test: MiniWallet tag_name issues #30528

Closed

1 task

hodlinator approved these changes Jul 26, 2024

View reviewed changes

glozow merged commit ec700f0 into bitcoin:master Jul 26, 2024
13 checks passed

theStack deleted the 202405-MiniWallet-fix_script_path_spend_missing_sign_bit branch July 26, 2024 10:59

bitcoin locked and limited conversation to collaborators Jul 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

theStack commented May 9, 2024 •

edited

Loading

Uh oh!

DrahtBot commented May 9, 2024 •

edited

Loading

Uh oh!

rkrux left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

theStack commented May 11, 2024

Uh oh!

rkrux commented May 12, 2024

Uh oh!

theStack commented Jun 11, 2024

Uh oh!

glozow left a comment

Uh oh!

rkrux left a comment

Uh oh!

rkrux Jun 17, 2024

Uh oh!

rkrux Jun 17, 2024 •

edited

Loading

Uh oh!

theStack Jun 18, 2024

Uh oh!

rkrux commented Jun 18, 2024 via email

Uh oh!

hodlinator left a comment

Uh oh!

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

theStack commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

rkrux left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theStack commented May 11, 2024

Uh oh!

rkrux commented May 12, 2024

Uh oh!

theStack commented Jun 11, 2024

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

rkrux Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

rkrux commented Jun 18, 2024 via email

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

theStack commented May 9, 2024 •

edited

Loading

DrahtBot commented May 9, 2024 •

edited

Loading

rkrux left a comment •

edited

Loading

rkrux Jun 17, 2024 •

edited

Loading