tests: Run both descriptor and legacy tests within a single test invocation #20892

pull achow101 wants to merge 11 commits into bitcoin:master from achow101:better-descriptor-tests changing 48 files +305 −275
  1. achow101 commented at 3:50 AM on January 9, 2021: member

    Instead of having the tests only do one type or the other, depending on command line arguments, these tests should be running with both types of wallets. Any test that calls add_wallet_options will be run for each wallet type that it specifies a wallet option for if none are give in the command line. If a particular wallet is specified with --legacy-wallet or --descriptors, the options are still respected.

  2. fanquake added the label Tests on Jan 9, 2021
  3. DrahtBot commented at 10:34 AM on January 9, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK promag, josibake
    Stale ACK rajarshimaitra, aureleoules

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28528 (test: Use test framework utils in functional tests by osagie98)
    • #28454 (wallet: allowing walletpassphrase timeout to be MAX_SLEEP_TIME if set to -1 by kevkevinpal)
    • #28403 (test: Bump walletpassphrase timeouts to avoid intermittent issues by MarcoFalke)
    • #28392 (test: Use pathlib over os path by ns-xvrn)
    • #28142 (wallet: Allow users to create a wallet that encrypts all database records by achow101)
    • #28027 (test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets by achow101)
    • #27231 (Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs by jonatack)
    • #27216 (wallet: Add wallet method to detect if a key is "active" by pinheadmz)
    • #27034 (rpc: make importaddress compatible with descriptors wallet by furszy)
    • #25038 (policy: nVersion=3 and Package RBF by glozow)

    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.

  4. promag commented at 3:37 PM on January 11, 2021: member

    Concept ACK.

  5. achow101 force-pushed on Jan 12, 2021
  6. DrahtBot commented at 11:45 AM on January 13, 2021: contributor

    <!--4a62be1de6b64f3ed646cdc7932c8cf5-->

    🕵️ @sipa @jonatack have been requested to review this pull request as specified in the REVIEWERS file.

  7. DrahtBot added the label Needs rebase on Jan 19, 2021
  8. achow101 force-pushed on Jan 26, 2021
  9. DrahtBot removed the label Needs rebase on Jan 26, 2021
  10. DrahtBot added the label Needs rebase on Jan 28, 2021
  11. achow101 force-pushed on Jan 28, 2021
  12. achow101 force-pushed on Jan 28, 2021
  13. DrahtBot removed the label Needs rebase on Jan 28, 2021
  14. DrahtBot added the label Needs rebase on Feb 5, 2021
  15. achow101 force-pushed on Feb 5, 2021
  16. DrahtBot removed the label Needs rebase on Feb 5, 2021
  17. S3RK commented at 9:01 AM on February 19, 2021: contributor

    -0 on this PR.

    I tend to agree with ryanofsky's arguments here. It's better to have explicit invocations in test_runner.py and faster tests, because it's easier to understand and debug.

  18. DrahtBot added the label Needs rebase on Feb 25, 2021
  19. achow101 force-pushed on Mar 3, 2021
  20. achow101 commented at 7:29 PM on March 3, 2021: member

    Rebased and removed the changes to test_runner.py. The test runner will still run with --legacy-wallet and --descriptors. For manual invocations without either option, the updated tests will run twice, once for each type of wallet.

  21. DrahtBot removed the label Needs rebase on Mar 3, 2021
  22. DrahtBot added the label Needs rebase on May 10, 2021
  23. achow101 force-pushed on May 10, 2021
  24. DrahtBot removed the label Needs rebase on May 11, 2021
  25. DrahtBot added the label Needs rebase on May 31, 2021
  26. achow101 force-pushed on Jun 1, 2021
  27. DrahtBot removed the label Needs rebase on Jun 1, 2021
  28. DrahtBot added the label Needs rebase on Jun 5, 2021
  29. achow101 force-pushed on Jun 7, 2021
  30. DrahtBot removed the label Needs rebase on Jun 7, 2021
  31. DrahtBot added the label Needs rebase on Jun 24, 2021
  32. achow101 force-pushed on Jul 1, 2021
  33. DrahtBot removed the label Needs rebase on Jul 1, 2021
  34. DrahtBot added the label Needs rebase on Aug 26, 2021
  35. achow101 force-pushed on Sep 1, 2021
  36. DrahtBot removed the label Needs rebase on Sep 1, 2021
  37. achow101 force-pushed on Sep 29, 2021
  38. DrahtBot added the label Needs rebase on Oct 4, 2021
  39. achow101 force-pushed on Oct 4, 2021
  40. achow101 force-pushed on Oct 4, 2021
  41. DrahtBot removed the label Needs rebase on Oct 4, 2021
  42. DrahtBot added the label Needs rebase on Oct 4, 2021
  43. achow101 force-pushed on Oct 4, 2021
  44. DrahtBot removed the label Needs rebase on Oct 5, 2021
  45. achow101 force-pushed on Oct 8, 2021
  46. DrahtBot added the label Needs rebase on Nov 17, 2021
  47. achow101 force-pushed on Nov 18, 2021
  48. DrahtBot removed the label Needs rebase on Nov 18, 2021
  49. DrahtBot added the label Needs rebase on Dec 7, 2021
  50. achow101 force-pushed on Dec 8, 2021
  51. DrahtBot removed the label Needs rebase on Dec 8, 2021
  52. rajarshimaitra commented at 7:00 PM on January 3, 2022: contributor

    tACK https://github.com/bitcoin/bitcoin/pull/20892/commits/0fb11a1ac675c0c8e86d08168e78f6c365e2f559

    Verified that the tests are considering both legacy and descriptors. --legacy-wallet and --descriptor flags are also working as expected.

    I found that the following wallet tests doesn't include the new BitcoinWalletTestFramework, but they are similar in sense of other tests and adding the new framework runs both the tests, unlike wallet_importmulti.py which is a "legacy only" test. Is there any specific reason for ommiting them?

    • wallet_listtransactions.py
    • wallet_orphanedreward.py
    • wallet_reorgrestore.py
    • wallet_send.py
    • wallet_signer.py
    • wallet_signmessagewithaddress.py
    • wallet_startup.py
    • wallet_taproot.py
    • wallet_transactiontime_rescan.py
    • wallet_txn_clone.py
    • wallet_upgradewallet.py
  53. achow101 force-pushed on Jan 10, 2022
  54. achow101 commented at 7:57 PM on January 10, 2022: member

    I've updated those tests to use BitcoinWalletTestFramework except for wallet_signer.py, wallet_taproot.py, and wallet_upgradewallet.py. The first two test features that are descriptor wallet only, and the last one is legacy wallet only.

  55. achow101 force-pushed on Jan 10, 2022
  56. DrahtBot added the label Needs rebase on Jan 25, 2022
  57. achow101 force-pushed on Jan 26, 2022
  58. DrahtBot removed the label Needs rebase on Jan 26, 2022
  59. DrahtBot added the label Needs rebase on Mar 15, 2022
  60. aureleoules commented at 2:37 AM on March 16, 2022: member

    tACK 0b88af678aa8bab9d6980f41cff4246ed8308604 (test/functional/test_runner.py). Tested on NixOS 22.05 64 bits.

    I executed wallet-related tests with --descriptors only and --legacy only, they behaved as expected.
    Running the test with no flags run both types of test as expected. I verified that a skipped test does not skip the other test and that a failed test does not run the other.

    It would have been nice to run tests with both flags (--descriptors --legacy) but it looks like it requires a lot of refactor that I believe is not worth the change.

  61. achow101 force-pushed on Mar 24, 2022
  62. DrahtBot removed the label Needs rebase on Mar 24, 2022
  63. DrahtBot added the label Needs rebase on Apr 14, 2022
  64. achow101 force-pushed on Apr 18, 2022
  65. DrahtBot removed the label Needs rebase on Apr 18, 2022
  66. DrahtBot added the label Needs rebase on Apr 25, 2022
  67. achow101 force-pushed on Apr 25, 2022
  68. DrahtBot removed the label Needs rebase on Apr 25, 2022
  69. DrahtBot added the label Needs rebase on Jun 16, 2022
  70. achow101 force-pushed on Jun 20, 2022
  71. DrahtBot removed the label Needs rebase on Jun 20, 2022
  72. DrahtBot added the label Needs rebase on Jul 11, 2022
  73. achow101 force-pushed on Jul 12, 2022
  74. DrahtBot removed the label Needs rebase on Jul 12, 2022
  75. achow101 force-pushed on Jul 12, 2022
  76. DrahtBot added the label Needs rebase on Jul 13, 2022
  77. achow101 force-pushed on Jul 13, 2022
  78. DrahtBot removed the label Needs rebase on Jul 13, 2022
  79. DrahtBot added the label Needs rebase on Jul 20, 2022
  80. achow101 force-pushed on Jul 21, 2022
  81. DrahtBot removed the label Needs rebase on Jul 21, 2022
  82. achow101 force-pushed on Jul 22, 2022
  83. achow101 force-pushed on Jul 26, 2022
  84. achow101 force-pushed on Jul 29, 2022
  85. achow101 force-pushed on Aug 1, 2022
  86. DrahtBot added the label Needs rebase on Aug 16, 2022
  87. achow101 force-pushed on Aug 17, 2022
  88. DrahtBot removed the label Needs rebase on Aug 17, 2022
  89. DrahtBot added the label Needs rebase on Sep 5, 2022
  90. achow101 force-pushed on Sep 6, 2022
  91. DrahtBot removed the label Needs rebase on Sep 6, 2022
  92. glozow requested review from maflcko on Oct 12, 2022
  93. maflcko commented at 9:21 AM on October 13, 2022: member

    Not sure how useful this refactor is. As bdb will be removed, this is only a temporary patch that will be reverted later.

    If this is not a refactor, it might be good to explain the goal, so that alternative solutions can be considered.

    Moreover, the CI will already run a set of tests with sqlite-only and bdb-only. (Test failures due to missing build module checks seem more frequent than wallet related test failures that are not caught with a simple call to test_runner.py on current master)

  94. achow101 commented at 2:18 PM on October 13, 2022: member

    Not sure how useful this refactor is. As bdb will be removed, this is only a temporary patch that will be reverted later.

    If this is not a refactor, it might be good to explain the goal, so that alternative solutions can be considered.

    Moreover, the CI will already run a set of tests with sqlite-only and bdb-only. (Test failures due to missing build module checks seem more frequent than wallet related test failures that are not caught with a simple call to test_runner.py on current master)

    The goal is not to refactor (that is never an outright goal for me) but rather to make it so that running a wallet test by itself (e.g. test/functional/rpc_fundrawtransaction.py) will run it with both --legacy-wallet and --descriptors. This is somewhat important for me because I have often forgotten to add one of those flags and accidentally thought that the tests were passing when in fact the relevant ones were not. While CI does do both and catches these failures, I think it is better to make it easier for developers to run such tests in both modes to catch errors before they are pushed to the PR.

    Since test_runner.py takes a long time to run, not everyone will always run it and may just run the specific test that involves what they are working on.

  95. maflcko commented at 2:50 PM on October 13, 2022: member

    Ok, I just don't like making this a new class, which seems to conflict with the command line options.

    My idea is to specify a wallet test in set_test_params and then provide the --descriptors and --legacy args in the parsers (a nice side effect would be to not provide them for tests that have nothing to do with the wallet).

    If set_test_params was moved before the arg parsing, any stuff that needs args would be moved out of set_test_params. This way there wouldn't be several different ways to make a test "wallet-y" (class vs command line options).

    Then, if no option is provided in the parser, run both legacy and descriptor wallets.

    Maybe even assert in the test runner that for wallet tests exactly one option is given? This would be another nice side effect to enforce consistency at CI time.

  96. achow101 commented at 3:04 PM on October 17, 2022: member

    My idea is to specify a wallet test in set_test_params and then provide the --descriptors and --legacy args in the parsers (a nice side effect would be to not provide them for tests that have nothing to do with the wallet).

    I'm open to reviewing alternatives.

  97. DrahtBot added the label Needs rebase on Oct 20, 2022
  98. achow101 force-pushed on Oct 20, 2022
  99. DrahtBot removed the label Needs rebase on Oct 20, 2022
  100. achow101 force-pushed on Oct 21, 2022
  101. achow101 force-pushed on Oct 21, 2022
  102. maflcko commented at 3:39 PM on November 10, 2022: member

    Sure, see #26480. I think (untested draft), you can drop the first two commits and replace them with a patch along the lines of:

    diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index d88cccecd9..61b5dfc7e9 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -100,6 +100,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         self.network_thread = None
         self.rpc_timeout = 60  # Wait for up to 60 seconds for the RPC server to respond
         self.supports_cli = True
+        self._runs_wallet_type = [None]
         self.bind_to_localhost_only = True
         self.parse_args()
         self.disable_syscall_sandbox = self.options.nosandbox or self.options.valgrind
@@ -128,33 +129,48 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 
         assert hasattr(self, "num_nodes"), "Test must set self.num_nodes in set_test_params()"
 
-        try:
-            self.setup()
-            self.run_test()
-        except JSONRPCException:
-            self.log.exception("JSONRPC error")
-            self.success = TestStatus.FAILED
-        except SkipTest as e:
-            self.log.warning("Test Skipped: %s" % e.message)
-            self.success = TestStatus.SKIPPED
-        except AssertionError:
-            self.log.exception("Assertion failed")
-            self.success = TestStatus.FAILED
-        except KeyError:
-            self.log.exception("Key error")
-            self.success = TestStatus.FAILED
-        except subprocess.CalledProcessError as e:
-            self.log.exception("Called Process failed with '{}'".format(e.output))
-            self.success = TestStatus.FAILED
-        except Exception:
-            self.log.exception("Unexpected exception caught during testing")
-            self.success = TestStatus.FAILED
-        except KeyboardInterrupt:
-            self.log.warning("Exiting after keyboard interrupt")
-            self.success = TestStatus.FAILED
-        finally:
-            exit_code = self.shutdown()
-            sys.exit(exit_code)
+        exit_codes = []  # The final exit code(s) of the test run(s). May have two values instead of one if both wallet types are run.
+        for wallet_type in self._runs_wallet_type:
+            self.options.descriptors=wallet_type
+            try:
+                self.setup()
+                self.run_test()
+            except JSONRPCException:
+                self.log.exception("JSONRPC error")
+                self.success = TestStatus.FAILED
+            except SkipTest as e:
+                self.log.warning("Test Skipped: %s" % e.message)
+                self.success = TestStatus.SKIPPED
+            except AssertionError:
+                self.log.exception("Assertion failed")
+                self.success = TestStatus.FAILED
+            except KeyError:
+                self.log.exception("Key error")
+                self.success = TestStatus.FAILED
+            except subprocess.CalledProcessError as e:
+                self.log.exception("Called Process failed with '{}'".format(e.output))
+                self.success = TestStatus.FAILED
+            except Exception:
+                self.log.exception("Unexpected exception caught during testing")
+                self.success = TestStatus.FAILED
+            except KeyboardInterrupt:
+                self.log.warning("Exiting after keyboard interrupt")
+                self.success = TestStatus.FAILED
+            finally:
+                self.shutdown()
+                exit_codes.append(self.success)
+
+        # Determine the exit code
+        # If one failed, the test failed.
+        # If both skipped, the test skipped.
+        # If one passed and the other skipped, the test passed
+        if any([ec == TestStatus.FAILED for ec in exit_codes]):
+            sys.exit(TEST_EXIT_FAILED)
+        if all([ec == TestStatus.SKIPPED for ec in exit_codes]):
+            sys.exit(TEST_EXIT_SKIPPED)
+        if any([ec == TestStatus.PASSED for ec in exit_codes]):
+            sys.exit(TEST_EXIT_PASSED)
+        assert False, "Unhandled exit scenario Legacy {}, descriptor {}".format(exit_codes[0], exit_codes[1])            
 
     def parse_args(self):
         previous_releases_path = os.getenv("PREVIOUS_RELEASES_DIR") or os.getcwd() + "/releases"
@@ -214,7 +230,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         elif self.options.descriptors is None:
             # Some wallet is either required or optionally used by the test.
             # Prefer BDB unless it isn't available
-            if self.is_bdb_compiled():
+            if self.is_bdb_compiled() and self.is_sqlite_compiled():
+                self._runs_wallet_type = [False,True]
+            elif self.is_bdb_compiled():
                 self.options.descriptors = False
             elif self.is_sqlite_compiled():
                 self.options.descriptors = True
  103. DrahtBot added the label Needs rebase on Nov 28, 2022
  104. achow101 force-pushed on Nov 28, 2022
  105. achow101 commented at 10:14 PM on November 28, 2022: member

    Rebased and reworked now that #26480 is merged. I've had to change how to decide whether to use descriptor wallets - instead of checking self.options.descriptors, we now check a self.use_descriptors.

    Additionally, the default wallet name is determined conditionally based on the wallet type we are using for the test. As such ,it can't be in __init__(), so I moved it to setup(). This breaks a couple of tests that used it in their set_test_params(), so I've also added a special case for True in self.wallet_names to signify that self.default_wallet_name should be used.

  106. achow101 force-pushed on Nov 28, 2022
  107. DrahtBot removed the label Needs rebase on Nov 29, 2022
  108. achow101 force-pushed on Nov 29, 2022
  109. in test/functional/test_framework/test_framework.py:226 in 4601e16242 outdated 
     222 | @@ -212,18 +223,24 @@ def parse_args(self):
 223 |              # It still needs to exist and be None in order for tests to work however.
 224 |              # So set it to None to force -disablewallet, because the wallet is not needed.
 225 |              self.options.descriptors = None
 226 | +            self.use_descriptors = OptionalBool()
    maflcko commented at 1:41 PM on November 29, 2022:

    in 4601e162421ec560309a3ae3b5da78cde728c208:

    Might be good to explain why this commit is needed. None will already force -disablewallet, and thus assert when a logic error occurs.

    Also, it doesn't seem to be working, given that the tests fail when only sqlite is installed?

    achow101 commented at 1:54 AM on November 30, 2022:

    Might be good to explain why this commit is needed. None will already force -disablewallet, and thus assert when a logic error occurs.

    self.use_descriptors was needed because we can't be overriding self.options.descriptors in main() while also respecting its value from parse_args(). I also wanted to change the self.options.descriptors to an array of the wallet types to run with, which was incompatible with our current usage of that value.

    OptionalBool was introduced to catch other logic errors within the tests themselves in areas that are not reliant on bitcoind running. For example, it caught the issue with the default wallet name.

    Also, it doesn't seem to be working, given that the tests fail when only sqlite is installed?

    That's unrelated to this particular change. That was occurring because some tests add the wallet options since they can run with a wallet, but do not require it. For those tests, they were being run with both wallet types always, even if one type (e.g. legacy) was not available, causing the failure. I've pushed a fix for that.

  110. maflcko commented at 2:08 PM on November 29, 2022: member

    left a question

  111. achow101 force-pushed on Nov 30, 2022
  112. achow101 force-pushed on Nov 30, 2022
  113. achow101 force-pushed on Nov 30, 2022
  114. achow101 force-pushed on Nov 30, 2022
  115. achow101 commented at 4:58 AM on November 30, 2022: member

    Ran into some issues with mempool_compatibility.py so I've added some commits that remove the usage of the wallet from that.

  116. in test/functional/test_runner.py:176 in 73535d3dda outdated 
     148 | @@ -149,8 +149,10 @@
 149 |      'tool_wallet.py --descriptors',
    maflcko commented at 9:39 AM on November 30, 2022:

    nit in 73535d3ddabd10c92b240ecdf749527eae21f08e:

    I wonder if the type should be enforced, because if it isn't, the test may run twice sequentially instead of in parallel, thus harming performance.

    This could be done with something like:

    diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index e77676a365..2cb0591fb9 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -453,8 +453,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             # If only one type can be chosen, set it as default
             kwargs["default"] = descriptors
         group = parser.add_mutually_exclusive_group(
-            # If only one type is allowed, require it to be set in test_runner.py
-            required=os.getenv("REQUIRE_WALLET_TYPE_SET") == "1" and "default" in kwargs)
+            # Require the type to be set in test_runner.py
+            required=os.getenv("REQUIRE_WALLET_TYPE_SET") == "1")
         if descriptors:
             group.add_argument("--descriptors", action='store_const', const=True, **kwargs,
                                help="Run test using a descriptor wallet", dest='descriptors')
    achow101 commented at 4:18 PM on November 30, 2022:

    I don't quite follow. Why would this change whether it runs sequentially or parallel?

    maflcko commented at 10:37 AM on December 5, 2022:

    My understanding is that a missing type setting will run both types, if possible, sequentially.

    The goal of test_runner is to run as much in parallel as possible. So if the test_runner setting is missing a wallet type setting in one line, it will run the test twice, sequentially.

    This can be fixed by forcing a type in the test runner, thus running at most one test per line, thus all lines/tests in parallel.

    achow101 commented at 9:46 PM on December 6, 2022:

    Done.

    There are a few tests that do add_wallet_options but don't really test anything with the wallet. For those tests, I've added an any_type to add_wallet_options so that those tests don't need either --legacy-wallet or --descriptors specified and won't be run twice by the test runner.

  117. DrahtBot added the label Needs rebase on Nov 30, 2022
  118. maflcko commented at 4:06 PM on November 30, 2022: member

    left a comment/nit

  119. achow101 force-pushed on Nov 30, 2022
  120. DrahtBot removed the label Needs rebase on Nov 30, 2022
  121. achow101 force-pushed on Nov 30, 2022
  122. in test/functional/mempool_compatibility.py:42 in c6e1064e0a outdated 
      39 |          self.log.info("Test that mempool.dat is compatible between versions")
  40 |  
  41 |          old_node, new_node = self.nodes
  42 | -        new_wallet = MiniWallet(new_node, mode=MiniWalletMode.RAW_P2PK)
  43 | +        new_wallet = MiniWallet(new_node, mode=MiniWalletMode.RAW_P2PK_RANDOM)
  44 | +        old_wallet = MiniWallet(old_node, mode=MiniWalletMode.RAW_P2PK_RANDOM)
    maflcko commented at 11:17 AM on December 5, 2022:

    Generally MiniWallet is designed to use the latest features of Bitcoin Core, so it may not work with older versions. Also, it should be sufficient to have a single wallet instance per test. Thus, it should be possible to remove the below generatetodescriptor and just replace 1 blocks with 2 in the line below. And also the "random" miniwallet mode is not needed.

    maflcko commented at 11:18 AM on December 5, 2022:

    Done in #26640 , which should hopefully allow to drop two commits from this pull and make it simpler.

  123. DrahtBot added the label Needs rebase on Dec 5, 2022
  124. achow101 force-pushed on Dec 6, 2022
  125. DrahtBot removed the label Needs rebase on Dec 6, 2022
  126. achow101 force-pushed on Dec 6, 2022
  127. achow101 commented at 9:47 PM on December 6, 2022: member

    I've added a commit which has the test runner automatically add the --legacy-wallet and --descriptors to wallet_* tests that don't have those options already specified. This reduces the need to remember to add those options to wallet tests.

  128. DrahtBot added the label Needs rebase on Dec 9, 2022
  129. achow101 force-pushed on Dec 9, 2022
  130. DrahtBot removed the label Needs rebase on Dec 9, 2022
  131. achow101 force-pushed on Dec 12, 2022
  132. DrahtBot added the label Needs rebase on Dec 19, 2022
  133. achow101 force-pushed on Dec 19, 2022
  134. achow101 force-pushed on Dec 19, 2022
  135. DrahtBot removed the label Needs rebase on Dec 19, 2022
  136. DrahtBot added the label Needs rebase on Dec 20, 2022
  137. achow101 force-pushed on Jan 3, 2023
  138. DrahtBot removed the label Needs rebase on Jan 3, 2023
  139. aureleoules commented at 10:51 AM on January 4, 2023: member

    CI is failing on

    1/270 - feature_pruning.py failed, Duration: 0 s
stdout:
stderr:
usage: feature_pruning.py [options]
feature_pruning.py: error: one of the arguments --descriptors --legacy-wallet is required

    https://cirrus-ci.com/task/5943446209298432?logs=functional_tests#L20

  140. achow101 force-pushed on Jan 7, 2023
  141. achow101 force-pushed on Jan 7, 2023
  142. DrahtBot added the label Needs rebase on Jan 10, 2023
  143. achow101 force-pushed on Jan 10, 2023
  144. achow101 force-pushed on Jan 10, 2023
  145. DrahtBot removed the label Needs rebase on Jan 11, 2023
  146. DrahtBot added the label Needs rebase on Jan 19, 2023
  147. achow101 force-pushed on Jan 23, 2023
  148. DrahtBot removed the label Needs rebase on Jan 23, 2023
  149. in test/functional/test_framework/test_framework.py:197 in 3ce08717a6 outdated 
     168 | @@ -169,6 +169,7 @@ def parse_args(self):
 169 |          parser.add_argument("--cachedir", dest="cachedir", default=os.path.abspath(os.path.dirname(os.path.realpath(__file__)) + "/../../cache"),
 170 |                              help="Directory for caching pregenerated datadirs (default: %(default)s)")
 171 |          parser.add_argument("--tmpdir", dest="tmpdir", help="Root directory for datadirs")
 172 | +        parser.add_argument("--tmpdir-prefix", dest="tmpdir_prefix", help="Prefix to use for datadirs", default=TMPDIR_PREFIX)
    maflcko commented at 2:53 PM on January 24, 2023:

    nit in the first commit: This makes the --tmpdirprefix option pretty useless (and confusing, given that there are now 3 tmpdir options). My recommendation would be to remove --tmpdirprefix when adding this option.

    --tmpdirprefix is also silently ignored when --tmpdir is passed, so this could even make sense as a separate bug-fix PR

    achow101 commented at 6:45 PM on January 24, 2023:

    I don't quite follow, --tmpdirprefix is for the test runner. If it can be left for a followup, I'd prefer to leave it for that.

    maflcko commented at 7:15 PM on January 24, 2023:

    Any option for a single test will be passed up to the test runner. (Similar to how **kwargs can be used to pass through options).

    If --tmpdir is used to set the root dir and --tmpdir-prefix to set the prefix folder name, and both options are passed up to test runner, then having a third and broken --tmpdirprefix, which only works for the test runner, seems increasingly confusing and there may even introduce new bugs.

    Moreover, I wonder which later commit in this pull requires the first commit(s). Looking at 2a60a78a6483d59b3ea09377d663ca834abc7fcf, it looks like you are using the test runner to parallelize the test, which should already correctly set the unique seed id in the temp prefix folder name?

    achow101 commented at 7:49 PM on January 24, 2023:

    I've added a commit which remove --tmpdirprefix

    Moreover, I wonder which later commit in this pull requires the first commit(s).

    They're necessary for 88b63a6e163045e63a7540427834a57799234407 "tests: Automatically run both wallet types in parallel for wallet tests". The single invocation needs to have different tmpdirs for each wallet option it runs with. Although --tmpdir-prefix might not be needed with the last commit.

    maflcko commented at 2:18 PM on May 3, 2023:

    They're necessary for https://github.com/bitcoin/bitcoin/commit/88b63a6e163045e63a7540427834a57799234407 "tests: Automatically run both wallet types in parallel for wallet tests".

    I looked at that commit and it looks like the test list for the tests to run in parallel is created before passing it to TestHandler. TestHandler should never create conflicting/duplicate datadirs, because each test has its own directory based on the global test_runner directore, the name of the test, and its portseed. The portseed should be different for the same test running for bdb and sqlite.

    So the new option is only needed for intermediate commits, or not at all? Sorry for asking so many question, but I don't feel comfortable reviewing, unless I fully understand what each commit does and whether it is needed. Overall, if something isn't needed in the final state of the pull, I have a slight preference to not add it in the first place.

    achow101 commented at 2:59 PM on May 3, 2023:

    Sorry, I think I gave the wrong commit. It should be a936a3f194f855e269a4dc2c25e17dbc55eb7a18 "tests: Run both descriptor and legacy wallet modes in single invocation".

    The tmpdir changes are needed for this commit because the entire test is restarted for the other wallet type, so it needs to be able to make a new datadir.

  150. achow101 force-pushed on Jan 24, 2023
  151. achow101 requested review from maflcko on Apr 25, 2023
  152. achow101 requested review from josibake on Apr 25, 2023
  153. achow101 requested review from Sjors on Apr 25, 2023
  154. josibake commented at 10:15 AM on May 3, 2023: member

    Concept ACK

    I think when I initially looked at this PR the BitcoinWalletTestFramework class was breaking TestShell, but looking through the PR now, it seems the approach has changed to not use a BitcoinWalletTestFramework class. Can you update the description detailing the approach you are now taking?

    Did a brief code review and the changes good look good, will test and review once you update the description so I can determine if the code is actually doing what you want it to do :smile:

  155. achow101 commented at 2:59 PM on May 3, 2023: member

    Can you update the description detailing the approach you are now taking?

    Done.

  156. DrahtBot added the label Needs rebase on May 8, 2023
  157. achow101 force-pushed on May 8, 2023
  158. DrahtBot removed the label Needs rebase on May 8, 2023
  159. DrahtBot added the label Needs rebase on May 26, 2023
  160. achow101 force-pushed on May 27, 2023
  161. DrahtBot removed the label Needs rebase on May 27, 2023
  162. achow101 force-pushed on May 27, 2023
  163. DrahtBot added the label CI failed on May 27, 2023
  164. DrahtBot removed the label CI failed on May 27, 2023
  165. DrahtBot added the label Needs rebase on Jun 14, 2023
  166. achow101 force-pushed on Jun 14, 2023
  167. DrahtBot removed the label Needs rebase on Jun 14, 2023
  168. DrahtBot added the label Needs rebase on Jun 16, 2023
  169. achow101 force-pushed on Jun 28, 2023
  170. DrahtBot removed the label Needs rebase on Jun 28, 2023
  171. DrahtBot added the label Needs rebase on Jun 29, 2023
  172. achow101 force-pushed on Jun 29, 2023
  173. DrahtBot removed the label Needs rebase on Jun 29, 2023
  174. DrahtBot added the label CI failed on Jun 29, 2023
  175. DrahtBot removed the label CI failed on Jun 30, 2023
  176. DrahtBot added the label Needs rebase on Jul 26, 2023
  177. achow101 force-pushed on Jul 27, 2023
  178. DrahtBot removed the label Needs rebase on Jul 27, 2023
  179. DrahtBot added the label CI failed on Jul 27, 2023
  180. achow101 force-pushed on Aug 2, 2023
  181. achow101 force-pushed on Aug 4, 2023
  182. DrahtBot removed the label CI failed on Aug 5, 2023
  183. DrahtBot added the label Needs rebase on Aug 22, 2023
  184. achow101 force-pushed on Aug 23, 2023
  185. DrahtBot removed the label Needs rebase on Aug 23, 2023
  186. DrahtBot added the label CI failed on Aug 24, 2023
  187. DrahtBot removed the label CI failed on Aug 24, 2023
  188. DrahtBot added the label Needs rebase on Sep 14, 2023
  189. achow101 force-pushed on Sep 14, 2023
  190. DrahtBot removed the label Needs rebase on Sep 14, 2023
  191. DrahtBot added the label CI failed on Sep 15, 2023
  192. achow101 force-pushed on Sep 15, 2023
  193. DrahtBot removed the label CI failed on Sep 15, 2023
  194. aureleoules commented at 12:32 PM on September 25, 2023: member

    I have the following error when running the test_runner with BDB compiled using depends:

    <details> <summary>Details</summary>

    1695642351381,"4/288 - wallet_backup.py --legacy-wallet failed, Duration: 43 s"
1695642351381,stdout:
1695642351381,2023-09-25T11:39:01.368000Z TestFramework (INFO): PRNG seed is: 8911868130033750645
1695642351381,2023-09-25T11:39:01.381000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20230925_113853/wallet_backup_272_3wjtcweb
1695642351381,2023-09-25T11:39:05.172000Z TestFramework (INFO): Generating initial blockchain
1695642351381,2023-09-25T11:39:09.767000Z TestFramework (INFO): Creating transactions
1695642351381,2023-09-25T11:39:25.216000Z TestFramework (INFO): Backing up
1695642351381,2023-09-25T11:39:25.622000Z TestFramework (INFO): More transactions
1695642351381,2023-09-25T11:39:39.536000Z TestFramework (INFO): Restoring wallets on node 3 using backup files
1695642351381,2023-09-25T11:39:40.434000Z TestFramework (INFO): Restoring using dumped wallet
1695642351381,2023-09-25T11:39:43.078000Z TestFramework (ERROR): Unexpected exception caught during testing
1695642351381,Traceback (most recent call last):
1695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/test_framework.py"", line 148, in main"
1695642351381,    self.run_test()
1695642351381,"  File ""/tmp/bitcoin/test/functional/wallet_backup.py"", line 231, in run_test"
1695642351381,"    self.nodes[node_num].createwallet(wallet_name=self.default_wallet_name, descriptors=self.use_descriptors, load_on_startup=True)"
1695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/test_node.py"", line 818, in createwallet"
1695642351381,"    return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer)"
1695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/coverage.py"", line 50, in __call__"
1695642351381,"    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)"
1695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/authproxy.py"", line 126, in __call__"
1695642351381,"    postdata = json.dumps(self.get_request(*args, **argsn), default=serialization_fallback, ensure_ascii=self.ensure_ascii)"
1695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/authproxy.py"", line 114, in get_request"
1695642351381,"    json.dumps(args or argsn, default=serialization_fallback, ensure_ascii=self.ensure_ascii),"
1695642351381,"  File ""/usr/lib/python3.10/json/__init__.py"", line 238, in dumps"
1695642351381,    **kw).encode(obj)
1695642351381,"  File ""/usr/lib/python3.10/json/encoder.py"", line 199, in encode"
1695642351381,"    chunks = self.iterencode(o, _one_shot=True)"
1695642351381,"  File ""/usr/lib/python3.10/json/encoder.py"", line 257, in iterencode"
1695642351381,"    return _iterencode(o, 0)"
1695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/authproxy.py"", line 68, in serialization_fallback"
1695642351381,"    raise TypeError(repr(o) + "" is not JSON serializable"")"
1695642351381,TypeError: <test_framework.test_framework.OptionalBool object at 0x7fbcf6e055a0> is not JSON serializable
1695642351381,2023-09-25T11:39:43.130000Z TestFramework (INFO): Stopping nodes
1695642351381,2023-09-25T11:39:43.507000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20230925_113853/wallet_backup_272_3wjtcweb
1695642351381,2023-09-25T11:39:43.507000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20230925_113853/wallet_backup_272_3wjtcweb/test_framework.log
1695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR): 
1695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR): Hint: Call /tmp/bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20230925_113853/wallet_backup_272_3wjtcweb' to consolidate all logs
1695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR): 
1695642351381,"2023-09-25T11:39:43.508000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log."
1695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
1695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR):

    </details>

  195. maflcko commented at 1:04 PM on September 25, 2023: member

    Not sure if this is worth it at this point.

    If there are independent test framework improvements, they can be submitted and reviewed and tested independently. However, I don't think the whole test framework should be rewritten for a temporary need.

    IIRC it should be possible to just remove the feature to create new bdb wallets after the branch-off. It bdb wallet support is going to be removed in the future, there shouldn't be any need for new wallets to be created in this format, especially with the 27.0 release next year. If someone (or the tests) really need to create a bdb wallet, they can just use the previous release (for example 25.0).

    Happy to review a pull request that removes the feature to create bdb wallets.

  196. DrahtBot added the label Needs rebase on Oct 3, 2023
  197. tests: Always use tempfile.mkdtemp and a dedicated tmpdir variable
    Instead of manually making the tmpdir depending on whether --tmpdir is
provided, we can always us tempfile.mkdtemp and provide the dir option
with self.options.tmpdir. This behaves the same as previously except
that the tmpdirs created when --tmpdir is passed will have additional
randomness on the directory names.

In order to get the correct tmpdir prefixes from the test runner, a
--tmpdir-prefix option is added.

Lastly, the tmpdir location is stored in a dedicated self.tmpdir
variable. We wll remove usage of self.options.tmpdir for that path in
later commits.
    a30cbfad70
  198. tests: Remove --tmpdirprefix
    --tmpdirprefix is now unnecessary and potentially confusing.
    e2b339581b
  199. scripted-diff: Use self.tmpdir instead of self.options.tmpdir
    Instead of retrieving the actual tmpdir path from self.options.tmpdir,
retrieve it from self.tmpdir.

-BEGIN VERIFY SCRIPT-
sed -i "s/self.options.tmpdir/self.tmpdir/" $(git grep -l "self.options.tmpdir" -- ":!test/functional/test_framework/test_framework.py")
sed -i "264,271b;s/self.options.tmpdir/self.tmpdir/" test/functional/test_framework/test_framework.py
-END VERIFY SCRIPT-
    6dc8beee15
  200. test: Remove setting self.options.tmpdir to tmpdir f011029832
  201. tests: Add --legacy-wallet and --descriptor variants to new wallet tests
    Some new wallet functional tests were added, these need to
--legacy-wallet and --descriptors variants in test_runner.py
    22d45b4352
  202. Add self.use_descriptors that is an optional bool
    self.use_descriptors tracks whether the test should be using a
descriptor. However, to avoid the fact that None evaluates to False, it
is a newly introduced OptionalBool class that will assert if it is None
when used as a bool rather than returning False.

test_framework.py is updated to use self.use_descriptors throughout
rather than self.options.descriptors. The next commit will update the
rest of the tests.
    4ef8a80786
  203. scripted-diff: Replace self.options.descriptors with use_descriptors
    Instead of using the options parser to hold whether we are doing
descriptor wallets, have a member in the test framework class to handle
it.

-BEGIN VERIFY SCRIPT-
sed -i -E "s/self.options.descriptors/self.use_descriptors/" $(git grep -l "self.options.descriptors" -- ":(exclude)*/test_framework.py")
-END VERIFY SCRIPT-
    9ef6ae9ec3
  204. Remove unnecessary self.options.descriptors assignments
    self.options.descriptors is no longer used by the tests directly, so we
don't need to be assigning it.
    2853c4d521
  205. tests: Move conditional default wallet name to setup()
    Determining the default wallet name based on the wallet type the test is
running with requires being in `setup()`, so move the setting of
`self.default_wallet_name` there. This requires changing a couple of
tests in the way that they can specify that the default wallet name
should be used in `self.wallet_names`.
    f4f4a3cec2
  206. tests: Run both descriptor and legacy wallet modes in single invocation
    Be able to run tests that can run in both descriptor and legacy wallet
modes with a single invocation of the test script. In order to
facilitate this, self.options.descriptors is changed to be a list. When
neither option is provided, but both are available, it is set to [False,
True] which will run the test twice, the first in legacy wallet mode,
the second in descriptor wallet mode.

For the tests that can only have one mode, add_wallet_options will set
self.options.descriptors to either [False] or [True] which will make the
correct type run.
    6cb7db8c35
  207. tests: Automatically run both wallet types in parallel for wallet tests
    Most wallet tests need to be run with both wallet types. Instead of
requiring developers to manually specify this, the test_runner can find
the wallet tests that do not have a wallet type specified and
automatically run the test with both types.
    99d7e2c546
  208. achow101 force-pushed on Oct 3, 2023
  209. DrahtBot removed the label Needs rebase on Oct 3, 2023
  210. DrahtBot commented at 4:58 PM on October 5, 2023: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

    🐙 This pull request conflicts with the target branch and needs rebase.

  211. DrahtBot added the label Needs rebase on Oct 5, 2023
  212. achow101 commented at 7:56 PM on October 5, 2023: member

    The intention is to remove the legacy wallet soon(tm), so I guess this isn't worth merging and then reverting in a few months.

  213. achow101 closed this on Oct 5, 2023
  214. bitcoin locked this on Oct 4, 2024
Contributors
achow101DrahtBotpromagS3RKrajarshimaitraaureleoulesmaflckojosibake


Sjorsmaflckojosibake

Labels
TestsNeeds rebase
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-04-28 09:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me