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

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.

fanquake added the label Tests on Jan 9, 2021

DrahtBot commented at 10:34 am on January 9, 2021: contributor

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

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.

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.

promag commented at 3:37 pm on January 11, 2021: member

Concept ACK.

achow101 force-pushed on Jan 12, 2021

DrahtBot commented at 11:45 am on January 13, 2021: contributor

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

DrahtBot added the label Needs rebase on Jan 19, 2021

achow101 force-pushed on Jan 26, 2021

DrahtBot removed the label Needs rebase on Jan 26, 2021

DrahtBot added the label Needs rebase on Jan 28, 2021

achow101 force-pushed on Jan 28, 2021

DrahtBot removed the label Needs rebase on Jan 28, 2021

DrahtBot added the label Needs rebase on Feb 5, 2021

achow101 force-pushed on Feb 5, 2021

DrahtBot removed the label Needs rebase on Feb 5, 2021

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.

DrahtBot added the label Needs rebase on Feb 25, 2021

achow101 force-pushed on Mar 3, 2021

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.

DrahtBot removed the label Needs rebase on Mar 3, 2021

DrahtBot added the label Needs rebase on May 10, 2021

achow101 force-pushed on May 10, 2021

DrahtBot removed the label Needs rebase on May 11, 2021

DrahtBot added the label Needs rebase on May 31, 2021

achow101 force-pushed on Jun 1, 2021

DrahtBot removed the label Needs rebase on Jun 1, 2021

DrahtBot added the label Needs rebase on Jun 5, 2021

achow101 force-pushed on Jun 7, 2021

DrahtBot removed the label Needs rebase on Jun 7, 2021

DrahtBot added the label Needs rebase on Jun 24, 2021

achow101 force-pushed on Jul 1, 2021

DrahtBot removed the label Needs rebase on Jul 1, 2021

DrahtBot added the label Needs rebase on Aug 26, 2021

achow101 force-pushed on Sep 1, 2021

DrahtBot removed the label Needs rebase on Sep 1, 2021

achow101 force-pushed on Sep 29, 2021

DrahtBot added the label Needs rebase on Oct 4, 2021

achow101 force-pushed on Oct 4, 2021

DrahtBot removed the label Needs rebase on Oct 4, 2021

DrahtBot added the label Needs rebase on Oct 4, 2021

achow101 force-pushed on Oct 4, 2021

DrahtBot removed the label Needs rebase on Oct 5, 2021

achow101 force-pushed on Oct 8, 2021

DrahtBot added the label Needs rebase on Nov 17, 2021

achow101 force-pushed on Nov 18, 2021

DrahtBot removed the label Needs rebase on Nov 18, 2021

DrahtBot added the label Needs rebase on Dec 7, 2021

achow101 force-pushed on Dec 8, 2021

DrahtBot removed the label Needs rebase on Dec 8, 2021

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

achow101 force-pushed on Jan 10, 2022

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.

achow101 force-pushed on Jan 10, 2022

DrahtBot added the label Needs rebase on Jan 25, 2022

achow101 force-pushed on Jan 26, 2022

DrahtBot removed the label Needs rebase on Jan 26, 2022

DrahtBot added the label Needs rebase on Mar 15, 2022

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.

achow101 force-pushed on Mar 24, 2022

DrahtBot removed the label Needs rebase on Mar 24, 2022

DrahtBot added the label Needs rebase on Apr 14, 2022

achow101 force-pushed on Apr 18, 2022

DrahtBot removed the label Needs rebase on Apr 18, 2022

DrahtBot added the label Needs rebase on Apr 25, 2022

achow101 force-pushed on Apr 25, 2022

DrahtBot removed the label Needs rebase on Apr 25, 2022

DrahtBot added the label Needs rebase on Jun 16, 2022

achow101 force-pushed on Jun 20, 2022

DrahtBot removed the label Needs rebase on Jun 20, 2022

DrahtBot added the label Needs rebase on Jul 11, 2022

achow101 force-pushed on Jul 12, 2022

DrahtBot removed the label Needs rebase on Jul 12, 2022

achow101 force-pushed on Jul 12, 2022

DrahtBot added the label Needs rebase on Jul 13, 2022

achow101 force-pushed on Jul 13, 2022

DrahtBot removed the label Needs rebase on Jul 13, 2022

DrahtBot added the label Needs rebase on Jul 20, 2022

achow101 force-pushed on Jul 21, 2022

DrahtBot removed the label Needs rebase on Jul 21, 2022

achow101 force-pushed on Jul 22, 2022

achow101 force-pushed on Jul 26, 2022

achow101 force-pushed on Jul 29, 2022

achow101 force-pushed on Aug 1, 2022

DrahtBot added the label Needs rebase on Aug 16, 2022

achow101 force-pushed on Aug 17, 2022

DrahtBot removed the label Needs rebase on Aug 17, 2022

DrahtBot added the label Needs rebase on Sep 5, 2022

achow101 force-pushed on Sep 6, 2022

DrahtBot removed the label Needs rebase on Sep 6, 2022

glozow requested review from maflcko on Oct 12, 2022

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)

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.

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.

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.

DrahtBot added the label Needs rebase on Oct 20, 2022

achow101 force-pushed on Oct 20, 2022

DrahtBot removed the label Needs rebase on Oct 20, 2022

achow101 force-pushed on Oct 21, 2022

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:

 0diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
 1index d88cccecd9..61b5dfc7e9 100755
 2--- a/test/functional/test_framework/test_framework.py
 3+++ b/test/functional/test_framework/test_framework.py
 4@@ -100,6 +100,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 5         self.network_thread = None
 6         self.rpc_timeout = 60  # Wait for up to 60 seconds for the RPC server to respond
 7         self.supports_cli = True
 8+        self._runs_wallet_type = [None]
 9         self.bind_to_localhost_only = True
10         self.parse_args()
11         self.disable_syscall_sandbox = self.options.nosandbox or self.options.valgrind
12@@ -128,33 +129,48 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
13 
14         assert hasattr(self, "num_nodes"), "Test must set self.num_nodes in set_test_params()"
15 
16-        try:
17-            self.setup()
18-            self.run_test()
19-        except JSONRPCException:
20-            self.log.exception("JSONRPC error")
21-            self.success = TestStatus.FAILED
22-        except SkipTest as e:
23-            self.log.warning("Test Skipped: %s" % e.message)
24-            self.success = TestStatus.SKIPPED
25-        except AssertionError:
26-            self.log.exception("Assertion failed")
27-            self.success = TestStatus.FAILED
28-        except KeyError:
29-            self.log.exception("Key error")
30-            self.success = TestStatus.FAILED
31-        except subprocess.CalledProcessError as e:
32-            self.log.exception("Called Process failed with '{}'".format(e.output))
33-            self.success = TestStatus.FAILED
34-        except Exception:
35-            self.log.exception("Unexpected exception caught during testing")
36-            self.success = TestStatus.FAILED
37-        except KeyboardInterrupt:
38-            self.log.warning("Exiting after keyboard interrupt")
39-            self.success = TestStatus.FAILED
40-        finally:
41-            exit_code = self.shutdown()
42-            sys.exit(exit_code)
43+        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.
44+        for wallet_type in self._runs_wallet_type:
45+            self.options.descriptors=wallet_type
46+            try:
47+                self.setup()
48+                self.run_test()
49+            except JSONRPCException:
50+                self.log.exception("JSONRPC error")
51+                self.success = TestStatus.FAILED
52+            except SkipTest as e:
53+                self.log.warning("Test Skipped: %s" % e.message)
54+                self.success = TestStatus.SKIPPED
55+            except AssertionError:
56+                self.log.exception("Assertion failed")
57+                self.success = TestStatus.FAILED
58+            except KeyError:
59+                self.log.exception("Key error")
60+                self.success = TestStatus.FAILED
61+            except subprocess.CalledProcessError as e:
62+                self.log.exception("Called Process failed with '{}'".format(e.output))
63+                self.success = TestStatus.FAILED
64+            except Exception:
65+                self.log.exception("Unexpected exception caught during testing")
66+                self.success = TestStatus.FAILED
67+            except KeyboardInterrupt:
68+                self.log.warning("Exiting after keyboard interrupt")
69+                self.success = TestStatus.FAILED
70+            finally:
71+                self.shutdown()
72+                exit_codes.append(self.success)
73+
74+        # Determine the exit code
75+        # If one failed, the test failed.
76+        # If both skipped, the test skipped.
77+        # If one passed and the other skipped, the test passed
78+        if any([ec == TestStatus.FAILED for ec in exit_codes]):
79+            sys.exit(TEST_EXIT_FAILED)
80+        if all([ec == TestStatus.SKIPPED for ec in exit_codes]):
81+            sys.exit(TEST_EXIT_SKIPPED)
82+        if any([ec == TestStatus.PASSED for ec in exit_codes]):
83+            sys.exit(TEST_EXIT_PASSED)
84+        assert False, "Unhandled exit scenario Legacy {}, descriptor {}".format(exit_codes[0], exit_codes[1])            
85 
86     def parse_args(self):
87         previous_releases_path = os.getenv("PREVIOUS_RELEASES_DIR") or os.getcwd() + "/releases"
88@@ -214,7 +230,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
89         elif self.options.descriptors is None:
90             # Some wallet is either required or optionally used by the test.
91             # Prefer BDB unless it isn't available
92-            if self.is_bdb_compiled():
93+            if self.is_bdb_compiled() and self.is_sqlite_compiled():
94+                self._runs_wallet_type = [False,True]
95+            elif self.is_bdb_compiled():
96                 self.options.descriptors = False
97             elif self.is_sqlite_compiled():
98                 self.options.descriptors = True

DrahtBot added the label Needs rebase on Nov 28, 2022

achow101 force-pushed on Nov 28, 2022

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.

achow101 force-pushed on Nov 28, 2022

DrahtBot removed the label Needs rebase on Nov 29, 2022

achow101 force-pushed on Nov 29, 2022

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.

maflcko commented at 2:08 pm on November 29, 2022: member

left a question

achow101 force-pushed on Nov 30, 2022

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.

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:

 0diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
 1index e77676a365..2cb0591fb9 100755
 2--- a/test/functional/test_framework/test_framework.py
 3+++ b/test/functional/test_framework/test_framework.py
 4@@ -453,8 +453,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 5             # If only one type can be chosen, set it as default
 6             kwargs["default"] = descriptors
 7         group = parser.add_mutually_exclusive_group(
 8-            # If only one type is allowed, require it to be set in test_runner.py
 9-            required=os.getenv("REQUIRE_WALLET_TYPE_SET") == "1" and "default" in kwargs)
10+            # Require the type to be set in test_runner.py
11+            required=os.getenv("REQUIRE_WALLET_TYPE_SET") == "1")
12         if descriptors:
13             group.add_argument("--descriptors", action='store_const', const=True, **kwargs,
14                                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.

DrahtBot added the label Needs rebase on Nov 30, 2022

maflcko commented at 4:06 pm on November 30, 2022: member

left a comment/nit

achow101 force-pushed on Nov 30, 2022

DrahtBot removed the label Needs rebase on Nov 30, 2022

achow101 force-pushed on Nov 30, 2022

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.

DrahtBot added the label Needs rebase on Dec 5, 2022

achow101 force-pushed on Dec 6, 2022

DrahtBot removed the label Needs rebase on Dec 6, 2022

achow101 force-pushed on Dec 6, 2022

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.

DrahtBot added the label Needs rebase on Dec 9, 2022

achow101 force-pushed on Dec 9, 2022

DrahtBot removed the label Needs rebase on Dec 9, 2022

achow101 force-pushed on Dec 12, 2022

DrahtBot added the label Needs rebase on Dec 19, 2022

achow101 force-pushed on Dec 19, 2022

DrahtBot removed the label Needs rebase on Dec 19, 2022

DrahtBot added the label Needs rebase on Dec 20, 2022

achow101 force-pushed on Jan 3, 2023

DrahtBot removed the label Needs rebase on Jan 3, 2023

aureleoules commented at 10:51 am on January 4, 2023: member

CI is failing on

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

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

achow101 force-pushed on Jan 7, 2023

DrahtBot added the label Needs rebase on Jan 10, 2023

achow101 force-pushed on Jan 10, 2023

DrahtBot removed the label Needs rebase on Jan 11, 2023

DrahtBot added the label Needs rebase on Jan 19, 2023

achow101 force-pushed on Jan 23, 2023

DrahtBot removed the label Needs rebase on Jan 23, 2023

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.

achow101 force-pushed on Jan 24, 2023

achow101 requested review from maflcko on Apr 25, 2023

achow101 requested review from josibake on Apr 25, 2023

achow101 requested review from Sjors on Apr 25, 2023

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:

achow101 commented at 2:59 pm on May 3, 2023: member

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

Done.

DrahtBot added the label Needs rebase on May 8, 2023

achow101 force-pushed on May 8, 2023

DrahtBot removed the label Needs rebase on May 8, 2023

DrahtBot added the label Needs rebase on May 26, 2023

achow101 force-pushed on May 27, 2023

DrahtBot removed the label Needs rebase on May 27, 2023

achow101 force-pushed on May 27, 2023

DrahtBot added the label CI failed on May 27, 2023

DrahtBot removed the label CI failed on May 27, 2023

DrahtBot added the label Needs rebase on Jun 14, 2023

achow101 force-pushed on Jun 14, 2023

DrahtBot removed the label Needs rebase on Jun 14, 2023

DrahtBot added the label Needs rebase on Jun 16, 2023

achow101 force-pushed on Jun 28, 2023

DrahtBot removed the label Needs rebase on Jun 28, 2023

DrahtBot added the label Needs rebase on Jun 29, 2023

achow101 force-pushed on Jun 29, 2023

DrahtBot removed the label Needs rebase on Jun 29, 2023

DrahtBot added the label CI failed on Jun 29, 2023

DrahtBot removed the label CI failed on Jun 30, 2023

DrahtBot added the label Needs rebase on Jul 26, 2023

achow101 force-pushed on Jul 27, 2023

DrahtBot removed the label Needs rebase on Jul 27, 2023

DrahtBot added the label CI failed on Jul 27, 2023

achow101 force-pushed on Aug 2, 2023

achow101 force-pushed on Aug 4, 2023

DrahtBot removed the label CI failed on Aug 5, 2023

DrahtBot added the label Needs rebase on Aug 22, 2023

achow101 force-pushed on Aug 23, 2023

DrahtBot removed the label Needs rebase on Aug 23, 2023

DrahtBot added the label CI failed on Aug 24, 2023

DrahtBot removed the label CI failed on Aug 24, 2023

DrahtBot added the label Needs rebase on Sep 14, 2023

achow101 force-pushed on Sep 14, 2023

DrahtBot removed the label Needs rebase on Sep 14, 2023

DrahtBot added the label CI failed on Sep 15, 2023

achow101 force-pushed on Sep 15, 2023

DrahtBot removed the label CI failed on Sep 15, 2023

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:

 01695642351381,"4/288 - wallet_backup.py --legacy-wallet failed, Duration: 43 s"
 11695642351381,stdout:
 21695642351381,2023-09-25T11:39:01.368000Z TestFramework (INFO): PRNG seed is: 8911868130033750645
 31695642351381,2023-09-25T11:39:01.381000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20230925_113853/wallet_backup_272_3wjtcweb
 41695642351381,2023-09-25T11:39:05.172000Z TestFramework (INFO): Generating initial blockchain
 51695642351381,2023-09-25T11:39:09.767000Z TestFramework (INFO): Creating transactions
 61695642351381,2023-09-25T11:39:25.216000Z TestFramework (INFO): Backing up
 71695642351381,2023-09-25T11:39:25.622000Z TestFramework (INFO): More transactions
 81695642351381,2023-09-25T11:39:39.536000Z TestFramework (INFO): Restoring wallets on node 3 using backup files
 91695642351381,2023-09-25T11:39:40.434000Z TestFramework (INFO): Restoring using dumped wallet
101695642351381,2023-09-25T11:39:43.078000Z TestFramework (ERROR): Unexpected exception caught during testing
111695642351381,Traceback (most recent call last):
121695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/test_framework.py"", line 148, in main"
131695642351381,    self.run_test()
141695642351381,"  File ""/tmp/bitcoin/test/functional/wallet_backup.py"", line 231, in run_test"
151695642351381,"    self.nodes[node_num].createwallet(wallet_name=self.default_wallet_name, descriptors=self.use_descriptors, load_on_startup=True)"
161695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/test_node.py"", line 818, in createwallet"
171695642351381,"    return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer)"
181695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/coverage.py"", line 50, in __call__"
191695642351381,"    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)"
201695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/authproxy.py"", line 126, in __call__"
211695642351381,"    postdata = json.dumps(self.get_request(*args, **argsn), default=serialization_fallback, ensure_ascii=self.ensure_ascii)"
221695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/authproxy.py"", line 114, in get_request"
231695642351381,"    json.dumps(args or argsn, default=serialization_fallback, ensure_ascii=self.ensure_ascii),"
241695642351381,"  File ""/usr/lib/python3.10/json/__init__.py"", line 238, in dumps"
251695642351381,    **kw).encode(obj)
261695642351381,"  File ""/usr/lib/python3.10/json/encoder.py"", line 199, in encode"
271695642351381,"    chunks = self.iterencode(o, _one_shot=True)"
281695642351381,"  File ""/usr/lib/python3.10/json/encoder.py"", line 257, in iterencode"
291695642351381,"    return _iterencode(o, 0)"
301695642351381,"  File ""/tmp/bitcoin/test/functional/test_framework/authproxy.py"", line 68, in serialization_fallback"
311695642351381,"    raise TypeError(repr(o) + "" is not JSON serializable"")"
321695642351381,TypeError: <test_framework.test_framework.OptionalBool object at 0x7fbcf6e055a0> is not JSON serializable
331695642351381,2023-09-25T11:39:43.130000Z TestFramework (INFO): Stopping nodes
341695642351381,2023-09-25T11:39:43.507000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20230925_113853/wallet_backup_272_3wjtcweb
351695642351381,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
361695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR): 
371695642351381,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
381695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR): 
391695642351381,"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."
401695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
411695642351381,2023-09-25T11:39:43.508000Z TestFramework (ERROR):

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.

DrahtBot added the label Needs rebase on Oct 3, 2023

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

tests: Remove --tmpdirprefix

--tmpdirprefix is now unnecessary and potentially confusing.

e2b339581b

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

test: Remove setting self.options.tmpdir to tmpdir f011029832

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

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

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

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

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

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

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

achow101 force-pushed on Oct 3, 2023

DrahtBot removed the label Needs rebase on Oct 3, 2023

DrahtBot commented at 4:58 pm on October 5, 2023: contributor

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

DrahtBot added the label Needs rebase on Oct 5, 2023

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.

achow101 closed this on Oct 5, 2023

bitcoin locked this on Oct 4, 2024

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

Reviews

Conflicts