Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable wallet key imports without rescan in pruned mode. #6645

Merged
merged 1 commit into from Sep 23, 2015
Merged

Enable wallet key imports without rescan in pruned mode. #6645

merged 1 commit into from Sep 23, 2015

Conversation

gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Sep 7, 2015

Complete rescan is incompatible with pruning, but rescan is optional on
our wallet key import RPCs. Import on use is very useful in some common
situations in conjunction with pruning, e.g. merchant payment tracking.

This reenables importprivkey/importaddress/importpubkey when rescan
is not used.

In the future we should consider changing the rescan argument to allow depth
or date to allow limited rescanning when compatible with the retained
block depth.

Complete rescan is incompatible with pruning, but rescan is optional on
 our wallet key import RPCs.  Import on use is very useful in some common
 situations in conjunction with pruning, e.g. merchant payment tracking.

This reenables importprivkey/importaddress/importpubkey when rescan
 is not used.

In the future we should consider changing the rescan argument to allow depth
 or date to allow limited rescanning when compatible with the retained
 block depth.
@gmaxwell
Copy link
Contributor Author

gmaxwell commented Sep 7, 2015

I only performed the most trivial of tests. I am opening this PR somewhat prematurely because I don't know why it wasn't done this way in the first place, and wanted to find out if there was some huge reason I was missing before I spent more time testing. :)

@gmaxwell
Copy link
Contributor Author

gmaxwell commented Sep 7, 2015

@jonasschnelli Above question is wrt 7a12119.

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Sep 7, 2015

Nice!

Right. I should have this done in the fist place.

utACK.

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Sep 7, 2015

There is also a PR that would optimize importing depth:
#6570

@gmaxwell
Copy link
Contributor Author

gmaxwell commented Sep 7, 2015

Awesome. Okay I'll run it through some more test cases then!

@gmaxwell gmaxwell added the Wallet label Sep 7, 2015
@fanquake
Copy link
Member

fanquake commented Sep 7, 2015

utACK

@dcousens
Copy link
Contributor

dcousens commented Sep 7, 2015

utACK && concept ACK

@gmaxwell
Copy link
Contributor Author

gmaxwell commented Sep 7, 2015

I've tested this more extensively now and can't seem to break it.

@@ -114,6 +112,9 @@ UniValue importprivkey(const UniValue& params, bool fHelp)
if (params.size() > 2)
fRescan = params[2].get_bool();

if (fRescan && fPruneMode)
throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled in pruned mode");
Copy link
Contributor

@btcdrak btcdrak Sep 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "Wallet rescanning is disabled in pruned mode"?

Copy link
Contributor Author

@gmaxwell gmaxwell Sep 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically it is the blockchain that is rescanned. But these options have a boolean parameter "rescan" which defaults, so what its saying is that this option is disabled.

I think if I were to clarify this further I would say that the rescan option was disabled. Or maybe to be kind to translaters say something like the requested rescan depth was too deep (as I expect we'll further parameterize this in the future).

Copy link
Contributor

@btcdrak btcdrak Sep 7, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just say "The -rescan option is disabled in pruned mode" which would be more clear (to me at least :)

Copy link
Member

@laanwj laanwj Sep 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this isn't about the -rescan option, but rescans triggered some other way. I think the message is OK.

Copy link
Member

@laanwj laanwj Sep 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only improvement I can think of would be to add a comment to the error message that it is possible to pass 'false' as the rescan parameter to avoid this error.

You don't have to worry about translators, RPC error messages aren't translated.

@laanwj
Copy link
Member

laanwj commented Sep 8, 2015

utACK

@laanwj laanwj merged commit 77c6072 into bitcoin:master Sep 23, 2015
1 check passed
laanwj added a commit that referenced this issue Sep 23, 2015
77c6072 Enable wallet key imports without rescan in pruned mode. (Gregory Maxwell)
luke-jr pushed a commit to luke-jr/bitcoin that referenced this issue Dec 28, 2015
Complete rescan is incompatible with pruning, but rescan is optional on
 our wallet key import RPCs.  Import on use is very useful in some common
 situations in conjunction with pruning, e.g. merchant payment tracking.

This reenables importprivkey/importaddress when rescan is not used.

In the future we should consider changing the rescan argument to allow depth
 or date to allow limited rescanning when compatible with the retained
 block depth.

Github-Pull: bitcoin#6645
Partial-Rebased-From: 77c6072
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants