ScanForWalletTransactions return value is incorrectly documented #11450

1. 
   jnewbery commented at 2:22 pm on October 4, 2017: member

   ScanForWalletTransactions() has the following comment about its return value:

   0 * Returns null if scan was successful. Otherwise, if a complete rescan was not
   1 * possible (due to pruning or corruption), returns pointer to the most recent
   2 * block that could not be scanned.
   

   This is not strictly true. If abortrescan is called, then ScanForWalletTransactions() will return nullptr, even though the scan wasn’t successful to the tip.

   This in turn causes a minor bug in importmulti, where the call doesn’t return the correct error and warning about potentially not finding transactions if abortrescan was called.

   I think the correct fix is for ScanForWalletTransactions() to inform the caller that the rescan was aborted.

2. fanquake added the label Wallet on Oct 4, 2017
3. 
   ryanofsky commented at 2:57 pm on October 4, 2017: member

   I think this is handled by #11200
4. 
   promag commented at 3:07 pm on October 4, 2017: member

   Regardless of #11200 IMO CWallet::ScanForWalletTransactions and CWallet::RescanFromTime should be improved.
5. 
   ryanofsky commented at 4:33 pm on October 4, 2017: member

   @promag noticed your comments in IRC (https://botbot.me/freenode/bitcoin-core-dev/msg/91898082/):

   <promag> jnewbery: ScanForWalletTransactions interface is a bit weird <promag> I think it should return the last block scanned <promag> and also all failed blocks <promag> not sure why only the most recent is useful

   It’s useful to know the most recent block that failed for the importmulti rescan. If multiple keys are imported, imports of keys with birthdays before the most recent failed block time are marked failed, and imports of later keys are marked successful.

6. 
   promag commented at 4:40 pm on October 4, 2017: member

   Yes I came to that conclusion following the result of these functions. However from these scan functions point of view, they should return all failed blocks. importmulti should use what fits better.
7. 
   MarcoFalke commented at 11:19 pm on April 13, 2018: member

   What is the status of this issue?
8. 
   Empact commented at 6:41 pm on April 25, 2018: member

   @MarcoFalke I’m on it.
9. 
   MarcoFalke commented at 6:58 pm on April 25, 2018: member

   Please note that there is #12275 by @ryanofsky, which might be related.
10. meshcollider closed this on Dec 12, 2018

    ---

11. meshcollider referenced this in commit ed2a2cebd3 on Dec 12, 2018
12. Munkybooty referenced this in commit 7d1676d3c3 on Aug 5, 2021
13. DrahtBot locked this on Sep 8, 2021
14. vijaydasmp referenced this in commit d762cd2b00 on Sep 12, 2021
15. vijaydasmp referenced this in commit ac2095a0a2 on Sep 12, 2021
16. vijaydasmp referenced this in commit f6c001f264 on Sep 12, 2021
17. vijaydasmp referenced this in commit 8a6c851a8c on Sep 13, 2021
18. vijaydasmp referenced this in commit 24d8a31cb9 on Sep 13, 2021
19. vijaydasmp referenced this in commit 01d73f460b on Sep 14, 2021
20. vijaydasmp referenced this in commit a894cb607c on Sep 14, 2021
21. gades referenced this in commit acda1a4a88 on May 31, 2022

Contributors
jnewbery ryanofsky promag MarcoFalke Empact

Labels
Wallet