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

wallet: Only fail rescan when blocks have actually been pruned #15870

Merged
merged 3 commits into from May 16, 2019

Conversation

Projects
None yet
@MarcoFalke
Copy link
Member

commented Apr 22, 2019

This brings the behaviour of the import* calls closer to importmulti. After this change, the difference between importmulti and the other import* calls is

  • that in importmulti you can "opt-out" of scanning early blocks by setting a later timestamp.
  • that in importmulti the wallet will successfully import the data, but fail to rescan. Whereas in the other calls, the wallet will abort before importing the data.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletRescanPruned branch from fa00f6e to fa12b93 Apr 22, 2019

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

This seems to affect importaddress, importpubkey, importprivkey, and importwallet but not importmulti. I wonder how the new behavior compares to importmulti behavior, and if there's a difference, whether that's justified.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

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

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletRescanPruned branch 2 times, most recently from 1ceacd8 to fad8451 Apr 23, 2019

@promag

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

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.

@promag
Copy link
Member

left a comment

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

@@ -326,7 +326,11 @@ class ChainImpl : public Chain
CFeeRate relayIncrementalFee() override { return ::incrementalRelayFee; }
CFeeRate relayDustFee() override { return ::dustRelayFee; }
CAmount maxTxFee() override { return ::maxTxFee; }
bool getPruneMode() override { return ::fPruneMode; }
bool havePruned() override

This comment has been minimized.

Copy link
@promag

promag Apr 24, 2019

Member

Move to the chain lock?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 24, 2019

Author Member

Why?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 24, 2019

Author Member

Sorry for the brief reply, but see here:

//! Interface for querying locked chain state, used by legacy code that
//! assumes state won't change between calls. New code should avoid using
//! the Lock interface and instead call higher-level Chain methods
//! that return more information so the chain doesn't need to stay locked
//! between calls.

The chain lock is deprecated and will be removed in the future.

@@ -157,8 +157,8 @@ UniValue importprivkey(const JSONRPCRequest& request)
if (!request.params[2].isNull())
fRescan = request.params[2].get_bool();

if (fRescan && pwallet->chain().getPruneMode()) {
throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled in pruned mode");
if (fRescan && pwallet->chain().havePruned()) {

This comment has been minimized.

Copy link
@promag

promag Apr 24, 2019

Member

A block can be pruned before RescanWallet (L202). What would be the error then?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Apr 24, 2019

Author Member

It will throw an error "Rescan was unable to fully rescan the blockchain. Some transactions may be missing."

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

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.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletRescanPruned branch from fad8451 to faa7e29 Apr 27, 2019

@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Apr 27, 2019

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletRescanPruned branch from faa7e29 to fa6838e Apr 29, 2019

@luke-jr

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Let's use prune height instead, so rescanblockchain can be picky about it?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

I am not touching rescanblockchain at all. Could you explain what you mean?

@luke-jr

This comment has been minimized.

Copy link
Member

commented May 1, 2019

I mean it would make sense to use a common interface with rescanblockchain, which would need prune-height rather than have-pruned.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Oh, the RPCs I touch are already deprecated in favor of importmulti, so I'd rather not add features to them.

@luke-jr

This comment has been minimized.

Copy link
Member

commented May 2, 2019

I'm commenting on the src/interfaces changes specifically.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Ah. Yeah, since we don't store the prune height, this would complicate my changes slightly. I think the additional code can be added later (when needed in rescanblockchain).

@jnewbery jnewbery added this to Blockers in High-priority for review May 3, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 3, 2019

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@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 (comment)

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 3, 2019

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!

@jnewbery
Copy link
Member

left a comment

utACK fa9fa5d

Will happily reACK a squashed branch.

Show resolved Hide resolved test/functional/wallet_import_rescan.py Outdated

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletRescanPruned branch from fa9fa5d to fa9f3cc May 3, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented May 3, 2019

utACK fa9f3cc

@jonatack
Copy link
Contributor

left a comment

A few code comments. More documentation and tests might make this change and the expected behavior before/after clearer, at least to me.

Show resolved Hide resolved src/wallet/rpcdump.cpp
Show resolved Hide resolved src/wallet/wallet.cpp Outdated
if (pwallet->chain().havePruned()) {
// Exit early and print an error.
// If a block is pruned after this check, we will import the key(s),
// but fail the rescan with a generic error.
throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled in pruned mode");

This comment has been minimized.

Copy link
@jonatack

jonatack May 6, 2019

Contributor

Does the code doc in 588 correspond correctly with the error message in 589?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 6, 2019

Author Member

Yes, "importing wallet" means "importing key(s)", I believe.

// Exit early and print an error.
// If a block is pruned after this check, we will import the key(s),
// but fail the rescan with a generic error.
throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned");

This comment has been minimized.

Copy link
@jonatack

jonatack May 6, 2019

Contributor

Would it be worth extracting this code block repeated in importprivkey, importaddress, importpubkey, and importwallet (the latter with a different error message).

    if (fRescan && pwallet->chain().havePruned()) {
        // Exit early and print an error.
        // If a block is pruned after this check, we will import the key(s),
        // but fail the rescan with a generic error.
        throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned");
    }

Edit: I understand it may be a premature or unnecessary optimisation. Just an observation.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 6, 2019

Author Member

Yeah, could make sense. Will do if I have to touch for other reasons.

This comment has been minimized.

Copy link
@promag

promag May 16, 2019

Member

faf3729

Yeah, in a follow up is fine.

MarcoFalke added some commits Apr 22, 2019

scripted-diff: Bump copyright headers in wallet
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./src/wallet/
-END VERIFY SCRIPT-

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:1904-walletRescanPruned branch from fa9f3cc to aaaa57c May 6, 2019

@sipa

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Concept ACK. My first read of this was that it'd permit rescanning pruned chains, as long as the to-be-rescanned blocks are not pruned, but it seems to just be about whether or not any blocks are actually pruned (independent of the ones to be rescanned), right?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 10, 2019

The legacy calls would always rescan the whole chain. As soon as one block is pruned, it fails.

@wpaulino

This comment has been minimized.

Copy link

commented May 15, 2019

Out of scope for this PR but somewhat related -- would there be any reason to not attempt to fetch the pruned blocks from the P2P network to allow the rescan to happen?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

Jup, but that might be something to do only after #15946

@ryanofsky
Copy link
Contributor

left a comment

utACK aaaa57c. The change seems fine, though it's not really clear what the motivation for it is. It might be good to add a release note describing the change and what benefits it has from a user perspective.

Show resolved Hide resolved src/wallet/rpcdump.cpp Outdated
@@ -39,23 +38,17 @@
class Variant(collections.namedtuple("Variant", "call data rescan prune")):
"""Helper for importing one key and verifying scanned transactions."""

def try_rpc(self, func, *args, **kwargs):
if self.expect_disabled:
assert_raises_rpc_error(-4, "Rescan is disabled in pruned mode", func, *args, **kwargs)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 15, 2019

Contributor

Now there is no longer any test coverage for the "rescan is disabled" cases, but I'm not sure if there's an easy way to add it back. Maybe a pruneblockchain RPC call could be added.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 15, 2019

Author Member

If a test was added, it should also (or more importantly) test importmulti, so I am not actually decreasing test coverage here.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 15, 2019

Author Member

I agree that a test would be nice for that case, but can be done in a follow up pull request.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky May 15, 2019

Contributor

it should also (or more importantly) test importmulti

In case it helps, there is currently a unit test for importmulti behavior with pruning:

UniValue response = importmulti(request);

@jamesob

This comment has been minimized.

Copy link
Member

commented May 15, 2019

utACK aaaa57c

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.

@promag

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@jamesob you mean an option to ensure a minimum free space?

@promag
Copy link
Member

left a comment

This PR would be just fine without aaaa57c IMO.

utACK fa7e311.

// Exit early and print an error.
// If a block is pruned after this check, we will import the key(s),
// but fail the rescan with a generic error.
throw JSONRPCError(RPC_WALLET_ERROR, "Rescan is disabled when blocks are pruned");

This comment has been minimized.

Copy link
@promag

promag May 16, 2019

Member

faf3729

Yeah, in a follow up is fine.

// Exit early and print an error.
// If a block is pruned after this check, we will import the key(s),
// but fail the rescan with a generic error.
throw JSONRPCError(RPC_WALLET_ERROR, "Importing wallets is disabled when blocks are pruned");

This comment has been minimized.

Copy link
@promag

promag May 16, 2019

Member

fa7e311

This should be in 1s commit.

@@ -573,7 +582,10 @@ UniValue importwallet(const JSONRPCRequest& request)
},
}.ToString());

if (pwallet->chain().getPruneMode()) {
if (pwallet->chain().havePruned()) {

This comment has been minimized.

Copy link
@promag

promag May 16, 2019

Member

faf3729

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 comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 16, 2019

Author Member

Sounds like a nice follow up pull request

@jamesob

This comment has been minimized.

Copy link
Member

commented May 16, 2019

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

@jnewbery
Copy link
Member

left a comment

utACK fa7e311

bool getPruneMode() override { return ::fPruneMode; }
bool havePruned() override
{
LOCK(cs_main);

This comment has been minimized.

Copy link
@jnewbery

jnewbery May 16, 2019

Member

I'm not suggesting you change this PR, but for future reference we now have a handy WITH_LOCK macro:

#define WITH_LOCK(cs, code) [&] { LOCK(cs); code; }()

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 16, 2019

Author Member

I fail to see how this makes the code any easier to read

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request May 16, 2019

Merge bitcoin#15870: wallet: Only fail rescan when blocks have actual…
…ly been pruned

fa7e311 [doc] rpcwallet: Only fail rescan when blocks have been pruned (MarcoFalke)
aaaa57c scripted-diff: Bump copyright headers in wallet (MarcoFalke)
faf3729 wallet: Only fail rescan when blocks have actually been pruned (MarcoFalke)

Pull request description:

  This brings the behaviour of the import* calls closer to importmulti. After this change, the difference between importmulti and the other import* calls is

  * that in importmulti you can "opt-out" of scanning early blocks by setting a later timestamp.
  * that in importmulti the wallet will successfully import the data, but fail to rescan. Whereas in the other calls, the wallet will abort before importing the data.

ACKs for commit fa7e31:
  promag:
    utACK fa7e311.
  jnewbery:
    utACK fa7e311

Tree-SHA512: a57d52ffea94b64e0eb9b5d3a7a63031325833908297dd14eb0c5251ffea3b2113b131003f1db4e9599e014369165a57f107a7150bb65e4c791e5fe742f33cb8

@MarcoFalke MarcoFalke merged commit fa7e311 into bitcoin:master May 16, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke deleted the MarcoFalke:1904-walletRescanPruned branch May 16, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented May 16, 2019

post-merge utACK

@laanwj laanwj removed this from Blockers in High-priority for review May 16, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 18, 2019

Merge bitcoin#15870: wallet: Only fail rescan when blocks have actual…
…ly been pruned

fa7e311 [doc] rpcwallet: Only fail rescan when blocks have been pruned (MarcoFalke)
aaaa57c scripted-diff: Bump copyright headers in wallet (MarcoFalke)
faf3729 wallet: Only fail rescan when blocks have actually been pruned (MarcoFalke)

Pull request description:

  This brings the behaviour of the import* calls closer to importmulti. After this change, the difference between importmulti and the other import* calls is

  * that in importmulti you can "opt-out" of scanning early blocks by setting a later timestamp.
  * that in importmulti the wallet will successfully import the data, but fail to rescan. Whereas in the other calls, the wallet will abort before importing the data.

ACKs for commit fa7e31:
  promag:
    utACK fa7e311.
  jnewbery:
    utACK fa7e311

Tree-SHA512: a57d52ffea94b64e0eb9b5d3a7a63031325833908297dd14eb0c5251ffea3b2113b131003f1db4e9599e014369165a57f107a7150bb65e4c791e5fe742f33cb8

@LitecoinZ LitecoinZ referenced this pull request May 31, 2019

Open

Backports from upstream #1

44 of 244 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.