wallet: Reload watchonly and solvables wallets after migration #28609

achow101 commented at 10:37 PM on October 6, 2023: member

Some incomplete/incorrect state as a result of migration can be mitigated/cleaned up by simply restarting the migrated wallets. We already do this for a wallet when it is migrated, but we do not for the new watchonly and solvables wallets that may be created. This PR introduces this behavior, in addition to creating those wallets initially without an attached chain.

While implementing this, I noticed that not all CWalletTx metadata was being copied over to the watchonly wallet and so some data, such as time received, was being lost. This PR fixes this as a side effect of not having a chain attached to the watchonly wallet. A test has also been added.

DrahtBot commented at 10:37 PM on October 6, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ishaanam, ryanofsky, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28610 (wallet: Migrate entire address book entries to watchonly and solvables too by achow101)
#28546 (bugfix: watchonly wallets created after migration have incorrect height values by ryanofsky)
#28264 (test: refactor: support sending funds with outpoint result by theStack)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Oct 6, 2023

in src/wallet/wallet.cpp:3933 in d73ecbd2d8 outdated

3929 |                      }
3930 | +                    CWalletTx& ins_wtx = (*ret.first).second;
3931 | +                    ins_wtx.CloneFrom(*wtx);
3932 | +                    ins_wtx.nOrderPos = IncOrderPosNext(watchonly_batch.get());
3933 | +                    data.watchonly_wallet->AddToSpends(ins_wtx);
3934 | +                    watchonly_batch->WriteTx(ins_wtx);

furszy commented at 1:54 PM on October 9, 2023:

In d73ecbd2d8:

What is we use LoadToWallet() instead?. It will also set m_it_wtxOrdered, which is not set atm. And also update the state if needed. Something like:

// Add to watchonly wallet
const uint256& hash = wtx->GetHash();
const CWalletTx& to_copy_wtx = *wtx;
if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) {
    if (!new_tx) return false;
    ins_wtx.SetTx(to_copy_wtx.tx);
    ins_wtx.CloneFrom(to_copy_wtx);
    ins_wtx.nOrderPos = data.watchonly_wallet->IncOrderPosNext(watchonly_batch.get());
    return true;
})) {
    error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex());
    return false;
}
watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash));
// Mark as to remove from this wallet
txids_to_delete.push_back(hash);
continue;

achow101 commented at 6:52 PM on October 10, 2023:

Done as suggested.

in src/wallet/wallet.cpp:3931 in d73ecbd2d8 outdated

3927 |                          error = _("Error: Could not add watchonly tx to watchonly wallet");
3928 |                          return false;
3929 |                      }
3930 | +                    CWalletTx& ins_wtx = (*ret.first).second;
3931 | +                    ins_wtx.CloneFrom(*wtx);
3932 | +                    ins_wtx.nOrderPos = IncOrderPosNext(watchonly_batch.get());

furszy commented at 2:41 PM on October 9, 2023:

In d73ecbd2:

Bug here, IncOrderPosNext() is a wallet method. So, the main wallet order pos is being increased, instead of the watch-only wallet one. Need to be:

data.watchonly_wallet->IncOrderPosNext(watchonly_batch.get())

achow101 commented at 6:52 PM on October 10, 2023:

Done as suggested.

in src/wallet/transaction.cpp:28 in d73ecbd2d8 outdated

  23 | @@ -24,4 +24,16 @@ int64_t CWalletTx::GetTxTime() const
  24 |      int64_t n = nTimeSmart;
  25 |      return n ? n : nTimeReceived;
  26 |  }
  27 | +
  28 | +void CWalletTx::CloneFrom(const CWalletTx& tx)

furszy commented at 2:53 PM on October 9, 2023:

In d73ecbd2d:

nit: the 'tx' naming shadows the CWalletTx::tx member. Maybe call it _tx.

achow101 commented at 6:52 PM on October 10, 2023:

Done as suggested.

in src/wallet/wallet.cpp:4274 in d550863e5b outdated

4259 | +        if (res.solvables_wallet) {
4260 | +            assert(res.solvables_wallet.use_count() == 1);
4261 | +            std::string solvables_wallet_name = res.solvables_wallet->GetName();
4262 | +            res.solvables_wallet.reset();
4263 | +            res.solvables_wallet = LoadWallet(context, solvables_wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
4264 | +        }

furszy commented at 3:18 PM on October 9, 2023:

In d550863e5:

As LoadWallet() can return a nullptr if loading fails. Would be good to cover this possibility too.

achow101 commented at 6:53 PM on October 10, 2023:

I've added handling to cleanup when reloading fails.

achow101 force-pushed on Oct 10, 2023

DrahtBot added the label Needs rebase on Oct 11, 2023

achow101 added this to the milestone 26.0 on Oct 11, 2023

achow101 force-pushed on Oct 11, 2023

DrahtBot removed the label Needs rebase on Oct 11, 2023

in src/wallet/transaction.cpp:36 in 4ccec8b371 outdated

  31 | +    vOrderForm = _tx.vOrderForm;
  32 | +    fTimeReceivedIsTxTime = _tx.fTimeReceivedIsTxTime;
  33 | +    nTimeReceived = _tx.nTimeReceived;
  34 | +    nTimeSmart = _tx.nTimeSmart;
  35 | +    fFromMe = _tx.fFromMe;
  36 | +    nOrderPos = _tx.nOrderPos;

furszy commented at 7:39 PM on October 11, 2023:

In 4ccec8b371:

Doesn't make much sense to copy nOrderPos. The field is strictly connected to the parent wallet. Depending on what the wallet is watching, the tx order position could be higher or lower.

achow101 commented at 6:18 PM on October 16, 2023:

I'd prefer to keep this function generic so it should copy everything even if we will overwrite stuff in actual usage.

in src/wallet/wallet.cpp:4253 in 6eb3df7c0b outdated

4249 | +    std::set<fs::path> wallet_dirs;
4250 |      if (success) {
4251 | -        // Migration successful, unload the wallet locally, then reload it.
4252 | +        // Migration successful, unload all wallets locally, then reload them.
4253 |          assert(local_wallet.use_count() == 1);
4254 | +        wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path());

furszy commented at 7:54 PM on October 11, 2023:

In https://github.com/bitcoin/bitcoin/commit/6eb3df7c0bbbad0873f5600d3869ac33bad6ed5e:

Why this line?. Isn't the local wallet datadir added at line 4282?

achow101 commented at 7:23 PM on October 16, 2023:

In case the wallet fails to reload, we can still cleanup at the end.

in src/wallet/wallet.cpp:4262 in 6eb3df7c0b outdated

4259 | +        success = res.wallet != nullptr;
4260 | +    }
4261 | +    if (success && res.watchonly_wallet) {
4262 | +        assert(res.watchonly_wallet.use_count() == 1);
4263 | +        std::string watchonly_wallet_name = res.watchonly_wallet->GetName();
4264 | +        wallet_dirs.insert(fs::PathFromString(res.watchonly_wallet->GetDatabase().Filename()).parent_path());

furszy commented at 8:00 PM on October 11, 2023:

In https://github.com/bitcoin/bitcoin/commit/6eb3df7c0bbbad0873f5600d3869ac33bad6ed5e:

Why this line?. The watch-only wallet datadir is added at line 4295. Same for the solvable one.

achow101 commented at 7:23 PM on October 16, 2023:

If the wallet fails to reload, we can still know to clean it up.

DrahtBot added the label Needs rebase on Oct 16, 2023

achow101 force-pushed on Oct 16, 2023

DrahtBot removed the label Needs rebase on Oct 16, 2023

DrahtBot added the label CI failed on Oct 16, 2023

in src/wallet/wallet.cpp:4280 in 177902519a outdated

4272 | +        wallet_dirs.insert(fs::PathFromString(res.solvables_wallet->GetDatabase().Filename()).parent_path());
4273 | +        res.solvables_wallet.reset();
4274 | +        res.solvables_wallet = LoadWallet(context, solvables_wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
4275 | +        success = res.solvables_wallet != nullptr;
4276 | +    }
4277 | +    if (!success) {

furszy commented at 3:17 AM on October 17, 2023:

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
--- a/src/wallet/wallet.cpp	(revision 014231c3fd7bc479784cb187560d417118000e61)
+++ b/src/wallet/wallet.cpp	(date 1697512277377)
@@ -4249,28 +4249,27 @@
     std::set<fs::path> wallet_dirs;
     if (success) {
         // Migration successful, unload all wallets locally, then reload them.
-        assert(local_wallet.use_count() == 1);
-        wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path());
-        local_wallet.reset();
-        res.wallet = LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
+        const auto& reload_wallet = [&](std::shared_ptr<CWallet>& to_reload) {
+            assert(to_reload.use_count() == 1);
+            std::string name = to_reload->GetName();
+            wallet_dirs.insert(fs::PathFromString(to_reload->GetDatabase().Filename()).parent_path());
+            to_reload.reset();
+            to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
+            return to_reload != nullptr;
+        };
+        
+        // Reload main wallet
+        success = reload_wallet(local_wallet);
+        res.wallet = local_wallet;
         res.wallet_name = wallet_name;
-        success = res.wallet != nullptr;
-    }
-    if (success && res.watchonly_wallet) {
-        assert(res.watchonly_wallet.use_count() == 1);
-        std::string watchonly_wallet_name = res.watchonly_wallet->GetName();
-        wallet_dirs.insert(fs::PathFromString(res.watchonly_wallet->GetDatabase().Filename()).parent_path());
-        res.watchonly_wallet.reset();
-        res.watchonly_wallet = LoadWallet(context, watchonly_wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
-        success = res.watchonly_wallet != nullptr;
-    }
-    if (success && res.solvables_wallet) {
-        assert(res.solvables_wallet.use_count() == 1);
-        std::string solvables_wallet_name = res.solvables_wallet->GetName();
-        wallet_dirs.insert(fs::PathFromString(res.solvables_wallet->GetDatabase().Filename()).parent_path());
-        res.solvables_wallet.reset();
-        res.solvables_wallet = LoadWallet(context, solvables_wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
-        success = res.solvables_wallet != nullptr;
+        // Reload watch-only wallet
+        if (success && res.watchonly_wallet) {
+            success = reload_wallet(res.watchonly_wallet);
+        }
+        // Reload solvables wallet
+        if (success && res.solvables_wallet) {
+            success = reload_wallet(res.solvables_wallet);
+        }
     }
     if (!success) {
         // Migration failed, cleanup

achow101 commented at 5:32 PM on October 17, 2023:

Done as suggested

DrahtBot commented at 7:48 AM on October 17, 2023: contributor

test/functional/wallet_migration.py:863:15: E275 missing whitespace after keyword
test/functional/wallet_migration.py:864:15: E275 missing whitespace after keyword
test/functional/wallet_migration.py:865:15: E275 missing whitespace after keyword
test/functional/wallet_migration.py:867:15: E275 missing whitespace after keyword
test/functional/wallet_migration.py:869:15: E275 missing whitespace after keyword

in test/functional/wallet_migration.py:878 in bdc828cfa0 outdated

 862 | +
 863 | +        wallet.unloadwallet()
 864 | +        with open(self.nodes[0].wallets_path / "failed" / "wallet.dat", "rb") as f:
 865 | +            data = f.read(16)
 866 | +            _, _, magic = struct.unpack("QII", data)
 867 | +            assert_equal(magic, BTREE_MAGIC)

furszy commented at 1:42 PM on October 17, 2023:

In bdc828cfa0c:

nit: I'm assuming this checks if the database is still bdb. Which, if that's the case, a comment indicating it wouldn't hurt. However, isn't the checksum validation performed earlier already covering this scenario?

achow101 commented at 5:32 PM on October 17, 2023:

Added a comment. This is a check for the original wallet, not the solvables wallet which the checksum covers.

furszy commented at 1:56 PM on October 17, 2023: member

Code review ACK 014231c3

Feel free to ignore the nits.

achow101 force-pushed on Oct 17, 2023

test: Check that a failed wallet migration is cleaned up 9af87cf348

achow101 force-pushed on Oct 17, 2023

DrahtBot removed the label CI failed on Oct 17, 2023

furszy commented at 8:36 PM on October 17, 2023: member

diff ACK e2dfe76

maflcko requested review from Sjors on Oct 19, 2023

maflcko requested review from ryanofsky on Oct 19, 2023

in src/wallet/transaction.cpp:37 in cc4fa13bcf outdated

  32 | +    fTimeReceivedIsTxTime = _tx.fTimeReceivedIsTxTime;
  33 | +    nTimeReceived = _tx.nTimeReceived;
  34 | +    nTimeSmart = _tx.nTimeSmart;
  35 | +    fFromMe = _tx.fFromMe;
  36 | +    nOrderPos = _tx.nOrderPos;
  37 | +    m_state = _tx.m_state;

ryanofsky commented at 5:50 PM on October 19, 2023:

In commit "wallet: Copy all tx metadata to watchonly wallet" (cc4fa13bcfa0102ab6366f67a275f583baa0e6ff)

I don't think this needs to manually copy each class member. It should just possible to make the copy constructor private instead of deleted, and have the public CloneFrom function just do *this = tx.

Would also suggest renaming CloneFrom to CopyFrom. AFAIK clone is rust term for what c++ calls copying.

(This also is a potential use-case for a DISABLE_IMPLICIT_COPIES macro suggested #24641 (comment) which would automatically generate Copy and CopyFrom methods)

achow101 commented at 10:04 PM on October 19, 2023:

Done as suggested.

furszy commented at 10:56 PM on October 20, 2023:

This isn't a blocking comment, just something that we should be aware of.

Each CWalletTx object contains a const_interator pointing to the parent wallet wtxOrdered map.. the field is called m_it_wtxOrdered. So, by copying the entire wtx object, we should be careful to update the iterator when the transaction is added to another wallet.

In this case, this isn't a problem because the copied wtx is added to the wallet calling LoadToWallet() which updates the iterator. But this doesn't make the code less fragile for any future usages.

I think we can remove this const iterator and also enforce order positioning uniqueness and sequentiality in a follow-up (already started here https://github.com/furszy/bitcoin-core/commits/2023_wallet_txs_order).

in src/wallet/wallet.cpp:3929 in cc4fa13bcf outdated

3926 | +                    const CWalletTx& to_copy_wtx = *wtx;
3927 | +                    if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) {
3928 | +                        if (!new_tx) return false;
3929 | +                        ins_wtx.SetTx(to_copy_wtx.tx);
3930 | +                        ins_wtx.CloneFrom(to_copy_wtx);
3931 | +                        ins_wtx.nOrderPos = data.watchonly_wallet->IncOrderPosNext(watchonly_batch.get());

ryanofsky commented at 6:02 PM on October 19, 2023:

In commit "wallet: Copy all tx metadata to watchonly wallet" (cc4fa13bcfa0102ab6366f67a275f583baa0e6ff)

Can you add a comment explaining this line? Is it just done just to avoid gaps in numbers? Is it necessary?

Also, it seems like writing the updating the ORDERPOSTNEXT field each time a transaction is copied is potentially slow, and it might be better just to copy the existing ORDERPOSTNEXT value directly from the old wallet to the new wallet once.

achow101 commented at 10:07 PM on October 19, 2023:

Good point. I've changed it to set and write nOrderPosNext before copying the txs.

in src/wallet/wallet.cpp:3929 in cc4fa13bcf outdated

3919 | @@ -3916,12 +3920,21 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
3920 |                  LOCK(data.watchonly_wallet->cs_wallet);
3921 |                  if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) {
3922 |                      // Add to watchonly wallet
3923 | -                    if (!data.watchonly_wallet->AddToWallet(wtx->tx, wtx->m_state)) {
3924 | -                        error = _("Error: Could not add watchonly tx to watchonly wallet");
3925 | +                    const uint256& hash = wtx->GetHash();
3926 | +                    const CWalletTx& to_copy_wtx = *wtx;
3927 | +                    if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) {

ryanofsky commented at 6:05 PM on October 19, 2023:

In commit "wallet: Copy all tx metadata to watchonly wallet" (cc4fa13bcfa0102ab6366f67a275f583baa0e6ff)

May be helpful to have a code comment saying that LoadToWallet is called instead of AddToWallet in order to preserve CWalletTx metadata.

The code looked simpler with AddToWallet so you wonder why it is jumping though all these hoops.

achow101 commented at 10:07 PM on October 19, 2023:

Added a comment.

in src/wallet/wallet.cpp:4264 in b57218e4f4 outdated

4256 | +        const auto& reload_wallet = [&](std::shared_ptr<CWallet>& to_reload) {
4257 | +            assert(to_reload.use_count() == 1);
4258 | +            std::string name = to_reload->GetName();
4259 | +            wallet_dirs.insert(fs::PathFromString(to_reload->GetDatabase().Filename()).parent_path());
4260 | +            to_reload.reset();
4261 | +            to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings);

ryanofsky commented at 8:14 PM on October 19, 2023:

In commit "wallet: Reload watchonly and solvables wallets after migration" (b57218e4f4543619822c16d54881104ae15a9190)

Just a thought, not for this PR, but maybe it would be good to load the new wallet on startup if the previous wallet loaded on startup.

Also it would be good to improve error reporting here. It seems like errors/warnings just get discarded which might make problems hard to debug. Though I guess we don't expect failure loading a newly creating wallet.

in src/wallet/wallet.cpp:4256 in b57218e4f4 outdated

4244 | @@ -4229,47 +4245,66 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4245 |          success = DoMigration(*local_wallet, context, error, res);
4246 |      }
4247 |  
4248 | +    // In case of reloading failure, we need to remember the wallet dirs to remove
4249 | +    std::set<fs::path> wallet_dirs;

ryanofsky commented at 8:21 PM on October 19, 2023:

In commit "wallet: Reload watchonly and solvables wallets after migration" (b57218e4f4543619822c16d54881104ae15a9190)

Not clear on why the vector is changing to a set here. Is it because duplicate directories might be added so using a set is needed to prevent the same directories from being deleted twice? It seems fine to use a set, but if there's a reason a set needs to be used it would be helpful if that could be mentioned / explained in a comment.

EDIT: Would suggest "// Set is used instead of a vector because it is populated with the same wallets twice, both before and after reloading. This ensures the set is complete even if one of the wallets fails to reload."

achow101 commented at 10:07 PM on October 19, 2023:

Added a comment as suggested.

in src/wallet/wallet.cpp:4315 in b57218e4f4 outdated

4322 |          }
4323 |  
4324 |          // Delete the wallet directories
4325 | -        for (fs::path& dir : wallet_dirs) {
4326 | +        for (const fs::path& dir : wallet_dirs) {
4327 |              fs::remove_all(dir);

ryanofsky commented at 8:46 PM on October 19, 2023:

In commit "wallet: Reload watchonly and solvables wallets after migration" (b57218e4f4543619822c16d54881104ae15a9190)

Not directly related to this PR, but this remove_all seems not great because if there were any non-wallet files in the wallet directory they will be recursively removed. I think it would be preferable to only delete files that we know about and leave files that we do not recognize alone.

This would also avoid the need to copy the backup file from the wallet directory to the wallet parent directory, and then copy it back in again when the wallet directory is recreated.

A lot of this logic seems pretty fragile and possible to simplify (Maybe furszy already has a PR to do it?)

furszy commented at 2:34 PM on October 21, 2023:

Not in a PR, but I do have something related in progress. Still, we should move forward with your suggestion; it is much more scoped than what I'm doing.

I'm rewriting the process, so the bdb->sqlite migration, the removal of "no longer ours" database records, and the backup creation steps are not needed. The goal is to leave the legacy wallet untouched for the entire migration process. Then, only if migration succeeds, execute the path rename. So, in case the user faces a crash or abortion during the process, the legacy wallet will open as if nothing has happened. Which is what the process lacks today; If it crashes right now, the user has to manually do the cleanup and previous wallet recovery.

This would indirectly solve the issue you mentioned because, in case of a failure, the process would only need to delete the newly created wallet directories and nothing else.

ryanofsky approved

ryanofsky commented at 9:16 PM on October 19, 2023: contributor

Code review ACK e2dfe76220fba0c8090605946187701a4397a758. This really seems like 3 unrelated changes smashed into one PR, but all the changes look good.

As far as I understand there is no test coverage to verify the fix in the third commit b57218e4f4543619822c16d54881104ae15a9190, but with the asserts in #28546 added, existing tests should fail. I'm happy to rebase #28546 on top of this after it is merged, though to avoid making this PR bigger.

achow101 force-pushed on Oct 19, 2023

wallet: Copy all tx metadata to watchonly wallet

When moving a tx to the watchonly wallet during migration, make sure
that all of the CWalletTx data follows it.

118f2d7d70

wallet: Reload watchonly and solvables wallets after migration

When migrating, create the watchonly and solvables wallets without a
context. Then unload and reload them after migration completes, as we do
for the actual wallet.

There is also additional handling for a failed reload.

d616d30ea5

test: Check tx metadata is migrated to watchonly 4814e4063e

achow101 force-pushed on Oct 19, 2023

achow101 commented at 10:14 PM on October 19, 2023: member

This really seems like 3 unrelated changes smashed into one PR

Two of the changes are actually required for the third.

The primary change here is to create the watchonly and solvables wallets without a chain context. This necessitates the second change of reloading at the end of migration. This also required changes to the cleanup, which prompted me to write a test for that.

It then turned out that AddToWallet requires a chain, and without one it segfaults. So this required changing the copying to use LoadToWallet. This has the side effect of copying the metadata, and when I realized that, I also wrote a test for it.

ishaanam commented at 8:45 PM on October 20, 2023: contributor

light code review ACK 4814e4063e674ad9b0a5c7e56059cd6a2bf9b764

This looks good so far, might review d616d30ea5fdfb897f8375ffd8b9f4536ae7835b a bit more later.

DrahtBot requested review from ryanofsky on Oct 20, 2023

DrahtBot requested review from furszy on Oct 20, 2023

ryanofsky approved

ryanofsky commented at 9:32 PM on October 20, 2023: contributor

Code review ACK 4814e4063e674ad9b0a5c7e56059cd6a2bf9b764. Just implemented the suggested orderpos, copyfrom, and path set comments since last review

furszy commented at 2:36 PM on October 21, 2023: member

ACK 4814e406

Open topic #28609 (review).

in src/wallet/transaction.h:330 in 118f2d7d70 outdated

 327 |      // Disable copying of CWalletTx objects to prevent bugs where instances get
 328 |      // copied in and out of the mapWallet map, and fields are updated in the
 329 |      // wrong copy.
 330 | -    CWalletTx(CWalletTx const &) = delete;
 331 | -    void operator=(CWalletTx const &x) = delete;
 332 | +    CWalletTx(const CWalletTx&) = default;

S3RK commented at 8:09 AM on October 23, 2023:

Should the comment above be updated?

achow101 commented at 2:34 PM on October 23, 2023:

I think it's still valid when these are private.

bitcoin deleted a comment on Oct 23, 2023

ryanofsky merged this on Oct 23, 2023

ryanofsky closed this on Oct 23, 2023

achow101 referenced this in commit 3da69c464f on Nov 7, 2023

Frank-GER referenced this in commit 5f98866bb8 on Nov 28, 2023

bitcoin locked this on Oct 22, 2024