Repository: bitcoin/bitcoin (language: cpp)

Issue:
Potential crash (assert) rescanning wallet
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Preface: This whole analysis was made using Elements Core, not Bitcoin Core, but the code and the conditions should line up in a way that makes this likely in Bitcoin too.

There's an assert crash in `assert(chain_depth >= 0); // coinbase tx should not be conflicted` calling `CWallet::GetTxBlocksToMaturity` because `CWallet::GetTxDepthInMainChain`  returns a negative value for `TxStateConfirmed` txs. The reason for this is that `m_last_block_processed` is set at the beginning of `AttachChain`, but the rescan loop keeps going beyond that point, so when `GetTxDepthInMainChain` is called right after the scan is done, m_last_block_processed is lower than the actual block in the latest txs, hence returning a negative value and crashing.

In discussions with @achow101 , it's possible that https://github.com/bitcoin/bitcoin/pull/30221 fixes this issue by calling `SetBestBlock` after the rescan is done. Another approach could be to stop the scan when `m_last_block_processed` is reached with something like this

```diff
@@ -1689,6 +1693,11 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
         if (max_height && block_height >= *max_height) {
             break;
         }
+        if (block_height >= GetLastBlockHeight()) {
+            WalletLogPrintf("Stopping scan. At block %d. Progress=%f\n", block_height, progress_current);
+            break;
+        }
+
         {
             if (!next_block) {
                 // break successfully when rescan has reached the tip, or
```

### Expected behaviour

The wallet data should be consistent to avoid the crash

### Steps to reproduce

This is reproduced using QT

1. Create a chain with many coinbase txs.
2. Attach a wallet (could be read only) with the address of the coinbase txs
3. While the wallet is still being loaded, create many more blocks with coinbase txs to that address
4. Assert happens when the wallet is done loading

### Relevant log output

_No response_

### How did you obtain Bitcoin Core

Compiled from source

### What version of Bitcoin Core are you using?

master@62bd61de110b057cbfd6e31e4d0b727d93119c72

### Operating system and version

Fedora 41

### Machine specifications

_No response_


Discussion / hints:
I tried to reproduce the crash locally (putting a sleep into AttachChain) but didn't manage - Not the biggest GUI or wallet expert so I may well be missing something, but here's what I tried:
- With qt, how do I create more transactions while the wallet is loading? The GUI blocks for me until the wallet is loaded.
- With bitcoind, I was able create more blocks during the rescan - but those will create `blockConnected()` notifications, and several RPCs that call `GetTxBlocksToMaturity` (such as `getbalance`)  use `BlockUntilSyncedToCurrentChain`, waiting for the notifications to be processed and causing  `m_last_block_processed` to be set to the correct value when `GetTxBlocksToMaturity` is finally called.

Even if I couldn't reproduce the crash, rescanning blocks higher than `m_last_block_processed` seems wrong, so the suggested fix makes sense to me.
> With qt, how do I create more transactions while the wallet is loading? The GUI blocks for me until the wallet is loaded.

Would it be possible to start the GUI with `-server=1` and then use one of the RPC threads to create more blocks while the GUI is blocked?
> Would it be possible to start the GUI with `-server=1` and then use one of the RPC threads to create more blocks while the GUI is blocked?

Good point, that worked! I could reproduce the crash in the GUI that way (with sleeps in `AttachChain()` and `blockConnected()` to avoid races) -  I can confirm that it's fixed by the patch in the OP, whereas I could still reproduce it with #30221, so that PR apparently doesn't fix it.
Thanks for reproducing! It would be good to share the backtrace. I haven't tried reproducing myself and I am still wondering if this is a GUI issue or a wallet issue.
From my tests, it's a wallet issue (inconsistent state), that only the GUI triggers, The wallet has an invalid state, but the "happy path" in the CLI version doesn't notice it
> From my tests, it's a wallet issue (inconsistent state), that only the GUI triggers, The wallet has an invalid state, but the "happy path" in the CLI version doesn't notice it

I agree, my understanding is this: There is a temporary inconsistent state in the wallet when during a rescan the node receives additional blocks (could be via RPC or p2p), that are prematurely processed without updating `m_last_block_processed`. This will self-correct when the `blockConnected` signals for the added blocks are processed and `m_last_block_processed` is set to its correct value. But if `CWallet::GetTxBlocksToMaturity` is called after the rescan finished and before the signals are processed, the node crashes if the coinbase is a wallet tx. The reason why it only crashes in the GUI is that many RPC that end up calling `GetTxBlocksToMaturity`, such as `getbalance`, will first call `BlockUntilSyncedToCurrentChain` that would correct the issue, so I didn't find a way to trigger. However, the GUI calls the function as part of `refreshWallet` / `getWalletTxs`, see the backtrace below.

> It would be good to share the backtrace.

```
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff5e4526e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff5e288ff in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff5e2881b in __assert_fail_base (fmt=0x7ffff5fd01e8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x555556007eba "chain_depth >= 0", 
    file=file@entry=0x555556007da1 "./wallet/wallet.cpp", line=line@entry=3482, function=function@entry=0x555555fcf578 "int wallet::CWallet::GetTxBlocksToMaturity(const wallet::CWalletTx&) const")
    at ./assert/assert.c:94
#6  0x00007ffff5e3b507 in __assert_fail (assertion=0x555556007eba "chain_depth >= 0", file=0x555556007da1 "./wallet/wallet.cpp", line=3482, 
    function=0x555555fcf578 "int wallet::CWallet::GetTxBlocksToMaturity(const wallet::CWalletTx&) const") at ./assert/assert.c:103
#7  0x0000555555c2ac94 in wallet::CWallet::GetTxBlocksToMaturity (this=this@entry=0x7fff1802a650, wtx=...) at ./wallet/wallet.cpp:3482
#8  0x0000555555c2accd in wallet::CWallet::IsTxImmatureCoinBase (this=this@entry=0x7fff1802a650, wtx=...) at ./wallet/wallet.cpp:3491
#9  0x0000555555bb4b6a in wallet::CachedTxGetCredit (wallet=..., wtx=..., filter=filter@entry=@0x7fff7cbfedd8: 3) at ./wallet/receive.cpp:114
#10 0x0000555555b9cddd in wallet::(anonymous namespace)::MakeWalletTx (wallet=..., wtx=...) at ./wallet/interfaces.cpp:79
#11 0x0000555555b9de40 in wallet::(anonymous namespace)::WalletImpl::getWalletTxs (this=0x7fff18222630) at ./wallet/interfaces.cpp:354
#12 0x000055555581f832 in TransactionTablePriv::refreshWallet (this=0x7fff182927e0, wallet=...) at ./qt/transactiontablemodel.cpp:115
#13 0x000055555581d168 in TransactionTableModel::TransactionTableModel (this=this@entry=0x7fff181247f0, _platformStyle=_platformStyle@entry=0x555556b4c9a0, parent=parent@entry=0x7fff18001450)
    at ./qt/transactiontablemodel.cpp:259
#14 0x0000555555792bc0 in WalletModel::WalletModel (this=this@entry=0x7fff18001450, wallet=std::unique_ptr<interfaces::Wallet> = {...}, client_model=..., platformStyle=0x555556b4c9a0, 
    parent=parent@entry=0x0) at ./qt/walletmodel.cpp:50
#15 0x000055555577efaa in WalletController::getOrCreateWallet (this=0x5555575987c0, wallet=std::unique_ptr<interfaces::Wallet> = {...}) at /usr/include/c++/13/bits/unique_ptr.h:197
#16 0x000055555577f510 in operator() (__closure=<optimized out>, wallet=std::unique_ptr<interfaces::Wallet> = {...}) at /usr/include/c++/13/bits/unique_ptr.h:197
#17 std::__invoke_impl<void, WalletController::WalletController(ClientModel&, const PlatformStyle*, QObject*)::<lambda(std::unique_ptr<interfaces::Wallet>)>&, std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> > > (__f=...) at /usr/include/c++/13/bits/invoke.h:61
#18 std::__invoke_r<void, WalletController::WalletController(ClientModel&, const PlatformStyle*, QObject*)::<lambda(std::unique_ptr<interfaces::Wallet>)>&, std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> > > (__fn=...) at /usr/include/c++/13/bits/invoke.h:111
#19 std::_Function_handler<void(std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> >), WalletController::WalletController(ClientModel&, const PlatformStyle*, QObject*)::<lambda(std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> >)> >::_M_invoke(const std::_Any_data &, std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> > &&) (
    __functor=..., __args#0=...) at /usr/include/c++/13/bits/std_function.h:290
#20 0x0000555555c28b2f in std::function<void (std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> >)>::operator()(std::unique_ptr<interfaces::Wallet, std::default_delete<interfaces::Wallet> >) const (__args#0=std::unique_ptr<interfaces::Wallet> = {...}, this=0x555556a12440) at /usr/include/c++/13/bits/std_function.h:591
#21 wallet::NotifyWalletLoaded (context=..., wallet=std::shared_ptr<wallet::CWallet> (use count 3, weak count 0) = {...}) at ./wallet/wallet.cpp:224
#22 0x0000555555c490ed in wallet::(anonymous namespace)::LoadWalletInternal (warnings=std::vector of length 0, capacity 0, error=..., status=@0x7fff7cbff694: wallet::DatabaseStatus::SUCCESS, 
    options=..., load_on_start=..., name="", context=...) at ./wallet/wallet.cpp:296
#23 wallet::LoadWallet (context=..., name="", load_on_start=..., load_on_start@entry=std::optional = {...}, options=..., status=@0x7fff7cbff694: wallet::DatabaseStatus::SUCCESS, error=..., 
    warnings=std::vector of length 0, capacity 0) at ./wallet/wallet.cpp:378
#24 0x0000555555b9873d in wallet::(anonymous namespace)::WalletLoaderImpl::loadWallet (this=0x7fff78002070, name="", warnings=std::vector of length 0, capacity 0) at /usr/include/c++/13/optional:213
#25 0x000055555577fd4c in operator() (__closure=0x55555723b0f0) at ./qt/walletcontroller.cpp:359
#26 QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, OpenWalletActivity::open(const std::string&)::<lambda()> >::call (arg=<optimized out>, f=...)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:146
#27 QtPrivate::Functor<OpenWalletActivity::open(const std::string&)::<lambda()>, 0>::call<QtPrivate::List<>, void> (arg=<optimized out>, f=...)
    at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:256
#28 QtPrivate::QFunctorSlotObject<OpenWalletActivity::open(const std::string&)::<lambda()>, 0, QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase *, QObject *, void **, bool *) (
    which=<optimized out>, this_=0x55555723b0e0, r=<optimized out>, a=<optimized out>, ret=<optimized out>) at /usr/include/x86_64-linux-gnu/qt5/QtCore/qobjectdefs_impl.h:443
#29 0x00007ffff6906343 in QObject::event(QEvent*) () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#30 0x00007ffff796bd45 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /lib/x86_64-linux-gnu/libQt5Widgets.so.5
--Type <RET> for more, q to quit, c to continue without paging--
#31 0x00007ffff68d8118 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#32 0x00007ffff68db94b in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#33 0x00007ffff6935c0f in ?? () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#34 0x00007ffff59145b5 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#35 0x00007ffff5973717 in ?? () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#36 0x00007ffff5913a53 in g_main_context_iteration () from /lib/x86_64-linux-gnu/libglib-2.0.so.0
#37 0x00007ffff6935279 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#38 0x00007ffff68d6a7b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#39 0x00007ffff66da36b in QThread::exec() () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#40 0x00007ffff66db674 in ?? () from /lib/x86_64-linux-gnu/libQt5Core.so.5
#41 0x00007ffff5e9ca94 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:447
#42 0x00007ffff5f29c3c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78
```

I'll open a fix (probably just @psgreco's suggested fix, but I want to look into it a bit more).
@mzumsande if you end up using my version, please look at the final version here https://github.com/ElementsProject/elements/pull/1376/files, which addresses the locking too

Fix patch:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index b971be5ddda89..adf9b602f5207 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1984,6 +1984,14 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
         if (max_height && block_height >= *max_height) {
             break;
         }
+        // If rescanning was triggered with cs_wallet permanently locked (AttachChain), additional blocks that were connected during the rescan
+        // aren't processed here but will be processed with the pending blockConnected notifications after the lock is released.
+        // If rescanning without a permanent cs_wallet lock, additional blocks that were added during the rescan will be re-processed if
+        // the notification was processed and the last block height was updated.
+        if (block_height >= WITH_LOCK(cs_wallet, return GetLastBlockHeight())) {
+            break;
+        }
+
         {
             if (!next_block) {
                 // break successfully when rescan has reached the tip, or
@@ -3266,12 +3274,12 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
     }
 
     // Register wallet with validationinterface. It's done before rescan to avoid
-    // missing block connections between end of rescan and validation subscribing.
-    // Because of wallet lock being hold, block connection notifications are going to
-    // be pending on the validation-side until lock release. It's likely to have
-    // block processing duplicata (if rescan block range overlaps with notification one)
-    // but we guarantee at least than wallet state is correct after notifications delivery.
-    // However, chainStateFlushed notifications are ignored until the rescan is finished
+    // missing block connections during the rescan.
+    // Because of the wallet lock being held, block connection notifications are going to
+    // be pending on the validation-side until lock release. Blocks that are connected while the
+    // rescan is ongoing will not be processed in the rescan but with the block connected notifications,
+    // so the wallet will only be completeley synced after the notifications delivery.
+    // chainStateFlushed notifications are ignored until the rescan is finished
     // so that in case of a shutdown event, the rescan will be repeated at the next start.
     // This is temporary until rescan and notifications delivery are unified under same
     // interface.


Test patch: