Add option to specify rescan starting timestamp to RPC import calls

promag commented at 2:56 pm on August 18, 2015: member

promag force-pushed on Aug 18, 2015

casey commented at 3:34 pm on August 18, 2015: contributor

I think this should check if the passed in height is too high, and throw an error if it is.

jonasschnelli commented at 3:43 pm on August 18, 2015: contributor

I like the idea of not rescanning from genesis in case of a imported key. But i think it should take the creation-timestamp instead of a height as parameter. The height should be calculated out of the given creation time minus a buffer of maybe 288 blocks (or less).

promag force-pushed on Aug 18, 2015

promag commented at 3:53 pm on August 18, 2015: member

@casey right, done.

promag commented at 3:57 pm on August 18, 2015: member

@jonasschnelli what about a mixed solution, if the param is greater than, for instance, 1231006505 (genesis timestamp) then use the param as a timestamp?

jonasschnelli commented at 4:05 pm on August 18, 2015: contributor

@promag I would guess most users prefer keeping the date when they have created the key instead of the height. Mixes solution sounds to clever at user input level (might be useful on script level to avoid soft-forks).

sipa commented at 4:32 pm on August 18, 2015: member

Just use importwallet RPC instead of importprivkey…

ruimarinho commented at 5:36 pm on August 18, 2015: none

@sipa I think one good use case is to import watch-only addresses and only rescan from a certain point in time.

sipa commented at 6:09 pm on August 18, 2015: member

Then we should add support for watch-only addresses to importwallet.

dcousens commented at 0:42 am on August 19, 2015: contributor

@sipa maybe I’m not following, but how does importwallet allow you to set the rescan from blockheight?

sipa commented at 5:12 am on August 19, 2015: member

It supports a birth time for each key, and automatically determines from where to rescan.

dcousens commented at 5:52 am on August 19, 2015: contributor

@sipa TIL, but importwallet also requires a file on disk right? (ref: https://bitcoin.org/en/developer-reference#importwallet) Doesn’t seem like an ideal solution if your wanting to do this as part of an API.

jonasschnelli commented at 6:20 am on August 19, 2015: contributor

I also don’t think importwallet is the right way to go. You can forge a dumped wallet and use it like sipa has explained. But that’s not a user friendly input, but might be a solution if you want to avoid a full rescan in case you like to import keys in the current version.

Supporting a creation date in importXXX would be nice.

MarcoFalke commented at 6:31 am on August 19, 2015: member

@jonasschnelli Can you explain what you mean by “might be useful on script level to avoid soft-forks” in regard to the “mixed solution”?

sipa commented at 6:31 am on August 19, 2015: member

The reason I’m advocating importwallet is because I think it’s unreasonable to ask the user to need to decide when to rescan in case of multiple addresses/keys(/hdchains?) being imported simultaneously. The efficiency is ridiculous when you rescan for all, and otherwise you at least have temporarily an inconsistent state where you have wallet keys which are not scanned for, and need to remember to not rescan exactly once at the end.

Importwallet solves all of that.

I understand that needing a file on disk is not appropriate for all purposes, but we could easily make a variant that takes the entire serialized file through RPC.

jonasschnelli commented at 6:38 am on August 19, 2015: contributor

@MarcoFalke: we have such threshold checks on script level (https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L1133) to allow reusing existing containers. But i think threshold detection on height/timestamp makes not much sense on the RPC user input level.

dcousens commented at 7:53 am on August 19, 2015: contributor

@sipa I think that depends on your user. I think a primary use case for this might be the generation of private keys in an external environment, coupled with the desire to watch them [using bitcoind] from a certain block height onwards. The user would be very aware of ‘from when’ they want to watch, because they just created them?

In any case, I think if the importwallet API supported a JSON input, then that solution would be fine, provided it doesn’t feel out-of-place.

ruimarinho commented at 9:44 am on August 19, 2015: none

Precisely what @dcousens said. Importing a single or even multiple addresses from another environment where we know the timestamp to start rescanning is an ideal use case for this change.

promag force-pushed on Aug 19, 2015

promag renamed this:
~~Add option to specify import rescan from block index~~
Add option to specify rescan starting timestamp to RPC import calls
on Aug 19, 2015

promag commented at 12:24 pm on August 19, 2015: member

Agree with @dcousens and @ruimarinho. @jonasschnelli changed to timestamp. @sipa created CChain::FindLatestBefore(int64_t nTime) because it is used more than once. I suppose it could do a binary search on CChain::vChain, but for now keeping the same implementation from importwallet.

Add option to specify rescan starting timestamp to RPC import calls a80b832038

promag force-pushed on Aug 19, 2015

in src/wallet/rpcdump.cpp: in a80b832038

114@@ -114,6 +115,14 @@ UniValue importprivkey(const UniValue& params, bool fHelp)
115     if (params.size() > 2)
116         fRescan = params[2].get_bool();
117 
118+    CBlockIndex *pindex = chainActive.Genesis();
119+    if (params.size() > 3) {
120+        int nTime = params[3].get_int();
121+        pindex = chainActive.FindLatestBefore(nTime);
122+        if (!pindex)
123+            throw JSONRPCError(RPC_INVALID_PARAMETER, "No block before timestamp");

MarcoFalke commented at 2:14 pm on August 19, 2015:

With the current implementation of FindLatestBefore() this is dead code and can never be thrown?

luke-jr commented at 2:26 pm on January 3, 2016:

It’s also a useless error - the user doesn’t care if there was a block before their timestamp… just rescan the entire chain in that case.

laanwj commented at 2:22 pm on August 19, 2015: member

Agree @sipa regarding ridiculous inefficiency of doing a rescan for each key, and temporary inconsistent state.

I also don’t like adding more positional arguments to import*. Getting positional arguments right is a scourge to users.

What about an RPC call that acceps an array of {privkey, birthdate, …} objects? By using an object it could be extendible in case more key metadata is desired. The same code as importwallet sans file reading could be used. It would rescan at the end if and from the height necessary. It could also support watch-only imports in the same batch.

dcousens commented at 0:55 am on August 20, 2015: contributor

In any case, I think if the importwallet API supported a JSON input, then that solution would be fine, provided it doesn’t feel out-of-place. @laanwj, I think importwallet, with an optional string filename or json object, could work well. It’d also be nice if you could optionally disable any RPC API’s that look can look anywhere on the file system, but that may be another issue.

promag commented at 1:40 am on August 20, 2015: member

So everyone agree on a generic import call with an array of multiple objects?

dcousens commented at 1:48 am on August 20, 2015: contributor

@promag are you suggesting merging the two commands?

promag commented at 2:06 am on August 20, 2015: member

I’m suggesting a new call, that can do the same as the others, and deprecate for a while the existing ones.

in src/chain.cpp: in a80b832038

57@@ -58,6 +58,13 @@ const CBlockIndex *CChain::FindFork(const CBlockIndex *pindex) const {
58     return pindex;
59 }
60 
61+CBlockIndex *CChain::FindLatestBefore(int64_t nTime) const {
62+    CBlockIndex *pindex = Tip();
63+    while (pindex && pindex->pprev && pindex->GetBlockTime() > nTime)

jonasschnelli commented at 6:57 am on August 20, 2015:

As already commented, i think we should go down serval blocks (~144) because of possible inaccurate timestamps.

dcousens commented at 7:07 am on August 20, 2015:

@jonasschnelli what is the variance on timestamps? Is ~144 enough (or to much)?

jonasschnelli commented at 7:15 am on August 20, 2015:

The cost of rescaning 144 blocks deeper (~1day) is pretty low (+some seconds/minutes). Given that the source of the timestamp is unknown (user could enter it manually because he know when he approx. created the key, or other wallet software did mess around with timezones, etc.) this cost of +144 blocks is probably worth taking.

promag commented at 9:33 am on August 20, 2015:

@jonasschnelli However if you know the exact timestamp then that extra time is unnecessary. For a person the offset may be useful, but for a system I think not.

ruimarinho commented at 0:15 am on August 21, 2015:

Agreed with @promag, a system can add the safety net of ~144 blocks to the timestamp if necessary and a person likely knows what they are doing if they opt in for this.

dcousens commented at 0:34 am on August 21, 2015:

@jonasschnelli have to agree with @ruimarinho, this safety net could easily be added by the API user, if desired. Overall, the only danger for this lies in people using the timestamp aspect of the API instead of a block height.

luke-jr commented at 2:24 pm on January 3, 2016:

Maybe just using the median time past is sufficient?

jtimon commented at 6:57 pm on January 3, 2016:

Using the median time past seems to solve the concern without introducing much extra complexity.

jonasschnelli commented at 7:00 am on August 20, 2015: contributor

Agree with @laanwj. Multiple imports with a provided timestamp ends up in inefficient rescans (multiple rescans). The idea of allowing a json objects for importing keys/addresses like {"key/address" : <timestamp>, "key/address" : <timestamp>, ...} would be great. The code than should retrieve the “oldest” key timestamp and use that date for calculating the rescan height.

laanwj added the label RPC on Aug 20, 2015

in src/wallet/rpcdump.cpp: in a80b832038

86             "\nAdds a private key (as returned by dumpprivkey) to your wallet.\n"
87             "\nArguments:\n"
88             "1. \"bitcoinprivkey\"   (string, required) The private key (see dumpprivkey)\n"
89             "2. \"label\"            (string, optional, default=\"\") An optional label\n"
90             "3. rescan               (boolean, optional, default=true) Rescan the wallet for transactions\n"
91+            "4. timestamp            (numeric, optional, default=0) Rescan starts at timestamp\n"

ruimarinho commented at 0:20 am on August 21, 2015:

I think the default should be 1231006505 (genesis block), as this is epoch time and therefor not a relative value.

dexX7 commented at 3:39 pm on August 24, 2015:

@ruimarinho: note that 1231006505 is only the timestamp of the mainnet genesis block, whereby it’s 1296688602 for testnet and regtest.

I’m not sure, how the time might be further affected by setmocktime, so I mention it just for completeness.

promag commented at 10:10 am on November 17, 2015:

I would leave the default value as 0 meaning Thu, 01 Jan 1970 00:00:00 GMT and mention in the description it is epoch time. I see no point to use the first main block timestamp.

luke-jr commented at 2:25 pm on January 3, 2016:

Why not just repurpose the rescan parameter? That is, if it’s a boolean, you get the current behaviour, but if it’s a Number, you interpret it as a timestamp.

jtimon commented at 6:58 pm on January 3, 2016:

@luke-jr that seems less clear and more confusing for no good reason (unless I’m missing the good reason, of course).

luke-jr commented at 9:22 pm on January 3, 2016:

It’s less ugly than two fields IMO.

MarcoFalke commented at 9:36 pm on January 3, 2016:

What about getting rid of the boolean in the help message and only mention the numeric? (The code could still accept boolean for some grace period, though)

luke-jr commented at 9:43 pm on January 3, 2016:

Without the boolean, there is no way to skip rescan altogether.

MarcoFalke commented at 9:47 pm on January 3, 2016:

Set the timestamp to block+1 or today+1, which will rescan only the block of the tip, iirc.

luke-jr commented at 9:55 pm on January 3, 2016:

Surely that is not a serious suggestion…

jtimon commented at 10:47 pm on January 3, 2016:

I prefer luke-jr’s solution provided it’s not “bool and time at the same time”. It could be always timestamp but 0 can mean no rescan or something like that. rescan=1 meaning “rescan true but with the dwfault timestamp which is not 1” is what seems too unintuitive to me (not having one parameter instead of two) and is what I understood luke was proposing (but please clarify if you were suggesting something else).

MarcoFalke commented at 11:02 pm on January 3, 2016:

@luke-jr So your suggestion is something like

"3. rescan               (boolean or numeric, optional, default=true) Rescan for wallet transactions starting at the genesis block. A numeric argument is interpreted as the timestamp of the block where to begin to rescan.\n"

?

jtimon commented at 11:09 pm on January 3, 2016:

What about something like…

"3. rescan (numeric, optional, default=1231006505) Rescan for wallet transactions starting at the timestamp provided (main chain genesis block by default). 0 indicates no rescan.\n"

?

MarcoFalke commented at 11:13 pm on January 3, 2016:

Would work for me. And then throw the JSON value is not an integer as expected (code -1) or accept it during a grace period?

Edit: fixed typo per @jtimon

jtimon commented at 11:18 pm on January 3, 2016:

you mean “is not numeric as expected”? I would prefer to do that than to have the argument be “bool or numeric” for some time. If we really want to do the grace period, better create a new one and remove the old one after the grace period.

luke-jr commented at 2:14 am on January 4, 2016:

So your suggestion is something like “3. rescan (boolean or numeric, optional, default=true) Rescan for wallet transactions starting at the genesis block. A numeric argument is interpreted as the timestamp of the block where to begin to rescan.\n”? @MarcoFalke Exactly. This is clean.

promag commented at 10:39 am on August 24, 2015: member

The current importwallet call doesn’t accept JSON and changing that is a breaking change. Therefore I propose a new RPC call importmulti with one JSON argument.

Here is a proposal of the JSON argument:

0[{
1  "type": "privkey | pubkey | ...",
2  "value": "...",
3  "timestamp": "...",         // optional
4  "label": "..."              // optional
5}]

The implementation will follow importwallet.

dcousens commented at 11:03 am on August 24, 2015: contributor

@promag timestamp is optionally a blockheight right?

promag commented at 11:08 am on August 24, 2015: member

@dcousens well, timestamp is something like 2015-04-24T10:33:24Z. If there is a need for block height then IMO the JSON should have an explicit key for that.

dcousens commented at 11:09 am on August 24, 2015: contributor

IMHO a block height (as height OR hash) is more valuable than a time stamp, especially if this is coming from an externally managed service.

promag commented at 11:11 am on August 24, 2015: member

@dcousens If an external service wants to creates an address then it has to make a call to know the current block count. It is easier to save the creation timestamp along with the address.

ruimarinho commented at 11:15 am on August 24, 2015: none

I’d say they are equivalent, as ISO 8601 is interchangeable with unix timestamp. Both could be supported.

promag commented at 11:29 am on August 24, 2015: member

@ruimarinho sure, however I don’t think timestamp should allow block height.

dcousens commented at 11:30 am on August 24, 2015: contributor

@dcousens If an external service wants to creates an address then it has to make a call to know the current block count.

Lets say I receive a transaction: X, I already know it was confirmed at block height Y.

Based on this, externally, I generate an address/key pair P based on some information in X. To then import P, you’re suggesting I then have to find out what the timestamp of block Y was? Sounds like a minimum RTT of 2 API requests.

I’m not against that, a solution is a solution, but, its a bit lame IMHO.

edit: reworded

promag commented at 11:40 am on August 24, 2015: member

Just to be clear, I’m fine with blockheight. However in your use case, if you know the block height of X then you can also know the block timestamp, right?

dcousens commented at 12:55 pm on August 24, 2015: contributor

However in your use case, if you know the block height of X then you can also know the block timestamp, right?

How do you figure that? Maybe I should, but, from the data I currently have, that isn’t the case.

promag commented at 1:31 pm on August 24, 2015: member

Off topic, but I was assuming you use blocknotify script, then get the block and txs.

laanwj commented at 9:22 am on August 25, 2015: member

@promag

0{
1  "type": "privkey | pubkey | ...",
2  "value": "...",
3  "timestamp": "...",         // optional
4  "label": "..."              // optional
5}]

Looks good to me!

promag commented at 9:35 am on August 25, 2015: member

@laanwj and importmulti or just import?

laanwj commented at 10:39 am on August 25, 2015: member

I’m very bad at bikeshedding names, but I’d say importmulti

ruimarinho commented at 10:43 am on August 25, 2015: none

I’d call it import for simplicity and flexibility. We should also discuss whether this new RPC call accepts a single object or only collections.

laanwj commented at 11:40 am on August 25, 2015: member

Simple: if you want to pass it one, just pass an array of one. You could even pass it an empty array.

ruimarinho commented at 3:14 pm on August 25, 2015: none

Are you open to a polymorphic method where a JSON object is converted internally to JSON collection of a single item? Not that important, but at least it’d be a little bit more flexible for devs.

0import { "type": "privkey", "value": "..." }
1import [{ "type": "privkey", "value": "..." }]

MarcoFalke commented at 3:03 pm on September 23, 2015: member

@promag Any updates on this?

gmaxwell commented at 1:38 am on November 17, 2015: contributor

Hm. Concept wise, I am really unhappy that right now pruning completely disables rescan. This means you can’t use pruning with many common use cases.

For example, pruning more than a month old + merchant wallet that watchadds right around the time it gives out a key– without the ability to do a limited rescan you have to get everything exactly right or risk missing a payment.

One thought I had on this (before realizing there was already a pull) was simply making it so the existing positional rescan argument can alternatively take a rescan depth. This would avoid adding more arguments. A thing to keep in mind is that a depth is not like a birthday, and I don’t think the importwallet arguments apply as strongly.

promag commented at 10:03 am on November 17, 2015: member

@MarcoFalke been busy, but I’ll resume soon.

nunofgs commented at 10:34 am on November 18, 2015: none

@promag: will you be adding tests for this feature?

promag commented at 11:47 am on November 18, 2015: member

@nunofgs sure thing mate :smile:

luke-jr commented at 2:28 pm on January 3, 2016: member

A lot of redundant code in the import* RPCs - maybe it can be abstracted better? Also, it would be ideal (but not necessary in this PR I suppose) to support rescanning in pruned mode when possible…

luke-jr commented at 2:29 pm on January 3, 2016: member

(and Concept ACK for importmulti)

dcousens commented at 10:44 pm on January 3, 2016: contributor

needs rebase

promag commented at 1:09 am on February 25, 2016: member

Deprecated by #7551.

promag closed this on Feb 25, 2016

promag commented at 1:10 am on February 25, 2016: member

@gmaxwell in #7551 rescan is done even if prunning is enabled.

DrahtBot locked this on Sep 8, 2021

Add option to specify rescan starting timestamp to RPC import calls #6570