Skip to content

[RPC] Add importmulti RPC command and functional test #2277

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
Merged
furszy merged 5 commits into from
Apr 6, 2021
Merged

[RPC] Add importmulti RPC command and functional test #2277

furszy merged 5 commits into from
Apr 6, 2021

Conversation

@random-zebra
Copy link

@random-zebra random-zebra commented Mar 30, 2021 •
edited
Loading

Based on top of:

Essentially backport bitcoin#7551 and bitcoin#9490, with more recent changes to adapt to the current sources.
Fix and re-enable the wallet_importmulti.py functional test.

@random-zebra random-zebra added this to the 6.0.0 milestone Mar 30, 2021
@random-zebra random-zebra self-assigned this Mar 30, 2021
@random-zebra random-zebra mentioned this pull request Mar 30, 2021
Merged
1 task
@random-zebra
Copy link
Author

random-zebra commented Apr 4, 2021

Rebased on master after #2240 merge.

@Fuzzbawls
Copy link
Collaborator

Fuzzbawls commented Apr 5, 2021

Functionally good. Looking at the upstream PR 7551, I did see that it was requested to change the second (optional) argument from bool to a JSON object so more options could be added in the future, but it looks like more options have not been added since.

Maybe it would be better for us to revert that argument back to a bool to simplify the command input? Could be done here, in a follow-up, or not at all, since this command is likely only ever going to be used by advanced users who wouldn't have an issue writing a JSON object for the single option.

Also, the help output for this is as-written at the time of the upstream PR is a bit messy and was ultimately cleaned up. This commit (Fuzzbawls@80d160a) can be cherry-picked on top of this to take care of such cleanup while assuming that the input arguments will stay as-is.

@Fuzzbawls Fuzzbawls added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Apr 5, 2021
@Fuzzbawls @random-zebra
Clean up importmulti help text
7e0c709 
Moves the result detailing above the examples and expands it to improve
readability. Also add missing `HelpRequiringPassphrase()`
@random-zebra
Copy link
Author

random-zebra commented Apr 5, 2021

Help text cleanup cherry-picked.
I would rather not simplify the second argument for now. As you said, this command is intended mostly for power-users and scripts.

Fuzzbawls
Fuzzbawls approved these changes Apr 6, 2021
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 7e0c709

furszy
furszy approved these changes Apr 6, 2021
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK 7e0c709 and merging..

@furszy furszy merged commit a279df6 into PIVX-Project:master Apr 6, 2021
random-zebra added a commit that referenced this pull request Apr 11, 2021
@random-zebra
Merge #2279: [RPC][Test] Fix wallet_import_rescan
91dc7f1 
465d1eb [BUG] Wallet: skip non-spendable outputs in CWallet::StakeableCoins (random-zebra)
623e146 [Tests] Fix and re-enable wallet_import_rescan (random-zebra)
e50dc0a Fix importmulti failure to return rescan errors (random-zebra)
c15cd17 Return errors from importmulti if complete rescans are not successful (random-zebra)
df20205 [BUG] net: Consistently use GetTimeMicros() for inactivity checks (random-zebra)
d92f883 [BUG][Wallet] Fix watchonly selection in AvailableCoins (random-zebra)
d6958b7 [RPC][Tests] Use 2 hour grace period for key timestamps in importmulti (random-zebra)

Pull request description:

  Based on top of
  - [x] #2277

  Keep going down the rabbit hole...
  Fix and resurrect `wallet_import_rescan` functional test.

ACKs for top commit:
  furszy:
    re-utACK 465d1eb
  Fuzzbawls:
    ACK 465d1eb

Tree-SHA512: 782ff71d2dfbf0c56381f7de3abc729088eb8fa24517d3f24e8a7a4592a5906315ef9073a9fd8ee491a7ae72e7e4da09bb0e6252bba1eabca930379052255365
@random-zebra random-zebra removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

+1 more reviewer

@furszy furszy furszy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@random-zebra random-zebra

Labels

RPC Tests Upstream Wallet

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

4 participants

@random-zebra @Fuzzbawls @furszy @sdaftuar