Skip to content

test: add functional test for upgradewallet rpc #32803

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
Closed
wants to merge 1 commit into from
Closed

test: add functional test for upgradewallet rpc #32803

wants to merge 1 commit into from

Conversation

Prabhat1308
Copy link
Contributor

followups up from #32438 (comment) to add coverage for upgradewallet rpc. PR adds 2 test cases , trying to upgrade from the latest version and asserting it stays the same and trying to downgrade from the latest version. Test case to actually upgrade is not possible currently since the createwallet rpc by default creates wallet with version FEATURE_LATEST which is the highest.

@DrahtBot DrahtBot added the Tests label Jun 24, 2025
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 24, 2025 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32803.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

@Prabhat1308 Prabhat1308 force-pushed the probot/test_upgradewallet_rpc branch from 833e348 to bc0c282 Compare June 24, 2025 15:28
@DrahtBot
Copy link
Contributor

🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/runs/44696601189
LLM reason (✨ experimental): Lint check failed due to trailing whitespace and missing trailing newline in the code.

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

@Prabhat1308 Prabhat1308 force-pushed the probot/test_upgradewallet_rpc branch from bc0c282 to 8621333 Compare June 24, 2025 15:33
@maflcko
Copy link
Member

maflcko commented Jun 24, 2025

are there any plans that the rpc will be used anytime soon in the next couple of years? If not, it could make sense to just remove it, and add it back, in the unlikely case it will be used?

@Prabhat1308
Copy link
Contributor Author

are there any plans that the rpc will be used anytime soon in the next couple of years? If not, it could make sense to just remove it, and add it back, in the unlikely case it will be used?

Looking through the code this is pretty much a dead rpc where no one can use it to actually upgrade wallet and there is only 1 check that multiwallet can't call it before this PR . I am not very sure about what the future plans are for wallet but the only case where I see this being used is if there is a need to upgradewallet version to quantum-compatible wallets. Other than that I see no future use case of the rpc in the near future.

@Prabhat1308
Copy link
Contributor Author

I am not so sure why the CI fails to call createwallet for the added test while works on other rpc tests . Will try to debug

pablomartin4btc
pablomartin4btc reviewed Jun 24, 2025
View reviewed changes
Copy link
Member

@pablomartin4btc pablomartin4btc left a comment

Choose a reason for hiding this comment

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

You need to add this (CI "no wallet..." failure) to test/functional/rpc_upgradewallet.py:

    def skip_test_if_missing_module(self):
        self.skip_if_no_wallet()

@Prabhat1308 Prabhat1308 force-pushed the probot/test_upgradewallet_rpc branch 2 times, most recently from e25a15f to 1d7b395 Compare June 24, 2025 18:09
@Prabhat1308
add functional test for upgradewallet rpc
cead0fa 
Adds functional test to add coverage for upgradewallet rpc .
@Prabhat1308 Prabhat1308 force-pushed the probot/test_upgradewallet_rpc branch from 1d7b395 to cead0fa Compare June 24, 2025 18:13
@pablomartin4btc
Copy link
Member

As @maflcko mentioned, not sure how useful would be to keep the RPC, and maybe you can create the PR for its removal. I thought perhaps you could move this test to wallet_backwards_compatibility.py, but again not sure about its purpose.

cc @achow101, @furszy

@kevkevinpal kevkevinpal mentioned this pull request Jun 24, 2025
Closed
@w0xlt w0xlt mentioned this pull request Jul 10, 2025
Merged
@Prabhat1308
Copy link
Contributor Author

closing in favour of #32944

@Prabhat1308 Prabhat1308 deleted the probot/test_upgradewallet_rpc branch July 11, 2025 06:48
achow101 added a commit that referenced this pull request Jul 25, 2025
@achow101
Merge #32944: wallet: Remove upgradewallet RPC
2e97541 
d89c6fa wallet: Remove `upgradewallet` RPC (w0xlt)

Pull request description:

  Based on discussions  in #32803, this PR proposes removing the ` upgradewallet`  RPC.

ACKs for top commit:
  maflcko:
    review ACK d89c6fa 🤙
  achow101:
    ACK d89c6fa
  pablomartin4btc:
    ACK d89c6fa
  brunoerg:
    Concept & light cr ACK d89c6fa

Tree-SHA512: 9ab89c9137ff83d7826da6b9d00d3617149a5d144129086a2685ee525087534c5ed06259075c0689ded52d33e075acb5067d185be04ecc638e27469f958f9a56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@pablomartin4btc pablomartin4btc pablomartin4btc left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Prabhat1308 @DrahtBot @maflcko @pablomartin4btc