Fix ScanForWalletTransactions to return an enum indicating scan result: success / failure / user

Empact commented at 8:31 pm on April 25, 2018: member

Return the failed block as an out arg.

in src/wallet/rpcwallet.cpp:3737 in 25a588ac2f outdated

3780-    else {
3781-        throw JSONRPCError(RPC_MISC_ERROR, "Rescan failed. Potentially corrupted data files.");
3782+    } else if (pwallet->IsAbortingRescan()) {
3783+        throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted.");
3784+    } else {
3785+        throw JSONRPCError(RPC_MISC_ERROR, strprintf("Rescan failed. Potentially corrupted data files. Failed at height %i.", stopBlock->nHeight));

Empact commented at 8:37 pm on April 25, 2018:

Note I added the failure height here. Happy to remove, but I assume it could be helpful.

in src/wallet/wallet.cpp:4172 in 25a588ac2f outdated

4166@@ -4167,11 +4167,11 @@ CWallet* CWallet::CreateWalletFromFile(const std::string& name, const fs::path&
4167         nStart = GetTimeMillis();
4168         {
4169             WalletRescanReserver reserver(walletInstance);
4170-            if (!reserver.reserve()) {
4171+            const CBlockIndex* stop_block = nullptr;
4172+            if (!reserver.reserve() || !walletInstance->ScanForWalletTransactions(pindexRescan, nullptr, stop_block, reserver, true)) {

Empact commented at 8:38 pm on April 25, 2018:

Note this did not previously fail when ScanForWalletTransactions failed, so this removes a silent failure.

promag commented at 9:33 am on April 26, 2018:

I think this is ok. This just adds another case where walletInstance is leaked on error.

Empact commented at 2:40 pm on April 26, 2018:

Should we be switching walletInstance to RAII then?

promag commented at 2:47 pm on April 26, 2018:

@Empact done in #13063.

Empact force-pushed on Apr 25, 2018

in src/wallet/wallet.cpp:1795 in 821c955a15 outdated

1790@@ -1791,7 +1791,7 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
1791         }
1792         ShowProgress(_("Rescanning..."), 100); // hide progress dialog in GUI
1793     }
1794-    return ret;
1795+    return (!fAbortRescan && !failed_block);

Empact commented at 9:14 pm on April 25, 2018:

Alternatively, this could be (!pindex && !failed_block)

Empact force-pushed on Apr 25, 2018

fanquake added the label Wallet on Apr 25, 2018

Empact force-pushed on Apr 26, 2018

in src/wallet/wallet.cpp:1691 in 2c4bc0018b outdated

1763@@ -1763,14 +1764,14 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
1764                 if (pindex && !chainActive.Contains(pindex)) {
1765                     // Abort scan if current block is no longer active, to prevent
1766                     // marking transactions as coming from the wrong block.
1767-                    ret = pindex;
1768+                    failed_block = pindex;

ryanofsky commented at 0:17 am on April 26, 2018:

I think you might want to drop this line and just return true here, to fix the regression described here: #12275 (comment).

Empact commented at 4:11 pm on April 29, 2018:

Not sure about that - my inclination is to maintain stricter checking at the risk of spurious failures over less strict checking at the risk of silent failure. How about opening a separate PR for that so it can be considered separately?

in src/wallet/wallet.cpp:1715 in 2c4bc0018b outdated

1713- * possible (due to pruning or corruption), returns pointer to the most recent
1714- * block that could not be scanned.
1715+ * Returns true if scan was successful. If a complete rescan was not
1716+ * possible (due to pruning or corruption), returns false and sets failed_block
1717+ * to the most recent block that could not be scanned. If the rescan was aborted
1718+ * before it could complete, returns false and does not set failed_block.

jonasschnelli commented at 7:43 am on April 26, 2018:

Not sure if in this PR, but would it make sense to just return an enum to better distinct between fail, ok and user-aborted?

promag commented at 9:25 am on April 26, 2018:

Could be done here IMO, not a big change and would be a lot better IMO.

Empact commented at 4:32 pm on April 29, 2018:

Updated to return an enum.

in src/wallet/wallet.cpp:1724 in 2c4bc0018b outdated

1720@@ -1719,7 +1721,7 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
1721  * the main chain after to the addition of any new keys you want to detect
1722  * transactions for.
1723  */
1724-CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, const WalletRescanReserver &reserver, bool fUpdate)
1725+bool CWallet::ScanForWalletTransactions(const CBlockIndex* const pindexStart, const CBlockIndex* const pindexStop, const CBlockIndex*& failed_block, const WalletRescanReserver& reserver, bool fUpdate)

promag commented at 9:23 am on April 26, 2018:

Output arguments could be last?

Empact commented at 5:29 am on April 30, 2018:

Made it second-to-last due to default value for update arg.

in src/qt/test/wallettests.cpp:160 in 2c4bc0018b outdated

155@@ -156,7 +156,8 @@ void TestGUI()
156         LOCK(cs_main);
157         WalletRescanReserver reserver(&wallet);
158         reserver.reserve();
159-        wallet.ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, true);
160+        const CBlockIndex* stop_block = nullptr;
161+        wallet.ScanForWalletTransactions(chainActive.Genesis(), nullptr, stop_block, reserver, true);

promag commented at 9:28 am on April 26, 2018:

Could assert return value and stop_block value?

promag commented at 9:33 am on April 26, 2018: member

Concept ACK.

Empact force-pushed on Apr 29, 2018

in src/wallet/wallet.cpp:1699 in 620581fca1 outdated

1694@@ -1695,8 +1695,9 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
1695     }
1696 
1697     if (startBlock) {
1698-        const CBlockIndex* const failedBlock = ScanForWalletTransactions(startBlock, nullptr, reserver, update);
1699-        if (failedBlock) {
1700+        const CBlockIndex* failedBlock = nullptr;
1701+        // TODO: this should take into account failure by ScanResult::USER_ABORTED

Empact commented at 4:31 pm on April 29, 2018:

I’ll address this todo in a follow-up.

Empact commented at 5:58 am on November 13, 2018:

Follow-up is here, I’ll wait to open until this is accepted: https://github.com/Empact/bitcoin/tree/rescan-from-time

Empact commented at 6:09 am on December 14, 2018:

#14942

Empact renamed this:
~~Fix ScanForWalletTransactions to return a bool indicating success or failure~~
Fix ScanForWalletTransactions to return an enum indicating scan result: success / failure / user_abort
on Apr 29, 2018

Empact force-pushed on Apr 29, 2018

Empact force-pushed on Apr 30, 2018

in src/wallet/wallet.h:919 in a4f3cb2f83 outdated

913@@ -914,7 +914,13 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
914     void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock) override;
915     bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate);
916     int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update);
917-    CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, const WalletRescanReserver& reserver, bool fUpdate = false);
918+
919+    enum class ScanResult : int {
920+        SUCCESS = 0,

jonasschnelli commented at 12:09 pm on April 30, 2018:

nit: not sure if SUCCESS should use a non C-ish, non null positive value (1 probably)

MarcoFalke commented at 12:29 pm on April 30, 2018:

note that there is no need for an enum class to derive from int or even specify values for the members.

Empact force-pushed on Apr 30, 2018

Empact commented at 2:56 pm on April 30, 2018: member

Dropped explicit values and type from ScanResult enum.

Empact force-pushed on May 3, 2018

Empact commented at 5:27 pm on May 3, 2018: member

Rebased and updated to accommodate #12507.

Empact force-pushed on May 18, 2018

Empact commented at 8:39 pm on May 18, 2018: member

Rebased and moved the shutdown / abort handling inline.

Empact force-pushed on Jun 1, 2018

sabran125 commented at 4:56 am on June 3, 2018: none

indicating the requested changes

DocOBITCOIN approved

DrahtBot commented at 3:16 pm on July 29, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14711 (Remove uses of chainActive and mapBlockIndex in wallet code by ryanofsky)
#14624 (Some simple improvements to the RNG code by sipa)
#10973 (Refactor: separate wallet from node by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot closed this on Jul 29, 2018

DrahtBot reopened this on Jul 29, 2018

DrahtBot added the label Needs rebase on Aug 6, 2018

in src/wallet/rpcwallet.cpp:3927 in c50f3d559c outdated

3930+    case CWallet::ScanResult::FAILURE:
3931         throw JSONRPCError(RPC_MISC_ERROR, "Rescan failed. Potentially corrupted data files.");
3932+    case CWallet::ScanResult::USER_ABORT:
3933+        throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted.");
3934+    default:
3935+        throw JSONRPCError(RPC_MISC_ERROR, strprintf("Unexpected rescan result: %i.", static_cast<int>(result)));

MarcoFalke commented at 9:39 pm on November 5, 2018:

Should remove the default case, so instead of a runtime-error the compiler can warn about missing cases.

0        // no default case, so the compiler can warn about missing cases

MarcoFalke commented at 9:42 pm on November 5, 2018: member

Concept ACK. Needs rebase

MarcoFalke added the label Up for grabs on Nov 5, 2018

Empact force-pushed on Nov 6, 2018

DrahtBot removed the label Needs rebase on Nov 6, 2018

Empact commented at 7:51 pm on November 7, 2018: member

Rebased. @MarcoFalke could you remove “Up for grabs”?

MarcoFalke removed the label Up for grabs on Nov 7, 2018

in src/qt/test/wallettests.cpp:148 in b52abc7164 outdated

143@@ -144,7 +144,12 @@ void TestGUI()
144         LOCK(cs_main);
145         WalletRescanReserver reserver(wallet.get());
146         reserver.reserve();
147-        wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, true);
148+        const CBlockIndex* const null_block = nullptr;
149+        const CBlockIndex* stop_block = nullptr;

MarcoFalke commented at 8:18 pm on November 7, 2018:

nit: No need to initialize a return value. In fact this might look like you are passing in something.

Also could inline nullptr for null_block below for brevity.

Empact commented at 0:09 am on November 13, 2018:

Removed the initialization of the out var, though this required setting the out var to null in the method for consistent behavior under test. Seems like a good change to me as this seems more predictable.

Can’t inline the null_block vars as that results in template ambiguity around <ostream, nullptr_t>

in src/wallet/rpcwallet.cpp:3300 in b52abc7164 outdated

3301-    if (!stopBlock) {
3302-        if (pwallet->IsAbortingRescan()) {
3303-            throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted.");
3304-        }
3305-        // if we got a nullptr returned, ScanForWalletTransactions did rescan up to the requested stopindex
3306+    const CBlockIndex* stopBlock = nullptr;

MarcoFalke commented at 8:19 pm on November 7, 2018:

nit: No need to initialize a return value. In fact this might look like you are passing in something.

in src/wallet/rpcwallet.cpp:3340 in b52abc7164 outdated

3306+    const CBlockIndex* stopBlock = nullptr;
3307+    CWallet::ScanResult result =
3308+        pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, stopBlock, true);
3309+    switch (result) {
3310+    case CWallet::ScanResult::SUCCESS:
3311         stopBlock = pindexStop ? pindexStop : pChainTip;

MarcoFalke commented at 8:23 pm on November 7, 2018:

Why are you overriding the return value?

Empact commented at 9:14 pm on November 8, 2018:

This has been the behavior since the rpc’s introduction in c77170fbdb

MarcoFalke commented at 9:26 pm on November 8, 2018:

Which doesn’t mean it is correct. (The tip can change when you don’t hold cs_main)

Empact commented at 9:27 pm on November 8, 2018:

Buf if we switch to returning the last block in stopBlock on success, this replacement won’t be necessary.

MarcoFalke commented at 9:27 pm on November 8, 2018:

Since you rework what the function returns, you might as well make it return the last successfully scanned block.

MarcoFalke commented at 9:11 pm on November 9, 2018:

I can pick this up and address my feedback middle of next week. (Let me know if you are currently busy or want to take a stab at this.)

in src/wallet/test/wallet_tests.cpp:55 in b52abc7164 outdated

49@@ -50,7 +50,9 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
50         AddKey(wallet, coinbaseKey);
51         WalletRescanReserver reserver(&wallet);
52         reserver.reserve();
53-        BOOST_CHECK_EQUAL(nullBlock, wallet.ScanForWalletTransactions(oldTip, nullptr, reserver));
54+        const CBlockIndex* stop_block = nullptr;
55+        BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, stop_block), CWallet::ScanResult::SUCCESS);
56+        BOOST_CHECK_EQUAL(nullBlock, stop_block);

MarcoFalke commented at 8:25 pm on November 7, 2018:

pIndexStop was nullptr, so this should be tip instead?

meshcollider commented at 1:49 pm on November 11, 2018:

@MarcoFalke stop_block is only set if the scan failed, it isn’t set to tip if the scan succeeds

in src/wallet/test/wallet_tests.cpp:296 in b52abc7164 outdated

282@@ -279,7 +283,10 @@ class ListCoinsTestingSetup : public TestChain100Setup
283         AddKey(*wallet, coinbaseKey);
284         WalletRescanReserver reserver(wallet.get());
285         reserver.reserve();
286-        wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver);
287+        const CBlockIndex* const null_block = nullptr;
288+        const CBlockIndex* stop_block = nullptr;
289+        BOOST_CHECK_EQUAL(wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, stop_block), CWallet::ScanResult::SUCCESS);
290+        BOOST_CHECK_EQUAL(stop_block, null_block);

MarcoFalke commented at 8:26 pm on November 7, 2018:

Should be tip instead of null_block?

meshcollider commented at 1:50 pm on November 11, 2018:

Same here as the other case :)

in src/wallet/wallet.cpp:1625 in b52abc7164 outdated

1619@@ -1619,18 +1620,20 @@ int64_t CWallet::RescanFromTime(int64_t startTime, const WalletRescanReserver& r
1620  * from or to us. If fUpdate is true, found transactions that already
1621  * exist in the wallet will be updated.
1622  *
1623- * Returns null if scan was successful. Otherwise, if a complete rescan was not
1624- * possible (due to pruning or corruption), returns pointer to the most recent
1625- * block that could not be scanned.
1626+ * @param[in] pindexStop if not a nullptr, the scan will stop at this block-index
1627+ * @param[out] failed_block if FAILURE is returned, will be set to the most
1628+ *    recent block that could not be scanned.

MarcoFalke commented at 8:34 pm on November 7, 2018:

nit: could return last_block, which will either be “the most recent block that could not be scanned” (failure/abort) or “pindexStop/::chainActive.Tip()” (success)

MarcoFalke approved

MarcoFalke commented at 8:37 pm on November 7, 2018: member

Concept ACK, but I’d prefer it the block was returned in the case of success as well. (Otherwise it is up to the caller to guess which was the last successfully scanned block, since the tip can change without holding cs_main at the call site)

meshcollider commented at 1:54 pm on November 11, 2018: contributor

Concept ACK / light-utACK https://github.com/bitcoin/bitcoin/pull/13076/commits/b52abc7164ba6403124e409af2e3ee2e2dad7f24

I agree with Marco that returning the last successful block is probably a good idea, but with that and the other comments, at least this doesn’t appear to make things worse, so it could be addressed in follow-ups if not here :)

ryanofsky commented at 5:12 pm on November 11, 2018: member

I agree with Marco that returning the last successful block is probably a good idea

Returning the last failed block instead of the last successful block is needed for importmulti. Importmulti uses the last failed block to return success for keys with birthdays after that block, and failures for keys with older birthdays.

I tried to clarify this in #12275 by returning both values (last_scanned and last_failed):

0    struct ScanResult
1    {
2        const CBlockIndex* first_scanned = nullptr;
3        const CBlockIndex* last_scanned = nullptr;
4        const CBlockIndex* first_failed = nullptr;
5        const CBlockIndex* last_failed = nullptr;
6    };

Which I still think would be an improvement over the current code.

Empact force-pushed on Nov 11, 2018

meshcollider commented at 0:02 am on November 12, 2018: contributor

@ryanofsky I think I mis-explained, I mean in the case of success - the failure case behaviour should remain unchanged

Empact force-pushed on Nov 12, 2018

Empact force-pushed on Nov 13, 2018

Make CWallet::ScanForWalletTransactions args and return value const bb24d68650

Return a status enum from ScanForWalletTransactions

Return the failed block as an out var.

This clarifies the outcome as the prior return value could
be null due to user abort or failure.

3002d6cf31

Empact force-pushed on Nov 13, 2018

Empact commented at 10:25 am on November 13, 2018: member

@MarcoFalke see the stop_block commit above. Any idea where the test failures are coming from? I don’t see the connection.

Add stop_block out arg to ScanForWalletTransactions

Accurately reports the last block successfully scanned, replacing a return of
the chain tip, which represented possibly inaccurated data in a race condition.

bd3b0361d8

in src/qt/test/wallettests.cpp:154 in 981adda5cc outdated

150+        const CBlockIndex* const null_block = nullptr;
151+        const CBlockIndex *stop_block, *failed_block;
152+        QCOMPARE(
153+            wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, failed_block, stop_block, true /* fUpdate */),
154+            CWallet::ScanResult::SUCCESS);
155+        QCOMPARE(stop_block, null_block);

MarcoFalke commented at 5:00 pm on November 13, 2018:

Travis failure:

0********* Start testing of WalletTests *********
1Config: Using QtTest library 5.9.6, Qt 5.9.6 (i386-little_endian-ilp32 static release build; by GCC 7.3.0)
2PASS   : WalletTests::initTestCase()
3FAIL!  : WalletTests::walletTests() Compared pointers are not the same
4   Loc: [qt/test/wallettests.cpp(154)]
5PASS   : WalletTests::cleanupTestCase()
6Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 245ms
7********* Finished testing of WalletTests *********

Empact commented at 10:53 pm on November 13, 2018:

Thanks! Solved.

Empact force-pushed on Nov 13, 2018

ryanofsky approved

ryanofsky commented at 5:40 pm on December 11, 2018: member

utACK bd3b0361d840bff95988a048abf70ade94d80524

jonasschnelli commented at 7:04 pm on December 11, 2018: contributor

utACK bd3b0361d840bff95988a048abf70ade94d80524

MarcoFalke commented at 7:41 pm on December 11, 2018: member

utACK bd3b0361d8

MarcoFalke added this to the milestone 0.18.0 on Dec 11, 2018

MarcoFalke added the label Refactoring on Dec 11, 2018

MarcoFalke added the label Docs on Dec 11, 2018

MarcoFalke removed the label Docs on Dec 11, 2018

meshcollider commented at 5:00 am on December 12, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/13076/commits/bd3b0361d840bff95988a048abf70ade94d80524

meshcollider merged this on Dec 12, 2018

meshcollider closed this on Dec 12, 2018

meshcollider referenced this in commit ed2a2cebd3 on Dec 12, 2018

in src/wallet/wallet.cpp:1657 in bd3b0361d8

1651@@ -1647,8 +1652,8 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlock
1652         assert(pindexStop->nHeight >= pindexStart->nHeight);
1653     }
1654 
1655-    CBlockIndex* pindex = pindexStart;
1656-    CBlockIndex* ret = nullptr;
1657+    const CBlockIndex* pindex = pindexStart;
1658+    failed_block = nullptr;
1659

ryanofsky commented at 9:21 pm on December 13, 2018:

Code is not setting stop_block null here, which seems unintended and could lead to uninitialized variables. Would be good to fix this or add documentation if it was done intentionally.

Empact commented at 7:15 am on December 14, 2018:

Ah, right, the code does contradict the comments on line 1634. Will open a PR.

Empact commented at 7:25 am on December 14, 2018:

#14957

Empact deleted the branch on Dec 14, 2018

deadalnix referenced this in commit 4e8e7bf615 on Mar 26, 2020

deadalnix referenced this in commit 14fb215597 on Mar 26, 2020

deadalnix referenced this in commit 83af2fc468 on Mar 26, 2020

ftrader referenced this in commit 3e0c56419d on Aug 17, 2020

ftrader referenced this in commit 82f232469f on Aug 17, 2020

Munkybooty referenced this in commit 7d1676d3c3 on Aug 5, 2021

MarcoFalke locked this on Sep 8, 2021

Fix ScanForWalletTransactions to return an enum indicating scan result: success / failure / user_abort #13076

Conflicts