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

pull theStack wants to merge 4 commits into bitcoin:master from theStack:202404-test-support_MiniWallet_tags changing 9 files +117 −79
  1. theStack commented at 11:13 am on April 23, 2024: contributor

    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 (review)). 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 (review). 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?

  2. DrahtBot commented at 11:13 am on April 23, 2024: contributor

    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.

  3. DrahtBot added the label Tests on Apr 23, 2024
  4. glozow commented at 2:19 pm on April 23, 2024: member
    Nice, concept ACK
  5. ismaelsadeeq commented at 4:56 pm on April 23, 2024: member

    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.

  6. brunoerg commented at 3:01 pm on April 26, 2024: contributor
    Concept ACK
  7. dasibcryptoidology approved
  8. in test/functional/test_framework/mempool_util.py:32 in 901ffa9e6e outdated 
    27@@ -25,8 +28,8 @@ def fill_mempool(test_framework, node, miniwallet):
28     It will not ensure mempools become synced as it
29     is based on a single node and assumes -minrelaytxfee
30     is 1 sat/vbyte.
31-    To avoid unintentional tx dependencies, it is recommended to use separate miniwallets for
32-    mempool filling vs transactions in tests.
33+    To avoid unintentional tx dependencies, the mempool filling txs are created with a
34+    separate tagged ephermal miniwallet instance.
    glozow commented at 9:23 am on April 30, 2024:
    901ffa9e6e57f7c0e0db967fba9c715139bbcf0b nit: did you mean ephemeral? (also in commit message)
    theStack commented at 4:19 pm on April 30, 2024:
    Ugh indeed, apparently I’ve used a word for several years without noticing that it doesn’t exist (mostly in “ephermal port”) :D fixed.
  9. in test/functional/test_framework/mempool_util.py:54 in 901ffa9e6e outdated 
    57-    assert_equal(len(confirmed_utxos), num_of_batches * tx_batch_size + 1)
58-
59     test_framework.log.debug("Create a mempool tx that will be evicted")
60-    tx_to_be_evicted_id = miniwallet.send_self_transfer(from_node=node, utxo_to_spend=confirmed_utxos[0], fee_rate=relayfee)["txid"]
61-    del confirmed_utxos[0]
62+    tx_to_be_evicted_id = miniwallet.send_self_transfer(from_node=node, fee_rate=relayfee)["txid"]
    glozow commented at 9:26 am on April 30, 2024:
    901ffa9e6e57f7c0e0db967fba9c715139bbcf0b shouldn’t this be sent from the ephemeral miniwallet?
    glozow commented at 9:27 am on April 30, 2024:
    Also I was imagining that, with the ephemeral miniwallet, we can just delete the miniwallet param
    theStack commented at 4:19 pm on April 30, 2024:

    901ffa9 shouldn’t this be sent from the ephemeral miniwallet?

    If the to-be-evicted tx and the filling txs originate from the same miniwallet instance, the problem fixed by e9dc511a7e9a562f953ff93f358102f555f583e6 is unfortunately re-introduced, as it could happen again that a filler tx spends from the to-be-evicted tx, increasing its’ effective fee-rate and thus avoiding the expected eviction (making the assertion assert tx_to_be_evicted_id not in node.getrawmempool() fail; this happens at least for the mempool_limit.py functional test) :/

    Also I was imagining that, with the ephemeral miniwallet, we can just delete the miniwallet param

    Agree that it would be nice to get completely rid of the miniwallet param. We could use two ephemeral MiniWallet instances though (one for the to-be-evicted test tx, one for the filling txs), happy to change if that is preferred.

    glozow commented at 4:47 pm on May 2, 2024:
    Could also continue to get all the UTXOs up front?
    theStack commented at 10:22 am on May 5, 2024:

    Could also continue to get all the UTXOs up front?

    Indeed. I thought it was a nice side-benefit of the ephemeral wallet if the manual coin selection wouldn’t be needed anymore, but keeping it seems better than the alternatives. Thanks, done.

  10. glozow commented at 9:28 am on April 30, 2024: member
    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.
  11. theStack force-pushed on Apr 30, 2024
  12. in test/functional/mempool_limit.py:10 in 45bf7dba02 outdated 
     5@@ -6,14 +6,16 @@
 6 
 7 from decimal import Decimal
 8 
 9+from test_framework.mempool_util import (
10+    fill_mempool,
    brunoerg commented at 4:41 pm on April 30, 2024:

    Not 100% related to these change, butfill_mempool requires -datacarriersize=100000 and -maxmempool=5 and in rpc_packages test, this is mentioned:

    0# Make chain of two transactions where parent doesn't make minfee threshold
1# but child is too high fee
2# Lower mempool limit to make it easier to fill_mempool
3self.restart_node(0, extra_args=[
4    "-datacarriersize=100000",
5    "-maxmempool=5",
6    "-persistmempool=0",
7])

    Perhaps, it would worth to also mention it in mempool_limit?

    theStack commented at 10:23 am on May 5, 2024:
    Good point. I’m planning a follow-up where these prerequisites (-datacarriersize=100000 and -maxmempool=5) are checked at run-time in fill_mempool, in order to give explicit error messages if fill_mempool users forget to set these parameters. Will include a commit with your suggestion to add comments in functional tests where this is missing (seems to be both in rpc_packages.py and p2p_tx_download.py).
  13. DrahtBot added the label CI failed on May 4, 2024
  14. DrahtBot commented at 2:37 am on May 4, 2024: contributor

    🚧 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

  15. theStack force-pushed on May 5, 2024
  16. test: refactor: move fill_mempool to new module mempool_util 
    This is needed to avoid circular dependencies in later commits.
Can be reviewed via `--color-moved=dimmed-zebra`.
    4f347140b1
  17. test: refactor: eliminate COINBASE_MATURITY magic number in fill_mempool c8e6d08236
  18. test: add MiniWallet tagging support to avoid UTXO mixing 
    Note that this commit doesn't change behaviour yet, as tagging isn't
used in any MiniWallet instance.
    b2037ad4ae
  19. test: use tagged ephemeral MiniWallet instance in fill_mempool dd8fa86193
  20. theStack force-pushed on May 5, 2024
  21. theStack commented at 10:39 am on May 5, 2024: contributor
    Rebased on master (necessary since #28970 introduced new tests with fill_mempool call-sites) and addressed review comment #29939 (review).
  22. DrahtBot removed the label CI failed on May 5, 2024
  23. glozow commented at 9:11 am on May 9, 2024: member
    ACK dd8fa861939
  24. DrahtBot requested review from ismaelsadeeq on May 9, 2024
  25. DrahtBot requested review from brunoerg on May 9, 2024
  26. in test/functional/test_framework/mempool_util.py:32 in dd8fa86193 
    27@@ -25,8 +28,8 @@ def fill_mempool(test_framework, node, miniwallet):
28     It will not ensure mempools become synced as it
29     is based on a single node and assumes -minrelaytxfee
30     is 1 sat/vbyte.
31-    To avoid unintentional tx dependencies, it is recommended to use separate miniwallets for
32-    mempool filling vs transactions in tests.
33+    To avoid unintentional tx dependencies, the mempool filling txs are created with a
34+    tagged ephemeral miniwallet instance.
    rkrux commented at 10:07 am on May 9, 2024:
    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!
  27. in test/functional/test_framework/mempool_util.py:46 in dd8fa86193 
    41@@ -39,19 +42,20 @@ def fill_mempool(test_framework, node, miniwallet):
42     # Generate UTXOs to flood the mempool
43     # 1 to create a tx initially that will be evicted from the mempool later
44     # 75 transactions each with a fee rate higher than the previous one
45-    test_framework.generate(miniwallet, 1 + (num_of_batches * tx_batch_size))
46+    ephemeral_miniwallet = MiniWallet(node, tag_name="fill_mempool_ephemeral_wallet")
47+    test_framework.generate(ephemeral_miniwallet, 1 + num_of_batches * tx_batch_size)
    rkrux commented at 10:11 am on May 9, 2024:
    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?
    glozow commented at 10:30 am on May 9, 2024:
    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.
    rkrux commented at 11:40 am on May 9, 2024:

    where we do this multiple times

    Do what exactly?

    glozow commented at 11:42 am on May 9, 2024:
    fill_mempool
    rkrux commented at 12:05 pm on May 9, 2024:
    Oh I see.
  28. rkrux approved
  29. rkrux commented at 10:12 am on May 9, 2024: none

    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.

  30. brunoerg commented at 1:53 pm on May 9, 2024: contributor

    utACK dd8fa861939d5b8bdd844ad7cab015d08533a91a

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

  31. theStack referenced this in commit e529e928ff on May 9, 2024
  32. achow101 commented at 8:39 pm on May 9, 2024: member
    ACK dd8fa861939d5b8bdd844ad7cab015d08533a91a
  33. achow101 merged this on May 9, 2024
  34. achow101 closed this on May 9, 2024
  35. theStack referenced this in commit e4abac2fce on May 9, 2024
  36. ismaelsadeeq commented at 11:58 am on May 10, 2024: member
    Post merge Tested ACK dd8fa861939d5b8bdd844ad7cab015d08533a91a
  37. theStack deleted the branch on May 11, 2024


theStack DrahtBot glozow ismaelsadeeq brunoerg dasibcryptoidology rkrux achow101

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me