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

wallet: Initialize stop_block in CWallet::ScanForWalletTransactions #14957

Merged
merged 1 commit into from Dec 18, 2018

Conversation

Projects
None yet
7 participants
@Empact
Copy link
Member

commented Dec 14, 2018

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

@fanquake fanquake added the Wallet label Dec 14, 2018

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

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

@ryanofsky
Copy link
Contributor

left a comment

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:

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

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.

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

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

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

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Dec 14, 2018

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

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

@Empact Empact force-pushed the Empact:stop-block-null branch Dec 14, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2018

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

@Empact Empact force-pushed the Empact:stop-block-null branch Dec 14, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Dec 14, 2018

Split the importmulti test into its own case for independence.

@Empact Empact force-pushed the Empact:stop-block-null branch Dec 15, 2018

@Empact Empact force-pushed the Empact:stop-block-null branch Dec 16, 2018

@promag
Copy link
Member

left a comment

Needs rebase to account for #14979.

ACK 1c19a0b, just a comment in the tests.

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

This comment has been minimized.

Copy link
@promag

promag Dec 17, 2018

Member

Irrelevant for this test case?

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Dec 17, 2018

Member

Shouldn't hurt either

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

This comment has been minimized.

Copy link
@promag

promag Dec 17, 2018

Member

Same as above.

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

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

utACK 1c19a0b4812ba3b9e0a21985d8064801ca25e334

@ryanofsky
Copy link
Contributor

left a comment

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

Show resolved Hide resolved src/wallet/test/wallet_tests.cpp Outdated
Show resolved Hide resolved src/wallet/test/wallet_tests.cpp Outdated
wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTra…
…nsactions

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

@Empact Empact force-pushed the Empact:stop-block-null branch to 8b9171c Dec 17, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Dec 17, 2018

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
Copy link
Contributor

left a comment

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 17, 2018

re-utACK 8b9171c

@meshcollider

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2018

"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

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

Ok, utACK 8b9171c

@meshcollider meshcollider merged commit 8b9171c into bitcoin:master Dec 18, 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 18, 2018

Merge #14957: wallet: Initialize stop_block in CWallet::ScanForWallet…
…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

@Empact Empact deleted the Empact:stop-block-null branch Dec 18, 2018

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

auto locked_chain = chain->lock();

// Verify ScanForWalletTransactions accomodates a null start block.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jan 4, 2019

Contributor

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

This comment has been minimized.

Copy link
@fanquake

fanquake Jan 4, 2019

Member

Should be fixed in #15102.

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.