test: add coverage for bech32m in `wallet_keypool_topup` #28928

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2023-11-test-keypool-bech32m changing 1 files +15 −9
  1. brunoerg commented at 5:29 PM on November 22, 2023: contributor

    0dcac51049cdd924a50d62629757effc8d542046 added coverage for all keypool addresses types in wallet_keypool_topup (4y ago). Now we have bech23m, so this PR adds it.

  2. DrahtBot commented at 5:30 PM on November 22, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK marcofleon, furszy, achow101
    Stale ACK davidgumberg

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on Nov 22, 2023
  4. furszy commented at 7:01 PM on November 22, 2023: member

    we should probably rewrite this test using only one node, and multiple wallets, instead of five different nodes.

  5. brunoerg commented at 11:49 PM on November 22, 2023: contributor

    we should probably rewrite this test using only one node, and multiple wallets, instead of five different nodes.

    sgtm, perhaps 2 nodes? why only one? Also, I have no problem in addressing it here, but perhaps we could leave it for a follow-up/other PR?

  6. furszy commented at 12:43 PM on November 23, 2023: member

    we should probably rewrite this test using only one node, and multiple wallets, instead of five different nodes.

    sgtm, perhaps 2 nodes? why only one?

    The test is solely verifying the wallet key pool (re)generation. This can be tested by loading and unloading wallets instead of restarting nodes (this was a restriction from the past, prev to introducing multi-wallets support). No extra nodes nor restart is needed.

    Also, I have no problem in addressing it here, but perhaps we could leave it for a follow-up/other PR?

    I just saw the old test and threw the idea out :). Either way is fine for me.

  7. brunoerg force-pushed on Nov 29, 2023
  8. brunoerg commented at 12:57 PM on November 30, 2023: contributor

    This can be tested by loading and unloading wallets instead of restarting nodes (this was a restriction from the past, prev to introducing multi-wallets support). No extra nodes nor restart is needed.

    Just addressed it here

  9. furszy commented at 1:29 PM on November 30, 2023: member

    how much faster the test got? Some numbers would encourage more reviewers to come here.

  10. brunoerg commented at 9:37 PM on December 2, 2023: contributor

    Good idea, @furszy.

    master: 1.78s user 0.57s system 28% cpu 8.157 total

    this PR: 0.76s user 0.20s system 35% cpu 2.668 total

  11. DrahtBot added the label CI failed on Dec 21, 2023
  12. DrahtBot removed the label CI failed on Dec 26, 2023
  13. DrahtBot added the label CI failed on Jan 16, 2024
  14. brunoerg force-pushed on Jan 22, 2024
  15. brunoerg commented at 4:55 PM on January 22, 2024: contributor

    Rebased

  16. DrahtBot removed the label CI failed on Jan 22, 2024
  17. fanquake requested review from achow101 on Mar 6, 2024
  18. in test/functional/wallet_keypool_topup.py:28 in 656dddb47e outdated 
      28 |      def run_test(self):
  29 | -        wallet_path = self.nodes[1].wallets_path / self.default_wallet_name / self.wallet_data_filename
  30 | -        wallet_backup_path = self.nodes[1].datadir_path / "wallet.bak"
  31 | -        self.generate(self.nodes[0], COINBASE_MATURITY + 1)
  32 | +        node = self.nodes[0]
  33 | +        self.generate(node, COINBASE_MATURITY + 10)
    davidgumberg commented at 5:26 AM on March 8, 2024:

    Nit: Why did this change to COINBASE_MATURITY + 10

    brunoerg commented at 5:59 PM on March 9, 2024:

    Good point. Probably unnecessary, it's a leftover from prev changes.

  19. in test/functional/wallet_keypool_topup.py:56 in 656dddb47e outdated 
      77 |              self.log.info("Send funds to wallet")
  78 | -            self.nodes[0].sendtoaddress(addr_oldpool, 10)
  79 | -            self.generate(self.nodes[0], 1)
  80 | -            self.nodes[0].sendtoaddress(addr_extpool, 5)
  81 | -            self.generate(self.nodes[0], 1)
  82 | +            default_wallet = node.get_wallet_rpc(self.default_wallet_name)
    furszy commented at 12:38 PM on March 9, 2024:

    Could place this line out of the loop.

    brunoerg commented at 5:58 PM on March 9, 2024:

    done

  20. in test/functional/wallet_keypool_topup.py:58 in 656dddb47e outdated 
      79 | -            self.generate(self.nodes[0], 1)
  80 | -            self.nodes[0].sendtoaddress(addr_extpool, 5)
  81 | -            self.generate(self.nodes[0], 1)
  82 | +            default_wallet = node.get_wallet_rpc(self.default_wallet_name)
  83 | +            default_wallet.sendtoaddress(addr_oldpool, 10)
  84 | +            default_wallet.sendtoaddress(addr_extpool, 5)
    furszy commented at 12:39 PM on March 9, 2024:

    nit: Could unify this two lines. default_wallet.send(outputs=[{addr_oldpool:10}, {addr_extpool:5}])

    brunoerg commented at 5:58 PM on March 9, 2024:

    done

  21. in test/functional/wallet_keypool_topup.py:63 in 656dddb47e outdated 
      90 | -            self.start_node(idx, self.extra_args[idx])
  91 | -            self.connect_nodes(0, idx)
  92 | -            self.sync_all()
  93 | +            node.unloadwallet(wallet_name)
  94 | +            node.loadwallet(wallet_name)
  95 | +            wallet = node.get_wallet_rpc(str(output_type))
    furszy commented at 12:40 PM on March 9, 2024:

    Could replace str(output_type) for wallet_name

    brunoerg commented at 5:58 PM on March 9, 2024:

    done

  22. furszy commented at 12:44 PM on March 9, 2024: member

    Code reviewed 656dddb47e. Looks good. Left few nits, nothing blocking.

    If you change the commits order, the bech32m commit will be shorter.

  23. brunoerg force-pushed on Mar 9, 2024
  24. brunoerg commented at 6:11 PM on March 9, 2024: contributor

    Force-pushed addressing: #28928 (review) #28928 (review) #28928 (review) #28928 (review)

  25. in test/functional/wallet_keypool_topup.py:28 in 6755e60fff outdated 
      39 | -        self.connect_nodes(0, 3)
  40 | -
  41 | -        for i, output_type in enumerate(["legacy", "p2sh-segwit", "bech32"]):
  42 | -
  43 | +        node = self.nodes[0]
  44 | +        self.generate(node, COINBASE_MATURITY + 1)
    furszy commented at 9:01 PM on March 9, 2024:

    Can remove this line by dropping the setup_clean_chain set line:

    diff --git a/test/functional/wallet_keypool_topup.py b/test/functional/wallet_keypool_topup.py
--- a/test/functional/wallet_keypool_topup.py	(revision 6755e60fffd7748e7cbc16c9b18d84e62f26c697)
+++ b/test/functional/wallet_keypool_topup.py	(date 1710017695860)
@@ -16,7 +16,6 @@
         self.add_wallet_options(parser)
 
     def set_test_params(self):
-        self.setup_clean_chain = True
         self.num_nodes = 1
         self.extra_args = [['-keypool=100']]
 
@@ -25,7 +24,6 @@
 
     def run_test(self):
         node = self.nodes[0]
-        self.generate(node, COINBASE_MATURITY + 1)
 
         output_types = ["legacy", "p2sh-segwit", "bech32"]
         if self.options.descriptors:
    brunoerg commented at 10:07 PM on March 9, 2024:

    Done

  26. furszy commented at 9:04 PM on March 9, 2024: member

    utACK 6755e60fff

  27. brunoerg force-pushed on Mar 9, 2024
  28. brunoerg commented at 10:07 PM on March 9, 2024: contributor

    Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/28928/files#r1518662935 and changed the order of the commits to simplify the changes. Thanks, @furszy.

  29. DrahtBot added the label CI failed on Mar 9, 2024
  30. DrahtBot commented at 11:13 PM on March 9, 2024: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <sub>Debug: https://github.com/bitcoin/bitcoin/runs/22473234262</sub>

  31. brunoerg force-pushed on Mar 9, 2024
  32. DrahtBot removed the label CI failed on Mar 10, 2024
  33. brunoerg commented at 9:36 AM on March 10, 2024: contributor

    CI failure is unrelated.

  34. furszy commented at 1:31 PM on March 10, 2024: member

    ACK 5a2ca3672

  36. marcofleon commented at 9:40 PM on March 11, 2024: contributor

    ACK 5a2ca3672c3965fc4f70dc31e042feb044ab2bdf. Built with Berkeley DB enabled and ran all functional tests. No issues were observed. I introduced some changes into the derivation path check at the end for bech32m to see if the test would properly fail and it did.

    Before these changes:

    5.38s user 0.57s system 47% cpu 12.593 total

    After:

    1.43s user 0.12s system 85% cpu 1.821 total
  37. fanquake assigned achow101 on Mar 12, 2024
  38. in test/functional/wallet_keypool_topup.py:54 in bb54a7ad0c outdated 
      83 | -            self.start_node(idx, self.extra_args[idx])
  84 | -            self.connect_nodes(0, idx)
  85 | -            self.sync_all()
  86 | +            node.unloadwallet(wallet_name)
  87 | +            node.loadwallet(wallet_name)
  88 | +            wallet = node.get_wallet_rpc(wallet_name)
    achow101 commented at 2:04 PM on March 12, 2024:

    In bb54a7ad0cf22686e3f6735213bac1dd4453cb15 "test: use multiple wallets instead of nodes in wallet_keypool_topup"

    This is incorrect and removing the thing that the test is actually testing for.

    The purpose of the test is to ensure that receiving to something in the keypool will cause everything up to that address being removed from the keypool, that the keypool is subsequently topped up, and that an address from past the end of the initial keypool will also be detected after the first topup, and trigger it's own topup. That's why this has a whole thing with creating and restoring a backup - so that there is a wallet with an untouched keypool to do the test on.

    Simply unloading and reloading the wallet does not test that the keypool gets topped up correctly when it discovers stuff in the keypool.

    furszy commented at 2:49 AM on March 18, 2024:

    It needs some more love but I just re-wrote the first commit correcting achow's points: https://github.com/furszy/bitcoin-core/commit/738860039d1f2e2038e64b7d8d0cd23f3e6b88a5.

    Got a similar speedup for the descriptors wallet:

    Before:
./wallet_keypool_topup.py 5.63s user 0.57s system 56% cpu 10.945 total

After:
./wallet_keypool_topup.py 1.69s user 0.19s system 72% cpu 2.605 total

    But got a slowdown for the legacy wallet:

    Before:
./wallet_keypool_topup.py --legacy-wallet  9.52s user 2.07s system 32% cpu 35.514 total

After:
./wallet_keypool_topup.py --legacy-wallet  5.03s user 2.15s system 16% cpu 44.458 total

    @brunoerg, if you like it, cherry-pick it and see if you can clean it further. I did not spend much time on it.

    brunoerg commented at 3:06 PM on March 18, 2024:

    Thanks, @furszy. I'm checking it now.

    brunoerg commented at 2:58 PM on March 20, 2024:

    @furszy Do you want to address it on a follow-up?

    furszy commented at 2:59 PM on March 20, 2024:

    @furszy Do you want to address it on a follow-up?

    Sure.

  39. in test/functional/wallet_keypool_topup.py:50 in bb54a7ad0c outdated 
      73 | -            self.nodes[0].sendtoaddress(addr_oldpool, 10)
  74 | -            self.generate(self.nodes[0], 1)
  75 | -            self.nodes[0].sendtoaddress(addr_extpool, 5)
  76 | -            self.generate(self.nodes[0], 1)
  77 | +            default_wallet.send(outputs=[{addr_oldpool:10}, {addr_extpool:5}])
  78 | +            self.generate(node, 1)
    achow101 commented at 2:07 PM on March 12, 2024:

    In bb54a7ad0cf22686e3f6735213bac1dd4453cb15 "test: use multiple wallets instead of nodes in wallet_keypool_topup"

    The original method with separate transactions in separate blocks was the intended and correct behavior for this test. It needs to be that way so that the address with the lower index is found first to trigger the topup that will result in the second address being found. Since send shuffles the output order, we may get the second address before the first address which would result in an intermittent failure. Furthermore, even just separate transactions is not enough since the transactions could be in the incorrect order. Hence it needs to be separate transactions in separate blocks.

    furszy commented at 2:54 AM on March 18, 2024:

    That needs to be mentioned in the test case.

  40. achow101 commented at 2:08 PM on March 12, 2024: member

    Approach NACK. The test changes make the test no longer check what it is supposed to be checking for.

  41. test: add coverage for bech32m in `wallet_keypool_topup` a8bfc3dea1
  42. brunoerg force-pushed on Mar 20, 2024
  43. brunoerg commented at 2:57 PM on March 20, 2024: contributor

    Force-pushed removing the commit with the speed-up due to @achow101's review (see #28928 (review)). I left the coverage addition for bech32 which will be simpler to review.

  44. marcofleon approved
  45. marcofleon commented at 3:04 AM on March 25, 2024: contributor

    ACK a8bfc3dea1d986b458202bf5e49cf1944392d676. Definitely a more straightfoward addition to the test. Reviewed the code, built the PR branch and ran all functional tests without issues.

    Also ran the test individually as is and after making some simple changes to the assert_equal lines to test for correct failure.

  46. DrahtBot requested review from achow101 on Mar 25, 2024
  47. DrahtBot requested review from furszy on Mar 25, 2024
  48. DrahtBot requested review from davidgumberg on Mar 25, 2024
  49. in test/functional/wallet_keypool_topup.py:45 in a8bfc3dea1 
      41 | @@ -40,12 +42,13 @@ def run_test(self):
  42 |          self.stop_node(1)
  43 |          shutil.copyfile(wallet_path, wallet_backup_path)
  44 |          self.start_node(1, self.extra_args[1])
  45 | -        self.connect_nodes(0, 1)
  46 | -        self.connect_nodes(0, 2)
  47 | -        self.connect_nodes(0, 3)
  48 | -
  49 | -        for i, output_type in enumerate(["legacy", "p2sh-segwit", "bech32"]):
  50 | +        for i in [1, 2, 3, 4]:
    furszy commented at 9:04 PM on March 25, 2024: 
    for i in range(1, 4):
  50. furszy commented at 9:05 PM on March 25, 2024: member

    utACK a8bfc3dea

  51. achow101 commented at 9:42 PM on March 25, 2024: member

    ACK a8bfc3dea1d986b458202bf5e49cf1944392d676

  52. achow101 merged this on Mar 25, 2024
  53. achow101 closed this on Mar 25, 2024
  54. bitcoin locked this on Mar 25, 2025
Contributors
brunoergDrahtBotfurszydavidgumbergmarcofleonachow101


davidgumbergachow101

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: 2026-05-02 03:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me