The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

@promag im using the block height because everything that calls it is using the block height

@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

Code review ACK dbd8498, dropped unnecessary statement since last review

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

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

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.

Another option could be to drop the start_height argument and keep the start_block argument

Closing for now.