refactor: polish CoinJoin interface, use WalletLoader in Dash wallet initialization code, introduce new RPC coinjoin status
#6594
Conversation
WalkthroughThe changes update various components related to CoinJoin and wallet functionality. A new RPC command, ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
src/coinjoin/interfaces.cpp (3)
1-1: Please fix the Clang format issues.The pipeline indicates Clang format differences. Kindly run
clang-formator the appropriate code formatting step to resolve style discrepancies before merging.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
8-15: Include order and spacing look fine, but formatting might need re-check.These new headers seem necessary for the added functionality. However, please ensure that final formatting matches Clang’s style requirements (see pipeline failures).
91-96: Constructor logic is valid.Setting
CCoinJoinClientOptions::SetEnabled(false)ensures mixing is off until wallets are added. Consider adding a short comment clarifying the rationale behind this initialization step.src/wallet/init.cpp (2)
1-1: Please fix the Clang format issues.As noted in the pipeline logs, formatting discrepancies need resolution before merging.
207-229: CoinJoin settings initialization logic is sound.Enabling CoinJoin based on wallet presence, then selectively starting or stopping mixing depending on locking state, is coherent. Consider adding more descriptive logging if some wallets are locked while others start mixing.
src/rpc/coinjoin.cpp (1)
1-1: Resolve Clang format differences.Kindly run the formatter tool before merging these changes.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
src/test/util/setup_common.cpp (1)
320-329: Caution with mixing raw pointers and smart pointers.Storing
wallet_loader.get()separately while also moving theunique_ptrintom_node.chain_clientscan be safe, but the destructor must carefully manage the destruction order. It's currently handled, but consider more robust ownership patterns if usage expands.src/interfaces/init.cpp (1)
15-16: Remove extra semicolon and clarify method stubs.Returning empty pointers is valid as a placeholder, but consider providing actual implementations or confirming it's part of a future plan. Also remove the trailing semicolon to follow consistent code style.
-std::unique_ptr<CoinJoin::Loader> Init::makeCoinJoinLoader() { return {}; }; +std::unique_ptr<CoinJoin::Loader> Init::makeCoinJoinLoader() { return {}; }src/interfaces/init.h (1)
35-36: Improved initialization design.The separation of
makeCoinJoinLoaderand updatedmakeWalletLoadersignature reflects a better separation of concerns:
- CoinJoin loader creation is now independent
- Wallet loader receives a reference instead of managing ownership of the CoinJoin loader
This design change improves modularity and reduces coupling between wallet and CoinJoin components.
src/coinjoin/client.h (1)
330-330: Improved status reporting design.The change from
bilingual_strtostd::vector<std::string>is a good improvement:
- Better separation of concerns (separates data from presentation)
- More flexible for handling multiple status messages
- Aligns with the new
coinjoin statusRPC command requirementsThis change makes the status reporting more modular and easier to extend.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
doc/release-notes-6594.md(1 hunks)src/Makefile.test_util.include(1 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/client.h(1 hunks)src/coinjoin/interfaces.cpp(3 hunks)src/dummywallet.cpp(3 hunks)src/init.cpp(1 hunks)src/init/bitcoin-node.cpp(2 hunks)src/init/bitcoind.cpp(2 hunks)src/interfaces/coinjoin.h(3 hunks)src/interfaces/init.cpp(2 hunks)src/interfaces/init.h(2 hunks)src/interfaces/wallet.h(2 hunks)src/node/blockstorage.cpp(0 hunks)src/qt/test/addressbooktests.cpp(0 hunks)src/qt/test/wallettests.cpp(0 hunks)src/rpc/coinjoin.cpp(8 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/util/setup_common.h(0 hunks)src/test/validation_chainstatemanager_tests.cpp(0 hunks)src/wallet/context.cpp(1 hunks)src/wallet/context.h(1 hunks)src/wallet/init.cpp(3 hunks)src/wallet/interfaces.cpp(5 hunks)src/wallet/rpcwallet.cpp(3 hunks)src/wallet/test/init_test_fixture.cpp(1 hunks)src/wallet/test/wallet_test_fixture.cpp(1 hunks)src/wallet/test/wallet_test_fixture.h(2 hunks)src/walletinitinterface.h(2 hunks)
💤 Files with no reviewable changes (5)
- src/node/blockstorage.cpp
- src/qt/test/wallettests.cpp
- src/qt/test/addressbooktests.cpp
- src/test/util/setup_common.h
- src/test/validation_chainstatemanager_tests.cpp
✅ Files skipped from review due to trivial changes (2)
- doc/release-notes-6594.md
- src/wallet/rpcwallet.cpp
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/coinjoin.cpp
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
src/coinjoin/interfaces.cpp
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (54)
src/coinjoin/interfaces.cpp (10)
47-50: New method looks good.The
getJsonInfo()method simply delegates tom_clientman.GetJsonInfo(obj). No immediate issues found.
55-58: Session statuses retrieval is straightforward.Returning a
std::vector<std::string>from the underlying manager is clear and efficient.
98-102: Wallet addition triggers CoinJoin settings initialization.Invoking
g_wallet_init_interface.InitCoinJoinSettings(*this, wallet_loader())for each wallet is consistent with the new design. No immediate issues found.
105-106: RemoveWallet is consistent with AddWallet.Re-invoking
InitCoinJoinSettingsafter removal is logical here. No further concerns.
110-110: FlushWallet call is straightforward.Simple pass-through to
walletman().Flush(name). No issues found.
114-114: Client creation via manager is valid.Returns a
CoinJoinClientImplif the manager is found, otherwisenullptr. This pattern is safe.
118-119: Storing a NodeContext reference is acceptable.Exposes node context for loader interactions. This is consistent with the new design.
125-125: Factory function aligns with interface changes.
MakeCoinJoinLoader(NodeContext& node)is good for constructing the new loader with node context references.
79-83:❓ Verification inconclusive
Double Assert call may hide null-pointer risk.
return *Assert(Assert(m_node.cj_ctx)->walletman);could cause a crash ifm_node.cj_ctxorwalletmanis null.If any instantiation appears before the CoinJoin context is initialized, this might lead to null dereferences.
🏁 Script executed:
#!/bin/bash # Check all instantiations of CoinJoinLoaderImpl to ensure m_node.cj_ctx is properly set ast-grep --pattern $'new CoinJoinLoaderImpl($_)'Length of output: 215
I'll generate an additional verification script that restricts the search to C++ source files (i.e. *.cpp and *.h) to avoid interference from non-code files. This should help us determine whether any instantiations of
CoinJoinLoaderImploccur before the CoinJoin context is set:Could you please run this script and share the output? This will help us determine if there’s a potential null-pointer issue related to
m_node.cj_ctxor itswalletmanmember.
🏁 Script executed:
#!/bin/bash # Re-verify instantiations of CoinJoinLoaderImpl in C++ source files only rg --glob '*.cpp' --glob '*.h' "new[[:space:]]+CoinJoinLoaderImpl\\(" .Length of output: 72
Action: Review double asserts in walletman() for potential hidden null-pointer risks.
Our additional automated searches targeting
CoinJoinLoaderImpldid not reveal any usage patterns that could lead to premature access of an uninitialized CoinJoin context. However, the nested use ofAssertinreturn *Assert(Assert(m_node.cj_ctx)->walletman);still obscures which pointer might be null if a failure occurs. Please:
- Verify that
m_node.cj_ctxis always properly initialized before any call towalletman().- Consider separating the assertions or adding detailed logging to clearly indicate whether it’s
m_node.cj_ctxor itswalletmanmember that is null, to improve future debugging.- Revisit related code sections to confirm that the initialization order is consistently maintained.
85-88:❓ Verification inconclusive
Confirm the non-null wallet_loader pointer.
Similarly,
return *Assert(m_node.wallet_loader);can crash ifwallet_loaderis uninitialized. Verify that all call sites guarantee a valid pointer before usage.
🏁 Script executed:
#!/bin/bash # Ensure node.wallet_loader is always set prior to CoinJoinLoaderImpl usage ast-grep --pattern $'node.wallet_loader = $_'Length of output: 212
Below is a revised verification script that excludes the JSON files (like those under
src/univalue/) causing the encoding errors. This script will search for assignments tom_node.wallet_loaderin C++ files and help us confirm whether the wallet loader pointer is always set before use:Please run the above script and share its output. Based on the results, we can conclusively verify that all call sites ensure a valid pointer before using
wallet_loader(). Until that output is confirmed, manual verification remains necessary.
🏁 Script executed:
[run_scripts] #!/bin/bash # Exclude problematic JSON files and search for C++ assignments to m_node.wallet_loader rg --glob '!src/univalue/*' "m_node\.wallet_loader\s*=" -tcppLength of output: 331
Review Action Required: Validate Initialization of
wallet_loaderPointerThe current implementation in
src/coinjoin/interfaces.cpp:interfaces::WalletLoader& wallet_loader() { return *Assert(m_node.wallet_loader); }assumes that
m_node.wallet_loaderis always non-null. However, our search uncovered two assignment patterns in test code:
- In
src/test/util/setup_common.cpp, there is an assignment wherem_node.wallet_loaderis set to a valid pointer with:m_node.wallet_loader = wallet_loader.get();- In the same file, there is also an assignment where it’s explicitly set to
nullptr:m_node.wallet_loader = nullptr;Action Items:
- Production Usage: Verify that all production call sites guarantee that
m_node.wallet_loaderis initialized (non-null) before callingwallet_loader(). This will prevent a potential crash when dereferencing a null pointer.- Test Code Consideration: Confirm that setting
m_node.wallet_loadertonullptrin tests is intentional and that those code paths do not inadvertently invoke thewallet_loader()method. If they do, add appropriate safeguards or modify the test to avoid triggering the assertion.Please manually verify these scenarios to ensure the design meets the required safety guarantees.
src/wallet/init.cpp (4)
9-9: New include is appropriate.
#include <interfaces/coinjoin.h>is needed to define the added functionality. Looks good.
50-51: Updated method signatures match the new interfaces.Accepting
interfaces::WalletLoaderandinterfaces::CoinJoin::Loaderbrings wallet and CoinJoin logic together cleanly.
192-193: Construct method now properly initializes loaders.Assigning
node.coinjoin_loaderthen creating a wallet loader with it is consistent and resolves previous initialization order issues.
199-204: AutoLockMasternodeCollaterals is straightforward.Iterating over all wallets and calling
autoLockMasternodeCollaterals()is correct. Consider logging or handling potential lock failures if needed.src/rpc/coinjoin.cpp (9)
48-48: New "status" command documented.The help text is updated to list the new "status" command. Good addition.
88-89: Reset flow uses safe client acquisition.Using
CHECK_NONFATALensures the loader exists and the client is valid, thenresetPool()is called. No major issues identified.
128-134: Start mixing refactor.The code checks if the wallet is unlocked, then starts mixing. Updated error messages are clear and consistent.
139-174: New coinjoin_status command is well-structured.Ensures a valid wallet, checks if mixing is enabled, throws an error if not mixing, and returns session statuses in an array. This approach is user-friendly.
200-210: Stop mixing logic is consistent.Confirms mixing is running before calling
stopMixing(). Appropriately throws if there's no ongoing session.
274-275: Salt generation or override checks.Guards against changing salt while mixing to avoid conflicts. Implementation is correct.
376-377: Salt set logic parallels generate.Again checks for an active mix session before overwriting the salt. Consistency is maintained.
471-472: Client retrieval for getcoinjoininfo.
CHECK_NONFATAL(node.coinjoin_loader)->GetClient(...)is used safely, returning a client to gather additional JSON info.
500-500: Registers new "coinjoin_status" RPC command.Completes the setup for the new status method, enabling it to be called by end users.
src/test/util/setup_common.cpp (3)
66-66: Header inclusion looks correct.This newly added
#include <interfaces/wallet.h>is appropriate for the subsequent wallet-related logic.
316-319: Ensure reference lifetime is valid.Passing references of
m_nodemembers toCJContextis fine as long as their lifetimes are guaranteed to exceed the context's usage. Please verify that no concurrency or teardown sequence can invalidate these references prematurely.
339-347: Destruction order is consistent.Resetting chain clients before clearing the raw pointer references ensures a proper teardown flow. This prevents dangling pointers and is safely managed.
src/wallet/context.cpp (1)
7-7: Confirm old constructor references are removed.This default constructor replaces the previous constructor with a
coinjoin_loaderparameter. Verify that all call sites expecting the old constructor are updated accordingly.src/interfaces/init.cpp (1)
6-6: Header inclusion is consistent.Including
<interfaces/coinjoin.h>is necessary to define and return theCoinJoin::Loaderobjects in subsequent methods.src/wallet/test/wallet_test_fixture.cpp (1)
11-13: LGTM! The initialization order and ownership semantics are correct.The changes properly integrate the CoinJoin loader into the wallet testing setup:
m_coinjoin_loaderis initialized firstm_wallet_loaderuses the CoinJoin loader after null checkm_walletuses the CoinJoin loader directlysrc/wallet/test/wallet_test_fixture.h (1)
11-11: LGTM! The header changes maintain proper dependencies and RAII.The changes correctly:
- Add the necessary include for CoinJoin interfaces
- Declare the CoinJoin loader member with proper ownership semantics
- Maintain correct declaration order for dependent members
Also applies to: 25-25
src/wallet/context.h (1)
31-31:✅ Verification successful
Verify ownership management of the CoinJoin loader.
The change from a unique_ptr reference to a raw pointer aligns with the struct's purpose as a collection of state pointers. However, please ensure that:
- The lifetime of the CoinJoin loader is properly managed elsewhere
- The loader is not null when used
Let's verify the ownership management:
Also applies to: 36-36
🏁 Script executed:
#!/bin/bash # Search for CoinJoin loader ownership management rg -A 5 "MakeCoinJoinLoader|coinjoin_loader.*=|coinjoin_loader.*std::unique_ptr"Length of output: 18547
Ownership management verified for CoinJoin loader.
The search results confirm that the CoinJoin loader is constructed via
MakeCoinJoinLoader(returning astd::unique_ptr) and then consistently passed as a raw pointer throughout the codebase (e.g., in wallet initialization and RPC calls). This pattern aligns with the design of the context struct as a collection of non-owning state pointers, and null checks (e.g., in RPC functions) ensure safe usage.
- The raw pointer is assigned from a unique pointer managed elsewhere.
- Usage across wallets and tests consistently checks for null, ensuring proper lifetime management.
No further action is needed regarding ownership management.
src/walletinitinterface.h (1)
10-15:❓ Verification inconclusive
Verify that all implementations are updated for the interface changes.
The interface changes improve modularity by explicitly passing loaders. However, please ensure that all implementations of
WalletInitInterfaceare updated to match the new method signatures:
AutoLockMasternodeCollaterals(interfaces::WalletLoader&)InitCoinJoinSettings(interfaces::CoinJoin::Loader&, interfaces::WalletLoader&)Let's verify the implementations:
Also applies to: 29-30
🏁 Script executed:
#!/bin/bash # Search for WalletInitInterface implementations ast-grep --pattern 'class $_ : public WalletInitInterface { $$$ }'Length of output: 70
Action Required: Verify Implementations Updated for New Interface Methods
The new forward declarations and updated method signatures for
AutoLockMasternodeCollaterals(interfaces::WalletLoader&)andInitCoinJoinSettings(interfaces::CoinJoin::Loader&, interfaces::WalletLoader&)improve modularity by clearly defining loader dependencies. However, the automated AST-grep search did not return any matching implementations for classes inheriting fromWalletInitInterface. This absence of output means we couldn’t conclusively verify that all implementations have been updated.Before finalizing, please ensure or manually verify that all concrete classes implementing
WalletInitInterfaceare updated accordingly. For further confirmation, you might consider running additional searches, for example:#!/bin/bash echo "Searching for WalletInitInterface implementations using rg" rg 'class .*: public WalletInitInterface' -n echo "Searching for AutoLockMasternodeCollaterals implementations" rg 'AutoLockMasternodeCollaterals\(' -n echo "Searching for InitCoinJoinSettings implementations" rg 'InitCoinJoinSettings\(' -nOnce you confirm that the updated method signatures are correctly implemented across the codebase, please update the PR accordingly.
src/Makefile.test_util.include (1)
46-48: LGTM! Good practice for conditional linking.The conditional inclusion of the wallet library ensures test utilities are only linked with wallet code when wallet functionality is enabled, improving modularity and build efficiency.
src/init/bitcoind.cpp (3)
6-6: LGTM! Required include for CoinJoin interface.The addition of
interfaces/coinjoin.haligns with the new CoinJoin functionality.
28-31: LGTM! Clean implementation of CoinJoin loader factory.The new method provides a clean way to create CoinJoin loaders using the node context.
32-35: LGTM! Improved safety with reference parameters.The modified signature using references instead of pointers:
- Prevents null pointer dereference
- Makes ownership semantics clearer
src/interfaces/coinjoin.h (3)
10-15: LGTM! Required includes and forward declarations.Clean addition of necessary includes and forward declarations for new functionality.
27-28: LGTM! New methods support improved status reporting.The new methods enhance the Client interface:
getJsonInfoprovides structured data for RPC responsesgetSessionStatusesreturns individual session statuses, improving flexibility over concatenated strings
49-49: LGTM! Improved loader initialization.The signature change to use
NodeContextinstead ofCoinJoinWalletManageraligns with the PR's goal of removing direct wallet manager dependency.src/wallet/test/init_test_fixture.cpp (1)
17-18: LGTM! Proper initialization order and null checks.The changes:
- Initialize coinjoin loader before wallet loader
- Use Assert for pointer validation
- Follow the new dependency structure
src/interfaces/init.h (1)
18-20: LGTM! Clean namespace declaration.The CoinJoin namespace is properly declared with clear scope.
src/init/bitcoin-node.cpp (2)
33-36: LGTM! Clean implementation of CoinJoin loader creation.The implementation properly forwards the node context to create the CoinJoin loader.
37-40: LGTM! Proper error handling in makeWalletLoader.Good use of
Assertto validate node args before passing to wallet loader.src/dummywallet.cpp (2)
32-33: LGTM! Proper dummy implementations.Empty implementations correctly maintained for non-wallet build.
89-92: LGTM! Appropriate error handling.Proper error message when wallet functions are called in non-wallet build.
src/interfaces/wallet.h (2)
89-90: LGTM: New virtual method for locking masternode collaterals.The new pure virtual method
autoLockMasternodeCollaterals()provides a clear interface for locking masternode collaterals. The method is appropriately placed in the wallet interface.
450-450:✅ Verification successful
Verify the impact of the signature change.
The function signature has been changed from using a unique_ptr to a reference for
coinjoin_loader, and the parameter order has been modified. This could affect existing callers.Let's verify all callers of this function are updated:
🏁 Script executed:
#!/bin/bash # Search for calls to MakeWalletLoader rg "MakeWalletLoader\(" --type cppLength of output: 1125
Signature Consistency Confirmed – All Callers Updated
After verifying the call sites using the shell script output, all invocations of
MakeWalletLoadernow pass parameters in the new order—chain, followed byargs, thencoinjoin_loader. In cases wherecoinjoin_loaderis stored as a pointer (e.g., in test files), the caller correctly dereferences it; other callers pass an object directly. No outdated call or ordering issue was detected.src/wallet/interfaces.cpp (4)
150-150: LGTM: Simple delegation to wallet implementation.The implementation correctly delegates to the wallet's
AutoLockMasternodeCollateralsmethod.
557-562: LGTM: Updated constructor to use reference.The constructor has been updated to use a reference for
coinjoin_loaderand store it in the context, which aligns with the interface changes.
578-578: LGTM: Updated method calls to use reference.The methods have been updated to use the reference member
coinjoin_loaderconsistently.Also applies to: 593-593, 600-600, 605-605
642-644: LGTM: Updated factory function implementation.The
MakeWalletLoaderimplementation has been updated to match the new signature.src/coinjoin/client.cpp (2)
353-365: LGTM: Improved status reporting.The
GetStatuses()method now returns a vector of strings instead of concatenating them into a single bilingual string. This provides more flexibility in handling individual status messages.
917-923: LGTM: Simplified wallet manager methods.The
AddandRemovemethods have been simplified by removing unnecessary initialization calls.Also applies to: 934-936
src/init.cpp (1)
2205-2209: LGTM! The wallet initialization code has been updated to use WalletLoader.The code correctly checks if wallet is enabled before attempting to auto-lock masternode collaterals using the new
WalletLoaderinterface, which aligns with the PR's objective of refactoring wallet initialization code.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/coinjoin/interfaces.cpp (1)
1-127:⚠️ Potential issueFix clang-format issues.
The pipeline reports formatting differences.
Please run clang-format to fix the formatting issues.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
🧹 Nitpick comments (3)
src/coinjoin/interfaces.cpp (1)
91-96: Consider adding error handling for wallet manager initialization.The constructor initializes CoinJoin settings but doesn't handle potential failures in wallet manager initialization.
Consider adding error handling:
explicit CoinJoinLoaderImpl(NodeContext& node) : m_node(node) { + if (!m_node.cj_ctx || !m_node.cj_ctx->walletman) { + throw std::runtime_error("CoinJoin wallet manager not initialized"); + } // Enablement will be re-evaluated when a wallet is added or removed CCoinJoinClientOptions::SetEnabled(false); }src/wallet/init.cpp (1)
207-229: Consider adding error logging for CoinJoin initialization failures.The method handles wallet locking states but doesn't log initialization failures.
Consider adding error logging:
void WalletInit::InitCoinJoinSettings(interfaces::CoinJoin::Loader& coinjoin_loader, interfaces::WalletLoader& wallet_loader) const { const auto& wallets{wallet_loader.getWallets()}; CCoinJoinClientOptions::SetEnabled(!wallets.empty() ? gArgs.GetBoolArg("-enablecoinjoin", true) : false); if (!CCoinJoinClientOptions::IsEnabled()) { + LogPrintf("CoinJoin: disabled\n"); return; } bool fAutoStart = gArgs.GetBoolArg("-coinjoinautostart", DEFAULT_COINJOIN_AUTOSTART); for (auto& wallet : wallets) { auto manager = Assert(coinjoin_loader.GetClient(wallet->getWalletName())); + if (!manager) { + LogPrintf("CoinJoin: failed to initialize client for wallet %s\n", wallet->getWalletName()); + continue; + } if (wallet->isLocked(/*fForMixing=*/false)) { + LogPrintf("CoinJoin: stopping mixing for locked wallet %s\n", wallet->getWalletName()); manager->stopMixing(); } else if (fAutoStart) { + LogPrintf("CoinJoin: starting mixing for wallet %s\n", wallet->getWalletName()); manager->startMixing(); } }src/rpc/coinjoin.cpp (1)
88-89: Consider enhancing error handling in client manager access.While the use of
CHECK_NONFATALfor null checks is consistent, consider adding more descriptive error messages to help with debugging. For example:-auto cj_clientman = CHECK_NONFATAL(node.coinjoin_loader)->GetClient(wallet->GetName()); -CHECK_NONFATAL(cj_clientman); +auto cj_clientman = CHECK_NONFATAL(node.coinjoin_loader, "CoinJoin loader not initialized")->GetClient(wallet->GetName()); +CHECK_NONFATAL(cj_clientman, strprintf("Failed to get CoinJoin client for wallet %s", wallet->GetName()));Also applies to: 128-129, 164-165, 204-210, 471-472
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
doc/release-notes-6594.md(1 hunks)src/Makefile.test_util.include(1 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/client.h(1 hunks)src/coinjoin/interfaces.cpp(3 hunks)src/dummywallet.cpp(3 hunks)src/init.cpp(1 hunks)src/init/bitcoin-node.cpp(2 hunks)src/init/bitcoind.cpp(2 hunks)src/interfaces/coinjoin.h(3 hunks)src/interfaces/init.cpp(2 hunks)src/interfaces/init.h(2 hunks)src/interfaces/wallet.h(2 hunks)src/node/blockstorage.cpp(0 hunks)src/qt/test/addressbooktests.cpp(0 hunks)src/qt/test/wallettests.cpp(0 hunks)src/rpc/coinjoin.cpp(8 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/util/setup_common.h(0 hunks)src/test/validation_chainstatemanager_tests.cpp(0 hunks)src/wallet/context.cpp(1 hunks)src/wallet/context.h(1 hunks)src/wallet/init.cpp(3 hunks)src/wallet/interfaces.cpp(5 hunks)src/wallet/rpcwallet.cpp(3 hunks)src/wallet/test/init_test_fixture.cpp(1 hunks)src/wallet/test/wallet_test_fixture.cpp(1 hunks)src/wallet/test/wallet_test_fixture.h(2 hunks)src/walletinitinterface.h(2 hunks)
💤 Files with no reviewable changes (5)
- src/test/util/setup_common.h
- src/test/validation_chainstatemanager_tests.cpp
- src/qt/test/addressbooktests.cpp
- src/node/blockstorage.cpp
- src/qt/test/wallettests.cpp
🚧 Files skipped from review as they are similar to previous changes (12)
- doc/release-notes-6594.md
- src/Makefile.test_util.include
- src/wallet/test/wallet_test_fixture.h
- src/coinjoin/client.h
- src/wallet/context.cpp
- src/walletinitinterface.h
- src/wallet/test/init_test_fixture.cpp
- src/interfaces/init.h
- src/dummywallet.cpp
- src/wallet/test/wallet_test_fixture.cpp
- src/wallet/rpcwallet.cpp
- src/wallet/interfaces.cpp
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/coinjoin/interfaces.cpp
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
src/rpc/coinjoin.cpp
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
🔇 Additional comments (30)
src/test/util/setup_common.cpp (4)
66-66: LGTM!The addition of
interfaces/wallet.his necessary for the new wallet loader functionality.
316-318: LGTM!The CoinJoin context initialization is properly structured with all required dependencies, aligning with the PR's goal of refactoring the CoinJoin interface.
320-329: LGTM!The initialization sequence is well-structured:
- Proper conditional compilation with
ENABLE_WALLET- Correct initialization order: CoinJoin loader before wallet loader
- Appropriate integration with chain clients
339-348: LGTM!The cleanup sequence follows best practices:
- Proper conditional compilation with
ENABLE_WALLET- Correct cleanup order: reverse of initialization
- Complete resource cleanup
src/interfaces/init.cpp (2)
6-6: LGTM!The include directive for
interfaces/coinjoin.his correctly added to support the new CoinJoin functionality.
15-16: LGTM!The changes correctly introduce the new
makeCoinJoinLoadermethod and update themakeWalletLoadersignature to use references instead of unique pointers, improving ownership semantics.src/wallet/context.h (2)
31-31: LGTM!The change to use a raw pointer with nullptr initialization aligns with the struct's purpose as a collection of state pointers and improves safety by not requiring a loader at construction time.
36-36: LGTM!The default constructor is appropriate here as it allows for deferred initialization of the CoinJoin loader, which aligns with the PR objectives.
src/init/bitcoind.cpp (2)
6-6: LGTM!The include directives for
interfaces/coinjoin.handutil/check.hare correctly added to support the new functionality and assertions.Also applies to: 12-12
29-36: LGTM!The implementation correctly:
- Creates the CoinJoin loader using the node context
- Updates the wallet loader signature to use references
- Ensures m_node.args is not null using Assert
src/interfaces/coinjoin.h (3)
10-15: LGTM!The includes and forward declarations are correctly added and ordered to support the new functionality.
27-28: LGTM!The new virtual methods
getJsonInfoandgetSessionStatusesenhance the interface by providing structured access to CoinJoin information, which aligns with the PR objectives.
49-49: LGTM!The updated signature of
MakeCoinJoinLoadercorrectly usesNodeContextinstead ofCoinJoinWalletManager, aligning with the refactoring goals to remove the wallet manager dependency.src/init/bitcoin-node.cpp (3)
6-6: LGTM! Required includes added.The new includes support the CoinJoin loader functionality and assertion utilities.
Also applies to: 13-13
34-37: LGTM! Well-structured CoinJoin loader creation.The new
makeCoinJoinLoadermethod follows the established pattern of other loader creation methods in the class.
38-41: LGTM! Improved parameter passing for wallet loader.The change from
unique_ptrto reference parameter improves ownership semantics, as the loader's lifetime is managed elsewhere.src/coinjoin/interfaces.cpp (4)
8-11: LGTM! Required includes added.The new includes support node context integration and CoinJoin options.
47-50: LGTM! New JSON info retrieval method.The method provides a clean interface for getting JSON-formatted client information.
55-58: LGTM! New session status retrieval method.The method returns a vector of strings, which is more flexible than a concatenated string.
79-89: LGTM! Well-structured helper methods.The private helper methods
walletman()andwallet_loader()improve code organization and error handling through assertions.src/interfaces/wallet.h (2)
89-91: LGTM! New virtual method for masternode collateral management.The pure virtual method enforces implementation in derived classes.
450-450: LGTM! Improved parameter ordering in MakeWalletLoader.The signature change improves readability by grouping related parameters and using references appropriately.
src/wallet/init.cpp (2)
192-193: LGTM! Proper initialization order.The CoinJoin loader is correctly initialized before the wallet loader.
199-205: LGTM! Improved masternode collateral management.The method now uses the wallet loader interface to access wallets, improving encapsulation.
src/rpc/coinjoin.cpp (3)
48-48: LGTM! Command registration is properly updated.The help message and command table are correctly updated to include the new
coinjoin statuscommand.Also applies to: 500-500
139-174: LGTM! The new coinjoin_status implementation is well-structured.The implementation includes proper validation, error handling, and follows the RPC interface pattern consistently. The status messages are returned as an array, which provides better flexibility for clients.
128-133: LGTM! The simplified coinjoin_start response is appropriate.The change to return a simple "Mixing requested" message aligns with the PR objectives, as error reporting is now handled by the new
coinjoin statuscommand.src/coinjoin/client.cpp (2)
353-365: LGTM! The GetStatuses refactor improves data structure.The change from
bilingual_strtostd::vector<std::string>provides better flexibility and structure for status reporting. The use of uniform initialization forfWaitForBlockfollows modern C++ practices.
917-923: LGTM! Client manager initialization is simplified.The removal of
g_wallet_init_interface.InitCoinJoinSettingsand the updated initialization pattern simplify the code while maintaining functionality.Also applies to: 934-936
src/init.cpp (1)
2205-2209:❓ Verification inconclusive
Verify the wallet loader initialization.
The code correctly checks for the wallet being enabled before auto-locking masternode collaterals. However, since this is part of a larger refactor of wallet initialization code, ensure that
wallet_loaderis properly initialized at this point.Run the following script to verify the initialization order:
🏁 Script executed:
#!/bin/bash # Description: Check if wallet_loader is initialized before use # Search for wallet_loader initialization ast-grep --pattern $'node.wallet_loader = $_' # Search for any potential null checks on wallet_loader ast-grep --pattern $'if (node.wallet_loader) { $$$ }'Length of output: 269
Below is a revised review comment. Since the initial script produced no conclusive output (and even reported issues reading a couple of JSON files), please re-run a more targeted search to confirm that
node.wallet_loaderis indeed initialized before its use in this block. In the meantime, manually verify the initialization order insrc/init.cpp.Manual Verification Required: Check Wallet Loader Initialization
- The code at lines 2205–2209 correctly checks that the wallet is enabled before auto-locking masternode collaterals.
- However, the recent search did not confirm whether
node.wallet_loaderis initialized before its use.- Please verify that
node.wallet_loaderis assigned (e.g. via an assignment likenode.wallet_loader = …) prior to callingAutoLockMasternodeCollaterals().- It may be prudent to add an explicit null check or safeguard if there’s any risk of
wallet_loaderbeing uninitialized.For additional clarification, you can run this targeted script:
#!/bin/bash # Check for wallet_loader assignment and null checks specifically in src/init.cpp echo "Searching for wallet_loader assignment in src/init.cpp" rg -n "node\.wallet_loader\s*=" src/init.cpp echo "Searching for wallet_loader null check in src/init.cpp" rg -n "if\s*\(node\.wallet_loader\)" src/init.cppPlease verify the initialization manually and adjust as needed.
|
Seems to be working with no issues. Consider adding some of ai bot suggestions maybe? pls see 71659ff |
This helps us get rid of having to report it through `coinjoin start`. Additionally, we won't try to jump ahead of the scheduler by starting denomination routines ourselves, let the scheduler run its course. This matters because we don't plan on exposing that capability through the interface as we are migrating to the interface in the next commit.
`interfaces::CoinJoin::Loader` relies on `interfaces::WalletLoader` and vice-versa. Currently this isn't explicitly enumerated due to the global wallet state but when that goes away, we will need to defer access to one of them to break the tie. Let's do that now.
The previous commit removes the invocation of `MakeCoinJoinLoader` from `init.cpp`. For some reason this makes the linker unhappy when working with `bench_dash`. Fix it by adding the wallet library to `test_util`. ``` /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 ```
We now initialize the CoinJoin loader before initializing the wallet loader thanks to deferred access to `CoinJoinWalletManager`, so we no longer need the smart pointer ref workaround.
`validation_chainstatemanager_tests` can work without `CJContext`, we can safely reincorporate it into `TestingSetup`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/interfaces/init.cpp (1)
16-16: Consider documenting ownership semantics.The change from
const std::unique_ptr<CoinJoin::Loader>&toCoinJoin::Loader&suggests a shift in ownership model. Consider adding a comment to clarify that the caller retains ownership of the loader.src/rpc/coinjoin.cpp (3)
88-89: Consider adding error handling for GetClient result.While
CHECK_NONFATALis used, consider adding explicit error handling to provide a more informative error message to the user.- auto cj_clientman = CHECK_NONFATAL(node.coinjoin_loader)->GetClient(wallet->GetName()); - CHECK_NONFATAL(cj_clientman)->resetPool(); + auto cj_loader = CHECK_NONFATAL(node.coinjoin_loader); + if (!cj_loader) { + throw JSONRPCError(RPC_INTERNAL_ERROR, "CoinJoin loader not available"); + } + auto cj_clientman = cj_loader->GetClient(wallet->GetName()); + if (!cj_clientman) { + throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Failed to get CoinJoin client for wallet '%s'", wallet->GetName())); + } + cj_clientman->resetPool();
128-134: Improve error handling and return message.The return message is now less informative. Consider providing more context about the mixing status.
- return "Mixing requested"; + return strprintf("Mixing requested for wallet '%s'. Use 'coinjoin status' to check mixing progress.", wallet->GetName());
202-209: Consider adding error handling for GetClient result.Similar to earlier feedback, consider adding explicit error handling for the client manager retrieval.
- auto cj_clientman = node.coinjoin_loader->GetClient(wallet->GetName()); + auto cj_loader = CHECK_NONFATAL(node.coinjoin_loader); + if (!cj_loader) { + throw JSONRPCError(RPC_INTERNAL_ERROR, "CoinJoin loader not available"); + } + auto cj_clientman = cj_loader->GetClient(wallet->GetName()); + if (!cj_clientman) { + throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Failed to get CoinJoin client for wallet '%s'", wallet->GetName())); + }src/wallet/init.cpp (1)
199-204: Consider adding error handling for empty wallet list.The method could benefit from early validation of the wallet list.
void WalletInit::AutoLockMasternodeCollaterals(interfaces::WalletLoader& wallet_loader) const { // we can't do this before DIP3 is fully initialized + const auto& wallets = wallet_loader.getWallets(); + if (wallets.empty()) { + return; + } - for (const auto& wallet : wallet_loader.getWallets()) { + for (const auto& wallet : wallets) { wallet->autoLockMasternodeCollaterals(); } }src/coinjoin/interfaces.cpp (1)
80-83: Consider adding thread safety for wallet manager access.The helper methods should consider thread safety when accessing shared resources.
private: + mutable std::mutex m_mutex; + CoinJoinWalletManager& walletman() { + std::lock_guard<std::mutex> lock(m_mutex); return *Assert(Assert(m_node.cj_ctx)->walletman); } interfaces::WalletLoader& wallet_loader() { + std::lock_guard<std::mutex> lock(m_mutex); return *Assert(m_node.wallet_loader); }Also applies to: 85-88
src/dummywallet.cpp (2)
84-87: Consider improving error message.The error message could be more specific about the build configuration.
- throw std::logic_error("Wallet function called in non-wallet build."); + throw std::logic_error("CoinJoin loader requested in build without wallet support (compiled with -DENABLE_WALLET=0).");
94-97: Consider improving error message.Similar to above, the error message could be more specific.
- throw std::logic_error("Wallet function called in non-wallet build."); + throw std::logic_error("Wallet loader requested in build without wallet support (compiled with -DENABLE_WALLET=0).");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
doc/release-notes-6594.md(1 hunks)src/Makefile.test_util.include(1 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/client.h(1 hunks)src/coinjoin/interfaces.cpp(3 hunks)src/dummywallet.cpp(3 hunks)src/init.cpp(1 hunks)src/init/bitcoin-node.cpp(2 hunks)src/init/bitcoind.cpp(2 hunks)src/interfaces/coinjoin.h(3 hunks)src/interfaces/init.cpp(2 hunks)src/interfaces/init.h(2 hunks)src/interfaces/wallet.h(2 hunks)src/node/blockstorage.cpp(0 hunks)src/qt/test/addressbooktests.cpp(0 hunks)src/qt/test/wallettests.cpp(0 hunks)src/rpc/coinjoin.cpp(8 hunks)src/test/util/setup_common.cpp(3 hunks)src/test/util/setup_common.h(0 hunks)src/test/validation_chainstatemanager_tests.cpp(0 hunks)src/wallet/context.cpp(1 hunks)src/wallet/context.h(1 hunks)src/wallet/init.cpp(3 hunks)src/wallet/interfaces.cpp(5 hunks)src/wallet/rpcwallet.cpp(3 hunks)src/wallet/test/init_test_fixture.cpp(1 hunks)src/wallet/test/wallet_test_fixture.cpp(1 hunks)src/wallet/test/wallet_test_fixture.h(2 hunks)src/walletinitinterface.h(2 hunks)test/functional/rpc_coinjoin.py(2 hunks)
💤 Files with no reviewable changes (5)
- src/node/blockstorage.cpp
- src/qt/test/wallettests.cpp
- src/test/validation_chainstatemanager_tests.cpp
- src/test/util/setup_common.h
- src/qt/test/addressbooktests.cpp
🚧 Files skipped from review as they are similar to previous changes (12)
- src/wallet/test/wallet_test_fixture.h
- src/wallet/context.cpp
- src/Makefile.test_util.include
- src/wallet/test/wallet_test_fixture.cpp
- src/coinjoin/client.h
- doc/release-notes-6594.md
- src/interfaces/init.h
- src/walletinitinterface.h
- src/wallet/test/init_test_fixture.cpp
- src/interfaces/wallet.h
- src/wallet/rpcwallet.cpp
- src/interfaces/coinjoin.h
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/coinjoin/interfaces.cpp
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
src/rpc/coinjoin.cpp
[error] 1-1: Clang format differences found. Please run 'clang-format' to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: predict_conflicts
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (23)
test/functional/rpc_coinjoin.py (1)
69-70: LGTM! Good test coverage for the newcoinjoin statusRPC command.The test cases appropriately verify both success and error scenarios for the new status command:
- Verifies that status can be queried while mixing is active
- Verifies that status request fails when no mix session is running
Also applies to: 79-80
src/interfaces/init.cpp (1)
6-6: LGTM! Good addition of CoinJoin loader interface.The new
makeCoinJoinLoader()method and its header inclusion align with the PR's objective of refactoring CoinJoin initialization.Also applies to: 15-15
src/test/util/setup_common.cpp (3)
316-318: LGTM! Well-structured CoinJoin context initialization.The initialization parameters are comprehensive and in a logical order.
320-329: LGTM! Good wallet initialization with proper dependency handling.The code correctly:
- Creates the CoinJoin loader first
- Creates the wallet loader with proper dependencies
- Stores the loader in both direct and chain client references
339-348: LGTM! Proper cleanup of resources in reverse order.The cleanup sequence correctly:
- Resets chain clients
- Clears the wallet loader reference
- Resets the CoinJoin loader
- Resets the CoinJoin context
src/init.cpp (1)
2205-2209:❓ Verification inconclusive
Verify the wallet initialization changes.
The wallet initialization code looks correct, but let's verify that:
- The AutoLockMasternodeCollaterals call is properly handled when wallet is disabled
- The WalletLoader is properly initialized before this point
🏁 Script executed:
#!/bin/bash # Check if WalletLoader is initialized before use ast-grep --pattern $'node.wallet_loader = $_' # Check if AutoLockMasternodeCollaterals handles disabled wallet case ast-grep --pattern $'AutoLockMasternodeCollaterals($_)'Length of output: 435
Action Required: Verify WalletLoader Initialization
The wallet initialization logic appears to be correctly guarded by the
ENABLE_WALLETdirective and the-disablewalletflag. However, our automated search for the initialization ofnode.wallet_loaderand the call toAutoLockMasternodeCollateralsdid not produce conclusive output (likely due to non-code UTF-8 files in the repository). Please manually verify that:
node.wallet_loaderis reliably initialized prior to its use in this code segment.AutoLockMasternodeCollateralsbehaves as expected when the wallet is disabled and that its integration withWalletLoaderis correct.src/coinjoin/client.cpp (1)
353-365: LGTM! The changes improve code clarity and maintainability.The changes to
GetStatuses()method are well-structured:
- Return type changed from
bilingual_strtostd::vector<std::string>to return individual status strings.- Using uniform initialization syntax for
fWaitForBlock.- Properly initializing the return vector.
src/init/bitcoin-node.cpp (1)
34-41: LGTM! The changes improve code organization and ownership semantics.The changes are well-structured:
- New
makeCoinJoinLoader()method properly separates concerns.- Updated
makeWalletLoader()signature improves ownership semantics by using references.src/init/bitcoind.cpp (1)
29-36: LGTM! The changes maintain consistency with bitcoin-node.cpp.The changes mirror those in bitcoin-node.cpp:
- New
makeCoinJoinLoader()method properly separates concerns.- Updated
makeWalletLoader()signature improves ownership semantics by using references.src/wallet/context.h (2)
36-36: LGTM! Default constructor addition improves usability.The addition of a default constructor allows for more flexible initialization of WalletContext instances.
31-31:✅ Verification successful
Verify null pointer checks for
coinjoin_loader.The change from const reference to raw pointer simplifies ownership semantics, but ensure that all usages of
coinjoin_loaderinclude proper null pointer checks to prevent potential crashes.Run the following script to check for null pointer checks:
🏁 Script executed:
#!/bin/bash # Description: Check for null pointer checks on coinjoin_loader # Search for direct usage without null checks rg "coinjoin_loader->" -A 2 # Search for null checks before usage rg "if.*coinjoin_loader.*\)" -A 2Length of output: 2017
Review Verified: Null Pointer Checks for
coinjoin_loaderAre AdequateThe search results confirm that all usages of
coinjoin_loaderare safely guarded with null checks. Specifically:
- In src/rpc/coinjoin.cpp, usage occurs only after verifying
node.coinjoin_loader != nullptr.- In src/wallet/wallet.cpp, the pointer is used within
if (coinjoin_loader)conditions.- The test file src/wallet/test/wallet_tests.cpp also checks for the pointer before usage.
No further action is required.
src/rpc/coinjoin.cpp (2)
47-48: LGTM! Documentation updated for new command.The help message has been updated to include the new
statuscommand.
138-173: LGTM! Well-structured new RPC command.The new
coinjoin statuscommand is well-implemented with:
- Clear documentation
- Proper parameter validation
- Consistent error handling
- Structured return format
src/wallet/init.cpp (2)
192-195: LGTM! Proper initialization order and resource management.The initialization sequence is correct:
- Create CoinJoin loader
- Create wallet loader with chain and CoinJoin loader
- Store wallet loader in node context
- Add to chain clients for lifecycle management
207-231: LGTM! Well-structured CoinJoin initialization.The initialization logic is well-organized with:
- Early validation of wallet list
- Proper error handling
- Clear logging
- Consistent method naming
src/coinjoin/interfaces.cpp (3)
47-50: LGTM! Clean implementation of JSON info retrieval.The method correctly delegates to the client manager.
55-58: LGTM! Clean implementation of session status retrieval.The method correctly returns the vector of status strings.
91-96: LGTM! Clean initialization with proper documentation.The constructor correctly:
- Takes NodeContext reference
- Initializes member variables
- Sets initial CoinJoin state
- Documents the enablement behavior
src/dummywallet.cpp (1)
32-33: LGTM! Clean interface implementation.The dummy implementations correctly maintain the interface contract while doing nothing in a wallet-disabled build.
src/wallet/interfaces.cpp (4)
150-150: LGTM!The new method
autoLockMasternodeCollaterals()provides a clean interface to the underlying wallet functionality.
557-562: LGTM!Good change to use references instead of pointers for
coinjoin_loader. This enforces that a valid loader must be provided and cannot be null.
578-578: LGTM!The updates to use references instead of pointers in these methods are consistent with the constructor changes and maintain the same safety guarantees.
Also applies to: 593-593, 600-600, 605-605
642-644: LGTM!The
MakeWalletLoaderfunction signature has been correctly updated to match the constructor changes.
, bitcoin#22742, bitcoin#19101, bitcoin#22183, bitcoin#22009, bitcoin#22938, bitcoin#23288, bitcoin#24592 (wallet backports: part 2) e2eea71 merge bitcoin#24592: Delete old line of code that was commented out (Kittywhiskers Van Gogh) cf8d5a3 merge bitcoin#23288: remove usage of LegacyScriptPubKeyMan from some wallet tests (Kittywhiskers Van Gogh) e99d065 merge bitcoin#22938: Add remaining scenarios of 0 waste, in wallet waste_test (Kittywhiskers Van Gogh) 31f8f9e merge bitcoin#22009: Decide which coin selection solution to use based on waste metric (Kittywhiskers Van Gogh) 24695e1 merge bitcoin#22183: Remove `gArgs` from `wallet.h` and `wallet.cpp` (Kittywhiskers Van Gogh) 14aa05d merge bitcoin#19101: remove ::vpwallets and related global variables (Kittywhiskers Van Gogh) 0d64290 refactor: separate out Dash-specific RPCs that rely on wallet logic (Kittywhiskers Van Gogh) 1548058 partial bitcoin#19101: remove ::vpwallets and related global variables (Kittywhiskers Van Gogh) 1fcdc96 test: remove unneeded code from some `wallet_tests` (Kittywhiskers Van Gogh) 5dc5090 refactor: move tests to match upstream order, Dash tests at the tail (Kittywhiskers Van Gogh) 2c24c63 merge bitcoin#22742: Use proper target in do_fund_send (Kittywhiskers Van Gogh) 2f3ceba merge bitcoin#22686: Use GetSelectionAmount in ApproximateBestSubset (Kittywhiskers Van Gogh) f94993c merge bitcoin#22008: Cleanup and refactor CreateTransactionInternal (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6517 * Dependent on #6594 * [bitcoin#19101](bitcoin#19101) has been split into two due to it performing two refactors in one commit, the first half deals with utilizing interfaces and managers as defined in `WalletContext` (as opposed to either passing them in separately as arguments or accessing them through globals) and the second half deals with deglobalizing wallet variables like `::vpwallets` and `cs_wallets`. * `WalletContext` still needs to be initialized with a constructor as it stores a const-ref `std::unique_ptr` of `interfaces::CoinJoin::Loader`. This requirement hasn't been done away with. Tests that don't have access to `coinjoin_loader` will still need to pass `nullptr`. * Bitcoin registers wallet RPCs through `WalletClient::registerRpcs()` ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/interfaces.cpp#L512-L522)), which a part of the `ChainClient` interface ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/interfaces/chain.h#L292-L293)). They are enumerated ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/rpcwallet.cpp#L4702-L4776)) differently from non-wallet RPCs ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/rpc/blockchain.cpp#L2638-L2682)). Wallet RPCs aren't supposed to have knowledge of `NodeContext` and likewise non-wallet RPCs aren't supposed to have knowledge of `WalletContext`. So far, Bitcoin has reworked their RPCs to maintain this separation and upstream RPCs are a part of the `libbitcoin_wallet` library. This isn't the case for some Dash RPCs, many of which reside in `libbitcoin_server`, some of which fit into two categories. * Requires knowledge of both `NodeContext` and `WalletContext` * Knowledge of `WalletContext` wasn't mandatory before deglobalization as wallet information could be accessed through the global context courtesy of `::vpwallets` and friends but now that it is, many RPCs need to be able to access `WalletContext` when otherwise they wouldn't need to. * Due to the mutual exclusivity mentioned above, RPCs that access `WalletContext` _cannot_ access `NodeContext` as their RPC registration logic doesn't give them access to `NodeContext` ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/interfaces.cpp#L516-L517), `m_context` here is `WalletContext`) but in order to give those RPCs `WalletContext`, we need to register them as wallet RPCs, which prevent us from accessing `NodeContext`. * This has been **tentatively** worked around by including a pointer to `NodeContext` in `WalletContext` ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/wallet/context.h#L47-L52)) and modifying `EnsureAnyNodeContext()` to fetch from `WalletContext` if regular behavior doesn't yield a `NodeContext` ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/rpc/server_util.cpp#L28-L37)). * Are RPCs that possess both wallet and non-wallet functionality (i.e. have "reduced" capabilities if wallet support isn't compiled in as opposed to being absent altogether). * To enable wallet functionality, the RPCs need to be _registered_ as wallet RPCs. If wallet functionality is disabled, those RPCs are not registered. Simple enough, unless you have an RPC that can _partially_ work if wallet functionality as disabled, as the lack of registration would mean those RPCs are _entirely_ inaccessible. * This is also **tentatively** worked around by registering them as non-wallet RPCs in the cases where wallet RPCs aren't registered (i.e. if wallet support isn't compiled in **or** if wallet support is disabled at runtime). * Bitcoin doesn't use `#ifndef ENABLE_WALLET` for checking if support is absent, we're expected to use `!g_wallet_init_interface.HasWalletSupport()` (which will always evaluate `false` if support isn't compiled in, [source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/dummywallet.cpp#L19), as the stub wallet would be utilized to initialize the global in those cases, [source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/dummywallet.cpp#L57)). This has been done in the PR. * A notable change in this PR is that a lot of behavior that would be enabled if support was compiled in but disabled at runtime would now be disabled if support was disabled at runtime as `wallet_loader` won't be initialized if runtime disablement occurs ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/init.cpp#L128-L131)), which in turns means that anything that relies on `wallet_loader` wouldn't work either, such as `coinjoin_loader`. This means that we also need to register the RPC as a non-wallet RPC if support is compiled in but disabled at runtime ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/rpc/evo.cpp#L1841-L1843)). * To register RPCs as wallet RPCs, generally they're done through `WalletClient::registerRpcs()`, which iterates through `GetWalletRPCCommands()`. This is perfectly fine as both reside in `libbitcoin_wallet`. Unfortunately, our Dash RPCs reside in `libbitcoin_server` and `registerRpcs()` cannot be extended to enumerate through RPCs not included in its library. We cannot simply move the Dash RPCs either (at least in its current state) as they rely on non-wallet logic, so moving the source files for those RPCs into `libbitcoin_wallet` would pull more or less, all of `libbitcoin_server` along with it. * To **tentatively** get around this, a new method has been defined, `WalletLoader::registerOtherRpcs()`, which will accept any set of RPCs for registration ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/wallet/interfaces.cpp#L603-L606)) and we use it in RPC init logic ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/init.cpp#L1513-L1528)). * Some usage of `CoreContext` had to be removed ([source](b27d2dc#diff-157f588a09ff1da05b8c74acfcfb4da21917b6a97ed5d2e702228d621d23e66dL1024)) as without those removals, the test suite would crash. <details> <summary>Crash:</summary> ``` dash@01fd4f6cfa52:/src/dash$ lldb ./src/test/test_dash (lldb) target create "./src/test/test_dash" Current executable set to '/src/dash/src/test/test_dash' (x86_64). (lldb) r -t wallet_tests Process 653232 launched: '/src/dash/src/test/test_dash' (x86_64) Running 17 test cases... unknown location(0): fatal error: in "wallet_tests/CreateTransactionTest": unknown type wallet/test/wallet_tests.cpp(1052): last checkpoint *** 1 failure is detected in the test module "Dash Core Test Suite" Process 653232 exited with status = 201 (0x000000c9) ``` </details> * The assertion introduced in [bitcoin#22686](bitcoin#22686) ([source](https://github.com/achow101/bitcoin/blob/92885c4f69a5e6fc4989677d6e5be8a666fbff0d/src/wallet/spend.cpp#L781-L783)) has not been backported as this assertion is downgraded to an error in [bitcoin#26611](bitcoin#26611) and then further modified in [bitcoin#26643](bitcoin#26643) ([source](bitcoin@c1a84f1#diff-6e06b309cd494ef5da4e78aa0929a980767edd12342137f268b9219167064d13R1016-R1020)), portions of which are already included in [dash#6517](#6517) ([source](https://github.com/dashpay/dash/blob/012298acb071a45fa943653197ccc4f2c48a75a4/src/wallet/wallet.cpp#L3832-L3835)). * [bitcoin#23288](bitcoin#23288) finished what e3687f7 ([dash#6078](#6078)) started and `coinselection_tests` has now been realigned with upstream. * Dash-specific tests in `wallet_tests` (as introduced in [dash#3367](#3667)) have not been moved over to descriptor wallets and are still based on legacy wallets (to this effect, the old `AddKey` logic has been retained as `AddLegacyKey`, [source](https://github.com/dashpay/dash/blob/d4e8d1f1628933d9d9159fc261b22fc4427a1c7a/src/wallet/test/wallet_tests.cpp#L873-L879)). * To prevent merge conflicts, Dash-specific tests have been moved to the end of the source file and demarcated by a comment ([source](https://github.com/dashpay/dash/blob/d4e8d1f1628933d9d9159fc261b22fc4427a1c7a/src/wallet/test/wallet_tests.cpp#L868)). ## How Has This Been Tested? 6813863 was tested on Debian 12 (`bookworm`) mixing ~1 tDASH on default settings.  ## Breaking Changes * The RPCs `coinjoin`, `coinjoinsalt`, `getpoolinfo`, `gobject list-prepared`, `gobject prepare`, `gobject vote-alias`, `gobject vote-many`, `masternode outputs`, `protx update*`, `protx register*` and `protx revoke` will no longer be available if wallet support is disabled at runtime. This is not a change in **functionality** as they were already inoperative with disabled wallets but is a change in **reporting** as they would not be available at all. ## 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 **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: LGTM, utACK e2eea71 PastaPastaPasta: utACK e2eea71 Tree-SHA512: b3237670c2c861a353ce2486a1f0932b0bfcc6df488592cff28b85a49ed96c4612fc88866d0808e6d2cf0d409e4b8e8120e605acd7367b4bf554009672d3e9ea
3 participants
Motivation
Since bitcoin#22219 (introduced in dash#6568), the workarounds implemented for allowing the backport of bitcoin#19101 (a part of dash#6529) no longer work. A major reason for this is the initialization order assumed in dash#6529 is no longer holds true, requiring a rework of CoinJoin interfaces. This pull request aims to do that.
Additional Information
Dependency for backport: merge bitcoin#22008, #22686, #22742, #19101, #22183, #22009, #22938, #23288, #24592 (wallet backports: part 2) #6529
To allow for dropping
walletman()fromCoinJoin::Loader, bothCoinJoin::LoaderandCoinJoin::Client(accessible throughCoinJoin::Loader::GetClient()) need to account for all potential usage.To that end,
CoinJoin::Client::get{JsonInfo, SessionStatuses}()have been implemented.Though some invocations of
CCoinJoinClientSessionfunctions cannot (or rather, should not) be available through the interface (likeDoAutomaticDenominating()).To take care of one such invocation (in
coinjoin start, source), it has been removed altogether.coinjoin startwill now behave like the "Start CoinJoin" button in Dash Qt and won't try to get ahead of the scheduler and start denomination.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 throughcoinjoin startand encountering a fail state).CoinJoin::LoaderandWalletLoaderrely on each other asCoinJoin::Loader::{Add,Remove}Wallet()needs to be able to runWalletInitInterface::InitCoinJoinSettings(), which in turn needs to be able enumerate through all available wallets (source), currently doing so through the global wallet context but eventually throughWalletLoaderwhen said globals are no longer available (post-bitcoin#19101)WalletLoaderneedsCoinJoin::Loaderto load (source) and restore (source) wallets asCWalletneeds access toCoinJoin::Loaderin order to callCoinJoin::Loader::{Add,Remove}Wallet()(source, source)This interdependent relationship has currently been floating along by passing a const ref of the smart pointer (source), which makes for cumbersome but workable code but in combination with bitcoin#22219, it breaks bitcoin#19101 as it forces us to definitively initialize both
WalletLoaderandCoinJoin::Loaderbefore we get to initialize CoinJoin logic.To work around this, instead of requiring a fully initialized
CoinJoinWalletManagerto create aCoinJoin::Loader, we instead takeNodeContextand defer resolvingCoinJoinWalletManagerto when interface calls are made.By moving the initialization of the CoinJoin loader from
AppInitMain()toWalletInit::Construct(), the linker was unhappy with trying to emitbench_dash(see error below). This was remedied by linkinglibbitcoin_walletwithlibtest_util.Linker error:
bitcoin#22219 removes a
WalletInit::Construct()call fromsetup_common.cpp(source), which is necessary sinceWalletInit::Construct()now relies onnode.init(source).This isn't so much a problem for tests that use
WalletTestingSetuporInitWalletDirTestingSetupbut creates problems for tests based onTestChain100Setup(source, source), which is a specialization ofTestingSetupthat in turn, used to initializecoinjoin_loader(source) but since initialization has been moved toWalletInit::Construct()in line withInit::makeWalletLoader(), it is not done automatically anymore.To work around this,
WalletInit::Construct()-like code has been introduced to initialize bothcoinjoin_loaderandwallet_loader(source).Breaking Changes
coinjoin startwill 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