test: Bump walletpassphrase timeouts to avoid intermittent issues #28403

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2309-ci-bump-timeout- changing 6 files +15 −15
  1. maflcko 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)
  2. 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.

  3. DrahtBot added the label Tests on Sep 4, 2023
  4. 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?
    maflcko 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.

  5. 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)
    maflcko 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.
    maflcko 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.

  6. jonatack commented at 5:17 pm on September 5, 2023: member
    Concept ACK
  7. 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”?

  8. maflcko 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?

  9. 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!

  10. kevkevinpal referenced this in commit 299d21053b on Sep 11, 2023
  11. kevkevinpal referenced this in commit db16f187e4 on Sep 11, 2023
  12. 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

  13. kevkevinpal referenced this in commit 47c5212b74 on Sep 11, 2023
  14. maflcko 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)
  15. maflcko added this to the milestone 26.0 on Oct 1, 2023
  16. test: Bump walletpassphrase timeouts to avoid intermittent issues fa28f5a381
  17. maflcko force-pushed on Oct 5, 2023
  18. maflcko commented at 11:00 am on October 5, 2023: member

    Force pushed to modify two more lines.

    Should be trivial to re-ACK.

  19. achow101 commented at 3:12 pm on October 5, 2023: member
    ACK fa28f5a3819a4bb69b046529e05932016273170b
  20. achow101 merged this on Oct 5, 2023
  21. achow101 closed this on Oct 5, 2023
  22. maflcko deleted the branch on Oct 5, 2023
  23. Frank-GER referenced this in commit 6b3402da0c on Oct 13, 2023
  24. fanquake referenced this in commit 091d29c495 on Oct 19, 2023
  25. bitcoin locked this on Oct 4, 2024


maflcko DrahtBot jonatack achow101 kevkevinpal

Labels
Tests

Milestone
26.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 03:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me