Importmulti api is confusing in a way that could lead to funds loss.

gmaxwell commented at 8:09 am on January 8, 2017: contributor

Our normal import interfaces introduce keys with an effective timestamp of 0.

Importmulti introduces keys with a default effective timestamp of ’now'.

While documented it is highly surprising, especially because it contradicts the default on the rescan argument:

"rescan": <false>,         (boolean, optional, default: true) Stating if should rescan the blockchain after all imports

This means you can import a key and reasonably expect that it was rescanned (“I even set rescan true!”) but have it not be rescanned, causing you to think the key had been unused and then you give it away and or destroy it and lose funds.

There are a number of other bugs in importmulti (mishandles pruning, needlessly forces the wallet to be unlocked when not importing private keys, etc.) which I am fixing, but I need feedback on this API issue.

Should the default become zero, with -1 special cased as ’now’ or make timestamp mandatory? default it to the prune depth??

Should the result also tell us what it rescanned?

MarcoFalke added the label RPC/REST/ZMQ on Jan 8, 2017

MarcoFalke added the label Wallet on Jan 8, 2017

MarcoFalke added the label Bug on Jan 8, 2017

MarcoFalke added this to the milestone 0.14.0 on Jan 8, 2017

morcos commented at 10:12 pm on January 12, 2017: member

@gmaxwell I also found this surprising. My first inclination was that we should change it, but then it means if you ever leave off a timestamp, you’re stuck rescanning the whole chain.

Perhaps with proper documentation we could have 3 states. rescan: false - no rescan no rescan option set - rescan starts at the minimum of only the provided timestamps (current behavior) rescan: true - rescan starts at genesis unless all timestamps are provided and then starts at the minimum of the timestamps.

I only wish it could tell you what it was planning to rescan before it did it, so you could pull the plug on your computer if you got it wrong.

gmaxwell commented at 7:05 pm on January 19, 2017: contributor

One option is that if it concludes it should rescan everything, it could fail and force you to run with the rescan option set to true.

morcos commented at 11:22 am on January 24, 2017: member

So you must either provide all timestamps or provide rescan = true. And a left off timestamp = genesis. The sounds reasonable to me , but then I think we need the shorthand for now?

sipa commented at 8:40 pm on January 24, 2017: member

I think it would be safer if the timestamp defaults to the beginning of time. I think it’s a better situation to encourage people to use correct timestamps (and rescan=true), rather than have all keys default to ’now'.

TheBlueMatt commented at 10:34 pm on January 24, 2017: member

@gmaxwell that sounds good. So…

rescan: false - no rescan no rescan option set - rescan starts at the minimum of the provided timestamps, but if one is missing throw an exception rescan: true - rescan starts at the minimum of the provided timestamps, or genesis if one or more is missing.

sipa commented at 10:45 pm on January 24, 2017: member

@TheBlueMatt I believe the default for rescan should be true, and there should be no difference between not setting it and passing true.

Rationale: rescanning using -rescan on the command line would result in unexpected behaviour otherwise. The only reasonable condition under which -rescan should skip a block is when all key birthdates are known to be later in time.

sipa commented at 10:46 pm on January 24, 2017: member

Perhaps introducing a special value for ’timestamp’ to explicitly indicate a key is new is a possibility (perhaps “now” or -1)?

TheBlueMatt commented at 11:26 pm on January 24, 2017: member

@sipa hmm? How would -rescan result in strange behavior if you threw, instead of doing any importing? Because it failed to do anything -rescan will only look at the things that were previously there, which is unchanged behavior.

sipa commented at 7:12 pm on February 2, 2017: member

I think the simplest change for now is just making the timestamp mandatory. That requirement can later be relaxed in future versions if we agree on sensible defaults.

luke-jr commented at 7:12 pm on February 2, 2017: member

importmulti can import private keys, which is itself a risk to funds loss for end users who don’t know what they’re doing. So IMO this isn’t a blocker.

But not having a default allows for setting a default later, so if we’re unsure of what defaults ought to be, simply requiring the user to specify it seems like a good alternative.

jonasschnelli commented at 7:13 pm on February 2, 2017: contributor

[20:10:05] <wumpus> or have no default at all and require a time to be specified

I think we should make the field mandatory. IMO we can expect API are capable of providing a reasonable timestamp. If they set it to 0, so be it.

gmaxwell commented at 7:13 pm on February 2, 2017: contributor

We need a clean way to specify “now”.

achow101 commented at 7:14 pm on February 2, 2017: member

Is it possible to have the timestamp parameter be both an integer and a string? If so, we could just make it be a string “now” to specify “now”

jonasschnelli commented at 7:15 pm on February 2, 2017: contributor

We need a clean way to specify “now”.

Why would that be required? An imported key has very unlikely a generation timestamp of “now”?

sipa commented at 7:18 pm on February 2, 2017: member

Maybe it should be “unused” instead of “now”. From the point of the wallet, they should be treated identically. Jonas is right that “now” is very unlikely, but I think it is very likely that you’re importing keys that are known to be unused.

gmaxwell commented at 7:19 pm on February 2, 2017: contributor

right. Now means so far unused as far as I’m concerned.

ryanofsky referenced this in commit e8c7a1cbf1 on Feb 3, 2017

ryanofsky commented at 9:43 pm on February 3, 2017: member

#9682 has an implementation of the suggested importmulti changes.

ryanofsky referenced this in commit e6fabf6780 on Feb 3, 2017

ryanofsky referenced this in commit f8303e960b on Feb 6, 2017

ryanofsky referenced this in commit b8b56ffdc4 on Feb 9, 2017

ryanofsky referenced this in commit 442887f27f on Feb 10, 2017

laanwj closed this on Feb 14, 2017

codablock referenced this in commit af3fc298e4 on Jan 19, 2018

codablock referenced this in commit d363d03287 on Jan 20, 2018

codablock referenced this in commit 6bee150e8e on Jan 21, 2018

lateminer referenced this in commit 3e5964230d on Jan 4, 2019

andvgal referenced this in commit e05cb53a41 on Jan 6, 2019

CryptoCentric referenced this in commit 7f51d65522 on Feb 27, 2019

MarcoFalke locked this on Sep 8, 2021

Importmulti api is confusing in a way that could lead to funds loss. #9491