From 77de5c693ffe8dc0afa5e40126e9b0e9cc547e04 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 3 Jun 2022 10:46:14 -0300 Subject: [PATCH] wallet: guard and alert about a wallet invalid state during chain sync -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 --> which makes `SyncTransaction` skip the `MarkInputsDirty` call --> 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). --- src/wallet/wallet.cpp | 8 +++++++- src/wallet/wallet.h | 4 ++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 475627c76c7..eb072c2c0c1 100644 --- 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 index e2c3d76438f..3ebd26987b7 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -505,6 +505,10 @@ public: //! @return true if wtx is changed and needs to be saved to disk, otherwise false using UpdateWalletTxFn = std::function; + /** + * 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;