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

pull rkrux wants to merge 1 commits into bitcoin:master from rkrux:wallet-locator changing 2 files +14 −10

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.
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.
rkrux marked this as ready for review on May 21, 2025
Prabhat1308 commented at 4:25 PM on June 10, 2025: contributor

ACK 3112fb5
DrahtBot added the label CI failed on Jun 26, 2025

Type	Reviewers
ACK	achow101, pablomartin4btc
Stale ACK	Prabhat1308

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

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.

rkrux force-pushed on Jul 2, 2025
DrahtBot removed the label CI failed on Jul 2, 2025
achow101 commented at 8:34 PM on July 2, 2025: member

ACK 1b5c545e82fe3cf5027f16b43e2306aeb8d4ef9b
DrahtBot requested review from Prabhat1308 on Jul 2, 2025

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.

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.
achow101 merged this on Jul 9, 2025
achow101 closed this on Jul 9, 2025
maflcko commented at 9:03 AM on July 10, 2025: member

silent conflict fixed in https://github.com/bitcoin/bitcoin/pull/32932
stringintech referenced this in commit f19e7819e9 on Jul 19, 2025
alexanderwiederin referenced this in commit 9997c3d993 on Jul 25, 2025
alexanderwiederin referenced this in commit 8d6ebb9c4e on Jul 28, 2025
alexanderwiederin referenced this in commit 533a4585b5 on Jul 28, 2025
yuvicc referenced this in commit 22f55cf11d on Aug 26, 2025
bug-castercv502 referenced this in commit d4bd8a1259 on Sep 28, 2025
stickies-v referenced this in commit a19c56cd7c on Nov 4, 2025
luke-jr referenced this in commit 29c124486d on Mar 24, 2026
luke-jr referenced this in commit ad19b1efff on Mar 24, 2026
Kino1994 referenced this in commit 04faead395 on Jun 28, 2026
BigcoinBGC referenced this in commit 44a00446bf on Jun 30, 2026
bitcoin locked this on Jul 30, 2026

Contributors

Review Requested
Prabhat1308

Linked (view graph)

#30221 wallet: Ensure best block matches wallet scan state

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

Code Coverage & Benchmarks

Reviews