wallet: guard and alert about a wallet invalid state during chain sync

furszy · furszy · commit 77de5c693ffe · 2022-07-18T11:29:27.000-03:00
-Context:
If `AddToWallet` db write fails, the method returns a wtx nullptr without
removing the recently added transaction from the wallet's map.

-Problem:
When a db write error occurs, `AddToWalletIfInvolvingMe` return false even
when the tx is on the wallet's map already --&gt; which makes `SyncTransaction`
skip the `MarkInputsDirty` call --&gt; which leads to a wallet invalid state
where the inputs of this new transaction are not marked dirty, while the
transaction that spends them still exist on the in-memory wallet tx map.

Plus, as we only store arriving transaction inside `AddToWalletIfInvolvingMe`
when we synchronize/scan blocks from the chain and nowhere else, it makes sense
to treat the tx db write error as a runtime error to notify the user about the
problem. Otherwise, the user will lose all the not stored transactions after a
wallet shutdown (without be able to recover them automatically on the next
startup because the chain sync would be above the block where the txs arrived).
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -1130,7 +1130,13 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const SyncTxS
             // Block disconnection override an abandoned tx as unconfirmed
             // which means user may have to call abandontransaction again
             TxState tx_state = std::visit([](auto&& s) -> TxState { return s; }, state);
-            return AddToWallet(MakeTransactionRef(tx), tx_state, /*update_wtx=*/nullptr, /*fFlushOnClose=*/false, rescanning_old_block);
+            CWalletTx* wtx = AddToWallet(MakeTransactionRef(tx), tx_state, /*update_wtx=*/nullptr, /*fFlushOnClose=*/false, rescanning_old_block);
+            if (!wtx) {
+                // Can only be nullptr if there was a db write error (missing db, read-only db or a db engine internal writing error).
+                // As we only store arriving transaction in this process, and we don't want an inconsistent state, let's throw an error.
+                throw std::runtime_error("DB error adding transaction to wallet, write failed");
+            }
+            return true;
         }
     }
     return false;
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -505,6 +505,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     //! @return true if wtx is changed and needs to be saved to disk, otherwise false
     using UpdateWalletTxFn = std::function<bool(CWalletTx& wtx, bool new_tx)>;
 
+    /**
+     * Add the transaction to the wallet, wrapping it up inside a CWalletTx
+     * @return the recently added wtx pointer or nullptr if there was a db write error.
+     */
     CWalletTx* AddToWallet(CTransactionRef tx, const TxState& state, const UpdateWalletTxFn& update_wtx=nullptr, bool fFlushOnClose=true, bool rescanning_old_block = false);
     bool LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void transactionAddedToMempool(const CTransactionRef& tx, uint64_t mempool_sequence) override;
