Merge bitcoin/bitcoin#28546: wallet: prevent bugs from invalid transaction heights with asserts, comments, and refactoring

f06016d77d wallet: Add asserts to detect unset transaction height values (Ryan Ofsky)
262a78b133 wallet, refactor: Add CWalletTx::updateState function (Ryan Ofsky)

Pull request description:

  Originally, this PR fixed a wallet migration bug that could cause the watchonly wallet created by legacy wallet migration to have incorrect transaction height values. A different fix for the bug was implemented in #28609, but that PR did not add any test coverage that would have caught the bug, and didn't include other changes from this PR intended to prevent problems from invalid transaction heights.

  This PR adds new asserts to catch invalid transaction heights, which would trigger test failures without bugfix in #28609. This PR also refactors code and adds comments to clarify assumptions and make it less likely a bug from invalid transaction height values would be introduced.

ACKs for top commit:
  achow101:
    ACK f06016d77d
  Sjors:
    utACK f06016d77d
  furszy:
    Code review ACK f06016d

Tree-SHA512: 82657c403724d60354f7676b53bcfcc95bdc5864e051a2eb8bfad09d8ad35615393b2d6b432b46f908def9be37bebded3a55ec9ae19e19371d35897fe842c92e

src/wallet/transaction.cpp

@@ -4,6 +4,10 @@
 
 #include <wallet/transaction.h>
 
+#include <interfaces/chain.h>
+
+using interfaces::FoundBlock;
+
 namespace wallet {
 bool CWalletTx::IsEquivalentTo(const CWalletTx& _tx) const
 {
@@ -25,6 +29,27 @@ int64_t CWalletTx::GetTxTime() const
     return n ? n : nTimeReceived;
 }
 
+void CWalletTx::updateState(interfaces::Chain& chain)
+{
+    bool active;
+    auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {
+        // If tx block (or conflicting block) was reorged out of chain
+        // while the wallet was shutdown, change tx status to UNCONFIRMED
+        // and reset block height, hash, and index. ABANDONED tx don't have
+        // associated blocks and don't need to be updated. The case where a
+        // transaction was reorged out while online and then reconfirmed
+        // while offline is covered by the rescan logic.
+        if (!chain.findBlock(hash, FoundBlock().inActiveChain(active).height(height)) || !active) {
+            state = TxStateInactive{};
+        }
+    };
+    if (auto* conf = state<TxStateConfirmed>()) {
+        lookup_block(conf->confirmed_block_hash, conf->confirmed_block_height, m_state);
+    } else if (auto* conf = state<TxStateConflicted>()) {
+        lookup_block(conf->conflicting_block_hash, conf->conflicting_block_height, m_state);
+    }
+}
+
 void CWalletTx::CopyFrom(const CWalletTx& _tx)
 {
     *this = _tx;

src/wallet/transaction.h

@@ -22,6 +22,10 @@
 #include <variant>
 #include <vector>
 
+namespace interfaces {
+class Chain;
+} // namespace interfaces
+
 namespace wallet {
 //! State of transaction confirmed in a block.
 struct TxStateConfirmed {
@@ -326,6 +330,10 @@ public:
     template<typename T> const T* state() const { return std::get_if<T>(&m_state); }
     template<typename T> T* state() { return std::get_if<T>(&m_state); }
 
+    //! Update transaction state when attaching to a chain, filling in heights
+    //! of conflicted and confirmed blocks
+    void updateState(interfaces::Chain& chain);
+
     bool isAbandoned() const { return state<TxStateInactive>() && state<TxStateInactive>()->abandoned; }
     bool isConflicted() const { return state<TxStateConflicted>(); }
     bool isInactive() const { return state<TxStateInactive>(); }

src/wallet/wallet.cpp

@@ -1184,23 +1184,7 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx
     // If wallet doesn't have a chain (e.g when using bitcoin-wallet tool),
     // don't bother to update txn.
     if (HaveChain()) {
-        bool active;
-        auto lookup_block = [&](const uint256& hash, int& height, TxState& state) {
-            // If tx block (or conflicting block) was reorged out of chain
-            // while the wallet was shutdown, change tx status to UNCONFIRMED
-            // and reset block height, hash, and index. ABANDONED tx don't have
-            // associated blocks and don't need to be updated. The case where a
-            // transaction was reorged out while online and then reconfirmed
-            // while offline is covered by the rescan logic.
-            if (!chain().findBlock(hash, FoundBlock().inActiveChain(active).height(height)) || !active) {
-                state = TxStateInactive{};
-            }
-        };
-        if (auto* conf = wtx.state<TxStateConfirmed>()) {
-            lookup_block(conf->confirmed_block_hash, conf->confirmed_block_height, wtx.m_state);
-        } else if (auto* conf = wtx.state<TxStateConflicted>()) {
-            lookup_block(conf->conflicting_block_hash, conf->conflicting_block_height, wtx.m_state);
-        }
+      wtx.updateState(chain());
     }
     if (/* insertion took place */ ins.second) {
         wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
@@ -3319,8 +3303,10 @@ int CWallet::GetTxDepthInMainChain(const CWalletTx& wtx) const
 {
     AssertLockHeld(cs_wallet);
     if (auto* conf = wtx.state<TxStateConfirmed>()) {
+        assert(conf->confirmed_block_height >= 0);
         return GetLastBlockHeight() - conf->confirmed_block_height + 1;
     } else if (auto* conf = wtx.state<TxStateConflicted>()) {
+        assert(conf->conflicting_block_height >= 0);
         return -1 * (GetLastBlockHeight() - conf->conflicting_block_height + 1);
     } else {
         return 0;

src/wallet/wallet.h

@@ -503,6 +503,13 @@ public:
      * <0  : conflicts with a transaction this deep in the blockchain
      *  0  : in memory pool, waiting to be included in a block
      * >=1 : this many blocks deep in the main chain
+     *
+     * Preconditions: it is only valid to call this function when the wallet is
+     * online and the block index is loaded. So this cannot be called by
+     * bitcoin-wallet tool code or by wallet migration code. If this is called
+     * without the wallet being online, it won't be able able to determine the
+     * the height of the last block processed, or the heights of blocks
+     * referenced in transaction, and might cause assert failures.
      */
     int GetTxDepthInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     bool IsTxInMainChain(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet)