Skip to content

Optional update rescan option in importmulti RPC #11484

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

Optional update rescan option in importmulti RPC #11484

pedrobranco wants to merge 1 commit into from

Conversation

@pedrobranco
Copy link
Contributor

@pedrobranco pedrobranco commented Oct 11, 2017 •
edited by DrahtBot
Loading

This PR adds a new option rescanUpdate in importmulti RPC method to optionally do not rescan transactions that already exist in the wallet.

This is very important for large wallets when importing a single address with a old timestamp, which in this moment triggers a rescan to the whole wallet, replaying several already known wallet events (via notify-wallet).

promag
promag reviewed Oct 11, 2017
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Maybe option should be just update? Needs a test thought.

src/wallet/rpcdump.cpp Outdated
Copy link
Contributor

@promag promag Oct 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Remove : <false>, these <false> and <true> will be removed IMO.

Copy link
Contributor

@promag promag Oct 11, 2017

Choose a reason for hiding this comment

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

See:

bitcoin/src/wallet/rpcwallet.cpp

Line 3060 in 364da2c

" \"replaceable\" (boolean, optional, default true) Whether the new transaction should still be\n"

src/wallet/rpcdump.cpp Outdated
Copy link
Contributor

@promag promag Oct 11, 2017

Choose a reason for hiding this comment

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

Remove comment.

@pedrobranco pedrobranco force-pushed the enhancement/optional-update-rescan-on-importmulti branch from f8cb517 to b20645a Compare October 11, 2017 11:28
@pedrobranco
Copy link
Contributor Author

pedrobranco commented Oct 18, 2017

Maybe option should be just update? Needs a test thought.

Just update isn't too generic? Does not refer to a rescan property.

promag
promag reviewed Oct 18, 2017
View reviewed changes
Copy link
Contributor

@promag promag left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Missing test test.

Regarding the option name, even rescanUpdate doesn't really explain the underlying consequence. Maybe onlyNotifyNew=false or notifyExisting=true.

src/wallet/rpcdump.cpp Outdated
Copy link
Contributor

@promag promag Oct 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Nit, care to remove : <false> here too?

@pedrobranco
Copy link
Contributor Author

pedrobranco commented Oct 18, 2017

Regarding the option name, even rescanUpdate doesn't really explain the underlying consequence. Maybe onlyNotifyNew=false or notifyExisting=true.

Maybe notifyKnownTransactions=true or notifyKnown=true?

Missing test test.

Yes, I will add it soon.

@pedrobranco
Copy link
Contributor Author

pedrobranco commented Mar 12, 2018

After checking the Wallet::RescanFromTime() method, the argument which triggers the notifiers is called fUpdate, so I'm going by your approach of calling it update.

The -help of the command importmulti should be the one that should explain the underlying consequence of using update.

@pedrobranco pedrobranco force-pushed the enhancement/optional-update-rescan-on-importmulti branch from b20645a to dc31515 Compare March 12, 2018 14:29
@DrahtBot DrahtBot mentioned this pull request Jul 13, 2018
Merged
@DrahtBot DrahtBot closed this Jul 21, 2018
@DrahtBot DrahtBot reopened this Jul 21, 2018
This was referenced Oct 20, 2018
Closed
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 20, 2018 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14942 (wallet: Make scan / abort status private to CWallet by Empact)
  • #14918 (RPCHelpMan: Check default values are given at compile-time by MarcoFalke)

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.

maflcko
maflcko reviewed Dec 5, 2018
View reviewed changes
src/wallet/rpcdump.cpp Outdated
Copy link
Member

@maflcko maflcko Dec 5, 2018

Choose a reason for hiding this comment

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

New code should be snake_case, without the prefix

Copy link
Contributor Author

@pedrobranco pedrobranco Jan 26, 2019

Choose a reason for hiding this comment

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

Should we also rename the other variables on this method?

Copy link
Contributor

@promag promag Jan 26, 2019

Choose a reason for hiding this comment

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

Just new code.

@pedrobranco pedrobranco force-pushed the enhancement/optional-update-rescan-on-importmulti branch from dc31515 to 0482356 Compare January 26, 2019 15:05
@pedrobranco pedrobranco force-pushed the enhancement/optional-update-rescan-on-importmulti branch from 0482356 to 2fd985d Compare January 26, 2019 15:06
@promag
Copy link
Contributor

promag commented Jan 26, 2019

This definitely needs a test.

maflcko
maflcko reviewed Feb 12, 2019
View reviewed changes
src/wallet/rpcdump.cpp
{"options", RPCArg::Type::OBJ, /* opt */ true, /* default_val */ "null", "",
{
{"rescan", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "true", "Stating if should rescan the blockchain after all imports"},
{"update", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "true", "Stating if rescan should notify existent transactions"},
Copy link
Member

@maflcko maflcko Feb 12, 2019

Choose a reason for hiding this comment

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

Suggested change
{"update", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "true", "Stating if rescan should notify existent transactions"},
{"update", RPCArg::Type::BOOL, /* default */ "true", "Stating if rescan should notify existent transactions"},

@bitcoin bitcoin deleted a comment from DrahtBot Feb 12, 2019
@promag promag mentioned this pull request May 19, 2019
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@promag promag promag left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pedrobranco @DrahtBot @promag @maflcko @laanwj @fanquake