Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: Bump walletpassphrase timeout to avoid intermittent issue #28089

Closed
wants to merge 1 commit into from
Closed

test: Bump walletpassphrase timeout to avoid intermittent issue #28089

wants to merge 1 commit into from

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jul 17, 2023

There is no risk increasing the timeout, but it avoids intermittent issues (when running in valgrind) of the form:

 node1 2023-07-15T06:17:37.827609Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:45950 
 node1 2023-07-15T06:17:37.833168Z [httpworker.3] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=signrawtransactionwithwallet user=__cookie__ 
 test  2023-07-15T06:17:38.037000Z 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 137, in run_test
                                       self.test_many_inputs_send()
                                     File "/root/b-c-ci/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_fundrawtransaction.py", line 687, in test_many_inputs_send
                                       fundedAndSignedTx = self.nodes[1].signrawtransactionwithwallet(fundedTx['hex'])
                                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     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: Error: Please enter the wallet passphrase with walletpassphrase first. (-13)

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 17, 2023 •
edited

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 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:

  • #28139 (test: create wallet specific for test_locked_wallet case by furszy)

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 DrahtBot added the Tests label Jul 17, 2023
furszy
furszy reviewed Jul 17, 2023
View reviewed changes
test/functional/wallet_fundrawtransaction.py
Comment on lines +634 to 637
self.nodes[1].walletpassphrase("test", 999999)
signedTx = self.nodes[1].signrawtransactionwithwallet(fundedTx['hex'])
self.nodes[1].sendrawtransaction(signedTx['hex'])
self.generate(self.nodes[1], 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fishy. It seems that we unlock the wallet inside a specific test case, and use the unlocked period for other test cases.

A better approach would be to relock the wallet at the end of this test case ( self.nodes[1].walletlock()). And unlock-lock the wallet in the other cases when it is needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. An alternative might be to not use a passphrase after this test. Do you want to submit either as a pull request? Happy to close mine then. Otherwise, I'll leave the feedback for a follow-up, as the goal here is to not change any logic, only the constant.

Copy link
Member

@furszy furszy Jul 17, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative might be to not use a passphrase after this test

Yeah. Would be even better to create a wallet specific to this test and unload it at the end of it.

Do you want to submit either as a pull request? Happy to close mine then. Otherwise, I'll leave the feedback for a follow-up, as the goal here is to not change any logic, only the constant.

Will try to do it later today. Shouldn't take me much.
Still, all good if this one gets merged before it.

@maflcko maflcko added this to the 26.0 milestone Jul 19, 2023
ishaanam
ishaanam reviewed Jul 23, 2023
View reviewed changes
Copy link
Contributor

@ishaanam ishaanam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa257a7

I would also ACK a PR that creates a wallet specific to this test as discussed above.

@furszy furszy mentioned this pull request Jul 24, 2023
Merged
@fanquake
Copy link
Member

I think this PR can now be closed given #28139?

@maflcko
Copy link
Member Author

maflcko commented Jul 25, 2023

Thanks, yes.

@maflcko maflcko closed this Jul 25, 2023
@maflcko maflcko deleted the 2307-test-int-time- branch July 25, 2023 11:11
fanquake added a commit to bitcoin-core/gui that referenced this pull request Jul 26, 2023
@fanquake
Merge bitcoin/bitcoin#28139: test: create wallet specific for test_lo…
f033a98 
…cked_wallet case

c648bdb test: create wallet specific for test_locked_wallet case (furszy)

Pull request description:

  Coming from bitcoin/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/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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 9, 2023
@fanquake @sidhujag
Merge bitcoin#28139: test: create wallet specific for test_locked_wal…
747a407 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@furszy furszy furszy left review comments

@ishaanam ishaanam ishaanam left review comments

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
26.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@maflcko @DrahtBot @fanquake @furszy @ishaanam