Disable and fix tests for when BDB is not compiled #20267
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
I'm having trouble figuring out the logic here. Is there a more clear and concise way to determine descriptors?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@mjdietzx , you can use Quine-McCluskey to derive a minimal function:
>>> from newqm import QM
>>> qm = QM(['A','B','C'])
>>> def aj(a,b,c): # a is self.options.descriptors, b is bdb, c is sqlite compilation
... descriptors = a
... if not b and c and not a:
... descriptors = True
... elif b and not c and a:
... descriptors = False
... return descriptors
...
>>> from itertools import product
>>> values = [idx for (idx,x) in enumerate(product([False,True], repeat=3)) if aj(*x)]
>>> qm.get_function(qm.solve(values,[])[1])
'(((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:
option_set = self.options.descriptors
bdb = self.is_bdb_compiled()
sqlite = self.is_sqlite_compiled()
return ((NOT bdb) AND sqlite) \
OR (option_set AND (NOT bdb)) \
OR (option_set AND sqlite)
There was a problem hiding this comment.
Are you able to use self. get_use_descriptors() here? To me it seems like logic might be getting duplicated
There was a problem hiding this comment.
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.
src/wallet/walletdb.cpp
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
src/wallet/walletdb.cpp
Outdated
There was a problem hiding this comment.
same, should it be #elif USE_SQLITE here?
There was a problem hiding this comment.
No, if both are compiled, then we need both headers.
test/functional/feature_filelock.py
Outdated
There was a problem hiding this comment.
Can this be condensed to a one-liner: check_wallet_filelock(self.is_sqlite_compiled())
There was a problem hiding this comment.
When both are compiled, we want to test both cases.
achow101
force-pushed
the
tests-opt-sqlite-bdb
branch
from
846ba62 to
a940086
Compare
achow101
force-pushed
the
tests-opt-sqlite-bdb
branch
2 times, most recently
from
55deb76 to
1cae522
Compare
|
achow101
force-pushed
the
tests-opt-sqlite-bdb
branch
from
1cae522 to
d4a85b6
Compare
|
Fixed the linter error. 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 |
Some tests are intended to test only legacy wallet behavior. With automatic switching of wallet type, we need to make them explicit
Fixes the wallet setup so this test works with descriptor wallets. Also enabled explicit descriptor and legacy wallet testing in the test runner.
Should be resolved now. |
achow101
force-pushed
the
tests-opt-sqlite-bdb
branch
from
91a364b to
a831ba6
Compare
There was a problem hiding this comment.
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 (#20267 (comment) and #20267 (comment)) 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.
|
Started doing review. Have only one small question, but I'll get back with more detailed feedback later |
achow101
force-pushed
the
tests-opt-sqlite-bdb
branch
from
a831ba6 to
49797c3
Compare
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 49797c3. Only change since last review is dropping last commit. Previous review w/ suggestions for future followup is #20267 (review)
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 ACK 49797c3 |
|
UBsan error in the CI (https://github.com/bitcoin/bitcoin/pull/20267/checks?check_run_id=1832884065) is: during the fuzz tests. As these are not touched here, I don't think it needs to block merge. |
Github-Pull: bitcoin#20267 Rebased-From: b9b88f5
Github-Pull: bitcoin#20267 Rebased-From: 1f20cac
Github-Pull: bitcoin#20267 Rebased-From: c77975a
Github-Pull: bitcoin#20267 Rebased-From: 1f1bef8
Github-Pull: bitcoin#20267 Rebased-From: 7c71c62
Github-Pull: bitcoin#20267 Rebased-From: 4d03ef9
Github-Pull: bitcoin#20267 Rebased-From: fbaea7b
Fixes the wallet setup so this test works with descriptor wallets. Also enabled explicit descriptor and legacy wallet testing in the test runner. Github-Pull: bitcoin#20267 Rebased-From: 1194cf9
0c845e3 test: Fix wallet_listdescriptors.py if bdb is not compiled (Hennadii Stepanov) Pull request description: If build system is configured `--without-bdb`, the `wallet_listdescriptors.py` fails: ``` $ test/functional/wallet_listdescriptors.py --descriptors 2021-07-14T13:20:52.931000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_02p7o1c9 2021-07-14T13:21:23.377000Z TestFramework (INFO): Test that the command is not available for legacy wallets. 2021-07-14T13:21:23.381000Z TestFramework (ERROR): JSONRPC error Traceback (most recent call last): File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main self.run_test() File "test/functional/wallet_listdescriptors.py", line 34, in run_test node.createwallet(wallet_name='w1', descriptors=False) File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_node.py", line 685, in createwallet return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer) File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__ raise JSONRPCException(response['error'], status) test_framework.authproxy.JSONRPCException: Compiled without bdb support (required for legacy wallets) (-4) 2021-07-14T13:21:23.436000Z TestFramework (INFO): Stopping nodes 2021-07-14T13:21:24.092000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_02p7o1c9 2021-07-14T13:21:24.092000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_02p7o1c9/test_framework.log 2021-07-14T13:21:24.092000Z TestFramework (ERROR): 2021-07-14T13:21:24.092000Z TestFramework (ERROR): Hint: Call /home/hebasto/GitHub/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_02p7o1c9' to consolidate all logs 2021-07-14T13:21:24.092000Z TestFramework (ERROR): 2021-07-14T13:21:24.092000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 2021-07-14T13:21:24.092000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues 2021-07-14T13:21:24.092000Z TestFramework (ERROR): ``` This PR fixes this issue. Also see #20267. ACKs for top commit: achow101: ACK 0c845e3 Tree-SHA512: d7116a9ae30c7b7e3f55f55d2eea66f9e293c38d6757ed66d0477e4256ff5fedca5ddedafa0ef0c09f4dc1f7f973163e5a46090da26b067fdddbd9ea2ee76633
…t compiled 0c845e3 test: Fix wallet_listdescriptors.py if bdb is not compiled (Hennadii Stepanov) Pull request description: If build system is configured `--without-bdb`, the `wallet_listdescriptors.py` fails: ``` $ test/functional/wallet_listdescriptors.py --descriptors 2021-07-14T13:20:52.931000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_02p7o1c9 2021-07-14T13:21:23.377000Z TestFramework (INFO): Test that the command is not available for legacy wallets. 2021-07-14T13:21:23.381000Z TestFramework (ERROR): JSONRPC error Traceback (most recent call last): File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main self.run_test() File "test/functional/wallet_listdescriptors.py", line 34, in run_test node.createwallet(wallet_name='w1', descriptors=False) File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_node.py", line 685, in createwallet return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer) File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__ return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs) File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__ raise JSONRPCException(response['error'], status) test_framework.authproxy.JSONRPCException: Compiled without bdb support (required for legacy wallets) (-4) 2021-07-14T13:21:23.436000Z TestFramework (INFO): Stopping nodes 2021-07-14T13:21:24.092000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_02p7o1c9 2021-07-14T13:21:24.092000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_02p7o1c9/test_framework.log 2021-07-14T13:21:24.092000Z TestFramework (ERROR): 2021-07-14T13:21:24.092000Z TestFramework (ERROR): Hint: Call /home/hebasto/GitHub/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_02p7o1c9' to consolidate all logs 2021-07-14T13:21:24.092000Z TestFramework (ERROR): 2021-07-14T13:21:24.092000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 2021-07-14T13:21:24.092000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues 2021-07-14T13:21:24.092000Z TestFramework (ERROR): ``` This PR fixes this issue. Also see bitcoin#20267. ACKs for top commit: achow101: ACK 0c845e3 Tree-SHA512: d7116a9ae30c7b7e3f55f55d2eea66f9e293c38d6757ed66d0477e4256ff5fedca5ddedafa0ef0c09f4dc1f7f973163e5a46090da26b067fdddbd9ea2ee76633
10 participants
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.