wallet, bench: Move commonly used functions to their own file and fix a bug #27666
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
src/bench/util/wallet_common.cpp
Outdated
| NotifyWalletLoaded(context, wallet); | ||
| if (context.chain) { | ||
| wallet->postInitProcess(); | ||
| } |
There was a problem hiding this comment.
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?.
There was a problem hiding this comment.
Yes, I think they'll be no-op for the most part.
There was a problem hiding this comment.
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?
c116933 to
8fa0111
Compare
Good point, made that change. It required a few changes to the |
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.
This static address is usable for other wallet tests and benchmarks, so make it available to them.
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.
8fa0111 to
7379a54
Compare
| BOOST_CHECK_EQUAL(addtx_count, 2); | ||
| // Since mempool transactions are requested at the end of loading, there will | ||
| // be 2 additional AddToWallet calls, one from the previous test, and a duplicate for mempool_tx | ||
| BOOST_CHECK_EQUAL(addtx_count, 2 + 2); |
There was a problem hiding this comment.
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 looks like you ACK'd the wrong commit here. |
|
oops, fixed. That was the one from the comment. Thanks @fanquake. |
|
utACK 7379a54 |
…heir own file and fix a bug 7379a54 bench: Remove incorrect LoadWallet call in WalletBalance (Andrew Chow) 846b2fe tests: Move ADDRESS_BCRT1_UNSPENDABLE to wallet/test/util.h (Andrew Chow) c61d3f0 tests, bench: Consolidate {Test,Bench}Un/LoadWallet helper (Andrew Chow) Pull request description: 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. ACKs for top commit: Sjors: utACK 7379a54 furszy: ACK 7379a54 Tree-SHA512: 47773887a16c69ac7121c699d3446a8c399bd792a6a31714998b7b7a19fea179c6d3b29cb898b04397b2962c1b4120d57009352b8460b8283e188d4cb480c9ba
…g called in the wrong places bd7f5d3 wallet: Assert that the wallet is not initialized in LoadWallet (Andrew Chow) fb0b6ca tests, bench: Remove incorrect LoadWallet() calls (Andrew Chow) Pull request description: `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 ACKs for top commit: S3RK: ACK bd7f5d3 furszy: ACK bd7f5d3 Tree-SHA512: 7664f12b8452994e7fc4d7d4f77697fb5f75edb0dba95ba99a4a23ec03d5b8e0ecbdcb7635547a0e8b4f89f708f98dcb5d039df0559e24b1ae411ed630e16e14
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
LoadWalletin 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.