test: create wallet specific for test_locked

furszy commented at 1:56 PM on July 24, 2023: member

Several test cases are relying on the node1 default wallet, which thanks to 'test_locked_wallet' is encrypted. And can be only accessed within a specific timeframe (100ms), a duration internally set by the same test.

This situation introduces a potential race condition, where other tests must complete their operations within the specified 100ms window to pass (otherwise the wallet gets re-locked and they fail).

This can be seen running the test in valgrind (https://github.com/bitcoin/bitcoin/pull/28089), where other test cases fail due the wallet re-locking itself after the 100ms.

DrahtBot commented at 1:56 PM on July 24, 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	MarcoFalke, ishaanam

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:

#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 Jul 24, 2023

test: create wallet specific for test_locked_wallet case

Other tests are also relying on the node1 default wallet,
which thanks to 'test_locked_wallet' is encrypted.
And can only be accessed within a specific timeframe (100ms)
set internally by the same test.

This make other tests susceptible to races. They can only
perform their operations successfully within the specified
time.

This can be seen running the test in valgrind, where other
test cases fail due the wallet re-locking itself after the
100ms.

c648bdbda2

furszy force-pushed on Jul 24, 2023

maflcko commented at 2:18 PM on July 24, 2023: member

lgtm ACK c648bdbda21c7ae90c6b40e506ca4ed62b1dbb6c

fanquake commented at 3:37 PM on July 24, 2023: member

cc @ishaanam

ishaanam commented at 7:02 PM on July 24, 2023: contributor

utACK c648bdbda21c7ae90c6b40e506ca4ed62b1dbb6c

maflcko added this to the milestone 26.0 on Jul 25, 2023

fanquake merged this on Jul 26, 2023

fanquake closed this on Jul 26, 2023

furszy deleted the branch on Jul 26, 2023

in test/functional/wallet_fundrawtransaction.py:621 in c648bdbda2

 624 |          outputs = {self.nodes[0].getnewaddress():value}
 625 | -        rawtx = self.nodes[1].createrawtransaction(inputs, outputs)
 626 | +        rawtx = wallet.createrawtransaction(inputs, outputs)
 627 |          # fund a transaction that does not require a new key for the change output
 628 | -        self.nodes[1].fundrawtransaction(rawtx)
 629 | +        funded_tx = wallet.fundrawtransaction(rawtx)

maflcko commented at 3:25 PM on August 4, 2023:

This fails locally:

 node1 2023-08-02T03:08:04.791676Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getnewaddress user=__cookie__ 
 node1 2023-08-02T03:08:04.798619Z [httpworker.2] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool reserve 3 
 node1 2023-08-02T03:08:04.807232Z [httpworker.2] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool keep 3 
 node1 2023-08-02T03:08:04.883158Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for /wallet/locked_wallet from 127.0.0.1:37278 
 node1 2023-08-02T03:08:04.883976Z [httpworker.0] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getrawchangeaddress user=__cookie__ 
 node1 2023-08-02T03:08:04.889129Z [httpworker.0] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool reserve 4 
 node1 2023-08-02T03:08:04.893017Z [httpworker.0] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [locked_wallet] keypool keep 4 
 node1 2023-08-02T03:08:04.897331Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for /wallet/locked_wallet from 127.0.0.1:37278 
 node1 2023-08-02T03:08:04.898090Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=listunspent user=__cookie__ 
 node0 2023-08-02T03:08:05.004756Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:59058 
 node0 2023-08-02T03:08:05.005782Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getnewaddress user=__cookie__ 
 node0 2023-08-02T03:08:05.198476Z [httpworker.1] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [default wallet] keypool added 1 keys (1 internal), size=2 (1 internal) 
 node0 2023-08-02T03:08:05.201311Z [httpworker.1] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [default wallet] keypool reserve 30 
 node0 2023-08-02T03:08:05.288462Z [httpworker.1] [wallet/scriptpubkeyman.h:254] [WalletLogPrintf] [default wallet] keypool keep 30 
 node1 2023-08-02T03:08:05.299953Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for /wallet/locked_wallet from 127.0.0.1:37278 
 node1 2023-08-02T03:08:05.301853Z [httpworker.3] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=createrawtransaction user=__cookie__ 
 node1 2023-08-02T03:08:05.593637Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for /wallet/locked_wallet from 127.0.0.1:37278 
 node1 2023-08-02T03:08:05.595303Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=fundrawtransaction user=__cookie__ 
 node1 2023-08-02T03:08:06.194715Z [httpworker.2] [policy/fees.cpp:382] [EstimateMedianVal] [estimatefee] FeeEst: 1 > 60% decay 0.96200: feerate: 1000 from (0 - 1e+99) 100.00% 14.2/(14.2 0 mem 0.0 out) Fail: (-1 - -1) 0.00% 0.0/(0.0 0 mem 0.0 out) 
 node1 2023-08-02T03:08:06.198808Z [httpworker.2] [policy/fees.cpp:382] [EstimateMedianVal] [estimatefee] FeeEst: 3 > 85% decay 0.96200: feerate: 1000 from (0 - 1e+99) 100.00% 14.2/(14.2 0 mem 0.0 out) Fail: (-1 - -1) 0.00% 0.0/(0.0 0 mem 0.0 out) 
 node1 2023-08-02T03:08:06.200053Z [httpworker.2] [policy/fees.cpp:382] [EstimateMedianVal] [estimatefee] FeeEst: 6 > 95% decay 0.96200: feerate: 1000 from (0 - 1e+99) 100.00% 14.2/(14.2 0 mem 0.0 out) Fail: (-1 - -1) 0.00% 0.0/(0.0 0 mem 0.0 out) 
 node1 2023-08-02T03:08:06.204141Z [httpworker.2] [policy/fees.cpp:382] [EstimateMedianVal] [estimatefee] FeeEst: 1 > 60% decay 0.96200: feerate: 1000 from (0 - 1e+99) 100.00% 14.2/(14.2 0 mem 0.0 out) Fail: (-1 - -1) 0.00% 0.0/(0.0 0 mem 0.0 out) 
 node1 2023-08-02T03:08:06.205478Z [httpworker.2] [policy/fees.cpp:382] [EstimateMedianVal] [estimatefee] FeeEst: 3 > 85% decay 0.96200: feerate: 1000 from (0 - 1e+99) 100.00% 14.2/(14.2 0 mem 0.0 out) Fail: (-1 - -1) 0.00% 0.0/(0.0 0 mem 0.0 out) 
 node1 2023-08-02T03:08:06.206162Z [httpworker.2] [policy/fees.cpp:382] [EstimateMedianVal] [estimatefee] FeeEst: 6 > 95% decay 0.96200: feerate: 1000 from (0 - 1e+99) 100.00% 14.2/(14.2 0 mem 0.0 out) Fail: (-1 - -1) 0.00% 0.0/(0.0 0 mem 0.0 out) 
 test  2023-08-02T03:08:06.709000Z TestFramework (ERROR): JSONRPC error 
                                   Traceback (most recent call last):
                                     File "/root/b-c-ci/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
                                       self.run_test()
                                     File "/root/b-c-ci/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 135, in run_test
                                       self.test_locked_wallet()
                                     File "/root/b-c-ci/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 607, in test_locked_wallet
                                       funded_tx = wallet.fundrawtransaction(rawtx)
                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/root/b-c-ci/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/root/b-c-ci/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 129, in __call__
                                       raise JSONRPCException(response['error'], status)
                                   test_framework.authproxy.JSONRPCException: Transaction needs a change address, but we can't generate it. Error: Keypool ran out, please call keypoolrefill first (-4)

maflcko commented at 3:31 PM on August 4, 2023:

https://drahtbot.space/temp_scratch/wallet_fundrawtransaction_251.tar.xz

furszy commented at 2:59 AM on August 5, 2023:

Haven't been able to reproduce it. It is always passing here. But, I imagine that it's related to the dynamic fee rate calculation. Which.. is not needed in this test case; the test exercises the creation of a changeless transaction, the fee rate used is not really important. So, when you can, try https://github.com/furszy/bitcoin-core/commit/557c50deb0253af299dc88856b34399b0d626bd3.

maflcko commented at 9:41 AM on August 6, 2023:

I wonder why implicit fee estimation passed in the first place. Previously there were two inputs and a fee of 550sat. Now there is a fee of 2200 sat with one input.

Mind creating a pull, as I can't test intermittent issues? I think this needs clarification/fixup even absent an intermittent issue.

furszy commented at 11:08 PM on August 6, 2023:

I wonder why implicit fee estimation passed in the first place. Previously there were two inputs and a fee of 550sat. Now there is a fee of 2200 sat with one input.

That is because the default wallets have a custom tx fee (set here), which is much lower than the one used in the wallet created by this test case (which runs the default fee estimation).

Side note; the implicit fee estimation suffered other adaptations already:

So.. seems that this number has been adapted every now and then. And wouldn't be bad to provide a specific fee rate to fundrawtransaction() and document the expected behavior as I did in https://github.com/furszy/bitcoin-core/commit/557c50deb0253af299dc88856b34399b0d626bd3.

Mind creating a pull, as I can't test intermittent issues? I think this needs clarification/fixup even absent an intermittent issue.

Sure. Let me know if we are sync and will create the PR.

maflcko commented at 6:26 AM on August 7, 2023:

yeah, if the fee was hardcoded before, it makes sense to keep it hardcoded. Especially if removing the hard code makes the test fail.

sidhujag referenced this in commit 747a4072fc on Aug 9, 2023

fanquake referenced this in commit aadaa5625e on Aug 14, 2023

sidhujag referenced this in commit 946d885c70 on Aug 15, 2023

bitcoin locked this on Aug 6, 2024

test: create wallet specific for test_locked_wallet case #28139

Reviews

Conflicts