Optional update rescan option in importmulti RPC

pedrobranco commented at 10:33 am on October 11, 2017: contributor

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).

fanquake added the label RPC/REST/ZMQ on Oct 11, 2017

in src/wallet/rpcdump.cpp:1066 in f8cb517ffe outdated

1062@@ -1063,6 +1063,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1063             "2. options                 (json, optional)\n"
1064             "  {\n"
1065             "     \"rescan\": <false>,         (boolean, optional, default: true) Stating if should rescan the blockchain after all imports\n"
1066+            "     \"rescanUpdate\": <false>,   (boolean, optional, default: true) Stating if rescan should notify existent transactions\n"

promag commented at 11:04 am on October 11, 2017:

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

promag commented at 11:06 am on October 11, 2017:

See: https://github.com/bitcoin/bitcoin/blob/364da2c52942ebfe92d1d1495a309a8a9dd18ef3/src/wallet/rpcwallet.cpp#L3060

in src/wallet/rpcdump.cpp:1140 in f8cb517ffe outdated

1136@@ -1131,7 +1137,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1137     }
1138 
1139     if (fRescan && fRunScan && requests.size()) {
1140-        int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, true /* update */);
1141+        int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, fRescanUpdate /* update */);

promag commented at 11:08 am on October 11, 2017:

Remove comment.

promag commented at 11:11 am on October 11, 2017: member

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

pedrobranco force-pushed on Oct 11, 2017

pedrobranco commented at 2:08 pm on October 18, 2017: contributor

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

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

in src/wallet/rpcdump.cpp:1065 in b20645a68c outdated

1062@@ -1063,6 +1063,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1063             "2. options                 (json, optional)\n"
1064             "  {\n"
1065             "     \"rescan\": <false>,         (boolean, optional, default: true) Stating if should rescan the blockchain after all imports\n"

promag commented at 2:10 pm on October 18, 2017:

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

promag commented at 2:16 pm on October 18, 2017: member

Missing test test.

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

pedrobranco commented at 2:55 pm on October 18, 2017: contributor

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 commented at 11:25 am on March 12, 2018: contributor

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 force-pushed on Mar 12, 2018

DrahtBot closed this on Jul 21, 2018

DrahtBot reopened this on Jul 21, 2018

DrahtBot commented at 11:16 am on October 20, 2018: member

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.

DrahtBot added the label Needs rebase on Dec 5, 2018

in src/wallet/rpcdump.cpp:1155 in dc315159db outdated

1151@@ -1151,13 +1152,18 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1152 
1153     //Default options
1154     bool fRescan = true;
1155+    bool fUpdate = true;

MarcoFalke commented at 6:36 pm on December 5, 2018:

New code should be snake_case, without the prefix

pedrobranco commented at 2:57 pm on January 26, 2019:

Should we also rename the other variables on this method?

promag commented at 3:00 pm on January 26, 2019:

Just new code.

pedrobranco force-pushed on Jan 26, 2019

Optional update rescan on importmulti 2fd985dd29

pedrobranco force-pushed on Jan 26, 2019

DrahtBot removed the label Needs rebase on Jan 26, 2019

promag commented at 4:14 pm on January 26, 2019: member

This definitely needs a test.

MarcoFalke added the label Needs rebase on Feb 12, 2019

in src/wallet/rpcdump.cpp:1192 in 2fd985dd29

1188@@ -1189,6 +1189,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1189                     {"options", RPCArg::Type::OBJ, /* opt */ true, /* default_val */ "null", "",
1190                         {
1191                             {"rescan", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "true", "Stating if should rescan the blockchain after all imports"},
1192+                            {"update", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "true", "Stating if rescan should notify existent transactions"},

MarcoFalke commented at 11:57 pm on February 12, 2019:

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

MarcoFalke deleted a comment on Feb 12, 2019

pedrobranco closed this on May 19, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

MarcoFalke locked this on Dec 16, 2021

Optional update rescan option in importmulti RPC #11484

Conflicts