wallet: Only fail rescan when blocks have actually been pruned by maflcko · Pull Request #15870 · bitcoin/bitcoin

maflcko · 2019-04-22T17:55:00Z

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.

gmaxwell · 2019-04-22T18:44:06Z

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 · 2019-04-22T19:16:50Z

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 · 2019-04-22T19:30:28Z

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

Conflicts

No conflicts as of last run.

maflcko · 2019-04-22T19:34:12Z

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

promag · 2019-04-24T20:12:51Z

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.

maflcko · 2019-04-24T20:23:33Z

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

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

promag · 2019-04-24T20:33:49Z

src/interfaces/chain.cpp

Move to the chain lock?

Sorry for the brief reply, but see here:

https://github.com/bitcoin/bitcoin/blob/fad84519391a05f2ad42381cca9135323b709657/src/interfaces/chain.h#L69-L73

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

promag · 2019-04-24T20:38:11Z

src/wallet/rpcdump.cpp

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

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

maflcko · 2019-04-24T21:03:53Z

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.

luke-jr · 2019-05-01T06:02:21Z

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

maflcko · 2019-05-01T14:02:10Z

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

luke-jr · 2019-05-01T23:26:52Z

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

maflcko · 2019-05-02T13:10:42Z

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

luke-jr · 2019-05-02T18:17:24Z

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

maflcko · 2019-05-02T18:21:29Z

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 · 2019-05-03T17:25:58Z

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.

maflcko · 2019-05-03T17:49:27Z

@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 · 2019-05-03T17:57:12Z

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

utACK fa9fa5d647092710aa03dadf92433584e9fe26a3

Will happily reACK a squashed branch.

test/functional/wallet_import_rescan.py

jnewbery · 2019-05-03T19:46:56Z

utACK fa9f3cc89442e9b332a27b81b04201bb4a0caf28

jonatack

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

src/wallet/rpcdump.cpp

src/wallet/wallet.cpp

jonatack · 2019-05-06T16:36:31Z

src/wallet/rpcdump.cpp

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

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

jonatack · 2019-05-06T16:45:40Z

src/wallet/rpcdump.cpp

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.

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

faf3729

Yeah, in a follow up is fine.

-BEGIN VERIFY SCRIPT- ./contrib/devtools/copyright_header.py update ./src/wallet/ -END VERIFY SCRIPT-

sipa · 2019-05-09T22:59:06Z

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?

maflcko · 2019-05-10T12:22:33Z

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

wpaulino · 2019-05-15T04:40:40Z

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?

maflcko · 2019-05-15T12:00:37Z

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

ryanofsky

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.

src/wallet/rpcdump.cpp

ryanofsky · 2019-05-15T17:07:07Z

test/functional/wallet_import_rescan.py


-    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)


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.

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

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

it should also (or more importantly) test importmulti

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

bitcoin/src/wallet/test/wallet_tests.cpp

Line 158 in 2d16fb7

UniValue response = importmulti(request);

jamesob · 2019-05-15T17:23:12Z

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 · 2019-05-15T17:29:35Z

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

promag

This PR would be just fine without aaaa57c IMO.

utACK fa7e311.

promag · 2019-05-16T13:53:10Z

src/wallet/rpcdump.cpp

+        // 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");


fa7e311

This should be in 1s commit.

promag · 2019-05-16T13:44:38Z

src/wallet/rpcdump.cpp

faf3729

Yeah, in a follow up is fine.

promag · 2019-05-16T13:55:09Z

src/wallet/rpcdump.cpp

            }.ToString());

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


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.

Sounds like a nice follow up pull request

jamesob · 2019-05-16T14:13:43Z

@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

utACK fa7e311

jnewbery · 2019-05-16T14:50:51Z

src/interfaces/chain.cpp

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


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

bitcoin/src/sync.h

Line 209 in fd61b9f

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

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

…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

Sjors · 2019-05-16T15:54:11Z

post-merge utACK

…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

maflcko force-pushed the 1904-walletRescanPruned branch from fa00f6e to fa12b93 Compare April 22, 2019 18:02

DrahtBot added RPC/REST/ZMQ Tests Wallet labels Apr 22, 2019

maflcko force-pushed the 1904-walletRescanPruned branch 2 times, most recently from 1ceacd8 to fad8451 Compare April 23, 2019 13:49

promag reviewed Apr 24, 2019

View reviewed changes

maflcko mentioned this pull request Apr 26, 2019

wallet: Fast rescan with BIP157 block filters #15845

Closed

maflcko force-pushed the 1904-walletRescanPruned branch from fad8451 to faa7e29 Compare April 27, 2019 13:34

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

maflcko force-pushed the 1904-walletRescanPruned branch from faa7e29 to fa6838e Compare April 29, 2019 12:56

jnewbery reviewed May 3, 2019

View reviewed changes

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

maflcko force-pushed the 1904-walletRescanPruned branch from fa9fa5d to fa9f3cc Compare May 3, 2019 19:32

jonatack reviewed May 6, 2019

View reviewed changes

MarcoFalke added 2 commits May 6, 2019 14:03

wallet: Only fail rescan when blocks have actually been pruned

faf3729

scripted-diff: Bump copyright headers in wallet

aaaa57c

-BEGIN VERIFY SCRIPT- ./contrib/devtools/copyright_header.py update ./src/wallet/ -END VERIFY SCRIPT-

maflcko force-pushed the 1904-walletRescanPruned branch from fa9f3cc to aaaa57c Compare May 6, 2019 18:06

ryanofsky approved these changes May 15, 2019

View reviewed changes

[doc] rpcwallet: Only fail rescan when blocks have been pruned

fa7e311

promag reviewed May 16, 2019

View reviewed changes

jnewbery reviewed May 16, 2019

View reviewed changes

maflcko merged commit fa7e311 into bitcoin:master May 16, 2019

maflcko deleted the 1904-walletRescanPruned branch May 16, 2019 15:21

maflcko mentioned this pull request Oct 15, 2019

Remove wallet access to some node arguments #17138

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

maflcko commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Apr 22, 2019

Uh oh!

ryanofsky commented Apr 22, 2019

Uh oh!

DrahtBot commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Apr 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Apr 24, 2019

Uh oh!

maflcko commented Apr 24, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 24, 2019

Uh oh!

luke-jr commented May 1, 2019

Uh oh!

maflcko commented May 1, 2019

Uh oh!

luke-jr commented May 1, 2019

Uh oh!

maflcko commented May 2, 2019

Uh oh!

luke-jr commented May 2, 2019

Uh oh!

maflcko commented May 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented May 3, 2019

Uh oh!

maflcko commented May 3, 2019

Uh oh!

jnewbery commented May 3, 2019

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented May 3, 2019

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack May 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented May 9, 2019

Uh oh!

maflcko commented May 10, 2019

Uh oh!

maflcko commented Apr 22, 2019 •

edited

Loading

DrahtBot commented Apr 22, 2019 •

edited

Loading

maflcko commented Apr 22, 2019 •

edited

Loading

maflcko commented May 2, 2019 •

edited

Loading

jonatack May 6, 2019 •

edited

Loading

promag May 16, 2019 •

edited

Loading