This brings the behaviour of the import* calls closer to importmulti. After this change, the difference between importmulti and the other import* calls is
Concept ACK.
There is an argument against doing things like this: that it’s better to fail deterministically rather than fail randomly at some point in the future and have created incorrect expectations of functionality. But setting prune to some large “future” amount is a perfectly reasonable configuration. And this doesn’t cause it to fail randomly, but rather work until it doesn’t. So I don’t believe the general argument against this sort of thing offsets the benefit.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
@ryanofsky The only difference between importmulti and the other calls is that in importmulti you can “opt-out” of scanning early blocks by setting a later timestamp. Otherwise, my change makes all calls behave in the same way.
Edit: I believe the only difference is that importmulti will import the keys and then fail the rescan, whereas the other calls will exit early and not attempt to import anything.
Edit: I believe the only difference is that importmulti will import the keys and then fail the rescan, whereas the other calls will exit early and not attempt to import anything.
Why a different behavior? It’s already possible to call importmulti ... '{"rescan": false}' and then rescanblockchain ...? I think RPCs should (try to) be atomic so that the client doesn’t have to figure out what succeeded or not.
Why a different behavior?
I don’t know. I didn’t write the importmulti RPC and I don’t plan to change it in this pull request.
325@@ -326,7 +326,11 @@ class ChainImpl : public Chain
326 CFeeRate relayIncrementalFee() override { return ::incrementalRelayFee; }
327 CFeeRate relayDustFee() override { return ::dustRelayFee; }
328 CAmount maxTxFee() override { return ::maxTxFee; }
329- bool getPruneMode() override { return ::fPruneMode; }
330+ bool havePruned() override
Sorry for the brief reply, but see here:
The chain lock is deprecated and will be removed in the future.
156@@ -157,8 +157,8 @@ UniValue importprivkey(const JSONRPCRequest& request)
157 if (!request.params[2].isNull())
158 fRescan = request.params[2].get_bool();
159
160- if (fRescan && pwallet->chain().getPruneMode()) {
161- throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled in pruned mode");
162+ if (fRescan && pwallet->chain().havePruned()) {
"Rescan was unable to fully rescan the blockchain. Some transactions may be missing."
@MarcoFalke I wasn’t suggesting changing that behavior.
Concept ACK. Correct me if I’m wrong, but it doesn’t have to fail if all required blocks are available right?
Concept ACK. Correct me if I’m wrong, but it doesn’t have to fail if all required blocks are available right?
importmulti does not fail the rescan when all blocks after the timestamp are available, but it fails after the import if a block is missing. the other import* (after my change) also do not fail the rescan when all blocks after time=0 are available. However, they fail before the import if a block is missing.
rescanblockchain can be picky about it?
rescanblockchain at all. Could you explain what you mean?
rescanblockchain, which would need prune-height rather than have-pruned.
I’m a little concerned about this change. Anywhere that havePruned() is called without cs_main is racy, since a block may be pruned after havePruned() is called. We therefore need to make sure that the code after those havePruned() calls is robust against being run when blocks have been pruned. If that’s the case, why not just remove the chain().getPruneMode() check entirely and let the error-handling in the later code handle when a block is missing?
The four call sites in rpcdump do not hold cs_main. The call site in CreateWalletFromFile() does hold cs_main indirectly through locked_chain, but a goal is to remove that lock.
@jnewbery The check is only there to provide a nice error message. As you correctly observe it is not needed for correctness. Should I clarify that with a comment?
The error message otherwise would be #15870 (review)
The check is only there to provide a nice error message. As you correctly observe it is not needed for correctness. Should I clarify that with a comment?
Yes, that sounds good. Thanks!
38@@ -40,9 +39,6 @@ class Variant(collections.namedtuple("Variant", "call data rescan prune")):
39 """Helper for importing one key and verifying scanned transactions."""
40
41 def try_rpc(self, func, *args, **kwargs):
42- if self.expect_disabled:
43- assert_raises_rpc_error(-4, "Rescan is disabled in pruned mode", func, *args, **kwargs)
44- else:
45 return func(*args, **kwargs)
nit: indentation
EDIT: just remove this function entirely
utACK fa9fa5d647092710aa03dadf92433584e9fe26a3
Will happily reACK a squashed branch.
156@@ -157,8 +157,11 @@ UniValue importprivkey(const JSONRPCRequest& request)
157 if (!request.params[2].isNull())
// Copyright (c) 2009-2018 The Bitcoin Core developers
4220@@ -4221,8 +4221,11 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
4221 {
4222 //We can't rescan beyond non-pruned blocks, stop and throw an error
4223 //this might happen if a user uses an old wallet within a pruned node
581@@ -573,7 +582,10 @@ UniValue importwallet(const JSONRPCRequest& request)
582 },
583 }.ToString());
584
585- if (pwallet->chain().getPruneMode()) {
586+ if (pwallet->chain().havePruned()) {
587+ // Exit early and print an error.
588+ // If a block is pruned after this check, we will import the key(s),
589+ // but fail the rescan with a generic error.
590 throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled in pruned mode");
s/in pruned mode/when blocks are pruned/ for consistency with the other error messages.
321- throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled in pruned mode");
322+ if (fRescan && pwallet->chain().havePruned()) {
323+ // Exit early and print an error.
324+ // If a block is pruned after this check, we will import the key(s),
325+ // but fail the rescan with a generic error.
326+ throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned");
Would it be worth extracting this code block repeated in importprivkey, importaddress, importpubkey, and importwallet (the latter with a different error message).
0 if (fRescan && pwallet->chain().havePruned()) {
1 // Exit early and print an error.
2 // If a block is pruned after this check, we will import the key(s),
3 // but fail the rescan with a generic error.
4 throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned");
5 }
Edit: I understand it may be a premature or unnecessary optimisation. Just an observation.
faf3729242c5e9486e137e549f05bf20bd3908a0
Yeah, in a follow up is fine.
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./src/wallet/
-END VERIFY SCRIPT-
37@@ -39,23 +38,17 @@
38 class Variant(collections.namedtuple("Variant", "call data rescan prune")):
39 """Helper for importing one key and verifying scanned transactions."""
40
41- def try_rpc(self, func, *args, **kwargs):
42- if self.expect_disabled:
43- assert_raises_rpc_error(-4, "Rescan is disabled in pruned mode", func, *args, **kwargs)
pruneblockchain RPC call could be added.
importmulti, so I am not actually decreasing test coverage here.
it should also (or more importantly) test importmulti
In case it helps, there is currently a unit test for importmulti behavior with pruning: https://github.com/bitcoin/bitcoin/blob/2d16fb7a2b6a9e5a2535295d2de03e27c2438d1f/src/wallet/test/wallet_tests.cpp#L158
utACK https://github.com/bitcoin/bitcoin/pull/15870/commits/aaaa57c2aad3a6278559ae3db5d953c847c6ffcb
Makes a lot of sense to allow rescanning for as long as we can as @gmaxwell notes above - and indeed it might even be prudent for most users to set a future-proofing prune value that reflects their disk limitations at startup time. Better that your node start pruning vs. run out of disk space and exit.
Maybe in the future we could even default to such a thing based on the datadir device if pruning-enabled-but-not-done-yet behavior is equivalent to a non-pruning node. Or introduce an “adaptive” pruning mode, which periodically polls for available disk space and adjusts the prune target accordingly. But of course this is all out of scope for this change.
586- throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled in pruned mode");
587+ if (pwallet->chain().havePruned()) {
588+ // Exit early and print an error.
589+ // If a block is pruned after this check, we will import the key(s),
590+ // but fail the rescan with a generic error.
591+ throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled when blocks are pruned");
fa7e311e169349bfcf1dab8b980724e8ddf4e749
This should be in 1s commit.
581@@ -573,7 +582,10 @@ UniValue importwallet(const JSONRPCRequest& request)
582 },
583 }.ToString());
584
585- if (pwallet->chain().getPruneMode()) {
586+ if (pwallet->chain().havePruned()) {
faf3729242c5e9486e137e549f05bf20bd3908a0
The dump file contains timestamps so, correct me if I’m wrong, it could only throw this error if earliest is pruned - the same for importmulti if timestamps are given.
Edit: especially when manual pruning.
This PR would be just fine without aaaa57c2aad3a6278559ae3db5d953c847c6ffcb IMO.
utACK fa7e311e169349bfcf1dab8b980724e8ddf4e749.
@jamesob you mean an option to ensure a minimum free space?
If you mean “prune as little as possible and only until necessitated by lack of space,” yes. Some kind of adaptive prune would just be a measure to avoid hitting an out-of-disk error for as long as possible. But this is out of scope in terms of this PR so maybe I’ll file an issue later or something.
324@@ -325,7 +325,11 @@ class ChainImpl : public Chain
325 CFeeRate relayMinFee() override { return ::minRelayTxFee; }
326 CFeeRate relayIncrementalFee() override { return ::incrementalRelayFee; }
327 CFeeRate relayDustFee() override { return ::dustRelayFee; }
328- bool getPruneMode() override { return ::fPruneMode; }
329+ bool havePruned() override
330+ {
331+ LOCK(cs_main);
WITH_LOCK macro: https://github.com/bitcoin/bitcoin/blob/fd61b9fc22faaa946ca43211c6c43b40ca4a860c/src/sync.h#L209
