test: Bump walletpassphrase timeouts to avoid intermittent issues

MarcoFalke commented at 2:57 pm on September 4, 2023: member

This bumps all timeouts for all walletpassphrase to avoid intermittent issues in valgrind (or other sanitizers).

As an idea for a follow-up, walletpassphrase could be changed to treat 0 as “no timeout” instead of “instant timeout”.

Example failure:

 0 node0 2023-09-03T22:44:38.374955Z [httpworker.3] [rpc/server.cpp:594] [RPCRunLater] [rpc] queue run of timer lockwallet(w6) in 60 seconds (using HTTP) 
 1 test  2023-09-03T22:44:40.173000Z TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-rpcwallet=w6', 'getnewaddress', '', 'legacy'] 
 2 node0 2023-09-03T22:44:59.810893Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for /wallet/w6 from 127.0.0.1:48928 
 3 node0 2023-09-03T22:44:59.813132Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getnewaddress user=__cookie__ 
 4 node0 2023-09-03T22:44:59.837183Z [httpworker.1] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20230903_183350/wallet_createwallet_171/node0/regtest/w6/wallet.dat] SQLite Statement: INSERT or REPLACE into main values(?, ?) 
 5 node0 2023-09-03T22:44:59.929735Z [httpworker.1] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20230903_183350/wallet_createwallet_171/node0/regtest/w6/wallet.dat] SQLite Statement: INSERT or REPLACE into main values(?, ?) 
 6 node0 2023-09-03T22:44:59.934484Z [httpworker.1] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20230903_183350/wallet_createwallet_171/node0/regtest/w6/wallet.dat] SQLite Statement: INSERT or REPLACE into main values(?, ?) 
 7 node0 2023-09-03T22:44:59.935467Z [httpworker.1] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20230903_183350/wallet_createwallet_171/node0/regtest/w6/wallet.dat] SQLite Statement: INSERT or REPLACE into main values(?, ?) 
 8 test  2023-09-03T22:45:02.328000Z TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-rpcwallet=w6', 'signmessage', 'mqatqH4VQmrZ81nxUfrnfcLnxgbzhZb4PC', 'test'] 
 9 node0 2023-09-03T22:45:20.269375Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for /wallet/w6 from 127.0.0.1:44618 
10 node0 2023-09-03T22:45:20.270670Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=signmessage user=__cookie__ 
11 test  2023-09-03T22:45:23.490000Z TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-rpcwallet=w6', 'keypoolrefill', '1'] 
12 node0 2023-09-03T22:45:40.244603Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for /wallet/w6 from 127.0.0.1:32854 
13 node0 2023-09-03T22:45:40.293021Z [httpworker.0] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=keypoolrefill user=__cookie__ 
14 test  2023-09-03T22:45:41.852000Z TestFramework (ERROR): JSONRPC error 
15                                   Traceback (most recent call last):
16                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
17                                       self.run_test()
18                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_createwallet.py", line 156, in run_test
19                                       w6.keypoolrefill(1)
20                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 732, in __call__
21                                       return self.cli.send_cli(self.command, *args, **kwargs)
22                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
23                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 795, in send_cli
24                                       raise JSONRPCException(dict(code=int(code), message=message))
25                                   test_framework.authproxy.JSONRPCException: Error: Please enter the wallet passphrase with walletpassphrase first. (-13)

DrahtBot commented at 2:57 pm on September 4, 2023: 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
ACK	achow101
Concept ACK	jonatack

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:

#28454 (wallet: allowing walletpassphrase timeout to be MAX_SLEEP_TIME if set to -1 by kevkevinpal)
#27216 (wallet: Add wallet method to detect if a key is “active” by pinheadmz)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation 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.

DrahtBot added the label Tests on Sep 4, 2023

in test/functional/wallet_createwallet.py:112 in faac104184 outdated

108@@ -109,7 +109,7 @@ def run_test(self):
109         assert_raises_rpc_error(-4, "Error: This wallet has no available keys", w4.getnewaddress)
110         assert_raises_rpc_error(-4, "Error: This wallet has no available keys", w4.getrawchangeaddress)
111         # Now set a seed and it should work. Wallet should also be encrypted
112-        w4.walletpassphrase('pass', 60)
113+        w4.walletpassphrase("pass", 999000)

jonatack commented at 5:15 pm on September 5, 2023:

Maybe move WALLET_PASSPHRASE_TIMEOUT in wallet_bumpfee.py to the framework and use that everywhere?

MarcoFalke commented at 6:34 am on September 14, 2023:

Going to leave this as-is for now.

Happy to review an alternative that does this.

Even better would be to implement a Python with context manager that accepts a locked wallet, unlocks it “forever”, then does the needed operations, and immediately re-locks it when done.

This should make tests less fragile and also easier to read, because it clarifies and enforces what operations the wallet needs to be unlocked for.

Though, again, I am not going to work on any of this, but I am happy to review anything in that regard.

in test/functional/wallet_encryption.py:100 in faac104184 outdated

 96@@ -97,7 +97,7 @@ def run_test(self):
 97         self.nodes[0].walletpassphrasechange(passphrase2, passphrase_with_nulls)
 98         # walletpassphrasechange should not stop at null characters
 99         assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrase, passphrase_with_nulls.partition("\0")[0], 10)
100-        self.nodes[0].walletpassphrase(passphrase_with_nulls, 10)
101+        self.nodes[0].walletpassphrase(passphrase_with_nulls, 999000)

jonatack commented at 5:17 pm on September 5, 2023:

Also at https://github.com/bitcoin/bitcoin/pull/28403/files#diff-6ab4bc75c6edb7e0afcc38ea1695080ea2ff04b3a581a2bf6f58c3e4c455fa22R50? (edit: some other sites also, it seems)

MarcoFalke commented at 6:28 am on September 6, 2023:

Why? This is used to check the timeout arg. If it is modified, the test will fail, no?

achow101 commented at 6:49 pm on October 4, 2023:

Perhaps the one on line 71? That one isn’t testing the arg.

MarcoFalke commented at 11:00 am on October 5, 2023:

Perhaps the one on line 71? That one isn’t testing the arg.

Ah, thx, done.

jonatack commented at 5:17 pm on September 5, 2023: contributor

Concept ACK

kevkevinpal commented at 7:26 pm on September 5, 2023: contributor

As an idea for a follow-up, walletpassphrase could be changed to treat 0 as “no timeout” instead of “instant timeout”.

Instead of 0, would it make sense if that arg was optional and if not specified then to treat it as “no timeout”?

MarcoFalke commented at 2:41 pm on September 11, 2023: member

Instead of 0, would it make sense if that arg was optional and if not specified then to treat it as “no timeout”? @kevkevinpal Sgtm. Do you want to work on this and create a pull?

kevkevinpal commented at 5:25 pm on September 11, 2023: contributor

Instead of 0, would it make sense if that arg was optional and if not specified then to treat it as “no timeout”?

@kevkevinpal Sgtm. Do you want to work on this and create a pull?

yea I can work on this, will open a pull when ready thanks!

kevkevinpal referenced this in commit 299d21053b on Sep 11, 2023

kevkevinpal referenced this in commit db16f187e4 on Sep 11, 2023

kevkevinpal commented at 11:04 pm on September 11, 2023: contributor

@MarcoFalke I created a PR #28454

I didnt modify any of the tests to not include a value, I’m not sure if you want to do that in this PR or I can do it in the one I opened

kevkevinpal referenced this in commit 47c5212b74 on Sep 11, 2023

MarcoFalke commented at 7:20 am on September 14, 2023: member

Just ran into this again:

 0 node0 2023-09-13T14:49:51.799422Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=keypoolrefill user=__cookie__ 
 1 test  2023-09-13T14:49:53.112000Z TestFramework (ERROR): JSONRPC error 
 2                                   Traceback (most recent call last):
 3                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
 4                                       self.run_test()
 5                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_createwallet.py", line 156, in run_test
 6                                       w6.keypoolrefill(1)
 7                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 732, in __call__
 8                                       return self.cli.send_cli(self.command, *args, **kwargs)
 9                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 795, in send_cli
11                                       raise JSONRPCException(dict(code=int(code), message=message))
12                                   test_framework.authproxy.JSONRPCException: Error: Please enter the wallet passphrase with walletpassphrase first. (-13)

MarcoFalke added this to the milestone 26.0 on Oct 1, 2023

test: Bump walletpassphrase timeouts to avoid intermittent issues fa28f5a381

MarcoFalke force-pushed on Oct 5, 2023

MarcoFalke commented at 11:00 am on October 5, 2023: member

Force pushed to modify two more lines.

Should be trivial to re-ACK.

achow101 commented at 3:12 pm on October 5, 2023: member

ACK fa28f5a3819a4bb69b046529e05932016273170b

achow101 merged this on Oct 5, 2023

achow101 closed this on Oct 5, 2023

MarcoFalke deleted the branch on Oct 5, 2023

test: Bump walletpassphrase timeouts to avoid intermittent issues #28403

Reviews

Conflicts