tests, bench: Fix issue with CWallet::LoadWallet() being called in the wrong places #29055

1. 
   achow101 commented at 8:24 pm on December 11, 2023: member

   CWallet::LoadWallet() expects to be called after a CWallet is constructed, but before any of its member functions called. Doing so invalidates pointers which causes issues with some PRs and branches that I am working on. This was being used incorrectly in a few tests and benchmarks, resulting in segfaults.

   As a precaution for this kind of issue in the future, I’ve also added a few asserts to LoadWallet() so that developers will notice when it is used incorrectly.

   As similar issue was fixed in #27666

2. 
   DrahtBot commented at 8:24 pm on December 11, 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	furszy, S3RK

   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:

   • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
   • #27286 (wallet: Keep track of the wallet’s own transaction outputs in memory 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.

3. DrahtBot renamed this:
   tests, bench: Fix issue with CWalelt::LoadWallet() being called in the wrong places
   tests, bench: Fix issue with CWalelt::LoadWallet() being called in the wrong places
   on Dec 11, 2023
4. achow101 renamed this:
   tests, bench: Fix issue with CWalelt::LoadWallet() being called in the wrong places
   tests, bench: Fix issue with CWallet::LoadWallet() being called in the wrong places
   on Dec 11, 2023
5. tests, bench: Remove incorrect LoadWallet() calls

   LoadWallet() must only be called immediately after a CWallet is
   constructed, or not at all. Doing so after any other CWallet member
   functions have been called may cause pointers and other objects
   setup by other those functions to become invalidated.
   
   Since these tests and benchmarks are using completely new wallets with
   mock databases, it's not necessary to call LoadWallet() anyways, so
   these can be dropped.

   fb0b6ca4e5
6. wallet: Assert that the wallet is not initialized in LoadWallet

   LoadWallet() cannot be run after the wallet has been initialized. So
   assert that to avoid making this mistake in the future.

   bd7f5d33e3
7. achow101 force-pushed on Dec 11, 2023
8. DrahtBot added the label CI failed on Dec 11, 2023
9. furszy approved
10. 
    furszy commented at 10:45 pm on December 11, 2023: member

    ACK bd7f5d33

    For a follow-up, we should make CWallet::LoadWallet() private and replace all those calls for CWallet::Create().

11. DrahtBot removed the label CI failed on Dec 12, 2023
12. 
    S3RK commented at 8:18 am on December 12, 2023: contributor

    ACK bd7f5d33e3b9e01cd600afe1682b7afa935f68a3
13. fanquake merged this on Dec 12, 2023
14. fanquake closed this on Dec 12, 2023

    ---

Contributors
achow101 DrahtBot furszy S3RK