test: create wallet specific for test_locked_wallet case #28139
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
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.
furszy
force-pushed
the
2023_isolate_test_locked_wallet
branch
from
6a47a3d to
c648bdb
Compare
Member
|
lgtm ACK c648bdb |
Member
|
cc @ishaanam |
Contributor
|
utACK c648bdb |
maflcko
reviewed
Aug 4, 2023
| rawtx = wallet.createrawtransaction(inputs, outputs) | ||
| # fund a transaction that does not require a new key for the change output | ||
| self.nodes[1].fundrawtransaction(rawtx) | ||
| funded_tx = wallet.fundrawtransaction(rawtx) |
Member
There was a problem hiding this comment.
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)
Member
There was a problem hiding this comment.
Member
Author
There was a problem hiding this comment.
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 furszy@557c50d.
Member
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
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 furszy@557c50d.
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.
Member
There was a problem hiding this comment.
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
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 9, 2023
…let case c648bdb test: create wallet specific for test_locked_wallet case (furszy) Pull request description: Coming from bitcoin#28089 (comment). 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 (bitcoin#28089), where other test cases fail due the wallet re-locking itself after the 100ms. ACKs for top commit: MarcoFalke: lgtm ACK c648bdb ishaanam: utACK c648bdb Tree-SHA512: 01cde5a4a0cb3405adb9ea3c1f73841f3fa237d1162268ed06f0d49ca38541006b423a029e0b5e5955e1aa7e018c4600d894e555a68cf17ff60a4b8be58f4aa9
fanquake
added a commit
to bitcoin-core/gui
that referenced
this pull request
Aug 14, 2023
…timation 5364dd8 test: locked_wallet, skip default fee estimation (furszy) Pull request description: Coming from bitcoin/bitcoin#28139 (comment). No test case in this file is meant to exercise fee estimation. All default wallets have a custom tx fee set [here](https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/test/functional/wallet_fundrawtransaction.py#L100). The only one missing is the one created for `locked_wallet`. ACKs for top commit: theStack: ACK 5364dd8 Tree-SHA512: 514c02708081d18330d759d10e306cee16c6350de243c68f0973777d2582f5d81968a237393c1f59aba245297e03f3f98d3ae5249a042469d0d016255f568719
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 15, 2023
5364dd8 test: locked_wallet, skip default fee estimation (furszy) Pull request description: Coming from bitcoin#28139 (comment). No test case in this file is meant to exercise fee estimation. All default wallets have a custom tx fee set [here](https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/test/functional/wallet_fundrawtransaction.py#L100). The only one missing is the one created for `locked_wallet`. ACKs for top commit: theStack: ACK 5364dd8 Tree-SHA512: 514c02708081d18330d759d10e306cee16c6350de243c68f0973777d2582f5d81968a237393c1f59aba245297e03f3f98d3ae5249a042469d0d016255f568719
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Coming from #28089 (comment).
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 (#28089), where other test cases fail due the wallet re-locking
itself after the 100ms.