Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Dec 12, 2018

Conversation

@Empact
Copy link
Member

commented Apr 25, 2018

Return the failed block as an out arg.

Fixes #11450.

/cc #12275

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

@Empact Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch Apr 25, 2018

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

@Empact Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch Apr 25, 2018

@fanquake fanquake added the Wallet label Apr 25, 2018

@Empact Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch Apr 26, 2018

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 26, 2018

Contributor

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

This comment has been minimized.

Copy link
@Empact

Empact Apr 29, 2018

Author Member

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?

Show resolved Hide resolved src/wallet/wallet.cpp Outdated
@promag
Copy link
Member

left a comment

Concept ACK.

Show resolved Hide resolved 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

@Empact Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch 2 times, most recently Apr 29, 2018

src/wallet/wallet.cpp Outdated
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

This comment has been minimized.

Copy link
@Empact

Empact Apr 29, 2018

Author Member

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

This comment has been minimized.

Copy link
@Empact

Empact Nov 13, 2018

Author Member

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

This comment has been minimized.

Copy link
@Empact

@Empact 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 Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch 9 times, most recently Apr 29, 2018

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

@Empact Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch Apr 30, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2018

Dropped explicit values and type from ScanResult enum.

@Empact Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch May 3, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented May 3, 2018

Rebased and updated to accommodate #12507.

@Empact Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch May 18, 2018

@Empact Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch 5 times, most recently Nov 12, 2018

Empact added some commits Apr 25, 2018

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.

@Empact Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch 3 times, most recently from 8d11f05 Nov 13, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2018

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

Show resolved Hide resolved src/qt/test/wallettests.cpp Outdated
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.

@Empact Empact force-pushed the Empact:scan-for-wallet-transactions-ret branch to bd3b036 Nov 13, 2018

@ryanofsky
Copy link
Contributor

left a comment

utACK bd3b036

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

utACK bd3b036

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

utACK bd3b036

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

@MarcoFalke MarcoFalke added Refactoring Docs and removed Docs labels Dec 11, 2018

@meshcollider

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

utACK bd3b036

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

meshcollider added a commit that referenced this pull request Dec 12, 2018

Merge #13076: Fix ScanForWalletTransactions to return an enum indicat…
…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
CBlockIndex* pindex = pindexStart;
CBlockIndex* ret = nullptr;
const CBlockIndex* pindex = pindexStart;
failed_block = nullptr;

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Dec 13, 2018

Contributor

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.

This comment has been minimized.

Copy link
@Empact

Empact Dec 14, 2018

Author Member

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

This comment has been minimized.

Copy link
@Empact

@Empact Empact deleted the Empact:scan-for-wallet-transactions-ret branch Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.