wallet, test: best block locator matches scan state follow-ups #32580

1. 
   rkrux commented at 2:50 pm on May 21, 2025: contributor

   Few follows-ups from #30221: Use SetLastBlockProcessedInMem more in AttachChain, add not null locator check in WriteBestBlock. Add log and few assertions in wallet_reorgstore test.
2. 
   DrahtBot commented at 2:50 pm on May 21, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32580.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	achow101, pablomartin4btc
   Stale ACK	Prabhat1308

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. rkrux marked this as ready for review on May 21, 2025
4. 
   Prabhat1308 commented at 4:25 pm on June 10, 2025: contributor

   ACK 3112fb5
5. DrahtBot added the label CI failed on Jun 26, 2025
6. wallet, test: best block locator matches scan state follow-ups

   Few follows-ups from #30221: Use `SetLastBlockProcessedInMem` more in
   `AttachChain`, add not null locator check in `WriteBestBlock`. Add log
   and few assertions in `wallet_reorgstore` test.

   1b5c545e82
7. in test/functional/wallet_reorgsrestore.py:130 in 3112fb5c5b outdated

   127         # Upon reload after the crash, since the chainstate was not flushed, the tip contains the previously abandoned
   128-        # coinbase. This should be rescanned and now un-abandoned.
   129+        # coinbase. This was rescanned and now un-abandoned.
   130         wallet = node.get_wallet_rpc("reorg_crash")
   131         assert_equal(wallet.gettransaction(coinbase_tx_id)['details'][0]['abandoned'], False)
   132+        assert_greater_than(wallet.getwalletinfo()['immature_balance'], 0)
   

   ---

   ---

   

   maflcko commented at 4:06 pm on July 1, 2025:

                                  KeyError: 'immature_balance'
   

   ---

   

   pablomartin4btc commented at 4:16 pm on July 1, 2025:

   It was removed in #32721.

   ---

   

   rkrux commented at 9:01 am on July 2, 2025:

   Thanks, fixed.
8. rkrux force-pushed on Jul 2, 2025
9. DrahtBot removed the label CI failed on Jul 2, 2025
10. 
    achow101 commented at 8:34 pm on July 2, 2025: member

    ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b
11. DrahtBot requested review from Prabhat1308 on Jul 2, 2025
12. in test/functional/wallet_reorgsrestore.py:93 in 1b5c545e82

    89@@ -90,7 +90,7 @@ def test_coinbase_automatic_abandon_during_startup(self):
    90         assert_equal(wallet0.gettransaction(descendant_tx_id)['details'][0]['abandoned'], True)
    91 
    92     def test_reorg_handling_during_unclean_shutdown(self):
    93-        self.log.info("Test that wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown")
    94+        self.log.info("Test that wallet transactions are un-abandoned in case of temporarily invalidated blocks and wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown")
    

    ---

    ---

    

    pablomartin4btc commented at 4:38 am on July 3, 2025:

    nit: I haven’t seen the format I’m proposing here but just to make it more clear if it makes sense…

    0        self.log.info(
    1            "Test that on a duplicate block disconnection event after unclean shutdown:\n"
    2            "    - wallet transactions are un-abandoned after temporarily invalidated blocks;\n"
    3            "    - wallet doesn't crash.")
    

    edited: moved the closing parenthesis up

    ---

    

    rkrux commented at 8:55 am on July 3, 2025:

    I recall spending some time to come up with a log format specifically for this but I was not satisfied with the ones I came up, and thus defaulted to the regular one. This suggestion looks better to me, I will use it if I end up retouching.
13. 
    pablomartin4btc commented at 4:39 am on July 3, 2025: member

    cr-ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b

    Reviewed #30221 to verify the agreed follow-ups:

    • In wallet.cpp:
      • another opportunity to use setLastBlockProcessedInMem - #30221 (review)
      • (!loc.IsNull()) - #30221 (review)
    • In wallet_reorgsrestore.py:
      • update test log - #30221 (review)
      • assert rescanning - #30221 (review)
      • correct immature balance in test - #30221 (review)

    Left a small nit on the test log update.

14. achow101 merged this on Jul 9, 2025
15. achow101 closed this on Jul 9, 2025

    ---

16. 
    maflcko commented at 9:03 am on July 10, 2025: member

    silent conflict fixed in https://github.com/bitcoin/bitcoin/pull/32932

Contributors
rkrux DrahtBot Prabhat1308 maflcko pablomartin4btc achow101

Review Requested
Prabhat1308