wallet: Require a start block in ScanForWalletTransactions

MarcoFalke commented at 10:03 pm on November 12, 2018: member

ScanForWalletTransactions incorrectly documents that pindexStart is optional. Fix that by making that parameter a const reference.

To the best of my knowledge it is impossible to hit those code paths from functional tests and I think it is not worth to write a unit test for this edge case:

0pindexStart == nullptr /* i.e. "optional" */ && pindexStop != nullptr

MarcoFalke added the label Wallet on Nov 12, 2018

MarcoFalke force-pushed on Nov 12, 2018

in test/functional/wallet_rescan.py:31 in fa323c4eba outdated

26+
27+        self.log.info("Mine some (current time) blocks to rescan")
28+        self.restart_node(0)
29+        self.nodes[0].generate(10)
30+
31+        self.log.info("Check that rescaning the wallet from future does not crash")

practicalswift commented at 8:09 am on November 13, 2018:

“Rescanning” :-)

MarcoFalke commented at 3:18 pm on November 13, 2018:

Thx. Fixed.

in src/wallet/rpcwallet.cpp:3335 in fa323c4eba outdated

3331@@ -3332,7 +3332,7 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
3332         }
3333     }
3334 
3335-    CBlockIndex *stopBlock = pwallet->ScanForWalletTransactions(pindexStart, pindexStop, reserver, true);
3336+    const CBlockIndex* stopBlock = pwallet->ScanForWalletTransactions(*pindexStart, pindexStop, reserver, true);

Empact commented at 10:01 am on November 13, 2018:

Genesis can return nullptr - maybe throw on that above rather than possibly get a null dereference error?

MarcoFalke commented at 3:18 pm on November 13, 2018:

Good point. Fixed

Empact commented at 10:02 am on November 13, 2018: member

Concept ACK, could squash

meshcollider commented at 10:04 am on November 13, 2018: contributor

Good find, but this is made redundant in #14711

EDIT: I see your comment there, fair enough if you want to clean this up before that PR goes in

Empact commented at 10:23 am on November 13, 2018: member

I prefer getting this in before #14711

MarcoFalke commented at 3:14 pm on November 13, 2018: member

@MeshCollider While unlikely to be hit in practice, this is a bugfix and not fixed by the refactoring in #14711

DrahtBot commented at 3:19 pm on November 13, 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)
#13076 (Fix ScanForWalletTransactions to return an enum indicating scan result: success / failure / user_abort by Empact)

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.

MarcoFalke force-pushed on Nov 13, 2018

Empact commented at 7:05 pm on November 13, 2018: member

FYI the associated test is passing for me on master:

0$ python3 test/functional/wallet_rescan.py 
12018-11-13T19:05:22.673000Z TestFramework (INFO): Initializing test directory /var/folders/_v/0klckbk95bxgt21rhcj_6qb40000gn/T/testg2yddy4y
22018-11-13T19:05:23.510000Z TestFramework (INFO): Create wallet with timestamps in the future
32018-11-13T19:05:24.475000Z TestFramework (INFO): Mine some (current time) blocks to rescan
42018-11-13T19:05:25.079000Z TestFramework (INFO): Check that rescanning the wallet from future does not crash
52018-11-13T19:05:25.831000Z TestFramework (INFO): Stopping nodes
62018-11-13T19:05:26.211000Z TestFramework (INFO): Cleaning up /var/folders/_v/0klckbk95bxgt21rhcj_6qb40000gn/T/testg2yddy4y on exit
72018-11-13T19:05:26.211000Z TestFramework (INFO): Tests successful

MarcoFalke force-pushed on Nov 13, 2018

MarcoFalke renamed this:
~~wallet: Fix crash when rescanning wallet from future~~
wallet: Fix rescan of wallets from the future
on Nov 13, 2018

MarcoFalke force-pushed on Nov 13, 2018

MarcoFalke renamed this:
~~wallet: Fix rescan of wallets from the future~~
wallet: Require a start block in ScanForWalletTransactions
on Nov 13, 2018

wallet: Require a start block in ScanForWalletTransactions fa76c56f33

MarcoFalke force-pushed on Nov 13, 2018

MarcoFalke commented at 8:30 pm on November 13, 2018: member

@Empact My bad. Indeed, the test only crashed on my machine because I added further asserts locally. To the best of my knowledge it is impossible to hit those code paths from functional tests and I think it is not worth to write a unit test for this edge case: Start block == nullptr && end block != nullptr.

MarcoFalke commented at 9:41 pm on November 13, 2018: member

On a second look @MeshCollider is correct. This is also fixed in #14711, so please review that instead.

MarcoFalke closed this on Nov 13, 2018

MarcoFalke deleted the branch on Nov 13, 2018

DrahtBot locked this on Sep 8, 2021

wallet: Require a start block in ScanForWalletTransactions #14712

Conflicts