Add option to specify rescan starting timestamp to RPC import calls

[promag]

No description provided.

[casey]

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

[jonasschnelli]

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]

@casey right, done.

[promag]

@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]

@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]

Just use importwallet RPC instead of importprivkey...

[ruimarinho]

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

[sipa]

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

[dcousens]

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

[sipa]

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

[dcousens]

@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]

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.

[maflcko]

@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]

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]

@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]

@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]

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]

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.

[maflcko]

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

[luke-jr]

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]

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]

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]

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

[promag]

@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]

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

[promag]

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

[dcousens]

@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]

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]

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]

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

[laanwj]

@promag

{
  "type": "privkey | pubkey | ...",
  "value": "...",
  "timestamp": "...",         // optional
  "label": "..."              // optional
}]

Looks good to me!

[promag]

@laanwj and importmulti or just import?

[laanwj]

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

[ruimarinho]

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]

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

[ruimarinho]

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.

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

[maflcko]

@promag Any updates on this?

[gmaxwell]

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]

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

[nunofgs]

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

[promag]

@nunofgs sure thing mate 😄

[luke-jr]

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]

(and Concept ACK for importmulti)

[dcousens]

needs rebase

[promag]

Deprecated by #7551.

[promag]

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