wallet: Remove first parameter to ScanForWalletTransactions start_hash #19216

1. 
   pstratem commented at 3:56 am on June 9, 2020: contributor

   Every caller looks up the block hash from a block height immediately before calling.
2. fanquake added the label Wallet on Jun 9, 2020
3. 
   DrahtBot commented at 10:25 am on June 9, 2020: 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:

   • #19310 (wallet: BerkeleyDatabase make BerkeleyDatabase::Create, CreateMock, and CreateDummy non-static functions by achow101)
   • #19195 (wallet: ScanForWalletTransactions cleanup by pstratem)
   • #19137 (wallettool: Add dump and createfromdump commands by achow101)
   • #19102 (wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101)
   • #19077 (wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101)
   • #18971 (wallet: Refactor the classes in wallet/db.{cpp/h} by achow101)
   • #18354 (Use shared pointers only in validation interface by bvbfan)
   • #15719 (Wallet passive startup 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.

4. in src/wallet/rpcwallet.cpp:3564 in 5586ef7a06 outdated

   3566@@ -3567,7 +3567,7 @@ UniValue rescanblockchain(const JSONRPCRequest& request)
   3567     }
   

   ---

   ---

   

   MarcoFalke commented at 11:41 am on June 9, 2020:

   Is there a reason you didn’t remove the start_block variable?

   ---

   

   pstratem commented at 4:44 am on June 11, 2020:

   i didn’t want to change the function called but i guess i should

   ---

   

   promag commented at 9:24 pm on June 11, 2020:

   I think this can be removed now that block hash is gone.

   ---

   

   pstratem commented at 10:05 pm on June 11, 2020:

   indeed
5. 
   promag commented at 9:13 pm on June 10, 2020: member

   Concept ACK dropping one of the start args. I have a preference to keep start_block hash instead.
6. pstratem force-pushed on Jun 11, 2020
7. 
   pstratem commented at 5:17 am on June 11, 2020: contributor

   @promag im using the block height because everything that calls it is using the block height
8. 
   promag commented at 9:33 pm on June 11, 2020: member

   Code review ACK 8d8e4d36c3d3cbe8b495869865aa0b8ff83fdbca. @pstratem the interface uses heights but internally block hash is unambiguous - a block at a certain height can change after a reorg - but not a strong opinion.
9. wallet: Remove first parameter to ScanForWalletTransactions start_hash

   Every caller looks up the block hash from a block height immediately before
   calling.

   dbd8498b81
10. pstratem force-pushed on Jun 11, 2020
11. 
    pstratem commented at 10:36 pm on June 11, 2020: contributor

    @promag the rescan is pretty ambiguous to start with, it’s scanning whatever the current active chain is

    edit: the entire concept in theory should do nothing so it’s a much “fuzzier” thing than other wallet ops that handle reorgs cleanly

12. 
    promag commented at 10:39 pm on June 11, 2020: member

    Code review ACK dbd8498b8134cb4cefa351eb21bef2769a7bdf18, dropped unnecessary statement since last review
13. 
    DrahtBot commented at 9:48 pm on June 18, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

14. DrahtBot added the label Needs rebase on Jun 18, 2020
15. 
    ryanofsky commented at 9:35 pm on June 30, 2020: member

    This isn’t a good change, I think. It doesn’t simplify the code very much, and makes the scanning code less correct and more fragile. The problem is the new getBlockHash call added here:

    https://github.com/pstratem/bitcoin/blob/dbd8498b8134cb4cefa351eb21bef2769a7bdf18/src/wallet/wallet.cpp#L1672

    getBlockHash is a synchronous method that will assert and the crash the process if it is called with an invalid height. Since #16426, the wallet can no longer lock the node, so reorgs can happen any time, and getBlockHash isn’t safe to call after startup. The last remaining uses of getBlockHash are removed in #15719.

    Calling getBlockHash here can also lead to less correct behavior even if it doesn’t fully crash the process. RescanFromTime and CreateWalletFromFile both locate starting blocks based on time, but after this change they only pass the starting block height, not the starting block hash to ScanForWalletTransactions, meaning if there is a reorg, blocks that should be scanned might not be and no errors will be returned.

    In general, the only correct way for wallet code to reference blocks is by hash and not by height, and this PR moves in the less correct direction.

    Also, while ScanForWalletTransactions code is very messy, I think effort would be better spent deleting it and trying to move rescans outside the wallet with ariard’s changes #11756 (comment), than trying to clean it up

16. 
    promag commented at 10:30 am on July 1, 2020: member

    Concept ACK dropping one of the start args. I have a preference to keep start_block hash instead.

    @pstratem the interface uses heights but internally block hash is unambiguous - a block at a certain height can change after a reorg - but not a strong opinion.

    I’ve mentioned this before, but considering @ryanofsky comment #19216 (comment) and @ariard work I agree that this is not worth it.

17. 
    ryanofsky commented at 3:51 pm on July 1, 2020: member

    Another option could be to drop the start_height argument and keep the start_block argument
18. 
    fanquake commented at 8:00 am on August 18, 2021: member

    Closing for now.
19. fanquake closed this on Aug 18, 2021

    ---

20. DrahtBot locked this on Aug 18, 2022

Contributors
pstratem DrahtBot MarcoFalke promag ryanofsky fanquake

Labels
Wallet Needs rebase