Optional parameter for rescans to start at a specified height #7984

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:rescan-fromheight changing 4 files +74 −13
  1. achow101 commented at 2:42 pm on May 1, 2016: member

    This PR adds optional parameters to the importprivkey, importaddress, and importpubkey RPCs to allow starting the rescan from a certain height instead of the genesis block. It also adds the optional paramter to the -rescan command line option.

    This allows rescans to be shortened if the user knows the block in which the first transaction for his addresses was included in.

  2. MarcoFalke added the label RPC/REST/ZMQ on May 1, 2016
  3. MarcoFalke commented at 4:07 pm on May 1, 2016: member

    Concept ACK.

    Related: #6570, #7061, #7551

  4. in src/wallet/rpcdump.cpp: in 5c2784eb49 outdated
    114@@ -114,6 +115,11 @@ UniValue importprivkey(const UniValue& params, bool fHelp)
    115     if (params.size() > 2)
    116         fRescan = params[2].get_bool();
    117 
    118+    // Get the start height
    


    kazcw commented at 5:59 pm on May 1, 2016:
    Why not define this in the if (fRescan) block where it’s used?

    achow101 commented at 6:39 pm on May 1, 2016:
    fixed
  5. in src/wallet/rpcdump.cpp: in 5c2784eb49 outdated
    228@@ -222,6 +229,11 @@ UniValue importaddress(const UniValue& params, bool fHelp)
    229     if (params.size() > 3)
    230         fP2SH = params[3].get_bool();
    231 
    232+    // Get the start height
    233+    CBlockIndex* start = chainActive.Genesis();
    234+    if (params.size() > 4 && fRescan)
    235+        start = chainActive[params[4].get_int()];
    


    kazcw commented at 6:03 pm on May 1, 2016:
    Indexing into chainActive isn’t safe until after locking cs_main

    achow101 commented at 6:32 pm on May 1, 2016:
    If I move it down to if (fRescan), cs_main should be locked, right?
  6. jonasschnelli commented at 6:04 pm on May 1, 2016: contributor

    I tend to concept NACK. What if you want to import 10 addresses? Self-calculate the oldest heigh and pass it in when you import the last address?

    IMO the importmulti #7551 call makes more sense maybe there could also be a reason to support the rescanblockchain #7061. But later should not be required if we do #7551 right.

  7. in src/wallet/wallet.cpp: in 5c2784eb49 outdated
    3139@@ -3140,7 +3140,9 @@ bool CWallet::InitLoadWallet()
    3140     RegisterValidationInterface(walletInstance);
    3141 
    3142     CBlockIndex *pindexRescan = chainActive.Tip();
    3143-    if (GetBoolArg("-rescan", false))
    3144+    if (GetBoolArg("-rescan", false) && GetArg("-rescan", 0) > 0)
    3145+        pindexRescan = chainActive[GetArg("-rescan", 0)];
    3146+    else if (GetBoolArg("-rescan", false))
    


    kazcw commented at 6:08 pm on May 1, 2016:
    This doesn’t need to be a special case since the default for -rescan is 0, and chainActive.Genesis() == chainActive[0]

    achow101 commented at 6:39 pm on May 1, 2016:
    fixed
  8. achow101 commented at 6:26 pm on May 1, 2016: member

    What if you want to import 10 addresses? Self-calculate the oldest heigh and pass it in when you import the last address?

    Sure, why not? The current behavior would have it rescan from genesis for the last address, and that behavior will still happen here.

    As for importmulti, I agree that would also be good. However, I think that the individual import RPCs should also have the same rescanning behavior that importmulti does.

  9. add parameters for optional start height for rescans
    When importing an address, privkey, or pubkey, there is now an option which allows users to set the height from which to start rescanning to save time.
    bb29ad388f
  10. Optional parameter to init rescan option
    Add the optional parameter to the -rescan command line option to allow rescanning from a block height.
    175e0f50ec
  11. Tests for rescanning from heights
    Add tests for the optional parameters to rescan from height for importaddress, importprivkey, and importpubkey.
    2297b6ca87
  12. achow101 force-pushed on May 1, 2016
  13. jonasschnelli commented at 6:54 am on May 2, 2016: contributor

    @achow101: Right. This is a point.

    Either we support no rescan for a single input (maybe together with a manual rescan like #7061) or each address/script/pubkey input call should support the same rescan flexibility.

    The question now is if we should support heights or timestamps (key birthday). IIRC discussions in #7061 showed that most devs prefer timestamps.

  14. laanwj commented at 11:35 am on May 2, 2016: member

    100% agree with @jonasschnelli, importmulti with explicit key birthdays is the answer here, not adding yet another positional argument to RPC calls (yes, you can also use importmulti to import just one key). Let’s focus on that pull and get it merged.

    In time we can deprecate import* in favor of the importmulti interface which can (but doesn’t need to) handle multiple keys. There are some other good reasons for that too: see discussion here #7551 (comment) on the importscript confusion. (and no one generally really wants to import keys one by one and rescan in between)

  15. promag commented at 12:36 pm on May 2, 2016: member

    Agree with @laanwj and @jonasschnelli. For me batch calls are preferred. Still not sure about the name importmulti thou.

    But let’s not put aside the concept of rescanblockchain. It makes perfect sense to trigger notifications from a specific block/timestamp.

  16. promag commented at 12:39 pm on May 2, 2016: member

    (and no one generally really wants to import keys one by one and rescan in between) @laanwj true but one can import one by one and then issue the rescan from the correct timestamp.

  17. laanwj commented at 12:40 pm on May 2, 2016: member

    @promag

    true but one can import one by one and then issue the rescan from the correct timestamp.

    I agree, but that’s inferior in every way to just having one self-contained operation.

  18. promag commented at 12:57 pm on May 2, 2016: member

    I agree, but that’s inferior in every way to just having one self-contained operation. @laanwj well this discussion is like *rawtransaction calls vs createtransaction on steroids. In terms of API it is more desirable to have simple, flexible, self explanatory calls instead of one super configurable call.

    However I see this like OpenGL glVertex vs glDrawArrays. There are too much locks and alike in import one by one vs impormulti.

    Regarding rescan option, I think it is very reasonable to be in importmulti, but I hope it can be possible to just issue a rescan (for instance #7061).

  19. sipa commented at 6:34 pm on June 2, 2016: member

    @promag I think this is a bit different.

    If we would have had something like importmulti as the only mechanism for importing, I think nobody would even know the concept of a rescan, which is just an artifact of how wallets used to work (without knowing up to where they were synced, and without birth time).

  20. jonasschnelli commented at 6:38 pm on June 2, 2016: contributor
    I think as long as we support/have a rescan RPC call, we should also support an optional “fromheight” argument. We could argue about dropping the rescan call.
  21. jonasschnelli commented at 6:41 pm on June 2, 2016: contributor
    Sorry, I meant -rescan as startup option (not as RPC call). IMO we could drop the -rescan startup argument in favor or “importmulti” or/and RPC “rescanblockchain” https://github.com/bitcoin/bitcoin/pull/7061/files
  22. sipa commented at 6:47 pm on June 2, 2016: member

    We’ve talked about changing -rescan to a dummy before, because people would keep on suggesting it as a magic fix for all sorts of problems for years on the forums, while it had absolutely no reason (only when you manually changed your wallet.dat file it would be needed).

    Since the introduction of importprivkey without rescan, it became useful again, since it was the only way to import multiple keys without a rescan after each.

    I really think we should stop encouraging these crazy schemes that require people to know there is such a thing as a rescan and rely on having blockchain data available. RPC calls should just work, not require hacks to bring the wallet in a consistent state after the fact, with possible atomicity issues.

  23. sipa commented at 6:48 pm on June 2, 2016: member
    Concept NACK
  24. sipa closed this on Aug 25, 2016

  25. achow101 deleted the branch on Oct 29, 2016
  26. achow101 restored the branch on Oct 29, 2016
  27. achow101 deleted the branch on Apr 5, 2017
  28. DrahtBot locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me