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

Empact · 2018-04-25T20:31:43Z

Return the failed block as an out arg.

Fixes #11450.

/cc #12275

src/wallet/rpcwallet.cpp

src/wallet/wallet.cpp

ryanofsky · 2018-04-26T00:17:38Z

src/wallet/wallet.cpp

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


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

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?

src/wallet/wallet.cpp

promag

Concept ACK.

src/wallet/wallet.cpp

src/qt/test/wallettests.cpp

src/wallet/wallet.cpp

Empact · 2018-04-29T16:31:54Z

src/wallet/wallet.cpp

-        const CBlockIndex* const failedBlock = ScanForWalletTransactions(startBlock, nullptr, reserver, update);
-        if (failedBlock) {
+        const CBlockIndex* failedBlock = nullptr;
+        // TODO: this should take into account failure by ScanResult::USER_ABORTED


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

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

src/wallet/wallet.h

Empact · 2018-04-30T14:56:45Z

Dropped explicit values and type from ScanResult enum.

Empact · 2018-05-03T17:27:51Z

Rebased and updated to accommodate #12507.

Empact · 2018-11-13T10:25:41Z

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

src/qt/test/wallettests.cpp

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

ryanofsky

utACK bd3b036

jonasschnelli · 2018-12-11T19:04:17Z

utACK bd3b036

maflcko · 2018-12-11T19:41:15Z

utACK bd3b036

meshcollider · 2018-12-12T05:00:05Z

utACK bd3b036

…ing scan result: success / failure / user_abort bd3b036 Add stop_block out arg to ScanForWalletTransactions (Ben Woosley) 3002d6c Return a status enum from ScanForWalletTransactions (Ben Woosley) bb24d68 Make CWallet::ScanForWalletTransactions args and return value const (Ben Woosley) Pull request description: Return the failed block as an out arg. Fixes #11450. /cc #12275 Tree-SHA512: 6a523e5425ebfe24e664a942ae21c797ccc1281c25b1bf8d02ad95c19dae343fd8051985ef11853474de7628fd6bed5f15190fbc087c3466ce6fdecab37d72a9

ryanofsky · 2018-12-13T21:21:44Z

src/wallet/wallet.cpp

-    CBlockIndex* pindex = pindexStart;
-    CBlockIndex* ret = nullptr;
+    const CBlockIndex* pindex = pindexStart;
+    failed_block = nullptr;



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.

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

Summary: Partial backport of core [[bitcoin/bitcoin#13076 | PR13076]]: bitcoin/bitcoin@bb24d68 Test Plan: ninja all check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5561

Summary: ``` 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. ``` Partial backport of core [[bitcoin/bitcoin#13076 | PR13076]]: bitcoin/bitcoin@3002d6c Depends on D5561. Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5562

Summary: ``` Accurately reports the last block successfully scanned, replacing a return of the chain tip, which represented possibly inaccurated data in a race condition. ``` Completes backport of core [[bitcoin/bitcoin#13076 | PR13076]]: bitcoin/bitcoin@bd3b036 Depends on D5562. Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5563

Summary: Partial backport of core [[bitcoin/bitcoin#13076 | PR13076]]: bitcoin/bitcoin@bb24d68 Test Plan: ninja all check Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5561

Summary: ``` 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. ``` Partial backport of core [[bitcoin/bitcoin#13076 | PR13076]]: bitcoin/bitcoin@3002d6c Depends on D5561. Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5562

…indicating scan result: success / failure / user_abort bd3b036 Add stop_block out arg to ScanForWalletTransactions (Ben Woosley) 3002d6c Return a status enum from ScanForWalletTransactions (Ben Woosley) bb24d68 Make CWallet::ScanForWalletTransactions args and return value const (Ben Woosley) Pull request description: Return the failed block as an out arg. Fixes bitcoin#11450. /cc bitcoin#12275 Tree-SHA512: 6a523e5425ebfe24e664a942ae21c797ccc1281c25b1bf8d02ad95c19dae343fd8051985ef11853474de7628fd6bed5f15190fbc087c3466ce6fdecab37d72a9

Empact commented Apr 25, 2018

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

Empact commented Apr 25, 2018

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

Empact force-pushed the scan-for-wallet-transactions-ret branch from 25a588a to 821c955 Compare April 25, 2018 21:08

Empact commented Apr 25, 2018

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

Empact force-pushed the scan-for-wallet-transactions-ret branch from 821c955 to 7b760bc Compare April 25, 2018 23:45

fanquake added the Wallet label Apr 25, 2018

Empact mentioned this pull request Apr 25, 2018

Improve ScanForWalletTransactions return value #12275

Closed

Empact force-pushed the scan-for-wallet-transactions-ret branch from 7b760bc to 2c4bc00 Compare April 26, 2018 00:01

ryanofsky reviewed Apr 26, 2018

View reviewed changes

jonasschnelli reviewed Apr 26, 2018

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

promag reviewed Apr 26, 2018

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

src/qt/test/wallettests.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

Empact force-pushed the scan-for-wallet-transactions-ret branch 2 times, most recently from 3cdfcc3 to 620581f Compare April 29, 2018 16:15

Empact commented Apr 29, 2018

View reviewed changes

Empact changed the title ~~Fix ScanForWalletTransactions to return a bool indicating success or failure~~ Fix ScanForWalletTransactions to return an enum indicating scan result: success / failure / user_abort Apr 29, 2018

Empact force-pushed the scan-for-wallet-transactions-ret branch 9 times, most recently from d922ab0 to a4f3cb2 Compare April 30, 2018 06:44

jonasschnelli reviewed Apr 30, 2018

View reviewed changes

src/wallet/wallet.h Outdated Show resolved Hide resolved

Empact force-pushed the scan-for-wallet-transactions-ret branch from a4f3cb2 to 8bf206b Compare April 30, 2018 14:49

Empact force-pushed the scan-for-wallet-transactions-ret branch from 8bf206b to 42fad11 Compare May 3, 2018 17:27

Empact force-pushed the scan-for-wallet-transactions-ret branch from 42fad11 to ea9d92b Compare May 18, 2018 20:38

Empact force-pushed the scan-for-wallet-transactions-ret branch 3 times, most recently from 8d11f05 to 981adda Compare November 13, 2018 09:43

maflcko reviewed Nov 13, 2018

View reviewed changes

src/qt/test/wallettests.cpp Outdated Show resolved Hide resolved

Add stop_block out arg to ScanForWalletTransactions

bd3b036

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

Empact force-pushed the scan-for-wallet-transactions-ret branch from 981adda to bd3b036 Compare November 13, 2018 17:51

ryanofsky approved these changes Dec 11, 2018

View reviewed changes

maflcko added this to the 0.18.0 milestone Dec 11, 2018

maflcko added Refactoring Docs and removed Docs labels Dec 11, 2018

meshcollider merged commit bd3b036 into bitcoin:master Dec 12, 2018

Empact mentioned this pull request Dec 13, 2018

wallet: Make scan / abort status private to CWallet #14942

Closed

ryanofsky reviewed Dec 13, 2018

View reviewed changes

Empact deleted the scan-for-wallet-transactions-ret branch December 14, 2018 06:10

ryanofsky mentioned this pull request Dec 17, 2018

Refactor: separate wallet from node #10973

Merged

ryanofsky mentioned this pull request Jan 8, 2019

Remove uses of chainActive and mapBlockIndex in wallet code #14711

Merged

Empact mentioned this pull request Jan 30, 2019

wallet: Clarify rescanblockchain doc #15279

Merged

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

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

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

Uh oh!

Conversation

Empact commented Apr 25, 2018 • edited by DrahtBot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky Apr 26, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Apr 29, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Empact Apr 29, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Nov 13, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Empact commented Apr 30, 2018

Uh oh!

Empact commented May 3, 2018

Uh oh!

Empact commented Nov 13, 2018

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Dec 11, 2018

Uh oh!

maflcko commented Dec 11, 2018

Uh oh!

meshcollider commented Dec 12, 2018

Uh oh!

ryanofsky Dec 13, 2018

Choose a reason for hiding this comment

Uh oh!

Empact Dec 14, 2018 • 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!

Empact Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Empact commented Apr 25, 2018 •

edited by DrahtBot

Loading

Empact Dec 14, 2018 •

edited

Loading