test: Bump walletpassphrase timeouts to avoid intermittent issues #28403

maflcko · 2023-09-04T14:57:02Z

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:

 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) 
 test  2023-09-03T22:44:40.173000Z TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-rpcwallet=w6', 'getnewaddress', '', 'legacy'] 
 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 
 node0 2023-09-03T22:44:59.813132Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getnewaddress user=__cookie__ 
 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(?, ?) 
 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(?, ?) 
 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(?, ?) 
 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(?, ?) 
 test  2023-09-03T22:45:02.328000Z TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-rpcwallet=w6', 'signmessage', 'mqatqH4VQmrZ81nxUfrnfcLnxgbzhZb4PC', 'test'] 
 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 
 node0 2023-09-03T22:45:20.270670Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=signmessage user=__cookie__ 
 test  2023-09-03T22:45:23.490000Z TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-rpcwallet=w6', 'keypoolrefill', '1'] 
 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 
 node0 2023-09-03T22:45:40.293021Z [httpworker.0] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=keypoolrefill user=__cookie__ 
 test  2023-09-03T22:45:41.852000Z TestFramework (ERROR): JSONRPC error 
                                   Traceback (most recent call last):
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
                                       self.run_test()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_createwallet.py", line 156, in run_test
                                       w6.keypoolrefill(1)
                                     File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 732, in __call__
                                       return self.cli.send_cli(self.command, *args, **kwargs)
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     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
                                       raise JSONRPCException(dict(code=int(code), message=message))
                                   test_framework.authproxy.JSONRPCException: Error: Please enter the wallet passphrase with walletpassphrase first. (-13)

DrahtBot · 2023-09-04T14:57:05Z

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.

jonatack

Concept ACK

jonatack · 2023-09-05T17:15:05Z

test/functional/wallet_createwallet.py

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


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

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.

jonatack · 2023-09-05T17:17:24Z

test/functional/wallet_encryption.py

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


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

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

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

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

Ah, thx, done.

kevkevinpal · 2023-09-05T19:26:04Z

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"?

maflcko · 2023-09-11T14:41:42Z

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 · 2023-09-11T17:25:55Z

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!

In this change we allow the timeout for walletpassphrase to be optional, if left empty then we then use the MAX_SLEEP_TIME amount context from bitcoin#28403 (comment)

kevkevinpal · 2023-09-11T23:04:12Z

@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

In this change we allow the timeout for walletpassphrase to be optional, if left empty then we then use the MAX_SLEEP_TIME amount context from PR bitcoin#28403

maflcko · 2023-09-14T07:20:35Z

Just ran into this again:

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

maflcko · 2023-10-05T11:00:44Z

Force pushed to modify two more lines.

Should be trivial to re-ACK.

achow101 · 2023-10-05T15:12:22Z

ACK fa28f5a

…termittent issues

004903e test: Add Wallet Unlock Context Manager (Brandon Odiwuor) Pull request description: Fixes #28601, see bitcoin/bitcoin#28403 (comment) Add Context Manager to manage the locking and unlocking of locked wallets with a passphrase during testing. ACKs for top commit: kevkevinpal: lgtm ACK [004903e](bitcoin/bitcoin@004903e) maflcko: lgtm ACK 004903e Tree-SHA512: ab234c167e71531df0d974ff9a31d444f7ce2a1d05aba5ea868cc9452f139845eeb24ca058d88f058bc02482b762adf2d99e63a6640b872cc71a57a0068abfe8

DrahtBot added the Tests label Sep 4, 2023

This was referenced Sep 4, 2023

wallet: Add wallet method to detect if a key is "active" #27216

Closed

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

Closed

jonatack reviewed Sep 5, 2023

View reviewed changes

kevkevinpal mentioned this pull request Sep 11, 2023

wallet: allowing walletpassphrase timeout to be MAX_SLEEP_TIME if set to -1 #28454

Closed

maflcko added this to the 26.0 milestone Oct 1, 2023

test: Bump walletpassphrase timeouts to avoid intermittent issues

fa28f5a

maflcko force-pushed the 2309-ci-bump-timeout- branch from faac104 to fa28f5a Compare October 5, 2023 10:57

achow101 merged commit db19a7e into bitcoin:master Oct 5, 2023

maflcko deleted the 2309-ci-bump-timeout- branch October 5, 2023 15:40

maflcko mentioned this pull request Oct 5, 2023

test: unlocked wallet context manager #28601

Closed

BrandonOdiwuor mentioned this pull request Oct 9, 2023

test: Add Wallet Unlock Context Manager #28617

Merged

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023

Merge bitcoin#28403: test: Bump walletpassphrase timeouts to avoid in…

6b3402d

…termittent issues

bitcoin locked and limited conversation to collaborators Oct 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: Bump walletpassphrase timeouts to avoid intermittent issues #28403

test: Bump walletpassphrase timeouts to avoid intermittent issues #28403

Uh oh!

maflcko commented Sep 4, 2023

Uh oh!

DrahtBot commented Sep 4, 2023 •

edited

Loading

Uh oh!

jonatack left a comment

Uh oh!

jonatack Sep 5, 2023 •

edited

Loading

Uh oh!

maflcko Sep 14, 2023

Uh oh!

jonatack Sep 5, 2023 •

edited

Loading

Uh oh!

maflcko Sep 6, 2023

Uh oh!

achow101 Oct 4, 2023 •

edited

Loading

Uh oh!

maflcko Oct 5, 2023

Uh oh!

kevkevinpal commented Sep 5, 2023

Uh oh!

maflcko commented Sep 11, 2023

Uh oh!

kevkevinpal commented Sep 11, 2023

Uh oh!

kevkevinpal commented Sep 11, 2023

Uh oh!

maflcko commented Sep 14, 2023

Uh oh!

maflcko commented Oct 5, 2023

Uh oh!

achow101 commented Oct 5, 2023

Uh oh!

Uh oh!

test: Bump walletpassphrase timeouts to avoid intermittent issues #28403

test: Bump walletpassphrase timeouts to avoid intermittent issues #28403

Uh oh!

Conversation

maflcko commented Sep 4, 2023

Uh oh!

DrahtBot commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

jonatack Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

kevkevinpal commented Sep 5, 2023

Uh oh!

maflcko commented Sep 11, 2023

Uh oh!

kevkevinpal commented Sep 11, 2023

Uh oh!

kevkevinpal commented Sep 11, 2023

Uh oh!

maflcko commented Sep 14, 2023

Uh oh!

maflcko commented Oct 5, 2023

Uh oh!

achow101 commented Oct 5, 2023

Uh oh!

Uh oh!

DrahtBot commented Sep 4, 2023 •

edited

Loading

jonatack Sep 5, 2023 •

edited

Loading

jonatack Sep 5, 2023 •

edited

Loading

achow101 Oct 4, 2023 •

edited

Loading