Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge #6594: refactor: polish CoinJoin interface, use
WalletLoader
…
…in Dash wallet initialization code, introduce new RPC `coinjoin status` 68cec8d refactor: use `interfaces::WalletLoader` in Dash-specific wallet init (Kittywhiskers Van Gogh) c9c5275 refactor: get rid of `DashPostChainstateSetup()` (Kittywhiskers Van Gogh) 6502e54 refactor: use `CoinJoin::Loader` in `InitCoinJoinSettings` (Kittywhiskers Van Gogh) 0d8de73 refactor: make `WalletContext::coinjoin_loader` a pointer (Kittywhiskers Van Gogh) 0760ae0 fix: resolve undefined reference linking error in `bench_dash` (Kittywhiskers Van Gogh) 846dc67 refactor: initialize CoinJoin loader in `WalletInit::Construct()` (Kittywhiskers Van Gogh) b14e55f chore: fix `MakeWallet` argument list in dummy wallet (Kittywhiskers Van Gogh) b3ac5cf refactor: place `ArgsManager` before `interfaces::CoinJoin::Loader` (Kittywhiskers Van Gogh) 73671dd refactor: fold `InitCoinJoinSettings` calls into `CoinJoin::Loader` (Kittywhiskers Van Gogh) 27f8d9c refactor: defer accessing `CoinJoinWalletManager` to interface call (Kittywhiskers Van Gogh) 398e9c3 refactor: stop exposing `CoinJoinWalletManager` from `CoinJoin::Loader` (Kittywhiskers Van Gogh) 48fccdd refactor: introduce `coinjoin status` for information on mix sessions (Kittywhiskers Van Gogh) Pull request description: ## Motivation Since [bitcoin#22219](bitcoin#22219) (introduced in [dash#6568](#6568)), the workarounds implemented for allowing the backport of [bitcoin#19101](bitcoin#19101) (a part of [dash#6529](#6529)) no longer work. A major reason for this is the initialization order assumed in [dash#6529](#6529) is no longer holds true, requiring a rework of CoinJoin interfaces. This pull request aims to do that. ## Additional Information * Dependency for #6529 * To allow for dropping `walletman()` from `CoinJoin::Loader`, both `CoinJoin::Loader` and `CoinJoin::Client` (accessible through `CoinJoin::Loader::GetClient()`) need to account for all potential usage. To that end, `CoinJoin::Client::get{JsonInfo, SessionStatuses}()` have been implemented. * Though some invocations of `CCoinJoinClientSession` functions cannot (or rather, should not) be available through the interface (like `DoAutomaticDenominating()`). * To take care of one such invocation (in `coinjoin start`, [source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/rpc/coinjoin.cpp#L135-L136)), it has been removed altogether. `coinjoin start` will now behave like the "Start CoinJoin" button in Dash Qt and won't try to get ahead of the scheduler and start denomination. * The loss of status information in fail cases due to this, has been made up for with a new RPC, `coinjoin status`, that will report status information during any active mix session (as opposed to earlier behavior where such information was only made available when starting a mix session through `coinjoin start` and encountering a fail state). * `CoinJoin::Loader` and `WalletLoader` rely on each other as * `CoinJoin::Loader::{Add,Remove}Wallet()` needs to be able to run `WalletInitInterface::InitCoinJoinSettings()`, which in turn needs to be able enumerate through all available wallets ([source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/wallet/init.cpp#L207)), currently doing so through the global wallet context but eventually through `WalletLoader` when said globals are no longer available (post-[bitcoin#19101](bitcoin#19101)) * `WalletLoader` needs `CoinJoin::Loader` to load ([source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/wallet/interfaces.cpp#601)) and restore ([source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/wallet/interfaces.cpp#L607)) wallets as `CWallet` needs access to `CoinJoin::Loader` in order to call `CoinJoin::Loader::{Add,Remove}Wallet()` ([source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/wallet/wallet.cpp#L3516), [source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/wallet/wallet.cpp#L148)) This interdependent relationship has currently been floating along by passing a const ref of the smart pointer ([source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/interfaces/init.h#L37)), which makes for cumbersome but workable code but in combination with [bitcoin#22219](bitcoin#22219), it breaks [bitcoin#19101](bitcoin#19101) as it forces us to definitively initialize both `WalletLoader` and `CoinJoin::Loader` _before_ we get to initialize CoinJoin logic. To work around this, instead of requiring a fully initialized `CoinJoinWalletManager` to create a `CoinJoin::Loader`, we instead take `NodeContext` and defer resolving `CoinJoinWalletManager` to when interface calls are made. * By moving the initialization of the CoinJoin loader from `AppInitMain()` to `WalletInit::Construct()`, the linker was unhappy with trying to emit `bench_dash` (see error below). This was remedied by linking `libbitcoin_wallet` with `libtest_util`. <details> <summary>Linker error:</summary> ``` AR libbitcoin_server.a CXXLD dashd CXXLD test/test_dash CXXLD bench/bench_dash CXXLD qt/dash-qt CXXLD qt/test/test_dash-qt /usr/bin/ld: libtest_util.a(libtest_util_a-setup_common.o): in function `DashPostChainstateSetup(NodeContext&)': /src/dash/src/test/util/setup_common.cpp:127:(.text+0x1a8e): undefined reference to `interfaces::MakeCoinJoinLoader(NodeContext&)' collect2: error: ld returned 1 exit status make[2]: *** [Makefile:7354: bench/bench_dash] Error 1 make[2]: *** Waiting for unfinished jobs.... make[2]: Leaving directory '/src/dash/src' make[1]: *** [Makefile:20648: all-recursive] Error 1 make[1]: Leaving directory '/src/dash/src' make: *** [Makefile:797: all-recursive] Error 1 ``` </details> * [bitcoin#22219](bitcoin#22219) removes a `WalletInit::Construct()` call from `setup_common.cpp` ([source](029572d#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6L195-L198)), which is necessary since `WalletInit::Construct()` now relies on `node.init` ([source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/wallet/init.cpp#L191)). This isn't so much a problem for tests that use `WalletTestingSetup` or `InitWalletDirTestingSetup` but creates problems for tests based on `TestChain100Setup` ([source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/wallet/test/coinjoin_tests.cpp#L128), [source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/wallet/test/wallet_tests.cpp#L205)), which is a specialization of `TestingSetup` that in turn, used to initialize `coinjoin_loader` ([source](https://github.com/dashpay/dash/blob/5c1a6270a73ea7025879ee0485be5095b3bc142b/src/test/util/setup_common.cpp#L127)) but since initialization has been moved to `WalletInit::Construct()` in line with `Init::makeWalletLoader()`, it is not done automatically anymore. To work around this, `WalletInit::Construct()`-like code has been introduced to initialize both `coinjoin_loader` and `wallet_loader` ([source](https://github.com/kwvg/dash/blob/038619d284c457610ebbf833991298469c3e9a3d/src/test/util/setup_common.cpp#L320-L329)). ## Breaking Changes * `coinjoin start` will no longer report errors from mix sessions. To see the status of mix sessions, a new RPC, `coinjoin status`, is available in its place. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 68cec8d PastaPastaPasta: utACK 68cec8d Tree-SHA512: 17ffc8f79ff19ffee9874787bf9cf982c8059c566475ed78634c3870506a24020f969c35ed6d23748fea0f993d9e646929d523c9c441fad27ea5eaf4b9438303
- Loading branch information