Avoid copying CWalletTx in LoadToWallet

Summary: The change in walletdb.cpp is easier to review ignoring whitespace. This change is need to get rid of CWalletTx copy constructor. This is a backport of Core [[bitcoin/bitcoin#9381 | PR9381]] [3/5] bitcoin/bitcoin@65b9d8f Depends on D9075 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9076
Bitcoin-ABC · Jan 27, 2021 · 6bde49b · 6bde49b
1 parent b740449
commit 6bde49b
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 41 deletions.
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -949,33 +949,36 @@ CWalletTx *CWallet::AddToWallet(CTransactionRef tx,
     return &wtx;
 }
 
-void CWallet::LoadToWallet(CWalletTx &wtxIn) {
+bool CWallet::LoadToWallet(const TxId &txid, const UpdateWalletTxFn &fill_wtx) {
+    const auto &ins =
+        mapWallet.emplace(std::piecewise_construct, std::forward_as_tuple(txid),
+                          std::forward_as_tuple(this, nullptr));
+    CWalletTx &wtx = ins.first->second;
+    if (!fill_wtx(wtx, ins.second)) {
+        return false;
+    }
     // If wallet doesn't have a chain (e.g wallet-tool), don't bother to update
     // txn.
     if (HaveChain()) {
         std::optional<int> block_height =
-            chain().getBlockHeight(wtxIn.m_confirm.hashBlock);
+            chain().getBlockHeight(wtx.m_confirm.hashBlock);
         if (block_height) {
             // Update cached block height variable since it not stored in the
             // serialized transaction.
-            wtxIn.m_confirm.block_height = *block_height;
-        } else if (wtxIn.isConflicted() || wtxIn.isConfirmed()) {
+            wtx.m_confirm.block_height = *block_height;
+        } else if (wtx.isConflicted() || wtx.isConfirmed()) {
             // 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.
-            wtxIn.setUnconfirmed();
-            wtxIn.m_confirm.hashBlock = BlockHash();
-            wtxIn.m_confirm.block_height = 0;
-            wtxIn.m_confirm.nIndex = 0;
+            wtx.setUnconfirmed();
+            wtx.m_confirm.hashBlock = BlockHash();
+            wtx.m_confirm.block_height = 0;
+            wtx.m_confirm.nIndex = 0;
         }
     }
-    const TxId &txid = wtxIn.GetId();
-    const auto &ins = mapWallet.emplace(txid, wtxIn);
-    CWalletTx &wtx = ins.first->second;
-    wtx.BindWallet(this);
     if (/* insertion took place */ ins.second) {
         wtx.m_it_wtxOrdered =
             wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
@@ -991,6 +994,7 @@ void CWallet::LoadToWallet(CWalletTx &wtxIn) {
             }
         }
     }
+    return true;
 }
 
 bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef &ptx,

diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -1059,7 +1059,8 @@ class CWallet final : public WalletStorage,
                            const CWalletTx::Confirmation &confirm,
                            const UpdateWalletTxFn &update_wtx = nullptr,
                            bool fFlushOnClose = true);
-    void LoadToWallet(CWalletTx &wtxIn) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    bool LoadToWallet(const TxId &txid, const UpdateWalletTxFn &fill_wtx)
+        EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
     void transactionAddedToMempool(const CTransactionRef &tx) override;
     void blockConnected(const CBlock &block, int height) override;
     void blockDisconnected(const CBlock &block, int height) override;

diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp
@@ -310,38 +310,45 @@ static bool ReadKeyValue(CWallet *pwallet, CDataStream &ssKey,
         } else if (strType == DBKeys::TX) {
             TxId txid;
             ssKey >> txid;
-            CWalletTx wtx(nullptr /* pwallet */, MakeTransactionRef());
-            ssValue >> wtx;
-            if (wtx.GetId() != txid) {
-                return false;
-            }
+            // LoadToWallet call below creates a new CWalletTx that fill_wtx
+            // callback fills with transaction metadata.
+            auto fill_wtx = [&](CWalletTx &wtx, bool new_tx) {
+                assert(new_tx);
+                ssValue >> wtx;
+                if (wtx.GetId() != txid) {
+                    return false;
+                }
 
-            // Undo serialize changes in 31600
-            if (31404 <= wtx.fTimeReceivedIsTxTime &&
-                wtx.fTimeReceivedIsTxTime <= 31703) {
-                if (!ssValue.empty()) {
-                    char fTmp;
-                    char fUnused;
-                    std::string unused_string;
-                    ssValue >> fTmp >> fUnused >> unused_string;
-                    strErr = strprintf("LoadWallet() upgrading tx ver=%d %d %s",
-                                       wtx.fTimeReceivedIsTxTime, fTmp,
-                                       txid.ToString());
-                    wtx.fTimeReceivedIsTxTime = fTmp;
-                } else {
-                    strErr =
-                        strprintf("LoadWallet() repairing tx ver=%d %s",
-                                  wtx.fTimeReceivedIsTxTime, txid.ToString());
-                    wtx.fTimeReceivedIsTxTime = 0;
+                // Undo serialize changes in 31600
+                if (31404 <= wtx.fTimeReceivedIsTxTime &&
+                    wtx.fTimeReceivedIsTxTime <= 31703) {
+                    if (!ssValue.empty()) {
+                        char fTmp;
+                        char fUnused;
+                        std::string unused_string;
+                        ssValue >> fTmp >> fUnused >> unused_string;
+                        strErr = strprintf(
+                            "LoadWallet() upgrading tx ver=%d %d %s",
+                            wtx.fTimeReceivedIsTxTime, fTmp, txid.ToString());
+                        wtx.fTimeReceivedIsTxTime = fTmp;
+                    } else {
+                        strErr = strprintf(
+                            "LoadWallet() repairing tx ver=%d %s",
+                            wtx.fTimeReceivedIsTxTime, txid.ToString());
+                        wtx.fTimeReceivedIsTxTime = 0;
+                    }
+                    wss.vWalletUpgrade.push_back(txid);
                 }
-                wss.vWalletUpgrade.push_back(txid);
-            }
 
-            if (wtx.nOrderPos == -1) {
-                wss.fAnyUnordered = true;
-            }
+                if (wtx.nOrderPos == -1) {
+                    wss.fAnyUnordered = true;
+                }
 
-            pwallet->LoadToWallet(wtx);
+                return true;
+            };
+            if (!pwallet->LoadToWallet(txid, fill_wtx)) {
+                return false;
+            }
         } else if (strType == DBKeys::WATCHS) {
             wss.nWatchKeys++;
             CScript script;