Return errors from importmulti if complete rescans are not successful #9773

ryanofsky · 2017-02-15T22:19:12Z

This is supposed to address the concern from @gmaxwell in #9761 that importmulti rescans on pruned nodes could unsuccessfully scan pruned blocks and fail to inform the user.

An alternative approach of just simply disabling importmulti rescans on pruned nodes was initially discussed in https://botbot.me/freenode/bitcoin-core-dev/msg/81023921/, but running being able to import keys and do partial rescans on pruned nodes was determined to be an important use-case.

morcos · 2017-02-15T22:48:21Z

Concept ACK
This looks like a reasonable approach to me.

Don't forget that you may want to include the grace period in detecting whether it scanned enough.
Also you might want to skip the rest of the loop if ReadBlockFromDisk is false.

laanwj · 2017-02-16T12:09:53Z

Concept ACK.

The code has not been tested at all, and I'm actually not sure how I'm going to go about writing a unit test because there doesn't seem to be an easy way to target individual blocks for pruning inside RPC tests.

There's pruneblockchain but yes pruning works on a per-file, not a per-block basis, so it would need some extra effort on regtest to generate full blocks instead of empty ones otherwise at block 30000 you're still in the first file, and that RPC will do nothing.

gmaxwell · 2017-02-16T12:16:10Z

Concept ACK. Will test/review further.

morcos · 2017-02-16T15:42:42Z

Please rebase on #9761 so we can verify the grace period works properly

ryanofsky · 2017-02-16T15:53:59Z

Rebased

morcos · 2017-02-16T18:53:18Z

src/wallet/wallet.cpp

            int posInBlock;
            for (posInBlock = 0; posInBlock < (int)block.vtx.size(); posInBlock++)
            {
-                if (AddToWalletIfInvolvingMe(*block.vtx[posInBlock], pindex, posInBlock, fUpdate))
-                    ret++;
+                AddToWalletIfInvolvingMe(*block.vtx[posInBlock], pindex, posInBlock, fUpdate);


I think this loop should also be skipped if ReadBlockFromDisk fails

Skipped in d6f6a23

…cessful Skip empty loop if ReadBlockFromDisk fails as suggested bitcoin#9773 (comment)

ryanofsky

Add test for CWallet::ScanForWalletTransactions in e3d4f25

ryanofsky · 2017-02-16T18:59:52Z

src/wallet/wallet.cpp

            int posInBlock;
            for (posInBlock = 0; posInBlock < (int)block.vtx.size(); posInBlock++)
            {
-                if (AddToWalletIfInvolvingMe(*block.vtx[posInBlock], pindex, posInBlock, fUpdate))
-                    ret++;
+                AddToWalletIfInvolvingMe(*block.vtx[posInBlock], pindex, posInBlock, fUpdate);


Skipped in d6f6a23

jonasschnelli · 2017-02-16T19:23:06Z

src/wallet/rpcdump.cpp

+                } else {
+                    UniValue result = UniValue(UniValue::VOBJ);
+                    result.pushKV("success", UniValue(false));
+                    result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, strprintf("Failed to rescan before time %" PRId64 ", transactions may be missing.", (long long)scannedRange->GetBlockTimeMax())));


just curious: Is there an advantage using PRId64 over %ul (rest of the code uses this)?

Actually I need to change this to either remove PRId64 or the (long long) cast.

You're supposed to use PRId64 whenever you pass an int64_t type to printf. %ld or %lld aren't always correct because longs and long longs just have minimum widths in standard c++, not fixed widths that are the same on all systems.

Removed cast in e49f039.

ryanofsky · 2017-02-16T20:13:23Z

Previously squashed e3d4f25 -> 455b8f7 (multicheck.2 -> multicheck.3)

…cessful Add missing response.setArray call and fix string formatting issue pointed out bitcoin#9773 (comment)

ryanofsky · 2017-02-16T20:19:30Z

Added C++ test for new importmulti RPC code in e64cdfb

Squashed e64cdfb -> b73252d (multicheck.4 -> multicheck.5)

sipa · 2017-02-16T20:29:07Z

Note that our strprintf is a wrapper around tfm::format, and doesn't need type specs at all.

morcos · 2017-02-16T20:30:04Z

lightly tested ACK b73252d
Only reviewed/tested code, not the tests.

jonasschnelli

utACK b73252d

laanwj · 2017-02-17T11:56:39Z

src/wallet/rpcdump.cpp

+                } else {
+                    UniValue result = UniValue(UniValue::VOBJ);
+                    result.pushKV("success", UniValue(false));
+                    result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, strprintf("Failed to rescan before time %" PRId64 ", transactions may be missing.", scannedRange->GetBlockTimeMax())));


Please remove the PRI64d here. It gives problems with some platforms, and as @sipa says don't need it.

Ok will change, but what are the problems with some platforms?

Removed in aaaf65a.

Reason I am asking about the problem you're seeing is that not using PRI64d for int64_t would prevent us from using compile time format string checking (as suggested in #9423 (comment)) which would be much better than the runtime checking we currently use.

Ok will change, but what are the problems with some platforms?

The conceptual problem is that PRI64d can evaluate to anything depending on the platform, e.g. "%ponyfood". tinyformat.h cannot know about all these platform-dependent escape sequences, so might produce garbled output or even raise an exception. We had a lot of problems with this back in the day with windows and mingw. It is wrong to pass platform-specific stuff to a platform-independent library. Also PRI64d is C99, so not all C++ compilers are guaranteed to have it at all.

Offline checking could still work, the script would just need to be smart enough to cope with a) translations b) tinyformat's specifiers and type-safety. (b) is actually a subset, not a superset of what platform printfs support (none of the insecure ones like %n) so if anything that should make it easier.

ryanofsky · 2017-02-17T12:12:34Z

Rebased b73252d -> 90671ee (multicheck.5 -> multicheck.6)

…cessful Avoid PRId64 as requested bitcoin#9773 (comment)

ryanofsky · 2017-02-17T12:24:57Z

Squashed aaaf65a -> 1425c72 (multicheck.7 -> multicheck.8)

maflcko · 2017-02-17T14:36:48Z

Somehow travis didn't like your commit.

…

On Fri, Feb 17, 2017 at 1:25 PM, Russell Yanofsky ***@***.***> wrote: Squashed aaaf65a <aaaf65a> -> 1425c72 <1425c72> (multicheck.7 <https://github.com/ryanofsky/bitcoin/commits/pr/multicheck.7> -> multicheck.8 <https://github.com/ryanofsky/bitcoin/commits/pr/multicheck.8>) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#9773 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGmvze2Mxoj5bq4mVJGQPY4YpClnvOlks5rdZGogaJpZM4MCUp0> .

ryanofsky · 2017-02-17T15:43:03Z

Somehow travis didn't like your commit.

Appears to be a spurious error. The travis run for b72521dd7f570b0c557439d534f7523a5be52999 which is aaaf65a merged into 9828f9a succeeded: https://travis-ci.org/bitcoin/bitcoin/builds/202600321

Only the travis run for 64072b74ebbf7b5585b39f6c117c6ffa9dc820f0 which is 1425c72 merged into 9828f9a failed: https://travis-ci.org/bitcoin/bitcoin/builds/202600549

There are no differences between the two commits:

$ git rev-parse aaaf65a90b87c18889d177ff3f0ab8a868f849f3^{tree} 1425c727863b335bf98bb4b17b575e59552d38e9^{tree}
9e695fd32f65b1596f3ff613b96ec756bb2f1d2b
9e695fd32f65b1596f3ff613b96ec756bb2f1d2b

ryanofsky · 2017-02-17T15:48:26Z

Touched CommitDate to rerun travis 1425c72 -> e2e2f4c (multicheck.8 -> multicheck.12)

jtimon · 2017-02-18T23:21:14Z

Concept ACK

…ot successful e2e2f4c Return errors from importmulti if complete rescans are not successful (Russell Yanofsky)

Github-Pull: #9773 Rebased-From: e2e2f4c

paveljanik · 2017-02-22T17:07:05Z

src/wallet/test/wallet_tests.cpp

+    // Verify ScanForWalletTransactions picks up transactions in both the old
+    // and new block files.
+    {
+        CWallet wallet;


This brings a few Wshadow warnings:

Wshadow statistics: 1 wallet/test/wallet_tests.cpp:377:17: warning: declaration shadows a variable in namespace 'wallet_tests' [-Wshadow] 1 wallet/test/wallet_tests.cpp:391:17: warning: declaration shadows a variable in namespace 'wallet_tests' [-Wshadow] 1 wallet/test/wallet_tests.cpp:399:17: warning: declaration shadows a variable in namespace 'wallet_tests' [-Wshadow]

Warnings introduced by commit e2e2f4c "Return errors from importmulti if complete rescans are not successful" and reported by Pavel Janík <Pavel@Janik.cz> in bitcoin#9773 and bitcoin#9827 wallet/test/wallet_tests.cpp: In member function ‘void wallet_tests::rescan::test_method()’: wallet/test/wallet_tests.cpp:377:17: warning: declaration of ‘wallet’ shadows a global declaration [-Wshadow] CWallet wallet;

…s are not successful e2e2f4c Return errors from importmulti if complete rescans are not successful (Russell Yanofsky)

ryanofsky mentioned this pull request Feb 15, 2017

Use 2 hour grace period for key timestamps in importmulti rescans #9761

Merged

fanquake added RPC/REST/ZMQ Wallet labels Feb 16, 2017

laanwj added this to the 0.14.0 milestone Feb 16, 2017

ryanofsky force-pushed the pr/multicheck branch from 094699f to 34e4209 Compare February 16, 2017 15:50

ryanofsky changed the title ~~WIP: Return errors from importmulti if complete rescans are not successful~~ WIP: Return errors from importmulti if complete rescans are not successful (on top of #9761) Feb 16, 2017

morcos reviewed Feb 16, 2017

View reviewed changes

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 16, 2017

fixup! Return errors from importmulti if complete rescans are not suc…

d6f6a23

…cessful Skip empty loop if ReadBlockFromDisk fails as suggested bitcoin#9773 (comment)

ryanofsky commented Feb 16, 2017

View reviewed changes

ryanofsky changed the title ~~WIP: Return errors from importmulti if complete rescans are not successful (on top of #9761)~~ Return errors from importmulti if complete rescans are not successful (on top of #9761) Feb 16, 2017

ryanofsky force-pushed the pr/multicheck branch from e3d4f25 to 455b8f7 Compare February 16, 2017 19:17

jonasschnelli reviewed Feb 16, 2017

View reviewed changes

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 16, 2017

fixup! Return errors from importmulti if complete rescans are not suc…

e49f039

…cessful Add missing response.setArray call and fix string formatting issue pointed out bitcoin#9773 (comment)

ryanofsky force-pushed the pr/multicheck branch from e64cdfb to b73252d Compare February 16, 2017 20:17

jonasschnelli approved these changes Feb 17, 2017

View reviewed changes

laanwj reviewed Feb 17, 2017

View reviewed changes

ryanofsky force-pushed the pr/multicheck branch from b73252d to 90671ee Compare February 17, 2017 12:10

ryanofsky changed the title ~~Return errors from importmulti if complete rescans are not successful (on top of #9761)~~ Return errors from importmulti if complete rescans are not successful Feb 17, 2017

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Feb 17, 2017

fixup! Return errors from importmulti if complete rescans are not suc…

aaaf65a

…cessful Avoid PRId64 as requested bitcoin#9773 (comment)

ryanofsky force-pushed the pr/multicheck branch from aaaf65a to 1425c72 Compare February 17, 2017 12:18

ryanofsky force-pushed the pr/multicheck branch 2 times, most recently from 1425c72 to 68da3ee Compare February 17, 2017 15:45

ryanofsky force-pushed the pr/multicheck branch from 68da3ee to 99d032e Compare February 17, 2017 16:32

Return errors from importmulti if complete rescans are not successful

e2e2f4c

ryanofsky force-pushed the pr/multicheck branch from 99d032e to e2e2f4c Compare February 17, 2017 19:12

laanwj merged commit e2e2f4c into bitcoin:master Feb 22, 2017

laanwj added a commit that referenced this pull request Feb 22, 2017

Merge #9773: Return errors from importmulti if complete rescans are n…

ba7220b

…ot successful e2e2f4c Return errors from importmulti if complete rescans are not successful (Russell Yanofsky)

laanwj pushed a commit that referenced this pull request Feb 22, 2017

Return errors from importmulti if complete rescans are not successful

9072395

Github-Pull: #9773 Rebased-From: e2e2f4c

paveljanik reviewed Feb 22, 2017

View reviewed changes

ryanofsky mentioned this pull request Feb 22, 2017

Improve ScanForWalletTransactions return value #9827

Merged

This was referenced Feb 22, 2017

Avoid -Wshadow warnings in wallet_tests #9828

Merged

Fix importmulti returning rescan errors for wrong keys #9829

Merged

Intermittent FAIL: test/test_bitcoin in Travis #9825

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Return errors from importmulti if complete rescans are not successful #9773

Return errors from importmulti if complete rescans are not successful #9773

Uh oh!

Conversation

ryanofsky commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

morcos commented Feb 15, 2017

Uh oh!

laanwj commented Feb 16, 2017

Uh oh!

gmaxwell commented Feb 16, 2017

Uh oh!

morcos commented Feb 16, 2017

Uh oh!

ryanofsky commented Feb 16, 2017

Uh oh!

morcos Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 16, 2017 • 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!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 16, 2017 • 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!

jonasschnelli Feb 16, 2017 • 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!

ryanofsky Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 16, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Feb 16, 2017

Uh oh!

ryanofsky commented Feb 16, 2017

Uh oh!

sipa commented Feb 16, 2017

Uh oh!

morcos commented Feb 16, 2017

Uh oh!

jonasschnelli left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj Feb 17, 2017 • 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!

ryanofsky Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 17, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj Feb 22, 2017 • 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!

ryanofsky commented Feb 17, 2017

Uh oh!

ryanofsky commented Feb 17, 2017

Uh oh!

maflcko commented Feb 17, 2017 via email

Uh oh!

ryanofsky commented Feb 17, 2017

Uh oh!

ryanofsky commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtimon commented Feb 18, 2017

Uh oh!

paveljanik Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ryanofsky commented Feb 15, 2017 •

edited

Loading

ryanofsky Feb 16, 2017 •

edited

Loading

ryanofsky Feb 16, 2017 •

edited

Loading

jonasschnelli Feb 16, 2017 •

edited

Loading

laanwj Feb 17, 2017 •

edited

Loading

laanwj Feb 22, 2017 •

edited

Loading

ryanofsky commented Feb 17, 2017 •

edited

Loading

paveljanik Feb 22, 2017 •

edited

Loading