Disable and fix tests for when BDB is not compiled

achow101 commented at 11:56 pm on October 29, 2020: member

This PR fixes tests for when BDB is not compiled. Tests which rely on or test legacy wallet behavior are disabled and skipped when BDB is not compiled. For the components of some tests that are for legacy wallet things, those parts of the tests are skipped.

For the majority of tests, changes are made so that they can be run with either legacy wallets or descriptor wallets without materially effecting the test. Most tests only need the wallet for balance and transactions, so the type of wallet is not an important part of those tests. Additionally, some tests are wallet agnostic and modified to instead use the test framework’s MiniWallet.

fanquake added the label Tests on Oct 30, 2020

fanquake added the label Wallet on Oct 30, 2020

DrahtBot commented at 1:15 am on October 30, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21008 (test: fix zmq test flakiness, improve speed by theStack)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
#20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
#20591 (wallet, bugfix: fix ComputeTimeSmart function during rescanning process. by BitcoinTsunami)
#20362 (test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke)
#20196 (net: fix GetListenPort() to derive the proper port by vasild)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)

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.

in test/functional/test_framework/test_framework.py:860 in 846ba62cb9 outdated

850+        descriptors = self.options.descriptors
851+        if not self.is_bdb_compiled() and self.is_sqlite_compiled() and not descriptors:
852+            descriptors = True
853+        elif self.is_bdb_compiled() and not self.is_sqlite_compiled() and descriptors:
854+            descriptors = False
855+        return descriptors

mjdietzx commented at 5:28 pm on October 30, 2020:

I’m having trouble figuring out the logic here. Is there a more clear and concise way to determine descriptors?

achow101 commented at 6:35 pm on October 30, 2020:

Not really.

The purpose of this function is to enable or disable features depending on whether support is compiled and the current settings, for those tests where the descriptors setting is not important to the test and only need to just work.

ysangkok commented at 8:59 pm on November 13, 2020:

@mjdietzx , you can use Quine-McCluskey to derive a minimal function:

 0>>> from newqm import QM
 1>>> qm = QM(['A','B','C'])
 2>>> def aj(a,b,c): # a is self.options.descriptors, b is bdb, c is sqlite compilation
 3...      descriptors = a
 4...      if not b and c and not a:
 5...          descriptors = True
 6...      elif b and not c and a:
 7...          descriptors = False
 8...      return descriptors
 9... 
10>>> from itertools import product
11>>> values = [idx for (idx,x) in enumerate(product([False,True], repeat=3)) if aj(*x)]
12>>> qm.get_function(qm.solve(values,[])[1])
13'(((NOT B) AND C) OR (A AND (NOT B)) OR (A AND C))'

If we color each conjunctive element separately, you can see which elements of the truth table it covers: https://imgur.com/vOVyQP8

Expanded out using the original bindings:

0option_set = self.options.descriptors
1bdb = self.is_bdb_compiled()
2sqlite = self.is_sqlite_compiled()
3
4return ((NOT bdb) AND sqlite) \
5  OR (option_set AND (NOT bdb)) \
6  OR (option_set AND sqlite)

in test/functional/test_framework/test_framework.py:797 in 846ba62cb9 outdated

781         if not self.is_wallet_compiled():
782             raise SkipTest("wallet has not been compiled.")
783+        if self.options.descriptors:
784+            self.skip_if_no_sqlite()
785+        else:
786+            self.skip_if_no_bdb()

mjdietzx commented at 5:30 pm on October 30, 2020:

Are you able to use self. get_use_descriptors() here? To me it seems like logic might be getting duplicated

achow101 commented at 6:34 pm on October 30, 2020:

No, get_use_descriptors() is used to switch between legacy and descriptors for the tests where the type is not explicitly being set. This is different from the wallet tests (that will call this skip_if_no_wallet()) which are testing specific descriptor and legacy behavior.

in test/functional/wallet_multiwallet.py:127 in 846ba62cb9 outdated

129 
130         # should raise rpc error if wallet path can't be created
131-        assert_raises_rpc_error(-1, "boost::filesystem::create_directory:", self.nodes[0].createwallet, "w8/bad", descriptors=False)
132+        err_code = -1
133+        if self.options.descriptors:
134+            err_code = -4

mjdietzx commented at 5:35 pm on October 30, 2020:

nit: in places like these would err_code = -4 if self.options.descriptors else -1 be easier to follow?

achow101 commented at 6:41 pm on October 30, 2020:

Done in upstream PR #18788

in src/wallet/walletdb.cpp:1021 in 846ba62cb9 outdated

1012@@ -1011,9 +1013,11 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas
1013 
1014     Optional<DatabaseFormat> format;
1015     if (exists) {
1016+#ifdef USE_BDB
1017         if (ExistsBerkeleyDatabase(path)) {
1018             format = DatabaseFormat::BERKELEY;
1019         }
1020+#endif
1021 #ifdef USE_SQLITE

mjdietzx commented at 5:44 pm on October 30, 2020:

It seems like usually you’re doing: #ifdef USE_BDB ... #elif USE_SQLITE, but here you’re doing #ifdef USE_BDB ... #ifdef USE_SQLITE

Should this be consistent, or does it matter? I’m guessing USE_BDB and USE_SQLITE can never both be set?

achow101 commented at 6:40 pm on October 30, 2020:

No, it’s supposed to #ifdef ... #ifdef because both USE_BDB and USE_SQLITE can both be set, and in fact, is the default.

The instances where #ifdef...#elif is used is only because one wallet type must be defaulted to, and for now, we default to BDB.

in src/wallet/walletdb.cpp:20 in 846ba62cb9 outdated

13@@ -14,7 +14,9 @@
14 #include <util/system.h>
15 #include <util/time.h>
16 #include <util/translation.h>
17+#ifdef USE_BDB
18 #include <wallet/bdb.h>
19+#endif
20 #ifdef USE_SQLITE

mjdietzx commented at 5:46 pm on October 30, 2020:

same, should it be #elif USE_SQLITE here?

achow101 commented at 6:41 pm on October 30, 2020:

No, if both are compiled, then we need both headers.

in test/functional/feature_filelock.py:46 in 846ba62cb9 outdated

43+                self.nodes[1].assert_start_raises_init_error(extra_args=['-walletdir={}'.format(wallet_dir), '-wallet=' + self.default_wallet_name, '-noserver'], expected_msg=expected_msg, match=ErrorMatch.PARTIAL_REGEX)
44+
45+            if self.is_bdb_compiled():
46+                check_wallet_filelock(False)
47+            if self.is_sqlite_compiled():
48+                check_wallet_filelock(True)

mjdietzx commented at 5:49 pm on October 30, 2020:

Can this be condensed to a one-liner: check_wallet_filelock(self.is_sqlite_compiled())

achow101 commented at 6:40 pm on October 30, 2020:

When both are compiled, we want to test both cases.

achow101 force-pushed on Oct 30, 2020

achow101 force-pushed on Nov 1, 2020

achow101 force-pushed on Nov 2, 2020

MarcoFalke commented at 7:52 am on November 4, 2020: member

0libbitcoin_wallet.lib(wallet_walletutil.obj) : error LNK2001: unresolved external symbol "bool __cdecl ExistsSQLiteDatabase(class boost::filesystem::path const &)" (?ExistsSQLiteDatabase@@YA_NAEBVpath@filesystem@boost@@@Z) [C:\projects\bitcoin\build_msvc\bench_bitcoin\bench_bitcoin.vcxproj]

0The subject line of commit hash 8ea48f202d800895abea798385ac0e67f7f3034d is followed by a non-empty line. Subject lines should always be followed by a blank line.
1
2^---- failure generated from test/lint/lint-git-commit-check.sh

achow101 force-pushed on Nov 4, 2020

achow101 commented at 5:36 pm on November 4, 2020: member

Fixed the linter error.

I’m not sure what is causing the appveyor failure, nor how to fix it.

ryanofsky commented at 6:21 pm on November 4, 2020: member

I’m not sure what is causing the appveyor failure, nor how to fix it.

I dug up the old link https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/36097732 and the link errors for unresolved symbols just come from removing wallet/bdb.cpp and wallet/salvage.cpp from original libbitcoin_wallet_a_SOURCES setting in Makefile.am. You might need to add these explicitly to build_msvc/libbitcoin_wallet/libbitcoin_wallet.vcxproj.in. Example of how to do this: 9eaeb7fb8d4ab0d4493849e6c17e314fd75fea9c

ryanofsky commented at 6:23 pm on November 4, 2020: member

Example of how to do this: 9eaeb7f

Better simpler example: 0660119ac372c2863d14060ac1bc9bc243771f94

achow101 commented at 6:52 pm on November 4, 2020: member

I’m not sure what is causing the appveyor failure, nor how to fix it.

I dug up the old link https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/36097732 and the link errors for unresolved symbols just come from removing wallet/bdb.cpp and wallet/salvage.cpp from original libbitcoin_wallet_a_SOURCES setting in Makefile.am. You might need to add these explicitly to build_msvc/libbitcoin_wallet/libbitcoin_wallet.vcxproj.in. Example of how to do this: 9eaeb7f

I think I’ve fixed it in the upstream PR #20202

achow101 force-pushed on Nov 4, 2020

achow101 force-pushed on Nov 5, 2020

in src/wallet/init.cpp:74 in 0cbff93037 outdated

70@@ -69,9 +71,12 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
71 #endif
72     argsman.AddArg("-walletrbf", strprintf("Send transactions with full-RBF opt-in enabled (RPC only, default: %u)", DEFAULT_WALLET_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET);
73 
74+#ifdef USE_BDB

ryanofsky commented at 10:04 pm on November 5, 2020:

In commit “Do not compile BDB things when USE_BDB is defined” (0cbff9303729dddaad781a4d285c141a86053687)

I doubt it matters, but for consistency with the enable wallet option, it might make sense to add these arguments argsman.AddHiddenArgs when use_bdb is disabled, so configuation files specifying these options aren’t rejected

achow101 commented at 6:37 pm on November 6, 2020:

Done in upstream #20202

in src/wallet/walletdb.cpp:1069 in 0cbff93037 outdated

1062@@ -1059,8 +1063,11 @@ std::unique_ptr<WalletDatabase> MakeDatabase(const fs::path& path, const Databas
1063 #else
1064     assert(format != DatabaseFormat::SQLITE);
1065 #endif
1066-
1067+#ifdef USE_BDB
1068     return MakeBerkeleyDatabase(path, options, status, error);
1069+#else
1070+    return nullptr;

ryanofsky commented at 10:39 pm on November 5, 2020:

In commit “Do not compile BDB things when USE_BDB is defined” (0cbff9303729dddaad781a4d285c141a86053687)

In MakeDatabase:

It doesn’t seem ideal to return null with success status and no error message if bdb isn’t available.
It also doesn’t seem ideal to assert false above if sqlite isn’t available.
It also puts an unnecessary burden on callers that don’t care which database backend is used to have to write code figuring out on their own what backends are available before calling this function. (Code added above in wallet_tests.cpp is an example).

All of this seems easy to fix. Would suggest:

 0    // If format not specified or detected, choose default format based on what
 1    // is available, and prefer BDB over SQLite.
 2    if (!format) {
 3#ifdef USE_SQLITE
 4        format = DatabaseFormat::SQLITE;
 5#endif
 6#ifdef USE_BDB
 7        format = DatabaseFormat::BDB;
 8#endif
 9    }
10
11    if (format == DatabaseFormat::SQLITE) {
12#ifdef USE_SQLITE
13        return MakeSQLiteDatabase(path, options, status, error);
14#endif
15        error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support SQLite database format.", path.string()));
16        status = DatabaseStatus::FAILED_BAD_FORMAT;
17        return nullptr;
18    }
19
20#ifdef USE_BDB
21    return MakeBerkeleyDatabase(path, options, status, error);
22#endif
23    error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", path.string()));
24    status = DatabaseStatus::FAILED_BAD_FORMAT;
25    return nullptr;

achow101 commented at 6:37 pm on November 6, 2020:

Done in upstream #20202

in src/wallet/test/wallet_tests.cpp:44 in 0cbff93037 outdated

40@@ -41,6 +41,11 @@ static std::shared_ptr<CWallet> TestLoadWallet(interfaces::Chain& chain)
41     DatabaseStatus status;
42     bilingual_str error;
43     std::vector<bilingual_str> warnings;
44+#ifdef USE_BDB

ryanofsky commented at 10:45 pm on November 5, 2020:

In commit “Do not compile BDB things when USE_BDB is defined” (0cbff9303729dddaad781a4d285c141a86053687)

This new code can be deleted with the suggested change to MakeDatabase above. The MakeDatabase function can figure out what formats are available without putting the burden on callers.

achow101 commented at 6:37 pm on November 6, 2020:

Done in upstream #20202

in test/functional/test_framework/test_framework.py:834 in d97e22f524 outdated

829+        """
830+        Determine whether descriptors should be used based on compilation support and option
831+        Should only be used for tests that explicitly check self.is_wallet_compiled()
832+        """
833+        descriptors = self.options.descriptors
834+        if not self.is_bdb_compiled() and self.is_sqlite_compiled() and not descriptors:

ryanofsky commented at 10:57 pm on November 5, 2020:

In commit “Add a helper to determine whether descriptors should be used” (d97e22f5244cb913ef84953309a3eaee07f54c1c)

I haven’t looked yet at how this function is used, but it seem dangerous if it will be used to silently override --descriptors or --legacy-wallet options that were specified. This would be bad because it would give the impression that requested tests passed when in reality they were never run, and different tests were done instead. A safer alternative would set self.option.descriptors to None when no option is passed, True when --descriptors is passed, and False when --legacy is passed. Then logic like the following would be used to determine how to run tests:

0descriptors = self.option.descriptors
1if descriptor is None:
2    # Current default is to use legacy instead of descriptor wallets unless bdb is unavailable
3    descriptors = not self.is_bdb_compiled()
4if descriptors and not self.is_sqlite_compiled():
5    raise SkipTest("sqlite has not been compiled.")
6if not descriptors and not self.is_bdb_compiled():
7    raise SkipTest("bdb has not been compiled.")
8return descriptors

achow101 commented at 9:29 pm on November 6, 2020:

This isn’t used to skip tests.

I’ve made a change that somewhat incorporates what you suggest.

ryanofsky commented at 5:29 pm on November 6, 2020: member

Started review (will update list below with progress).

PR title “Disable and fix tests for when BDB is not compiled” does not seem accurate. This goes beyond allowing BDB to be disabled and disabling corresponding tests, but also enables enables new tests and makes fixes not connected to BDB (e.g. “Fix mock SQLiteDatabases” and “Fix change detection of imported internal descriptors.”)

d4892a3415d8668d6524c2eb843a639c06d317fc Include wallet/bdb.h where it is actually being used (1/20)
0cbff9303729dddaad781a4d285c141a86053687 Do not compile BDB things when USE_BDB is defined (2/20)
cec77056b0945522c0305db8d4754614bfa66009 Enforce salvage is only for BDB wallets (3/20)
eb2e1680044484833c0e82b133678a77d8660233 RPC: Require descriptors=True for createwallet when BDB is not compiled (4/20)
98e8752fa3f8332f73a3e75436523a790364b0ce GUI: Force descriptor wallets when BDB is not compiled (5/20)
047d4e3df48f54f1063914cca9e000da33064b01 Allow disabling BDB in configure with –without-bdb (6/20)
f864402058b4a1a642001bc87d9ef1ce9603f81d Fix mock SQLiteDatabases (7/20)
d5157cfa759d2c24c98cda8ae35181c5acbee6b4 Fix change detection of imported internal descriptors (8/20)
d97e22f5244cb913ef84953309a3eaee07f54c1c Add a helper to determine whether descriptors should be used (9/20)
235bbb7f3f5bba9a72c4e452728ca9ec1b5d5eec Add skip_if_no_bdb and is_bdb_compiled helpers (10/20)
8a0f9fa368236e51b3722ee8f39ba4aca3b78449 Skip legacy wallet reliant tests if BDB is not compiled (11/20)
99f8ffac9c4c16677a9ee39351ba5b5ec299edbb tests: Don’t make any wallets unless wallet is required (12/20)
49e9ee040bb4fdb51c0b5ce10d2e5d4cdf1a5c77 Disable wallet_descriptor.py bdb format check if BDB is not compiled (13/20)
d53208cf7db7a3d92f4b701cdcdf7c9df0fbee42 Disable upgrades tests that require BDB if BDB is not compiled (14/20)
5fcf045e3299c3e0c6fd7a5d780bf4106c71b334 Have feature_filelock.py test both bdb and sqlite, depending on compiled (15/20)
cb126d43dfe3eb324405d9dccf2e5288af7592c7 Setup wallets with descriptors for feature_notifications (16/20) I- [ ] bd9d5a844ba4150f5d4988104decc46803fe78ad Setup wallets for interface_bitcoin_cli.py (17/20)
02b0ee670a3b4ce450ab1479a2fa56f6080c9c91 Handle when bdb is disabled in rpc_deprecated (18/20)
482ec906b266973d76bd70385621c26136c213eb Use MiniWallet in rpc_net.py (19/20)
cb3d26b317556737f3d2652494b4604b22a82ed0 Setup wallets for interface_zmq.py (20/20)

achow101 force-pushed on Nov 6, 2020

achow101 commented at 9:30 pm on November 6, 2020: member

Changed to automatically switch to using descriptor wallets if BDB is not compiled and --legacy-wallet is not compiled. This requires that several tests have --legacy-wallet be added in test_runner.py. Additionally, this broke wallet_send.py so those tests have to be updated to work with descriptor wallets. wallet_upgradewallet.py also had to be restricted to just a legacy wallet test.

Doing this switching means that we won’t skip a bunch of tests when BDB is not compiled. It also makes it possible to test all of our tests with both wallet types.

in build_msvc/bitcoin_config.h:42 in caf3062e62 outdated

37@@ -38,6 +38,12 @@
38 /* Define to 1 to enable wallet functions */
39 #define ENABLE_WALLET 1
40 
41+/* Define to 1 to enable BDB wallet */
42+#define USE_BDB 1
43+
44+/* Define to 1 to enable SQLite wallet */
45+#define USE_SQLITE 1

luke-jr commented at 3:04 pm on November 13, 2020:

Should we be adding this for 0.21?

ryanofsky commented at 3:15 pm on November 13, 2020:

Should we be adding this for 0.21?

No opinion (seems fine, not necessary), but #define USE_SQLITE 1 is part of base PR #20202. @achow101, PR description could be clearer and say This is based on #20202. or something along those lines since it seems like I’m not the only person to not realize this PR contains changes from another PR

in src/wallet/salvage.cpp:31 in caf3062e62 outdated

27@@ -27,6 +28,7 @@ bool RecoverDatabaseFile(const fs::path& file_path, bilingual_str& error, std::v
28     DatabaseStatus status;
29     options.require_existing = true;
30     options.verify = false;
31+    options.require_format = DatabaseFormat::BERKELEY;

luke-jr commented at 3:15 pm on November 13, 2020:

Is this another bugfix we should put in 0.21?

achow101 commented at 4:55 pm on November 13, 2020:

It’s not strictly necessary. Other stuff later like trying to actually open the wallet file if it isn’t BDB will still fail and we won’t do anything to the file. It will just have a slightly worse error message.

in doc/build-unix.md:45 in caf3062e62 outdated

40@@ -41,7 +41,7 @@ Optional dependencies:
41  Library     | Purpose          | Description
42  ------------|------------------|----------------------
43  miniupnpc   | UPnP Support     | Firewall-jumping support
44- libdb4.8    | Berkeley DB      | Wallet storage (only needed when wallet enabled)
45+ libdb4.8    | Berkeley DB      | Optional, wallet storage (only needed when wallet enabled)

ysangkok commented at 9:42 pm on November 13, 2020:

the sentence makes it sound like it is optional because the wallet can be disabled. wouldn’t it be better to say “Optional. Needed to open legacy wallets.”

luke-jr commented at 10:39 pm on November 13, 2020:

Probably shouldn’t use the word “legacy” (at least user-facing) while it isn’t actually legacy yet.

DrahtBot added the label Needs rebase on Nov 17, 2020

achow101 force-pushed on Nov 17, 2020

DrahtBot removed the label Needs rebase on Nov 17, 2020

DrahtBot added the label Needs rebase on Nov 18, 2020

achow101 force-pushed on Nov 18, 2020

DrahtBot removed the label Needs rebase on Nov 18, 2020

DrahtBot added the label Needs rebase on Nov 23, 2020

achow101 force-pushed on Nov 23, 2020

DrahtBot removed the label Needs rebase on Nov 23, 2020

ryanofsky commented at 3:49 pm on December 2, 2020: member

re: #20267#issue-512655353

Fixes the tests for #20202. When BDB is not compiled, the wallet tests that rely on the legacy wallet will be disabled. Other tests not necessarily requiring the wallet, but can optionally test behavior with the wallet, are updated to handle the different wallet types. One test, rpc_net.py, which only uses the wallet to setup a few things, is changed to use the test framework’s MiniWallet instead.

This PR is separate from and based on #20202 because it needs a few other PRs to make the tests work: #18788, #20262, and #20266

Starting to look at this but can the description be updated to maybe strip out the references to old PRs and say what behavior change this PR is implementing? What happens with test runner currently, and what happens after this PR? What happens running individual BDB tests currently, and what happens after?

I’m used to test runner showing a table of all tests Passed or Failed or Skipped. Here, it seems like in the absence of BDB some tests are partially skipped so I would expect to see some new Passed (partial) status instead of plain Passed. Or to avoid this, I’d expect these tests to be split and run twice with --legacy-wallet and --descriptors, so there is never any partially passed test only fully passed or fully skipped.

I guess I’m just confused about what the exact goal is for this PR, so it would be good to spell out the new behavior.

I’m also confused about why some code is looking at self.options.descriptors, and other code is calling self.get_use_descriptors(). And self.get_use_descriptors() is using self.options.descriptors internally but there is also self.options.descriptors = self.get_use_descriptors() so some kind of caching or recursive dependency? This might be worth simplifying or explaining some place.

achow101 force-pushed on Dec 2, 2020

achow101 commented at 5:37 pm on December 2, 2020: member

Starting to look at this but can the description be updated to maybe strip out the references to old PRs and say what behavior change this PR is implementing? What happens with test runner currently, and what happens after this PR? What happens running individual BDB tests currently, and what happens after?

Updated the OP.

I’m used to test runner showing a table of all tests Passed or Failed or Skipped. Here, it seems like in the absence of BDB some tests are partially skipped so I would expect to see some new Passed (partial) status instead of plain Passed. Or to avoid this, I’d expect these tests to be split and run twice with --legacy-wallet and --descriptors, so there is never any partially passed test only fully passed or fully skipped.

We already have tests which are “partial” as some parts are not tested if the wallet is not compiled. If we do want to have a “partial” indicator for tests where we disable components based on compiled support, then I think it should be done in a followup.

I’m also confused about why some code is looking at self.options.descriptors, and other code is calling self.get_use_descriptors(). And self.get_use_descriptors() is using self.options.descriptors internally but there is also self.options.descriptors = self.get_use_descriptors() so some kind of caching or recursive dependency? This might be worth simplifying or explaining some place.

This is a holdover from a previous iteration. I’ve removed the extra uses of self.get_use_descriptors() and just use self.options.descriptors. The purpose and behavior of self.get_use_descriptors is described in the OP.

laanwj commented at 1:36 pm on December 10, 2020: member

Concept ACK

ryanofsky commented at 1:13 am on December 16, 2020: member

Thanks for updates. Init sequence still seems too complicated. I don’t see a reason options can’t be set once simply in the constructor. In the current implementation cf5de8ec59ac6e2a49c728b886eb5d46d9705d1e:

self.options.descriptors value changes over time and is set to the return value of a function which circularly reads self.options.descriptors
default_wallet_name can’t be read/written from setup_params and is a dynamic property executing code and confusingly depending on the changing self.options.descriptors value

The following cleanups seem to work all right:

 0diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
 1index c0289bf491e..76b3815d25b 100755
 2--- a/test/functional/test_framework/test_framework.py
 3+++ b/test/functional/test_framework/test_framework.py
 4@@ -102,6 +102,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 5         self.supports_cli = True
 6         self.bind_to_localhost_only = True
 7         self.parse_args()
 8+        self.default_wallet_name = "default_wallet" if self.options.descriptors else ""
 9         self.wallet_data_filename = "wallet.dat"
10         # Optional list of wallet names that can be set in set_test_params to
11         # create and import keys to. If unset, default is len(nodes) *
12@@ -118,10 +119,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
13             self.options.timeout_factor = 99999
14         self.rpc_timeout = int(self.rpc_timeout * self.options.timeout_factor) # optionally, increase timeout by a factor
15 
16- [@property](/bitcoin-bitcoin/contributor/property/)
17-    def default_wallet_name(self):
18-        return "default_wallet" if self.options.descriptors else ""
19-
20     def main(self):
21         """Main function. This should not be overridden by the subclass test scripts."""
22 
23@@ -201,6 +198,18 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
24         self.options = parser.parse_args()
25         self.options.previous_releases_path = previous_releases_path
26 
27+        config = configparser.ConfigParser()
28+        config.read_file(open(self.options.configfile))
29+        self.config = config
30+
31+        if self.options.descriptors is None:
32+            # Prefer BDB unless it isn't available
33+            if self.is_bdb_compiled():
34+                self.options.descriptors = False
35+            elif self.is_sqlite_compiled():
36+                self.options.descriptors = True
37+            # If neither are compiled, tests requiring a wallet will be skipped and the value of self.options.descriptors won't matter
38+
39     def setup(self):
40         """Call this method to start up the test framework object with options set."""
41 
42@@ -210,9 +219,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
43 
44         self.options.cachedir = os.path.abspath(self.options.cachedir)
45 
46-        config = configparser.ConfigParser()
47-        config.read_file(open(self.options.configfile))
48-        self.config = config
49+        config = self.config
50+
51         fname_bitcoind = os.path.join(
52             config["environment"]["BUILDDIR"],
53             "src",
54@@ -231,9 +239,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
55             os.path.join(config['environment']['BUILDDIR'], 'src', 'qt'), os.environ['PATH']
56         ])
57 
58-        # Determine descriptors default
59-        self.options.descriptors = self.get_use_descriptors()
60-
61         # Set up temp directory and start logging
62         if self.options.tmpdir:
63             self.options.tmpdir = os.path.abspath(self.options.tmpdir)
64@@ -847,14 +852,3 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
65     def is_bdb_compiled(self):
66         """Checks whether the wallet module was compiled with BDB support."""
67         return self.config["components"].getboolean("USE_BDB")
68-
69-    def get_use_descriptors(self):
70-        descriptors = self.options.descriptors
71-        if descriptors is None:
72-            # Prefer BDB unless it isn't available
73-            if self.is_bdb_compiled():
74-                descriptors = False
75-            elif self.is_sqlite_compiled():
76-                descriptors = True
77-            # If neither are compiled, tests requiring a wallet will be skipped and the value of self.options.descriptors won't matter
78-        return descriptors

Would be great to see these changes backported so this PR is easy to understand and review without @property and get_use_descriptors complications

achow101 force-pushed on Jan 9, 2021

achow101 commented at 0:33 am on January 9, 2021: member

Included @ryanofsky’s suggestions. Also rebased to fix two silent merge conflicts.

in test/functional/test_framework/test_framework.py:203 in 86b3c3d84b outdated

197         self.options = parser.parse_args()
198         self.options.previous_releases_path = previous_releases_path
199 
200+        config = configparser.ConfigParser()
201+        config.read_file(open(self.options.configfile))
202+        self.config = config

ryanofsky commented at 8:46 pm on January 13, 2021:

In commit “tests: Set descriptors default based on compilation” (86b3c3d84bd57dffb3b5e47cd1a076bec62c7e92)

Would be good to move this to the bottom of the function so it is better organized. Set self.options then self.config instead of options then config then options again

achow101 commented at 1:09 am on January 26, 2021:

The is_*_compiled checks require self.config to already exist, so it must come before them and can’t be at the bottom of the function.

in test/functional/test_framework/test_framework.py:208 in 86b3c3d84b outdated

205+            # Prefer BDB unless it isn't available
206+            if self.is_bdb_compiled():
207+                self.options.descriptors = False
208+            elif self.is_sqlite_compiled():
209+                self.options.descriptors = True
210+            # If neither are compiled, tests requiring a wallet will be skipped and the value of self.options.descriptors won't matter

ryanofsky commented at 8:48 pm on January 13, 2021:

In commit “tests: Set descriptors default based on compilation” (86b3c3d84bd57dffb3b5e47cd1a076bec62c7e92)

If this comment is true, could maybe write:

0else:
1   del self.options.descriptors

To be sure the value is not used

achow101 commented at 1:09 am on January 26, 2021:

Done

achow101 commented at 5:53 pm on January 27, 2021:

Reverted this since it is causing problems when configured with --disable-wallet

in test/functional/test_framework/test_framework.py:417 in 972c9cf8a4 outdated

413@@ -411,6 +414,8 @@ def import_deterministic_coinbase_privkeys(self):
414             self.init_wallet(i)
415 
416     def init_wallet(self, i):
417+        if not self.requires_wallet:

ryanofsky commented at 8:57 pm on January 13, 2021:

In commit “tests: Don’t make any wallets unless wallet is required” (972c9cf8a4e2e27bd28a33630690404a1d3078d4)

If possible, it would seem clearer to just not call init_wallet() when requires_wallet is false than to call it and have it succeed without doing anything

achow101 commented at 1:09 am on January 26, 2021:

Done.

in test/functional/wallet_descriptor.py:26 in 297ab0e699 outdated

22@@ -23,11 +23,12 @@ def skip_test_if_missing_module(self):
23         self.skip_if_no_sqlite()
24 
25     def run_test(self):
26-        # Make a legacy wallet and check it is BDB
27-        self.nodes[0].createwallet(wallet_name="legacy1", descriptors=False)
28-        wallet_info = self.nodes[0].getwalletinfo()
29-        assert_equal(wallet_info['format'], 'bdb')
30-        self.nodes[0].unloadwallet("legacy1")
31+        if self.is_bdb_compiled():

ryanofsky commented at 9:12 pm on January 13, 2021:

In commit “Disable wallet_descriptor.py bdb format check if BDB is not compiled” (297ab0e699b26f9742dc6ebf75be1dd857d1e531)

What is the long term plan for these skipped checks? Personally I think it is bad to report that tests pass when parts of them are just being skipped. Do you disagree? I can think of two different ways we might address this in future PRs:

Add a “partially skipped” test result in test runner, extending the current “passed” / “failed” / “skipped” possibilities.
Move these skipped sections into separate tests, so each individual python test either passes, fails, or is skipped and there are no partially skipped tests.

I’d be curious if other reviewers have opinions, or think this is not a big deal.

As a short-term suggestion for this PR, I think it would be nice to at least add log.info or log.warning("Skipping BDB test") so running the tests individually at least gives better status information.

For this particular test, it might make sense to move this section to a wallet_legacy.py test mirroring wallet_descriptor.py and calling skip_if_no_bdb instead of skip_if_no_sqlite

achow101 commented at 11:57 pm on January 25, 2021:

I think that the optimal solution for these is to split them into separate tests for wallet and non-wallet components.

There are some other tests, notable feature_notifications.py and interface_zmq.py which have a significant portion of tests specifically for the wallet.

in test/functional/feature_filelock.py:46 in 49500debf0 outdated

46+                self.nodes[1].assert_start_raises_init_error(extra_args=['-walletdir={}'.format(wallet_dir), '-wallet=' + wallet_name, '-noserver'], expected_msg=expected_msg, match=ErrorMatch.PARTIAL_REGEX)
47+
48+            if self.is_bdb_compiled():
49+                check_wallet_filelock(False)
50+            if self.is_sqlite_compiled():
51+                check_wallet_filelock(True)

ryanofsky commented at 10:05 pm on January 13, 2021:

In commit “Have feature_filelock.py test both bdb and sqlite, depending on compiled” (49500debf06842ad0ee1499003c232c9a1c16597)

It seems like this is adding an unnecessary level of complexity. We already have a mechanism for running the same test without and without descriptors: we make the test respect the --descriptors option, and list the test twice in test_runner with and without the descriptors option.

I don’t know if this is the best mechanism, but it is one we already have, and it seems like it could work as just as well here as it does other places. If it’s not sufficient, it would be good to figure out some other another way of running tests twice that could be used consistently.

Another related problem with the test after this change is the way it treats --descriptors/--legacy-wallet options differently than other tests (It effectively ignores them).

achow101 commented at 0:02 am on January 26, 2021:

#20892 runs such tests twice. It also seems like this should be one of those tests that the wallet portion is split into a separate test.

in test/functional/feature_notifications.py:53 in 7c60f35da3 outdated

48@@ -49,6 +49,30 @@ def setup_network(self):
49         super().setup_network()
50 
51     def run_test(self):
52+        if self.is_wallet_compiled():
53+            # Setup the descriptors to be imported to the wallet

ryanofsky commented at 10:27 pm on January 13, 2021:

In commit “Setup wallets with descriptors for feature_notifications” (7c60f35da3117af7d6a0b8a1e4756c5df907bd1c)

Comment below “Give node 0 same wallet seed as node 1” should be moved here to reflect what this code is doing (and what the previous code is no longer doing)

achow101 commented at 1:10 am on January 26, 2021:

Done

in test/functional/feature_notifications.py:55 in 7c60f35da3 outdated

48@@ -49,6 +49,30 @@ def setup_network(self):
49         super().setup_network()
50 
51     def run_test(self):
52+        if self.is_wallet_compiled():
53+            # Setup the descriptors to be imported to the wallet
54+            seed = "cTdGmKFWpbvpKQ7ejrdzqYT2hhjyb3GPHnLAK7wdi5Em67YLwSm9"
55+            xpriv = "tprv8ZgxMBicQKsPfHCsTwkiM1KT56RXbGGTqvc2hgqzycpwbHqqpcajQeMRZoBD35kW4RtyCemu6j34Ku5DEspmgjKdt2qe4SvRch5Kk8B8A2v"

ryanofsky commented at 10:45 pm on January 13, 2021:

In commit “Setup wallets with descriptors for feature_notifications” (7c60f35da3117af7d6a0b8a1e4756c5df907bd1c)

Is there any way this test setup can be simplified, or do we have any ideas about how this might be simplified in the future if it is not worth simplifying now? The old setup was trivial: there were two wallets and one wallet had a sethdseed call to use the same seed as the other wallet.

Now we need hardcoded private keys, base64 strings, descriptor strings, hd paths, and checksums, wallet names, create calls, load_on_startup and timestamps options and internal/external chains just to get two wallets to use the same seed.

Maybe it belongs in a helper function, but it seems like it should be more straightforward to take a seed from one wallet and put it in another wallet.

achow101 commented at 0:30 am on January 26, 2021:

Until we implement private descriptor export, that won’t be possible.

in test/functional/interface_bitcoin_cli.py:67 in 02b69ded2f outdated

62@@ -63,6 +63,8 @@ def run_test(self):
63 
64         self.log.info("Test -getinfo returns expected network and blockchain info")
65         if self.is_wallet_compiled():
66+            self.nodes[0].createwallet(wallet_name=self.default_wallet_name, descriptors=self.options.descriptors)
67+            self.nodes[0].importprivkey(privkey=self.nodes[0].get_deterministic_priv_key().key, label='coinbase')

ryanofsky commented at 11:39 pm on January 13, 2021:

In commit “Setup wallets for interface_bitcoin_cli.py” (02b69ded2f8d2fe8276b63a9faa134623fc82ed5)

Correct if I’m wrong, but this seems like a workaround to deal with init_wallet setup now being skipped because this test does not call skip_if_no_wallet().

I think ideally it would be better to split this test up so wallet and non-wallet parts are in separate tests and skipped checks are clearly reported skipped. But in meantime, would something the following work in set_test_params instead of duplicating init_wallet code?

0if self.is_wallet_compiled():
1    self.requires_wallet = True

achow101 commented at 1:10 am on January 26, 2021:

Done

in test/functional/rpc_net.py:53 in 55c71320b6 outdated

48@@ -48,6 +49,8 @@ def set_test_params(self):
49         self.supports_cli = False
50 
51     def run_test(self):
52+        self.wallet = MiniWallet(self.nodes[0])

ryanofsky commented at 0:05 am on January 14, 2021:

In commit “Have feature_filelock.py test both bdb and sqlite, depending on compiled” (49500debf06842ad0ee1499003c232c9a1c16597)

This is great! Hope to see more of usage of MiniWallet. But it would be good to have a comment below where the miniwallet is used, saying what it is testing. I can’t figure it out. If the test never required a wallet and would pass whether or not sendtoaddress was called previously, what is the send_self_transfer call doing now and is it actually testing something?

achow101 commented at 1:10 am on January 26, 2021:

Added a comment. It is only used to generate a transaction so that last_transaction in getpeerinfo can be checked.

in test/functional/interface_zmq.py:75 in c8a809bd3b outdated

67@@ -68,6 +68,12 @@ def skip_test_if_missing_module(self):
68         self.skip_if_no_bitcoind_zmq()
69 
70     def run_test(self):
71+        if self.is_wallet_compiled():
72+            self.nodes[0].createwallet(wallet_name=self.default_wallet_name, descriptors=self.options.descriptors, load_on_startup=True)
73+            self.nodes[0].importprivkey(privkey=self.nodes[0].get_deterministic_priv_key().key, label='coinbase')
74+            self.nodes[1].createwallet(wallet_name=self.default_wallet_name, descriptors=self.options.descriptors, load_on_startup=True)
75+            self.nodes[1].importprivkey(privkey=self.nodes[1].get_deterministic_priv_key().key, label='coinbase')

ryanofsky commented at 0:16 am on January 14, 2021:

In commit “Setup wallets for interface_zmq.py” (c8a809bd3bc22707d336b16dc883ae7f35ab8fd3)

Is this another workaround for init_wallet setup now being skipped? Could this simply do if self.is_wallet_compiled(): self.requires_wallet = True in set_test_params as suggested previously? (Or divided into wallet/nonwallet tests using skip_if_no_wallet)

achow101 commented at 1:10 am on January 26, 2021:

Yes. Done

in test/functional/tool_wallet.py:354 in c191614587 outdated

347@@ -348,7 +348,8 @@ def test_dump_createfromdump(self):
348 
349         self.log.info('Checking createfromdump')
350         self.do_tool_createfromdump("load", "wallet.dump")
351-        self.do_tool_createfromdump("load-bdb", "wallet.dump", "bdb")
352+        if self.is_bdb_compiled():
353+            self.do_tool_createfromdump("load-bdb", "wallet.dump", "bdb")
354         if self.is_sqlite_compiled():
355             self.do_tool_createfromdump("load-sqlite", "wallet.dump", "sqlite")

ryanofsky commented at 0:22 am on January 14, 2021:

In commit “tests: Disable bdb dump test when no bdb” (c191614587f138c00277c99a9709dedaf8bbcfd7)

I guess this is another place where future test splitting or test runner improvements could provide feedback that test didn’t fully pass and checks were skipped.

in test/functional/mempool_compatibility.py:26 in 5a985cd665 outdated

22@@ -23,6 +23,7 @@ class MempoolCompatibilityTest(BitcoinTestFramework):
23     def set_test_params(self):
24         self.num_nodes = 2
25         self.wallet_names = [None]
26+        self.requires_wallet = True

ryanofsky commented at 0:35 am on January 14, 2021:

In commit “tests: mempool_compatibility needs requires_wallet set” (5a985cd6656afe976f73b3276017e22055a8050c)

Maybe it would be good to rename requires_wallet to something like init_default_wallet to more accurately reflect what the variable does. Also, requires_wallet = False seems basically equivalent to default_wallet_name = False and maybe the two variables should be combined at some point (though it seems like a bigger setup change that would be better in some cleanup PR)

achow101 commented at 1:12 am on January 26, 2021:

Perhaps, but that’s a bit more complicated.

S3RK commented at 10:03 am on February 4, 2021:

I can’t understand why do we need “the old versions have their wallets’ setup”. I’ve tried to run this test with the flag set to False and it makes no difference

achow101 commented at 5:39 pm on February 4, 2021:

It’s redundant now. Previously this variable was checked inside of init_wallet() which was called by import_deterministic_coinbase_privkeys() so we needed to set it to True to allow our call of that to work. But since checking requires_wallet is moved outside of that, it is no longer needed.

Removed and dropped the commit.

in test/functional/wallet_send.py:288 in 1fa9bbdb40 outdated

288-        self.test_send(from_wallet=w4, to_wallet=w1, amount=1, expect_error=(-4, "Insufficient funds"))
289-        res = self.test_send(from_wallet=w4, to_wallet=w1, amount=1, include_watching=True, add_to_wallet=False)
290-        res = w2.walletprocesspsbt(res["psbt"])
291-        assert res["complete"]
292+        if not self.options.descriptors:
293+            # This tests legacy watch-only behavior only.

ryanofsky commented at 0:47 am on January 14, 2021:

In commit “Fix wallet_send.py wallet setup to work with descriptors” (1fa9bbdb4076944272d5246d1140d29ba3ecb3a3)

Can comment be expanded to say why? My understanding is something like “Unlike legacy wallets, descriptor wallets don’t allow watch-only keys and spendable keys in the same wallet.” If this is the case, it’d be helpful to say explicitly. It would also be good to say whether or not it would be useful to be test the watch-only behavior here in the descriptor case (presumably just using 2 wallets instead of 1). If this would not be useful to test because there is already other coverage or watch-only descriptor stuff, that would be good to know.

achow101 commented at 1:12 am on January 26, 2021:

Added a comment.

in test/functional/wallet_send.py:240 in 1fa9bbdb40 outdated

257         self.nodes[0].generate(1)
258         self.sync_blocks()
259 
260+        if not self.options.descriptors:
261+            # w4 has private keys enabled, but only contains watch-only keys (from w2)
262+            # This is legacy wallet behavior only

ryanofsky commented at 0:53 am on January 14, 2021:

In commit “Fix wallet_send.py wallet setup to work with descriptors” (1fa9bbdb4076944272d5246d1140d29ba3ecb3a3)

Would be good to clarify with a reason like “Descriptor wallets do not allow spendable keys and watch only keys in the same wallet” (if this is the case).

achow101 commented at 1:12 am on January 26, 2021:

Done

in test/functional/wallet_send.py:174 in 1fa9bbdb40 outdated

168@@ -168,49 +169,91 @@ def run_test(self):
169         self.nodes[1].createwallet(wallet_name="w1")
170         w1 = self.nodes[1].get_wallet_rpc("w1")
171         # w2 contains the private keys for w3
172-        self.nodes[1].createwallet(wallet_name="w2")
173+        self.nodes[1].createwallet(wallet_name="w2", blank=True)
174         w2 = self.nodes[1].get_wallet_rpc("w2")
175+        xpriv = "tprv8ZgxMBicQKsPfHCsTwkiM1KT56RXbGGTqvc2hgqzycpwbHqqpcajQeMRZoBD35kW4RtyCemu6j34Ku5DEspmgjKdt2qe4SvRch5Kk8B8A2v"

ryanofsky commented at 1:05 am on January 14, 2021:

In commit “Fix wallet_send.py wallet setup to work with descriptors” (1fa9bbdb4076944272d5246d1140d29ba3ecb3a3)

This seems like the same code earlier copied from the feature_notifications test. You can see earlier comment, but I think it would be better to have a framework or util function that can easily create wallets with the same seed. (It would also seem better not to hard code that seed, but the hardcoding is less a problem than the complexity and boilerplate these changes add to the tests)

achow101 commented at 1:12 am on January 26, 2021:

Done

ryanofsky approved

ryanofsky commented at 1:28 am on January 14, 2021: member

Code review ACK 5a985cd6656afe976f73b3276017e22055a8050c. I think this PR is a nice improvement, getting tests less tied to the legacy wallet and also making test init order more rational and making behavior more explicit. I did leave a lot of suggestions for improvement and think it would be great if you could look into them.

re: Summary #20267#issue-512655353

Fixes the tests for #20202.

Ideally the PR would have a summary that doesn’t require digging into other issues, and indicate up front why would reviewers might be interested in this PR. Would change this to something like: “This PR fixes tests that currently fail when bitcoin is configured with the --without-bdb option, which was recently added in #20202.”

When BDB is not compiled, the wallet tests that rely on the legacy wallet will be disabled. Other tests not necessarily requiring the wallet, but can optionally test behavior with the wallet, are updated to handle the different wallet types. One test, rpc_net.py, which only uses the wallet to setup a few things, is changed to use the test framework’s MiniWallet instead.

For the majority of tests, changes are made so that they can be run with either legacy wallets or descriptor wallets without materially effecting the test. Most tests only need the wallet for balance and transactions, so the type of wallet is not an important part of those tests.

The tests which rely on or test specific legacy wallet behavior are disabled or skipped if BDB is not compiled. Since these tests can be testing for general wallet things as well, the test file itself may not always be outright skipped, but rather some components of the tests skipped.

This is very jumbled. It talks about disabled tests two places (at the beginning and end) and buries the most helpful section about the majority of tests in the middle. It also oddly describes the majority of tests strategy after describing one special case of it.

I think a better way to structure this would be. “For the most part, this PR tries to update tests which depend on legacy behavior, to work with both legacy and descriptor wallets. In one case, rpc_net.py this was done by […]. In cases where tests are more tied to legacy features, whole tests or portions of tests are skipped.”

Lastly a helper function get_use_descriptors() is added which determines whether descriptors should be used. It returns whatever was given in the command line. If no command line option was given, it will default to BDB if it as compiled, otherwise it will use SQLite (descriptors). If neither are compiled, then the wallet should be disabled and the option won’t be used. get_use_descriptors is used to set the value for self.options.descriptors when neither --legacy-wallet nor --descriptors is given.

This would be a helpful comment to have in the code on the get_use_descriptors function but I think it is out of place here and better to drop from the PR description.

DrahtBot added the label Needs rebase on Jan 19, 2021

achow101 force-pushed on Jan 26, 2021

achow101 commented at 1:15 am on January 26, 2021: member

Rebased. I think there are improvements that can be done in followup PRs. For example, #20892 implements running a test twice. But for now, I would like for this to be merged so that we can get more test coverage of descriptor wallets.

DrahtBot removed the label Needs rebase on Jan 26, 2021

laanwj commented at 1:16 pm on January 27, 2021: member

CI fail looks like an actual issue:

0.........Traceback (most recent call last):
1  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/create_cache.py", line 27, in <module>
2    CreateCache().main()
3  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 105, in __init__
4    self.default_wallet_name = "default_wallet" if self.options.descriptors else ""
5AttributeError: 'Namespace' object has no attribute 'descriptors'

tests: Set descriptors default based on compilation

Determines whether descriptors should be used based on whether the
--descriptor or --legacy-wallet option is set,
and the compiled support. If no option is set and both BDB and SQLite
are available, it defaults to legacy.

This is used to switch descriptor agnostic tests between descriptors and
legacy wallet.

6f36242389

Skip legacy wallet reliant tests if BDB is not compiled b9b88f57a9

tests: Don't make any wallets unless wallet is required 3641597d7e

Disable wallet_descriptor.py bdb format check if BDB is not compiled 1f20cac9d4

Disable upgrades tests that require BDB if BDB is not compiled c77975abc0

Have feature_filelock.py test both bdb and sqlite, depending on compiled 1f1bef8dba

Setup wallets with descriptors for feature_notifications 7c71c627d2

Setup wallets for interface_bitcoin_cli.py 4de23824b0

Use MiniWallet in rpc_net.py 4d03ef9a73

Setup wallets for interface_zmq.py 09514e1bef

Explicitly mark legacy wallet tests as such

Some tests are intended to test only legacy wallet behavior. With
automatic switching of wallet type, we need to make them explicit

b1b679e0ab

Require legacy wallet for wallet_upgradewallet.py fbaea7bfe4

Fix wallet_send.py wallet setup to work with descriptors

Fixes the wallet setup so this test works with descriptor wallets. Also
enabled explicit descriptor and legacy wallet testing in the test
runner.

1194cf9269

tests: Disable bdb dump test when no bdb 49797c3ccf

achow101 commented at 5:53 pm on January 27, 2021: member

CI fail looks like an actual issue:

Should be resolved now.

achow101 force-pushed on Jan 27, 2021

ryanofsky approved

ryanofsky commented at 4:07 pm on February 2, 2021: member

Code review ACK a831ba6014827d5afc9b423773361350359c94d5. Changes since last review: implementing many review suggestions (thanks!) adding comments, tweaking logic in small ways, and simplifying the PR description. Also dropped rpc_deprecated.py commit 42b627a3a3ab8ee30db5fa3f6b14be5869c2fcfe no longer needed after #20891

Would encourage others to review this. There are a lot of commits but almost all the commits are trivial (make sure to tick the ignore whitespace option).

Two things it would be good to clean up later:

It would be good to remove duplicate code added to wallet_send and feature_notifications tests (https://github.com/bitcoin/bitcoin/pull/20267#discussion_r556927365 and #20267 (review)) that creates different descriptor wallets with the same seeds. From comments it sounds like when more descriptor import stuff is implemented this code can be simplified by reusing existing seeds instead of hardcoding new seeds, but it seems like the tests could be more readable and maintainable even before this if they were refactored to call a common helper function to set up the wallets, instead of adding extra setup to already complicated tests.
IMO, handling of sqlite/bdb test variants is moving in a bad direction here and in followup #20892. I think individual test invocations should be as small and fast as possible, and broken up much as possible. I think invoking a test should run one single variant and test runner should responsible for iterating over variants. Advantages of this approach:
- Faster debug cycles. Tests that fail faster can be debugged faster. Broken up tests can also be parallelized more.
- More accurate reporting. There’s an increasing number of tests that are skipping checks based on compile options and misleadingly reporting skipped checks as passing. We could fix this by expanding test result status from Passing / Failed / Skipped to include a new Passing but partially skipped status. But it would be clearer to avoid this and to break these tests up so “Passing” can go back to meaning fully passing with no checks skipped.
- More straightforward usage. A few tests are ignoring descriptor/legacy options which is inconsistent and confusing.
- More readable code. Smaller tests are easier to understand and debug, and some tests are doing things like repeating code or defining unnecessary functions and calling them twice to cover multiple variants. Tests would be simpler if test_runner managed all the variations, and tests could just obey options instead of listing variants.

S3RK commented at 10:04 am on February 4, 2021: member

Started doing review. Have only one small question, but I’ll get back with more detailed feedback later

achow101 force-pushed on Feb 4, 2021

ryanofsky approved

ryanofsky commented at 11:40 am on February 5, 2021: member

Code review ACK 49797c3ccfbb9f7ac9c1fbb574d35b315c103805. Only change since last review is dropping last commit. Previous review w/ suggestions for future followup is #20267#pullrequestreview-581508843

laanwj commented at 1:21 pm on February 5, 2021: member

IMO, handling of sqlite/bdb test variants is moving in a bad direction here and in followup #20892. I think individual test invocations should be as small and fast as possible,

Yes, I think so too. And agree it’s good if skipping tests is reported explicitly. However I don’t think this PR makes this much worse. But yes it does add some if self.is_wallet_compiled():.

ACK 49797c3ccfbb9f7ac9c1fbb574d35b315c103805

laanwj commented at 1:23 pm on February 5, 2021: member

UBsan error in the CI (https://github.com/bitcoin/bitcoin/pull/20267/checks?check_run_id=1832884065) is:

0test/fuzz/system.cpp:57:159: runtime error: implicit conversion from type 'int' of value -2049 (32-bit, signed) to type 'unsigned int' changed the value to 4294965247 (32-bit, unsigned)

during the fuzz tests. As these are not touched here, I don’t think it needs to block merge.

laanwj merged this on Feb 5, 2021

laanwj closed this on Feb 5, 2021

sidhujag referenced this in commit 4a94c21ec4 on Feb 5, 2021

luke-jr referenced this in commit ec84c74344 on Jun 27, 2021

luke-jr referenced this in commit f03cb65bff on Jun 27, 2021

luke-jr referenced this in commit 7641442218 on Jun 27, 2021

luke-jr referenced this in commit 2dd010de2b on Jun 27, 2021

luke-jr referenced this in commit 5152df21a8 on Jun 27, 2021

luke-jr referenced this in commit b012aad418 on Jun 27, 2021

luke-jr referenced this in commit 4c89b104ca on Jun 27, 2021

luke-jr referenced this in commit 7a58d3bb58 on Jun 27, 2021

fanquake referenced this in commit fdf9b3eba3 on Jul 15, 2021

sidhujag referenced this in commit 3b3cf67681 on Jul 23, 2021

DrahtBot locked this on Aug 16, 2022

Disable and fix tests for when BDB is not compiled #20267

Conflicts