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

theStack · 2024-04-23T11:13:05Z

Different MiniWallet instances using the same mode (either ADDRESS_OP_TRUE, RAW_OP_TRUE or RAW_P2PK) currently always create and spend UTXOs with identical output scripts, which can cause unintentional tx dependencies (see e.g. the discussion in #29827 (comment)). In order to avoid mixing of UTXOs between instances, this PR introduces the possibility to provide a MiniWallet tag name, that is used to derive a different internal key for the taproot construction, leading to a different P2TR output script. Note that since we use script-path spending and only the key-path is changed here, no changes in the MiniWallet spending logic are needed.

The new tagging option is then used in the fill_mempool helper to create an ephemeral wallet for the filling txs, as suggested in #29827 (comment). To avoid circular dependencies, fill_mempool is moved to a new module mempool_util.py first.

I'm still not sure if a generic word like "tag" is the right term for what this tries to achieve, happy to pick up better suggestions. Also, maybe passing a tag name is overkill and a boolean flag like "random_output_script" is sufficient?

DrahtBot · 2024-04-23T11:13:08Z

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, brunoerg, achow101
Concept ACK	ismaelsadeeq

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)

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.

glozow · 2024-04-23T14:19:07Z

Nice, concept ACK

ismaelsadeeq

Concept ACK, having a separate util for mempool is nice, all mempool helpers will live there.

I'm still not sure if a generic word like "tag" is the right term for what this tries to achieve, happy to pick up better suggestions. Also, maybe passing a tag name is overkill and a boolean flag like "random_output_script" is sufficient?

I don't feel strongly on this but I like the second idea of just boolean flag.

brunoerg · 2024-04-26T15:01:11Z

Concept ACK

glozow

This solution seems really elegant! I don't think the tag is overkill and I can imagine wanting more than 2 wallets so don't think a bool is better.

test/functional/test_framework/mempool_util.py

test/functional/mempool_limit.py

DrahtBot · 2024-05-04T02:37:08Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/24434826323}

This is needed to avoid circular dependencies in later commits. Can be reviewed via `--color-moved=dimmed-zebra`.

Note that this commit doesn't change behaviour yet, as tagging isn't used in any MiniWallet instance.

theStack · 2024-05-05T10:39:38Z

Rebased on master (necessary since #28970 introduced new tests with fill_mempool call-sites) and addressed review comment #29939 (comment).

glozow · 2024-05-09T09:11:09Z

ACK dd8fa86

rkrux

tACK dd8fa86

Make successful, all functional tests pass.

Thanks for splitting the PR into multiple commits, made reviewing easy. I've asked couple questions for my understanding.

rkrux · 2024-05-09T10:07:37Z

test/functional/test_framework/mempool_util.py

-    To avoid unintentional tx dependencies, it is recommended to use separate miniwallets for
-    mempool filling vs transactions in tests.
+    To avoid unintentional tx dependencies, the mempool filling txs are created with a
+    tagged ephemeral miniwallet instance.


This moves the responsibility of avoiding unintentional tx dependencies from the caller to this function instead. Nice, moving this more towards being a pure function!

rkrux · 2024-05-09T10:11:30Z

test/functional/test_framework/mempool_util.py

+    ephemeral_miniwallet = MiniWallet(node, tag_name="fill_mempool_ephemeral_wallet")
+    test_framework.generate(ephemeral_miniwallet, 1 + num_of_batches * tx_batch_size)


For my understanding: I get that having an ephemeral wallet created and destroyed inside this function gets rid of any data sharing related to MiniWallet between the calls of fill_mempool(). Since the same tag name is being passed and subsequently the same internal_key is generated, how does adding the tag name really help here?

I wouldn't say it disambiguates between separate fill_mempool calls, just between fill_mempool and the other MiniWallets. I suppose we can add a parameter to modify the tag name further, but we don't have any functional tests where we do this multiple times.

where we do this multiple times

Do what exactly?

fill_mempool

brunoerg

utACK dd8fa86

Code lgtm and I do not see any problem with the "tag", and seems good for determinism.

…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.

achow101 · 2024-05-09T20:39:26Z

ACK dd8fa86

…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.

ismaelsadeeq

Post merge Tested ACK dd8fa86

…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.

…bit in leaf version) e4b0dab test: add functional test for tagged MiniWallet instances (Sebastian Falbesoner) 3162c91 test: fix MiniWallet internal key derivation for tagged instances (Sebastian Falbesoner) c9f7364 test: fix MiniWallet script-path spend (missing parity bit in leaf version) (Sebastian Falbesoner) 7774c31 test: refactor: return TaprootInfo from P2TR address creation routine (Sebastian Falbesoner) Pull request description: 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 diff --git a/test/functional/test_framework/mempool_util.py b/test/functional/test_framework/mempool_util.py index 148cc93..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. ACKs for top commit: glozow: ACK e4b0dab rkrux: reACK [e4b0dab](e4b0dab) hodlinator: ACK e4b0dab Tree-SHA512: a16f33f76bcb1012857cc3129438a9f6badf28aa2b1d25696da0d385ba5866b46de0f1f93ba777ed9263fe6952f98d7d9c44ea0c0170a2bcc86cbef90bf6ac58

…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.

DrahtBot added the Tests label Apr 23, 2024

ismaelsadeeq reviewed Apr 23, 2024

View reviewed changes

DrahtBot mentioned this pull request Apr 23, 2024

test: Add leaf_version parameter to taproot_tree_helper() #29371

Open

dasibcryptoidology approved these changes Apr 28, 2024

View reviewed changes

glozow reviewed Apr 30, 2024

View reviewed changes

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

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

theStack force-pushed the 202404-test-support_MiniWallet_tags branch from 5bb2cb9 to 45bf7db Compare April 30, 2024 16:18

brunoerg reviewed Apr 30, 2024

View reviewed changes

test/functional/mempool_limit.py Show resolved Hide resolved

DrahtBot added the CI failed label May 4, 2024

theStack force-pushed the 202404-test-support_MiniWallet_tags branch from 45bf7db to 176c8cc Compare May 5, 2024 10:22

theStack added 4 commits May 5, 2024 12:33

test: refactor: move fill_mempool to new module mempool_util

4f34714

This is needed to avoid circular dependencies in later commits. Can be reviewed via `--color-moved=dimmed-zebra`.

test: refactor: eliminate COINBASE_MATURITY magic number in fill_mempool

c8e6d08

test: add MiniWallet tagging support to avoid UTXO mixing

b2037ad

Note that this commit doesn't change behaviour yet, as tagging isn't used in any MiniWallet instance.

test: use tagged ephemeral MiniWallet instance in fill_mempool

dd8fa86

theStack force-pushed the 202404-test-support_MiniWallet_tags branch from 176c8cc to dd8fa86 Compare May 5, 2024 10:37

DrahtBot removed the CI failed label May 5, 2024

DrahtBot requested review from ismaelsadeeq and brunoerg May 9, 2024 09:11

rkrux approved these changes May 9, 2024

View reviewed changes

brunoerg reviewed May 9, 2024

View reviewed changes

achow101 merged commit 24572cf into bitcoin:master May 9, 2024

theStack mentioned this pull request May 9, 2024

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

Merged

ismaelsadeeq reviewed May 10, 2024

View reviewed changes

theStack deleted the 202404-test-support_MiniWallet_tags branch May 11, 2024 06:04

instagibbs mentioned this pull request May 12, 2024

Cluster size 2 package rbf #28984

Merged

hodlinator mentioned this pull request Jul 26, 2024

test: MiniWallet tag_name issues #30528

Closed

1 task

bitcoin locked and limited conversation to collaborators May 11, 2025

		ephemeral_miniwallet = MiniWallet(node, tag_name="fill_mempool_ephemeral_wallet")
		test_framework.generate(ephemeral_miniwallet, 1 + num_of_batches * tx_batch_size)

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

glozow commented Apr 23, 2024

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Apr 26, 2024

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented May 4, 2024

Uh oh!

theStack commented May 5, 2024

Uh oh!

glozow commented May 9, 2024

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux May 9, 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!

rkrux May 9, 2024

Choose a reason for hiding this comment

Uh oh!

glozow May 9, 2024

Choose a reason for hiding this comment

Uh oh!

rkrux May 9, 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!

glozow May 9, 2024

Choose a reason for hiding this comment

Uh oh!

rkrux May 9, 2024

Choose a reason for hiding this comment

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented May 9, 2024

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

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

theStack commented Apr 23, 2024 •

edited

Loading

DrahtBot commented Apr 23, 2024 •

edited

Loading

rkrux May 9, 2024 •

edited

Loading

rkrux May 9, 2024 •

edited

Loading