wallet: Initialize stop_block in CWallet::ScanForWalletTransactions #14957

Empact · 2018-12-14T07:22:02Z

Previously the argument would be untouched if the first block scan failed. This
makes the behavior predictable, and consistent with the documentation.

maflcko · 2018-12-14T19:01:36Z

Bug fixes should come with test coverage, so that they wouldn't break again in the future.

ryanofsky

utACK b5c20992a201c1711924cf09ab21c0a3ce6cbb93, thanks for addressing this so quickly. I agree a test would be nice, though looking more closely, maybe the bug doesn't really result in any consequences. For example, without file corruption I don't see a way to call rescanblockchain that would make the stopBlock->nHeight line segfault:

bitcoin/src/wallet/rpcwallet.cpp

Line 3439 in 9133227

response.pushKV("stop_height", stopBlock->nHeight);

maflcko · 2018-12-14T20:48:16Z

utACK b5c2099. I assumed it would segfault when there is a reorg during rescan, not sure how easy it is to write a test for that.

ryanofsky · 2018-12-14T21:05:38Z

I don't think even a reorg would be enough to cause a segfault with rescanblockchain unless the reorg happened in a racy way before even a single block could be read.

But alternately, it would be simple to write a c++ unit test that directly called ScanForWalletTransactions with an empty range and checked the value of stop_block afterwards. It would just be kind of a boring test.

maflcko · 2018-12-14T21:10:38Z

Isn't the start block inclusive in the range? In that case an empty range couldn't be passed.

ryanofsky · 2018-12-14T21:21:56Z

Passing null pindexStart might work, or using PruneOneBlockFile like the existing rescan tests:

bitcoin/src/wallet/test/wallet_tests.cpp

Line 65 in 9133227

PruneOneBlockFile(oldTip->GetBlockPos().nFile);

Empact · 2018-12-14T21:49:45Z

Thanks @ryanofsky, added the two cases you mention, both fail without the initialization. Looking into the importmulti test that follows immediately after...

Empact · 2018-12-14T22:03:54Z

Split the importmulti test into its own case for independence.

promag

Needs rebase to account for #14979.

ACK 1c19a0b, just a comment in the tests.

promag · 2018-12-17T15:14:26Z

src/wallet/test/wallet_tests.cpp

+        BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(nullptr, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS);
+        BOOST_CHECK_EQUAL(failed_block, null_block);
+        BOOST_CHECK_EQUAL(stop_block, null_block);
+        BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 0);


Irrelevant for this test case?

Shouldn't hurt either

promag · 2018-12-17T15:14:32Z

src/wallet/test/wallet_tests.cpp

+        BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::FAILURE);
+        BOOST_CHECK_EQUAL(failed_block, newTip);
+        BOOST_CHECK_EQUAL(stop_block, null_block);
+        BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 0);


Same as above.

src/wallet/test/wallet_tests.cpp

maflcko · 2018-12-17T18:20:58Z

utACK 1c19a0b4812ba3b9e0a21985d8064801ca25e334

ryanofsky

utACK 1c19a0b4812ba3b9e0a21985d8064801ca25e334, but one of the test comments appears to be wrong, so it would be good to fix that before merging.

src/wallet/test/wallet_tests.cpp

…nsactions Previously the argument would be untouched if the first block scan failed. This makes the behavior predictable, and consistent with the documentation.

Empact · 2018-12-17T21:10:59Z

Thanks, fixed the nits - many of these had been fixed in b5de045 but I accidentally overwrote it on a second machine rebasing yesterday.

Here's the diff -w with 1c19a0b4812ba3b9e0a21985d8064801ca25e334:

diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 28200123f..1ed1926af 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -53,7 +53,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
         AddKey(wallet, coinbaseKey);
         WalletRescanReserver reserver(&wallet);
         reserver.reserve();
-        const CBlockIndex *stop_block, *failed_block;
+        const CBlockIndex *stop_block = null_block + 1, *failed_block = null_block + 1;
         BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(nullptr, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS);
         BOOST_CHECK_EQUAL(failed_block, null_block);
         BOOST_CHECK_EQUAL(stop_block, null_block);
@@ -67,7 +67,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
         AddKey(wallet, coinbaseKey);
         WalletRescanReserver reserver(&wallet);
         reserver.reserve();
-        const CBlockIndex *stop_block, *failed_block;
+        const CBlockIndex *stop_block = null_block + 1, *failed_block = null_block + 1;
         BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS);
         BOOST_CHECK_EQUAL(failed_block, null_block);
         BOOST_CHECK_EQUAL(stop_block, newTip);
@@ -85,7 +85,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
         AddKey(wallet, coinbaseKey);
         WalletRescanReserver reserver(&wallet);
         reserver.reserve();
-        const CBlockIndex *stop_block, *failed_block;
+        const CBlockIndex *stop_block = null_block + 1, *failed_block = null_block + 1;
         BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::FAILURE);
         BOOST_CHECK_EQUAL(failed_block, oldTip);
         BOOST_CHECK_EQUAL(stop_block, newTip);
@@ -96,14 +96,13 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
     PruneOneBlockFile(newTip->GetBlockPos().nFile);
     UnlinkPrunedFiles({newTip->GetBlockPos().nFile});
 
-    // Verify ScanForWalletTransactions only picks transactions in the new block
-    // file.
+    // Verify ScanForWalletTransactions scans no blocks.
     {
         CWallet wallet(*chain, WalletLocation(), WalletDatabase::CreateDummy());
         AddKey(wallet, coinbaseKey);
         WalletRescanReserver reserver(&wallet);
         reserver.reserve();
-        const CBlockIndex *stop_block, *failed_block;
+        const CBlockIndex *stop_block = null_block + 1, *failed_block = null_block + 1;
         BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::FAILURE);
         BOOST_CHECK_EQUAL(failed_block, newTip);
         BOOST_CHECK_EQUAL(stop_block, null_block);
@@ -130,6 +129,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
     // Verify importmulti RPC returns failure for a key whose creation time is
     // before the missing block, and success for a key whose creation time is
     // after.
+    {
         std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(*chain, WalletLocation(), WalletDatabase::CreateDummy());
         AddWallet(wallet);
         UniValue keys;
@@ -164,6 +164,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
                               0, oldTip->GetBlockTimeMax(), TIMESTAMP_WINDOW));
         RemoveWallet(wallet);
     }
+}
 
 // Verify importwallet RPC starts rescan at earliest block with timestamp
 // greater or equal than key birthday. Previously there was a bug where
@@ -340,7 +341,7 @@ public:
         WalletRescanReserver reserver(wallet.get());
         reserver.reserve();
         const CBlockIndex* const null_block = nullptr;
-        const CBlockIndex *stop_block, *failed_block;
+        const CBlockIndex *stop_block = null_block + 1, *failed_block = null_block + 1;
         BOOST_CHECK_EQUAL(wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS);
         BOOST_CHECK_EQUAL(stop_block, chainActive.Tip());
         BOOST_CHECK_EQUAL(failed_block, null_block);

ryanofsky

utACK 8b9171c. Only suggested changes since last review (Thanks!)

maflcko · 2018-12-17T21:59:37Z

re-utACK 8b9171c

meshcollider · 2018-12-18T01:11:19Z

@ryanofsky is null_block + 1 defined behavior? null_block is a nullptr, doesn't feel like arithmetic on a nullptr should be defined. @practicalswift might know

DrahtBot · 2018-12-18T01:11:45Z

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)
#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.

Empact · 2018-12-18T02:06:47Z

"There exist implicit conversions from nullptr to null pointer value of any pointer type and any pointer to member type." so the arithmetic is on a 0-initialized const CBlockIndex*, so it's regular pointer math.
https://en.cppreference.com/w/cpp/language/nullptr
https://github.com/llvm-mirror/libcxx/blob/master/include/__nullptr

That said, I don't know how much less likely the value 1 is to be found in uninitialized memory, over the value 0.

meshcollider · 2018-12-18T02:32:48Z

Ok, utACK 8b9171c

…Transactions 8b9171c wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions (Ben Woosley) Pull request description: Previously the argument would be untouched if the first block scan failed. This makes the behavior predictable, and consistent with the documentation. Tree-SHA512: 3efadf9fd5e25ecd9450f32545f58e61a123ad883e921ef427b13e4782ffdd8ffe905c9ad3edc7e8f9e4953342cd72247bb4cc9eeaf9e5fd04291ac5c1bb5eec

ryanofsky · 2019-01-04T15:47:58Z

src/wallet/test/wallet_tests.cpp

@@ -47,14 +47,27 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)

    auto locked_chain = chain->lock();

+    // Verify ScanForWalletTransactions accomodates a null start block.


This seems to trigger a lint warning from test/lint/lint-all.sh on master:

src/wallet/test/wallet_tests.cpp:50: accomodates ==> accommodates ^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt

Should be fixed in #15102.

Summary: ``` Previously the argument would be untouched if the first block scan failed. This makes the behavior predictable, and consistent with the documentation. ``` Backport of core [[bitcoin/bitcoin#14957 | PR14957]] and the remaining of [[bitcoin/bitcoin#15321 | PR15321]] (see D5238). Depends on D5563. Test Plan: With Clang: cmake -GNinja .. -DCMAKE_BUILD_TYPE=Debug -DENABLE_WERROR=ON ninja all check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5564

…rWalletTransactions 8b9171c wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions (Ben Woosley) Pull request description: Previously the argument would be untouched if the first block scan failed. This makes the behavior predictable, and consistent with the documentation. Tree-SHA512: 3efadf9fd5e25ecd9450f32545f58e61a123ad883e921ef427b13e4782ffdd8ffe905c9ad3edc7e8f9e4953342cd72247bb4cc9eeaf9e5fd04291ac5c1bb5eec # Conflicts: # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

…rWalletTransactions 8b9171c wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions (Ben Woosley) Pull request description: Previously the argument would be untouched if the first block scan failed. This makes the behavior predictable, and consistent with the documentation. Tree-SHA512: 3efadf9fd5e25ecd9450f32545f58e61a123ad883e921ef427b13e4782ffdd8ffe905c9ad3edc7e8f9e4953342cd72247bb4cc9eeaf9e5fd04291ac5c1bb5eec # Conflicts: # src/wallet/test/wallet_tests.cpp

…rWalletTransactions 8b9171c wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions (Ben Woosley) Pull request description: Previously the argument would be untouched if the first block scan failed. This makes the behavior predictable, and consistent with the documentation. Tree-SHA512: 3efadf9fd5e25ecd9450f32545f58e61a123ad883e921ef427b13e4782ffdd8ffe905c9ad3edc7e8f9e4953342cd72247bb4cc9eeaf9e5fd04291ac5c1bb5eec # Conflicts: # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

…rWalletTransactions (#4538) * Merge bitcoin#14957: wallet: Initialize stop_block in CWallet::ScanForWalletTransactions 8b9171c wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions (Ben Woosley) Pull request description: Previously the argument would be untouched if the first block scan failed. This makes the behavior predictable, and consistent with the documentation. Tree-SHA512: 3efadf9fd5e25ecd9450f32545f58e61a123ad883e921ef427b13e4782ffdd8ffe905c9ad3edc7e8f9e4953342cd72247bb4cc9eeaf9e5fd04291ac5c1bb5eec # Conflicts: # src/wallet/test/wallet_tests.cpp * Add lock annotation * fix Co-authored-by: MeshCollider <dobsonsa68@gmail.com>

…rWalletTransactions 8b9171c wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions (Ben Woosley) Pull request description: Previously the argument would be untouched if the first block scan failed. This makes the behavior predictable, and consistent with the documentation. Tree-SHA512: 3efadf9fd5e25ecd9450f32545f58e61a123ad883e921ef427b13e4782ffdd8ffe905c9ad3edc7e8f9e4953342cd72247bb4cc9eeaf9e5fd04291ac5c1bb5eec # Conflicts: # src/wallet/test/wallet_tests.cpp # src/wallet/wallet.cpp

…rWalletTransactions (dashpay#4538) * Merge bitcoin#14957: wallet: Initialize stop_block in CWallet::ScanForWalletTransactions 8b9171c wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions (Ben Woosley) Pull request description: Previously the argument would be untouched if the first block scan failed. This makes the behavior predictable, and consistent with the documentation. Tree-SHA512: 3efadf9fd5e25ecd9450f32545f58e61a123ad883e921ef427b13e4782ffdd8ffe905c9ad3edc7e8f9e4953342cd72247bb4cc9eeaf9e5fd04291ac5c1bb5eec # Conflicts: # src/wallet/test/wallet_tests.cpp * Add lock annotation * fix Co-authored-by: MeshCollider <dobsonsa68@gmail.com>

fanquake added the Wallet label Dec 14, 2018

Empact mentioned this pull request Dec 14, 2018

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

Merged

Empact changed the title ~~wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions~~ wallet: Initialize stop_block in CWallet::ScanForWalletTransactions Dec 14, 2018

ryanofsky approved these changes Dec 14, 2018

View reviewed changes

maflcko added this to the 0.18.0 milestone Dec 14, 2018

Empact force-pushed the stop-block-null branch from b5c2099 to 7b6935f Compare December 14, 2018 21:49

Empact force-pushed the stop-block-null branch from 7b6935f to 3412d21 Compare December 14, 2018 22:03

Empact force-pushed the stop-block-null branch from 3412d21 to b5de045 Compare December 15, 2018 10:20

Empact mentioned this pull request Dec 15, 2018

tests: Fix cuckoocache_tests TSAN failure introduced in 14935. #14969

Merged

Empact force-pushed the stop-block-null branch from b5de045 to 1c19a0b Compare December 16, 2018 17:26

promag reviewed Dec 17, 2018

View reviewed changes

maflcko reviewed Dec 17, 2018

View reviewed changes

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

ryanofsky approved these changes Dec 17, 2018

View reviewed changes

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

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

wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTra…

8b9171c

…nsactions Previously the argument would be untouched if the first block scan failed. This makes the behavior predictable, and consistent with the documentation.

Empact force-pushed the stop-block-null branch from 1c19a0b to 8b9171c Compare December 17, 2018 21:07

ryanofsky approved these changes Dec 17, 2018

View reviewed changes

meshcollider merged commit 8b9171c into bitcoin:master Dec 18, 2018

Empact deleted the stop-block-null branch December 18, 2018 03:15

ryanofsky mentioned this pull request Dec 18, 2018

Refactor: separate wallet from node #10973

Merged

ryanofsky reviewed Jan 4, 2019

View reviewed changes

ryanofsky mentioned this pull request Jan 8, 2019

Remove uses of chainActive and mapBlockIndex in wallet code #14711

Merged

christiancfifi mentioned this pull request Oct 4, 2021

Backports 15747, 15532, 14556, 14594, 15136, 20491, 14715 dashpay/dash#4482

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

		@@ -47,14 +47,27 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)

		auto locked_chain = chain->lock();

		// Verify ScanForWalletTransactions accomodates a null start block.

wallet: Initialize stop_block in CWallet::ScanForWalletTransactions #14957

wallet: Initialize stop_block in CWallet::ScanForWalletTransactions #14957

Uh oh!

Conversation

Empact commented Dec 14, 2018

Uh oh!

maflcko commented Dec 14, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 14, 2018

Uh oh!

ryanofsky commented Dec 14, 2018

Uh oh!

maflcko commented Dec 14, 2018

Uh oh!

ryanofsky commented Dec 14, 2018

Uh oh!

Empact commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

promag Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Dec 17, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Empact commented Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 17, 2018

Uh oh!

meshcollider commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 18, 2018

Conflicts

Uh oh!

Empact commented Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Dec 18, 2018

Uh oh!

ryanofsky Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

fanquake Jan 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Empact commented Dec 14, 2018 •

edited

Loading

Empact commented Dec 14, 2018 •

edited

Loading

Empact commented Dec 17, 2018 •

edited

Loading

meshcollider commented Dec 18, 2018 •

edited

Loading

Empact commented Dec 18, 2018 •

edited

Loading