wallet, bench: Move commonly used functions to their own file and fix a bug

achow101 commented at 8:41 pm on May 15, 2023: member

I have a few PRs and branches that use these two commits, probably makes sense to split them into a separate PR to be merged sooner.

The first commit contains some things that end up being commonly used in new wallet benchmarks. These are moved into wallet_common.{h/cpp}.

The second commit contains a bugfix for the wallet_balance benchmark where it calls LoadWallet in the wrong place. It’s unnecessary to call that function in this benchmark. Although this does not cause any issues currently, it ends up causing issues in some PRs and branches that I’m working on.

DrahtBot commented at 8:41 pm on May 15, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, Sjors
Concept ACK	fanquake

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

Conflicts

No conflicts as of last run.

fanquake requested review from furszy on May 16, 2023

in src/bench/util/wallet_common.cpp:17 in c981c6eec3 outdated

12+    std::vector<bilingual_str> warnings;
13+    auto wallet = CWallet::Create(context, "", std::move(database), create_flags, error, warnings);
14+    NotifyWalletLoaded(context, wallet);
15+    if (context.chain) {
16+        wallet->postInitProcess();
17+    }

furszy commented at 6:44 pm on May 22, 2023:

little topic: it seems odd to re-use this two calls in other bench contexts.

NotifyWalletLoaded is looping over the wallet load callbacks, and postInitProcess syncs the wallet with the mempool, so guess that both will be no-op in most cases?.

achow101 commented at 9:17 pm on May 22, 2023:

Yes, I think they’ll be no-op for the most part.

in src/bench/util/wallet_common.h:14 in c981c6eec3 outdated

 9+#include <wallet/wallet.h>
10+
11+using wallet::CWallet;
12+using wallet::DatabaseOptions;
13+using wallet::WalletContext;
14+using wallet::WalletDatabase;

furszy commented at 11:34 pm on May 24, 2023:

tiny nit: could wrap the functions around a “namespace wallet {}” instead.

achow101 commented at 6:40 pm on May 25, 2023:

Dropped wallet_common.h, but still ended up doing this in wallet_loading and wallet_balance

furszy commented at 11:41 pm on May 24, 2023: member

Code ACK, small last topic:

Seems that BenchLoadWallet and BenchUnloadWallet are similar to TestLoadWallet and TestUnloadWallet from wallet_tests.cpp . The only diff seems to be in the db.

Maybe instead of creating a new file, we could move the bench functions to wallet/test/util.h and wallet/test/util.cpp so they are used equally for tests and benchmarks?

achow101 force-pushed on May 25, 2023

achow101 commented at 6:39 pm on May 25, 2023: member

Maybe instead of creating a new file, we could move the bench functions to wallet/test/util.h and wallet/test/util.cpp so they are used equally for tests and benchmarks?

Good point, made that change.

It required a few changes to the CreateWallet test since that wasn’t expecting postInitProcess to be run, but should be straightforward to review.

tests, bench: Consolidate {Test,Bench}Un/LoadWallet helper

The wallet tests and benchmarks both had helper functions for loading
and unloading the wallet for the test that were almost identical.
These functions are consolidated and reused.

c61d3f02f5

tests: Move ADDRESS_BCRT1_UNSPENDABLE to wallet/test/util.h

This static address is usable for other wallet tests and benchmarks, so
make it available to them.

846b2fe67e

bench: Remove incorrect LoadWallet call in WalletBalance

The WalletBalance benchmarks would incorrectly call LoadWallet after the
wallet has been setup. LoadWallet expects to be the first thing that is
called and for the CWallet to be in a fresh state. When it is not, it
results in bogus pointers which can cause segfaults during this
benchmark.

7379a54ec4

achow101 force-pushed on May 25, 2023

in src/wallet/test/wallet_tests.cpp:871 in c61d3f02f5 outdated

865@@ -885,7 +866,9 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
866             SyncWithValidationInterfaceQueue();
867         });
868     wallet = TestLoadWallet(context);
869-    BOOST_CHECK_EQUAL(addtx_count, 2);
870+    // Since mempool transactions are requested at the end of loading, there will
871+    // be 2 additional AddToWallet calls, one from the previous test, and a duplicate for mempool_tx
872+    BOOST_CHECK_EQUAL(addtx_count, 2 + 2);

furszy commented at 1:24 pm on May 30, 2023:

Not for this PR as it touches the wallet internals but the duplicated AddToWallet calls shouldn’t be needed if the wallet wtx state is equal to the one in the event (e.g. TxStateInMempool).

furszy approved

furszy commented at 1:24 pm on May 30, 2023: member

ACK 7379a54

fanquake requested review from Sjors on May 30, 2023

fanquake requested review from josibake on May 30, 2023

fanquake commented at 1:48 pm on May 30, 2023: member

@furszy looks like you ACK’d the wrong commit here.

furszy commented at 1:59 pm on May 30, 2023: member

oops, fixed. That was the one from the comment. Thanks @fanquake.

Sjors commented at 2:57 pm on May 30, 2023: member

utACK 7379a54ec416c8c0a029cc41835a23d42cb6d800

DrahtBot removed review request from Sjors on May 30, 2023

fanquake merged this on May 30, 2023

fanquake closed this on May 30, 2023

sidhujag referenced this in commit ebf0dc78dd on May 30, 2023

fanquake referenced this in commit 60f677375e on Dec 12, 2023

bitcoin locked this on May 29, 2024

wallet, bench: Move commonly used functions to their own file and fix a bug #27666

Reviews

Conflicts