wallet: Initialize stop_block in CWallet::ScanForWalletTransactions

Empact commented at 7:22 am on December 14, 2018: member

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

fanquake added the label Wallet on Dec 14, 2018

Empact renamed this:
~~wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions~~
wallet: Initialize stop_block in CWallet::ScanForWalletTransactions
on Dec 14, 2018

MarcoFalke commented at 7:01 pm on December 14, 2018: member

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

ryanofsky approved

ryanofsky commented at 8:36 pm on December 14, 2018: member

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:

https://github.com/bitcoin/bitcoin/blob/9133227298ad97bbb10c44ac038f614c0bd7f7c7/src/wallet/rpcwallet.cpp#L3439

MarcoFalke commented at 8:48 pm on December 14, 2018: member

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 added this to the milestone 0.18.0 on Dec 14, 2018

ryanofsky commented at 9:05 pm on December 14, 2018: member

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 commented at 9:10 pm on December 14, 2018: member

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

ryanofsky commented at 9:21 pm on December 14, 2018: member

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

https://github.com/bitcoin/bitcoin/blob/9133227298ad97bbb10c44ac038f614c0bd7f7c7/src/wallet/test/wallet_tests.cpp#L65

Empact force-pushed on Dec 14, 2018

Empact commented at 9:49 pm on December 14, 2018: member

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

in src/wallet/test/wallet_tests.cpp:56 in 7b6935f762 outdated

51+    {
52+        CWallet wallet(*chain, WalletLocation(), WalletDatabase::CreateDummy());
53+        AddKey(wallet, coinbaseKey);
54+        WalletRescanReserver reserver(&wallet);
55+        reserver.reserve();
56+        const CBlockIndex *stop_block, *failed_block;

ryanofsky commented at 10:02 pm on December 14, 2018:

Maybe set these to null_block + 1, since it’s possible they could be null initially by coincidence.

Empact force-pushed on Dec 14, 2018

Empact commented at 10:03 pm on December 14, 2018: member

Split the importmulti test into its own case for independence.

in src/wallet/test/wallet_tests.cpp:99 in 3412d21ec8 outdated

91@@ -79,44 +92,77 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
92         BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 50 * COIN);
93     }
94 
95+    // Prune the remaining block file.
96+    PruneOneBlockFile(newTip->GetBlockPos().nFile);
97+    UnlinkPrunedFiles({newTip->GetBlockPos().nFile});
98+
99+    // Verify ScanForWalletTransactions only picks transactions in the new block

ryanofsky commented at 10:08 pm on December 14, 2018:

Looks like this should say “does not pick up transactions” instead of “only picks transactions”

Empact force-pushed on Dec 15, 2018

Empact force-pushed on Dec 16, 2018

in src/wallet/test/wallet_tests.cpp:60 in 1c19a0b481 outdated

55+        reserver.reserve();
56+        const CBlockIndex *stop_block, *failed_block;
57+        BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(nullptr, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS);
58+        BOOST_CHECK_EQUAL(failed_block, null_block);
59+        BOOST_CHECK_EQUAL(stop_block, null_block);
60+        BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 0);

promag commented at 3:14 pm on December 17, 2018:

Irrelevant for this test case?

MarcoFalke commented at 6:17 pm on December 17, 2018:

Shouldn’t hurt either

in src/wallet/test/wallet_tests.cpp:109 in 1c19a0b481 outdated

105+        reserver.reserve();
106+        const CBlockIndex *stop_block, *failed_block;
107+        BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::FAILURE);
108+        BOOST_CHECK_EQUAL(failed_block, newTip);
109+        BOOST_CHECK_EQUAL(stop_block, null_block);
110+        BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 0);

promag commented at 3:14 pm on December 17, 2018:

Same as above.

promag commented at 3:16 pm on December 17, 2018: member

Needs rebase to account for #14979.

ACK 1c19a0b, just a comment in the tests.

in src/wallet/test/wallet_tests.cpp:85 in 1c19a0b481 outdated

128+    UnlinkPrunedFiles({oldTip->GetBlockPos().nFile});
129+
130     // Verify importmulti RPC returns failure for a key whose creation time is
131     // before the missing block, and success for a key whose creation time is
132     // after.
133-    {

MarcoFalke commented at 6:20 pm on December 17, 2018:

micro-nit: no need to flatten this scope. (Keeping it makes it easier to write additional tests in the future)

MarcoFalke commented at 6:20 pm on December 17, 2018: member

utACK 1c19a0b4812ba3b9e0a21985d8064801ca25e334

ryanofsky approved

ryanofsky commented at 6:56 pm on December 17, 2018: member

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

wallet: Initialize stop_block to nullptr in CWallet::ScanForWalletTransactions

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

8b9171ccf0

Empact force-pushed on Dec 17, 2018

Empact commented at 9:10 pm on December 17, 2018: member

Thanks, fixed the nits - many of these had been fixed in https://github.com/bitcoin/bitcoin/commit/b5de045b78dd3c2a9f12eb45f4ac2f44754de9f0 but I accidentally overwrote it on a second machine rebasing yesterday.

Here’s the diff -w with 1c19a0b4812ba3b9e0a21985d8064801ca25e334:

 0diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
 1index 28200123f..1ed1926af 100644
 2--- a/src/wallet/test/wallet_tests.cpp
 3+++ b/src/wallet/test/wallet_tests.cpp
 4@@ -53,7 +53,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
 5         AddKey(wallet, coinbaseKey);
 6         WalletRescanReserver reserver(&wallet);
 7         reserver.reserve();
 8-        const CBlockIndex *stop_block, *failed_block;
 9+        const CBlockIndex *stop_block = null_block + 1, *failed_block = null_block + 1;
10         BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(nullptr, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS);
11         BOOST_CHECK_EQUAL(failed_block, null_block);
12         BOOST_CHECK_EQUAL(stop_block, null_block);
13@@ -67,7 +67,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
14         AddKey(wallet, coinbaseKey);
15         WalletRescanReserver reserver(&wallet);
16         reserver.reserve();
17-        const CBlockIndex *stop_block, *failed_block;
18+        const CBlockIndex *stop_block = null_block + 1, *failed_block = null_block + 1;
19         BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS);
20         BOOST_CHECK_EQUAL(failed_block, null_block);
21         BOOST_CHECK_EQUAL(stop_block, newTip);
22@@ -85,7 +85,7 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
23         AddKey(wallet, coinbaseKey);
24         WalletRescanReserver reserver(&wallet);
25         reserver.reserve();
26-        const CBlockIndex *stop_block, *failed_block;
27+        const CBlockIndex *stop_block = null_block + 1, *failed_block = null_block + 1;
28         BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::FAILURE);
29         BOOST_CHECK_EQUAL(failed_block, oldTip);
30         BOOST_CHECK_EQUAL(stop_block, newTip);
31@@ -96,14 +96,13 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
32     PruneOneBlockFile(newTip->GetBlockPos().nFile);
33     UnlinkPrunedFiles({newTip->GetBlockPos().nFile});
34 
35-    // Verify ScanForWalletTransactions only picks transactions in the new block
36-    // file.
37+    // Verify ScanForWalletTransactions scans no blocks.
38     {
39         CWallet wallet(*chain, WalletLocation(), WalletDatabase::CreateDummy());
40         AddKey(wallet, coinbaseKey);
41         WalletRescanReserver reserver(&wallet);
42         reserver.reserve();
43-        const CBlockIndex *stop_block, *failed_block;
44+        const CBlockIndex *stop_block = null_block + 1, *failed_block = null_block + 1;
45         BOOST_CHECK_EQUAL(wallet.ScanForWalletTransactions(oldTip, nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::FAILURE);
46         BOOST_CHECK_EQUAL(failed_block, newTip);
47         BOOST_CHECK_EQUAL(stop_block, null_block);
48@@ -130,6 +129,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
49     // Verify importmulti RPC returns failure for a key whose creation time is
50     // before the missing block, and success for a key whose creation time is
51     // after.
52+    {
53         std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(*chain, WalletLocation(), WalletDatabase::CreateDummy());
54         AddWallet(wallet);
55         UniValue keys;
56@@ -164,6 +164,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
57                               0, oldTip->GetBlockTimeMax(), TIMESTAMP_WINDOW));
58         RemoveWallet(wallet);
59     }
60+}
61 
62 // Verify importwallet RPC starts rescan at earliest block with timestamp
63 // greater or equal than key birthday. Previously there was a bug where
64@@ -340,7 +341,7 @@ public:
65         WalletRescanReserver reserver(wallet.get());
66         reserver.reserve();
67         const CBlockIndex* const null_block = nullptr;
68-        const CBlockIndex *stop_block, *failed_block;
69+        const CBlockIndex *stop_block = null_block + 1, *failed_block = null_block + 1;
70         BOOST_CHECK_EQUAL(wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, failed_block, stop_block), CWallet::ScanResult::SUCCESS);
71         BOOST_CHECK_EQUAL(stop_block, chainActive.Tip());
72         BOOST_CHECK_EQUAL(failed_block, null_block);

ryanofsky approved

ryanofsky commented at 9:17 pm on December 17, 2018: member

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

MarcoFalke commented at 9:59 pm on December 17, 2018: member

re-utACK 8b9171ccf0a90583b5f3802ac3b4b212c230e0ed

meshcollider commented at 1:11 am on December 18, 2018: contributor

@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 commented at 1:11 am on December 18, 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)
#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 commented at 2:06 am on December 18, 2018: member

“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 commented at 2:32 am on December 18, 2018: contributor

Ok, utACK https://github.com/bitcoin/bitcoin/pull/14957/commits/8b9171ccf0a90583b5f3802ac3b4b212c230e0ed

meshcollider merged this on Dec 18, 2018

meshcollider closed this on Dec 18, 2018

meshcollider referenced this in commit 27f5a295d7 on Dec 18, 2018

Empact deleted the branch on Dec 18, 2018

in src/wallet/test/wallet_tests.cpp:50 in 8b9171ccf0

46@@ -47,14 +47,27 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
47 
48     auto locked_chain = chain->lock();
49 
50+    // Verify ScanForWalletTransactions accomodates a null start block.

ryanofsky commented at 3:47 pm on January 4, 2019:

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

0src/wallet/test/wallet_tests.cpp:50: accomodates  ==> accommodates
1^ 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

fanquake commented at 3:49 pm on January 4, 2019:

Should be fixed in #15102.

deadalnix referenced this in commit 07d651060a on Mar 26, 2020

PastaPastaPasta referenced this in commit ff82953340 on Oct 23, 2021

PastaPastaPasta referenced this in commit 2b4a149d9a on Oct 23, 2021

PastaPastaPasta referenced this in commit 20f5c314f5 on Oct 24, 2021

UdjinM6 referenced this in commit c2fa9af2d1 on Oct 25, 2021

PastaPastaPasta referenced this in commit d506af1a68 on Oct 25, 2021

PastaPastaPasta referenced this in commit 268bc4c772 on Nov 1, 2021

pravblockc referenced this in commit 84e764ffca on Nov 18, 2021

DrahtBot locked this on Dec 16, 2021

wallet: Initialize stop_block in CWallet::ScanForWalletTransactions #14957

Conflicts