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

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

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

   <!--021abf342d371248e50ceaed478a90ca-->

   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.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

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...

            self.log.info(
                "Test that on a duplicate block disconnection event after unclean shutdown:\n"
                "    - wallet transactions are un-abandoned after temporarily invalidated blocks;\n"
                "    - 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

rkruxDrahtBotPrabhat1308maflckopablomartin4btcachow101

Review Requested
Prabhat1308