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.
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.
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.
1583 int posInBlock;
1584 for (posInBlock = 0; posInBlock < (int)block.vtx.size(); posInBlock++)
1585 {
1586- if (AddToWalletIfInvolvingMe(*block.vtx[posInBlock], pindex, posInBlock, fUpdate))
1587- ret++;
1588+ AddToWalletIfInvolvingMe(*block.vtx[posInBlock], pindex, posInBlock, fUpdate);
1095+ if (GetImportTimestamp(request, now) - 7200 >= scannedRange->GetBlockTimeMax() || results.at(i).exists("error")) {
1096+ response.push_back(results.at(i));
1097+ } else {
1098+ UniValue result = UniValue(UniValue::VOBJ);
1099+ result.pushKV("success", UniValue(false));
1100+ result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, strprintf("Failed to rescan before time %" PRId64 ", transactions may be missing.", (long long)scannedRange->GetBlockTimeMax())));
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.
Added C++ test for new importmulti RPC code in e64cdfba4a7e8a670e76c7bf770c248dc6b78c3d
Squashed e64cdfba4a7e8a670e76c7bf770c248dc6b78c3d -> b73252de549984a06aae294252d0d055aca43a4a (multicheck.4 -> multicheck.5)
1096+ if (GetImportTimestamp(request, now) - 7200 >= scannedRange->GetBlockTimeMax() || results.at(i).exists("error")) {
1097+ response.push_back(results.at(i));
1098+ } else {
1099+ UniValue result = UniValue(UniValue::VOBJ);
1100+ result.pushKV("success", UniValue(false));
1101+ result.pushKV("error", JSONRPCError(RPC_MISC_ERROR, strprintf("Failed to rescan before time %" PRId64 ", transactions may be missing.", scannedRange->GetBlockTimeMax())));
Removed in aaaf65a90b87c18889d177ff3f0ab8a868f849f3.
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.
Somehow travis didn’t like your commit.
On Fri, Feb 17, 2017 at 1:25 PM, Russell Yanofsky notifications@github.com wrote:
Squashed aaaf65a https://github.com/bitcoin/bitcoin/commit/aaaf65a90b87c18889d177ff3f0ab8a868f849f3 -> 1425c72 https://github.com/bitcoin/bitcoin/commit/1425c727863b335bf98bb4b17b575e59552d38e9 (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 https://github.com/bitcoin/bitcoin/pull/9773#issuecomment-280636020, or mute the thread https://github.com/notifications/unsubscribe-auth/AGGmvze2Mxoj5bq4mVJGQPY4YpClnvOlks5rdZGogaJpZM4MCUp0 .
Somehow travis didn’t like your commit.
Appears to be a spurious error. The travis run for b72521dd7f570b0c557439d534f7523a5be52999 which is aaaf65a90b87c18889d177ff3f0ab8a868f849f3 merged into 9828f9a9962c1bee5c343847030b9cfd87a40a5e succeeded: https://travis-ci.org/bitcoin/bitcoin/builds/202600321
Only the travis run for 64072b74ebbf7b5585b39f6c117c6ffa9dc820f0 which is 1425c727863b335bf98bb4b17b575e59552d38e9 merged into 9828f9a9962c1bee5c343847030b9cfd87a40a5e failed: https://travis-ci.org/bitcoin/bitcoin/builds/202600549
There are no differences between the two commits:
0$ git rev-parse aaaf65a90b87c18889d177ff3f0ab8a868f849f3^{tree} 1425c727863b335bf98bb4b17b575e59552d38e9^{tree}
19e695fd32f65b1596f3ff613b96ec756bb2f1d2b
29e695fd32f65b1596f3ff613b96ec756bb2f1d2b
372+ CBlockIndex* newTip = chainActive.Tip();
373+
374+ // Verify ScanForWalletTransactions picks up transactions in both the old
375+ // and new block files.
376+ {
377+ CWallet wallet;
This brings a few Wshadow warnings:
0Wshadow statistics:
1 1 wallet/test/wallet_tests.cpp:377:17: warning: declaration shadows a variable in namespace 'wallet_tests' [-Wshadow]
2 1 wallet/test/wallet_tests.cpp:391:17: warning: declaration shadows a variable in namespace 'wallet_tests' [-Wshadow]
3 1 wallet/test/wallet_tests.cpp:399:17: warning: declaration shadows a variable in namespace 'wallet_tests' [-Wshadow]
Labels
Wallet
RPC/REST/ZMQ
Milestone
0.14.0