backport: merge bitcoin#21207, #22008, #22686, #22742, #19101, #22183, #22009, #22938, #23288, #24592 (wallet backports: part 2)

[kwvg]

Additional Information

• Dependent on backport: merge bitcoin#25497, #17331, #22155 (wallet backports: part 1) #6517

• 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), which a part of the ChainClient interface (source). They are enumerated (source) differently from non-wallet RPCs (source).

    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, 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) and modifying EnsureAnyNodeContext() to fetch from WalletContext if regular behavior doesn't yield a NodeContext (source).

    • 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, as the stub wallet would be utilized to initialize the global in those cases, source). 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), 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).

  • 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) and we use it in RPC init logic (source).

  • Some usage of CoreContext had to be removed (source) as without those removals, the test suite would crash.

    Crash:

    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)
    

• The assertion introduced in bitcoin#22686 (source) has not been backported as this assertion is downgraded to an error in bitcoin#26611 and then further modified in bitcoin#26643 (source), portions of which are already included in dash#6517 (source).

• bitcoin#23288 finished what e3687f7 (dash#6078) started and coinselection_tests has now been realigned with upstream.

  • Dash-specific tests in wallet_tests (as introduced in dash#3367) 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).

    • To prevent merge conflicts, Dash-specific tests have been moved to the end of the source file and demarcated by a comment (source).

Notable Changes

• CoinJoinWalletManager::{Add,Remove}() will no longer call WalletInit::InitCoinJoinSettings() if not called through the interface (i.e. interfaces::CoinJoin::Loader::{Add,Remove}()). If not using the interface, those calls will need to be made manually.

  It is recommended to use the interface where possible.

• NodeContext::coinjoin_loader will not be initialized at all if wallet support is disabled at runtime. (Earlier behavior allowed for initialization so long as wallet support was included at compile-time, now it must be enabled at runtime as well).

  • A similar behavior change is present for the WalletInit::AutoLockMasternodeCollaterals() (also moved out of ThreadImport()) during init but since if wallet support is disabled at runtime, there would be no collaterals to lock, this change isn't of concern.

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

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation (note: N/A)
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

This pull request has conflicts, please rebase.

[PastaPastaPasta]

we don't really need any of this; at least we should set DEFAULT_CONSOLIDATE_FEERATE to 0 or something to basically disable this logic?

[kwvg]

Not sure if this is the best idea since m_consolidate_feerate is used to define coin_selection_params.m_long_term_feerate in CreateTransactionInternal (source) though I did find that the old code generated a fee-per-KB of 1000 and the commit changes it to a fee-per-KB of 10000.

This has been corrected in latest push.

[coderabbitai]

Walkthrough

The changes span multiple components including coin selection, wallet management, RPC command registration, and testing. The coin selection logic has been updated with the renaming of a key method from “SelectCoinsMinConf” to “AttemptSelection” and the addition of a dedicated waste calculation function. Wallet management now consistently uses a new context structure ("WalletContext") and introduces a new WalletLoader class, leading to updates in wallet creation, loading, removal, and initialization methods. Legacy approaches—such as the use of the legacy script public key manager and address import routines—have been replaced with descriptor-based mechanisms. RPC command registrations are reorganized into modular functions with distinct implementations for wallet-specific commands, and tests have been modified accordingly, including the addition of a new test for fee calculation. These updates standardize naming conventions, improve context management, and refactor code across components while retaining overall functionality.

✨ Finishing Touches

• 📝 Generate Docstrings (Beta)

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 5

🔭 Outside diff range comments (3)

src/wallet/wallet.cpp (1)

412-412: ⚠️ Potential issue

Duplicate definition of AddWallet detected

A second bool AddWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet) function appears to be defined here, with nearly identical logic to the one at lines [112-126], risking a merge conflict or ODR violation. Remove or merge the extra definition to restore clarity and compilation safety:

-// Potential removal of the duplicate function starting at line 412:
-bool AddWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet)
-{
-    // ...
-    return true;
-}

Failure to address this duplication will likely break builds or cause runtime conflicts.

src/rpc/evo.cpp (1)

1-1: ⚠️ Potential issue

Resolve conflicts with other PRs.

The pipeline indicates potential conflicts with other pull requests. Please rebase your branch on the latest main branch and resolve any conflicts.

🧰 Tools

🪛 GitHub Actions: Check Potential Conflicts

[error] 1-1: Conflict detected with multiple pull requests.

src/init.cpp (1)

1-2460: ⚠️ Potential issue

Warning: Potential merge conflicts detected.

The pipeline failure indicates conflicts with other PRs. Please resolve the conflicts before merging.

🧰 Tools

🪛 Cppcheck (2.10-2)

[error] 830-830: syntax error

(syntaxError)

🪛 GitHub Actions: Check Potential Conflicts

[error] 1-1: Conflict detected with multiple pull requests.

🧹 Nitpick comments (28)

src/wallet/load.cpp (3)

58-58: Loading default (unnamed) wallet conditionally is clear.
If additional wallet constraints (like encryption or custom options) arise, review that this logic gracefully handles them.

---

125-131: Creation of wallet with CWallet::Create and immediate addition via AddWallet is well-sequenced.
Make sure that over time, if more wallet configuration steps are added, they occur before usage.

---

164-166: StopWallets approach
Closing wallets in a loop is straightforward. Ensure that any new potential concurrency with the scheduler is addressed (e.g., ensure tasks are not referencing closed wallets).

src/qt/test/wallettests.cpp (1)

118-127: Descriptor-based setup logic
Properly setting up scriptPubKey managers and adding the descriptor within the locked wallet is a clear workflow. Test coverage on these steps is recommended to validate descriptor function.

src/wallet/interfaces.cpp (1)

568-580: New RegisterRPCs method
Declares a local request object with context = m_context. This approach isolates wallet-specific RPCs. Encourage further decoupling of non-wallet RPC logic if needed.

src/wallet/test/coinselector_tests.cpp (3)

130-139: Assess code reuse for KnapsackGroupOutputs.
This helper function duplicates logic from existing grouping methods but imposes zero-based fees (CFeeRate(0)) for testing. If additional scenarios require different fee rates, consider parameterizing them to avoid code duplication.

---

271-271: Improve inline comment clarity.
The comment referencing effective value for AttemptSelection is fairly brief. Consider expanding it to clarify why BnB must handle negative effective values carefully.

---

300-300: Clarify block scope.
A new block is opened at line 300 without an explanatory comment. Consider adding a short note describing the test scope or using a separate test function for clarity.

src/wallet/wallet.h (3)

52-52: Context parameter injection.
Introducing WalletContext& as the first parameter in wallet management functions centralizes context usage. This is a good pattern to reduce global dependencies but may need updates in unit tests or legacy code that previously used static references.

---

63-70: Unified wallet load/unload functions.
Replacing separate calls with AddWallet, RemoveWallet, LoadWallet, CreateWallet, etc. that accept a WalletContext& clarifies the relationship between wallets and the node context. This enforces better scoping but watch for potential initialization order issues if called too early.

---

1090-1091: MaybeResendWalletTxs consolidation.
Global function now requires WalletContext&, ensuring that no global wallet lists are used. This is consistent with deglobalization efforts. Confirm older code uses the new pattern.

src/wallet/test/wallet_tests.cpp (1)

93-102: AddKey now leverages descriptor-based approach
Transitioning to descriptors is correct. However, relying on assert(false) for error handling can abruptly terminate tests. Consider throwing an exception or returning an error to facilitate robust handling.

src/wallet/wallet.cpp (6)

112-126: Check return values for downstream calls

While adding the wallet to the global list is carefully protected by a scoped mutex lock, note that some subsequent helper calls (ConnectScriptPubKeyManNotifiers, coinjoin_loader().AddWallet, etc.) do not have their return values checked. It might be beneficial to verify their success or at least log potential failures.

---

128-150: Consider error handling after RemoveWallet

This function properly locks the wallet list and removes the wallet. However, similar to the counterpart in AddWallet, the downstream call wallet->coinjoin_loader().RemoveWallet(name) is not checked for errors. Consider adding logging or conditional handling to help diagnose unexpected failures or partial states.

---

152-156: Delegate function could be consolidated

This overload simply delegates to the version accepting a std::vector<bilingual_str>& warnings. If there’s no compelling usage outside of simplifying calls, you could consider consolidating them to reduce maintenance overhead.

---

158-162: Linear lookup of wallets

The function locks the wallet vector and then returns it. For systems expected to carry many wallets, consider using a more direct data structure (e.g., an unordered container) if lookups or enumerations become performance-critical. Currently, this is fine for smaller sets of wallet contexts.

---

283-367: Streamline creation & initialization

This large block effectively orchestrates wallet creation, optional HD usage, essential calls to AddWallet, and UpdateWalletSetting. A few suggestions:

1. Repeated initMessage: The chain’s initMessage("Loading wallet…") is called in multiple places. Consider centralizing or deduplicating these progress messages if they appear in multiple flows.
2. Error propagation: Similar to AddWallet, error returns from calls like coinjoin_loader().AddWallet (if any) could be handled or at least logged to clarify partial failures.
3. -usehd logic: The HD-related branch at line 313 might be more self-documenting if you add logs clarifying whether the existing wallet is updated or remains as-is.

Overall, these refinements can make the creation process more robust and maintainable.

---

391-391: Missing null-check after LoadWallet

After calling LoadWallet(...), consider explicitly checking if wallet is null to log or handle the error path more verbosely, especially if status or error indicates a partial failure or ongoing conflict.

src/wallet/rpcwallet.cpp (2)

2956-2959: Refine reporting of database operation outcomes.

Storing the outcomes in status, error, and warnings is good practice. However, consider logging or returning partial diagnostic data to help pinpoint root causes when RestoreWallet fails or returns warnings. This extra insight can speed up debugging in production.

Also applies to: 2961-2961

---

3013-3013: Incorporate warning details during removal failure.

If RemoveWallet(...) fails, returning the collected warnings in the thrown error message can aid troubleshooting. This will give end users and developers more context into why the removal might have failed.

src/wallet/context.h (1)

47-52:

❓ Verification inconclusive

Plan removal of NodeContext dependency.

The temporary node_context member allows wallet RPCs to access non-wallet capabilities. While pragmatic for now, this should be removed to maintain proper separation of concerns.

Let's identify wallet RPCs that depend on NodeContext:

---

🏁 Script executed:

#!/bin/bash
# Search for wallet RPC implementations accessing NodeContext
rg "node_context->" src/wallet/rpc/

Length of output: 37

---

Plan to Remove NodeContext Dependency – Please Verify Indirect Usage

The temporary node_context member in src/wallet/context.h (lines 47–52) currently enables wallet RPCs to access non-wallet capabilities. An initial search for direct references (using node_context-> in src/wallet/rpc/) produced no hits. However, please manually verify that wallet RPCs or related code do not access NodeContext indirectly before proceeding with removal:

• Location: src/wallet/context.h (lines 47–52)
• Action: Confirm that no wallet RPC implementation (or other wallet-related modules) relies on node_context to access non-wallet features.
• Next Step: Once verified, plan for the complete removal of the node_context member to ensure a proper separation of concerns.

src/bench/wallet_balance.cpp (1)

47-50: Consider documenting the epoch iteration values.

While the epoch iteration values (2500, 8000, 16000) seem reasonable, it would be helpful to document why these specific values were chosen.

Add comments explaining the rationale:

-static void WalletBalanceDirty(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ true, /* add_mine */ true, 2500); }
-static void WalletBalanceClean(benchmark::Bench& bench) {WalletBalance(bench, /* set_dirty */ false, /* add_mine */ true, 8000); }
-static void WalletBalanceMine(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ false, /* add_mine */ true, 16000); }
-static void WalletBalanceWatch(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ false, /* add_mine */ false, 8000); }
+// Lower iterations for dirty benchmarks as they're slower
+static void WalletBalanceDirty(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ true, /* add_mine */ true, 2500); }
+// Clean operations can handle more iterations
+static void WalletBalanceClean(benchmark::Bench& bench) {WalletBalance(bench, /* set_dirty */ false, /* add_mine */ true, 8000); }
+// Mining operations need more iterations for accurate measurements
+static void WalletBalanceMine(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ false, /* add_mine */ true, 16000); }
+// Watch-only operations use moderate iterations
+static void WalletBalanceWatch(benchmark::Bench& bench) { WalletBalance(bench, /* set_dirty */ false, /* add_mine */ false, 8000); }

src/dummywallet.cpp (1)

36-36: Document the consolidatefeerate parameter format.

The new hidden argument needs documentation for its expected format and valid range.

Add a comment explaining the parameter:

-        "-consolidatefeerate=<amt>",
+        // Fee rate (in DASH/kB) for consolidation transactions
+        "-consolidatefeerate=<amt>",

src/coinjoin/interfaces.cpp (1)

69-74: Consider initializing order and member initialization list style.

While the initialization is correct, the style could be improved for consistency.

Consider this style:

-    explicit CoinJoinLoaderImpl(CoinJoinWalletManager& walletman, interfaces::WalletLoader& wallet_loader) :
-        m_walletman{walletman},
-        m_wallet_loader{wallet_loader}
-    {
-        g_wallet_init_interface.InitCoinJoinSettings(m_wallet_loader, m_walletman);
-    }
+    explicit CoinJoinLoaderImpl(
+        CoinJoinWalletManager& walletman,
+        interfaces::WalletLoader& wallet_loader)
+        : m_walletman(walletman)
+        , m_wallet_loader(wallet_loader)
+    {
+        g_wallet_init_interface.InitCoinJoinSettings(m_wallet_loader, m_walletman);
+    }

src/rpc/server_util.cpp (1)

29-33: Document the workaround's limitations and risks.

The TODO comment indicates this is a temporary solution. Consider documenting potential issues and limitations.

Add detailed documentation:

-    // We're now going to try our luck with WalletContext on the off chance
-    // we're being called by a wallet RPC that's trying to access NodeContext
-    // See comment on WalletContext::node_context for more information.
-    // TODO: Find a solution that removes the need for this workaround
+    // WORKAROUND: Attempt to retrieve NodeContext from WalletContext
+    // This is a temporary solution for wallet RPCs that need NodeContext access.
+    // 
+    // Limitations:
+    // 1. Creates implicit dependency between wallet and node contexts
+    // 2. May hide architectural issues in RPC design
+    // 3. Could lead to subtle bugs if context ownership changes
+    // 
+    // TODO: Implement proper context management system that:
+    // - Maintains clear separation of concerns
+    // - Provides explicit context access patterns
+    // - Eliminates need for context fallback logic

src/wallet/coinselection.cpp (1)

406-431: LGTM: New waste calculation function.

The GetSelectionWaste function is well-implemented with:

• Proper input validation using assert
• Clear waste calculation logic
• Appropriate handling of change costs
• Good documentation of the calculation process

However, consider adding a test case to verify the waste calculation logic.

Would you like me to generate a test case for the GetSelectionWaste function?

test/functional/rpc_fundrawtransaction.py (1)

957-959: Rename unused loop variable.

The loop control variable i is not used within the loop body. Consider renaming it to _ or _i to indicate it's intentionally unused.

-        for i in range(0, 3):
+        for _ in range(0, 3):

🧰 Tools

🪛 Ruff (0.8.2)

957-957: Loop control variable i not used within loop body

Rename unused i to _i

(B007)

src/rpc/evo.cpp (1)

1811-1848: Consider future refactoring of hybrid RPCs.

The current implementation includes a well-documented workaround for handling wallet-disabled scenarios. As noted in the TODO comment, consider splitting these hybrid RPCs into dedicated wallet-only and wallet-free RPCs in the future to eliminate this workaround.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 265d9b5 and 6813863.

📒 Files selected for processing (44)

• src/bench/coin_selection.cpp (1 hunks)
• src/bench/wallet_balance.cpp (2 hunks)
• src/bitcoin-cli.cpp (1 hunks)
• src/coinjoin/client.cpp (2 hunks)
• src/coinjoin/interfaces.cpp (3 hunks)
• src/dummywallet.cpp (2 hunks)
• src/init.cpp (4 hunks)
• src/interfaces/coinjoin.h (2 hunks)
• src/interfaces/wallet.h (5 hunks)
• src/node/blockstorage.cpp (0 hunks)
• src/qt/test/addressbooktests.cpp (2 hunks)
• src/qt/test/wallettests.cpp (3 hunks)
• src/rpc/coinjoin.cpp (3 hunks)
• src/rpc/evo.cpp (3 hunks)
• src/rpc/governance.cpp (1 hunks)
• src/rpc/masternode.cpp (1 hunks)
• src/rpc/register.h (2 hunks)
• src/rpc/server_util.cpp (1 hunks)
• src/test/util/setup_common.cpp (1 hunks)
• src/test/util/wallet.cpp (0 hunks)
• src/wallet/coinselection.cpp (3 hunks)
• src/wallet/coinselection.h (1 hunks)
• src/wallet/context.cpp (1 hunks)
• src/wallet/context.h (2 hunks)
• src/wallet/init.cpp (3 hunks)
• src/wallet/interfaces.cpp (8 hunks)
• src/wallet/load.cpp (7 hunks)
• src/wallet/load.h (2 hunks)
• src/wallet/rpc/util.cpp (1 hunks)
• src/wallet/rpcwallet.cpp (6 hunks)
• src/wallet/spend.cpp (5 hunks)
• src/wallet/test/coinjoin_tests.cpp (3 hunks)
• src/wallet/test/coinselector_tests.cpp (21 hunks)
• src/wallet/test/init_test_fixture.cpp (1 hunks)
• src/wallet/test/psbt_wallet_tests.cpp (3 hunks)
• src/wallet/test/util.cpp (2 hunks)
• src/wallet/test/wallet_test_fixture.cpp (1 hunks)
• src/wallet/test/wallet_tests.cpp (15 hunks)
• src/wallet/wallet.cpp (23 hunks)
• src/wallet/wallet.h (8 hunks)
• src/wallet/walletdb.cpp (1 hunks)
• src/wallet/walletdb.h (2 hunks)
• src/walletinitinterface.h (2 hunks)
• test/functional/rpc_fundrawtransaction.py (2 hunks)

💤 Files with no reviewable changes (2)

• src/test/util/wallet.cpp
• src/node/blockstorage.cpp

✅ Files skipped from review due to trivial changes (2)

• src/coinjoin/client.cpp
• src/bitcoin-cli.cpp

🧰 Additional context used

🪛 GitHub Actions: Check Potential Conflicts

src/init.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/test/util/setup_common.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/rpc/evo.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/rpc/masternode.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/wallet/rpcwallet.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/rpc/governance.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/wallet/wallet.h

[error] 1-1: Conflict detected with multiple pull requests.

src/wallet/wallet.cpp

[error] 1-1: Conflict detected with multiple pull requests.

🪛 GitHub Actions: Clang Diff Format Check

src/rpc/evo.cpp

[warning] 1805-1805: Clang format differences found. Please format the code according to Clang standards.

src/rpc/masternode.cpp

[warning] 740-740: Clang format differences found. Please format the code according to Clang standards.

src/rpc/coinjoin.cpp

[warning] 486-486: Clang format differences found. Please format the code according to Clang standards.

src/rpc/governance.cpp

[warning] 1075-1075: Clang format differences found. Please format the code according to Clang standards.

🪛 Ruff (0.8.2)

test/functional/rpc_fundrawtransaction.py

957-957: Loop control variable i not used within loop body

Rename unused i to _i

(B007)

🔇 Additional comments (159)

src/wallet/load.h (2)

15-15: Forward declaration is clear and standard.
This forward declaration avoids unnecessary includes and circular dependencies. No issues found.

---

25-40:

✅ Verification successful

Verify that all calling sites have been updated to use the new WalletContext& parameter.
Switching from multiple parameters (e.g., interfaces::Chain) to a consolidated WalletContext& is a good design for maintainability. However, please confirm that any surrounding code and all function call sites have been updated accordingly, to avoid build or runtime errors.

---

🏁 Script executed:

#!/bin/bash
# Description: Verify that no outdated calls remain
rg -A2 "VerifyWallets\(interfaces::Chain" 
rg -A2 "LoadWallets\(interfaces::Chain" 
rg -A2 "StartWallets\(interfaces::Chain"
rg -A2 "FlushWallets()"
rg -A2 "StopWallets()"
rg -A2 "UnloadWallets()"

Length of output: 1783

---

All calling sites correctly use the new WalletContext parameter
The searches confirmed that there are no outdated calls remaining that reference interfaces::Chain—all invocations now correctly pass the consolidated WalletContext&. No further modifications are necessary based on the current state of the code.

src/wallet/rpc/util.cpp (3)

56-57: Ensure request.context is valid.
Acquiring the wallet context with EnsureWalletContext(request.context) is appropriate. If request.context is ever null or uninitialized, EnsureWalletContext will correctly throw an error, preventing undefined behavior.

---

60-60: Context-based wallet retrieval.
Switching from a global or direct approach to using GetWallet(context, wallet_name) clarifies ownership and scope of wallet objects. The error handling for nonexistent wallets is also correct.

---

65-65: Context-based wallet enumeration.
Using GetWallets(context) ensures that all loaded wallets are retrieved in a context-aware manner, aligning with the updated wallet design.

src/qt/test/addressbooktests.cpp (2)

69-73: Switch to descriptor-based wallets with proper locking.
Enabling WALLET_FLAG_DESCRIPTORS and then calling SetupDescriptorScriptPubKeyMans() within a locked wallet context is a solid approach for modern descriptor wallets. This ensures thread safety during wallet setup.

---

117-120: Context-based wallet lifecycle management.
Acquiring the wallet context from node.walletLoader() and using AddWallet/RemoveWallet reflects a consistent, modular approach. This helps prevent concurrency issues and keeps wallet state transitions clear.

src/wallet/load.cpp (7)

14-14: No issues with adding these includes.
They provide necessary references for assertion checks and wallet context usage.

Also applies to: 18-18

---

26-29: Encapsulation of chain and args in WalletContext is a clean approach.
Consolidating these references into a single context object simplifies function signatures and increases maintainability.

---

31-49: Check for edge cases when forcing -walletdir.
Ensuring the canonical path is valid and absolute is good. However, if args.ForceSetArg is called repeatedly or under different contexts, consider verifying that subsequent wallet logic still behaves as intended.

---

104-106: Refactoring LoadWallets to accept WalletContext& aligns well with your new architecture.
This helps ensure all necessary objects (chain, args, etc.) are consistently accessible.

---

140-150: Scheduler captures context by reference.
Verify that context outlives all scheduled tasks to avoid dangling references if the scheduler outlives the main scope.

---

153-155: FlushWallets usage
Flushing wallets here maintains consistency with existing design. No additional concerns.

---

171-178: Cautious unload approach
Removing from the context prior to unloading ensures the wallet is no longer tracked. This is a standard pattern and looks correct.

src/qt/test/wallettests.cpp (3)

112-116: Initialization with WalletContext and immediate descriptor flag
Good transition to descriptor-based wallets. This ensures the new wallet is recognized from the start.

---

147-147: Constructing WalletModel with MakeWallet(context, wallet)
The updated approach is consistent and ensures the wallet's context is properly injected.

---

259-259: Removal of wallet from test environment
A direct call to RemoveWallet(context, wallet, std::nullopt) is correct for test teardown. No issues found.

src/wallet/interfaces.cpp (7)

130-131: WalletImpl constructor uses a WalletContext&
This centralizes context-dependent logic. Good step towards modularizing wallet functions.

---

150-150: autoLockMasternodeCollaterals() bridging function
Straightforward delegate call. Ensure any future masternode-collateral changes remain in sync.

---

515-515: Removing wallet with stored m_context
Using RemoveWallet(m_context, m_wallet, false) is consistent with the new architecture. Verify that this doesn’t conflict with any dynamic load/unload.

---

582-590: WalletLoaderImpl constructor
Initializing m_context with coinjoin loader, args, chain, and node context is a cohesive approach. The destructor call to UnloadWallets(m_context) ensures resources are freed.

---

593-604: ChainClient life-cycle methods
Delegating to VerifyWallets, LoadWallets, StartWallets, etc., keeps the code flows uniform. No immediate issues.

---

605-659: Other wallet loader methods
The consistent usage of m_context to create, load, or restore wallets fosters a stable code flow. Adding assert(m_context.coinjoin_loader) ensures the loader is always valid.

---

669-673: MakeWallet and MakeWalletLoader
Factories now require WalletContext& which unifies context usage. This simplifies code that previously had to juggle multiple references.

src/wallet/test/coinselector_tests.cpp (45)

44-54: Add checks for negative effective_value.
The logic for subtracting the fee from nValue could result in a negative effective_value if fee is unexpectedly larger than nValue. Consider adding a safety check or assertion to ensure that effective_value doesn't go negative and is handled gracefully.

---

125-128: Confirm correctness of KnapsackSolver overload.
This inline overload effectively forwards to the main KnapsackSolver. Ensure that the default arguments (fFulyMixedOnly=false, maxTxFee=DEFAULT_TRANSACTION_MAXFEE) are appropriate for all uses, especially in tests covering corner cases with high transaction fees.

---

289-289: Validate negative effective value rejection.
This test checks that a negative effective value triggers selection failure as expected. Good coverage of this edge case. No issues found.

---

296-296: Confirm fee subtraction behavior in test.
The test sets m_subtract_fee_outputs = true before selection, ensuring BnB can still succeed. Confirm that changes in upstream test logic won't break the scenario.

---

305-306: Descriptor wallet flag usage.
Enabling WALLET_FLAG_DESCRIPTORS and calling SetupDescriptorScriptPubKeyMans() is correct for descriptor-based testing. Confirm that the wallet can gracefully handle non-descriptor scenarios if tested side by side.

---

322-322: Knapsack solver test initialization.
This new test block is well-structured. Ensure coverage includes typical and corner cases (very large inputs, dust outputs, etc.).

---

338-338: Check repeated test runs.
Line 338 resets the coin list. This ensures consistent test permutations. No immediate issues; the logic is clear and isolated.

---

340-340: Validate filter_standard usage.
Test ensures that an immature coin isn't selected by standard filtering. This is correct. Keep an eye on future changes to default filter parameters.

---

344-344: Multiple consecutive checks.
Repeated BOOST_CHECK calls help verify the transition from no solution to a valid solution. Tests are well-structured.

---

346-346: Coverage for newly added 1-cent coin.
This ensures new, but immature coin selection still fails under standard constraints. Good negative test.

---

348-348: Enabling filter_confirmed for partial coverage.
Switching to filter_confirmed to allow maturity is consistent. The flow from no selection to successful selection is validated. Looks fine.

---

354-354: Insufficient mature coins scenario.
Ensures there’s no solution with only standard-eligible coins for 3 CENT. Good boundary coverage.

---

357-357: Confirm selection with newly added coins.
Switching to filter_confirmed to enable picking up the newly added 2 CENT coin is appropriate. Test logic is consistent.

---

367-367: Disallow new coins for large target.
Demonstrates that we cannot raise enough from old coins alone. Succeeds in showing standard extra filter failing. Looks correct.

---

369-369: Extra constraints test.
Using filter_standard_extra (6 confirmations for coins from external sources) clarifies a scenario with restricted coin usage. The negative check is valid.

---

371-371: Positive test with partial new coins.
Now allowing new coins from our own wallet demonstrates a successful selection. This progressive testing approach is good.

---

374-374: Full new coin acceptance test.
Confirms that if we accept all new coins, large sums like 38 CENT are reachable. No changes recommended.

---

378-378: Exact subset edge case.
Covers behavior when an exact solution is unavailable but a near subset is found. The test is well-defined and clarifies fallback strategy.

---

383-383: Smaller coin usage scenario.
Choosing combination of smaller coins for 7 CENT. The test verifies correct subset selection. No negative findings.

---

388-388: Exact match with multiple small coins.
Verifies successful 8-cent selection with three smaller coins. Good demonstration of knapsack’s subset approach.

---

394-394: Nearest coin fallback.
Selecting 9 CENT from a 10-cent coin tests fallback if smaller subsets can’t match exactly. Logical coverage is robust.

---

406-407: Testing boundary of total wallet amount.
Lines 406-407 confirm that 71 CENT is possible, but 72 is not. Thorough boundary coverage. No issues found.

---

411-411: One-coin preference.
Check that for 16 CENT, the solver picks the next largest single coin (20 CENT) if smaller subsets exceed. Good demonstration of knapsack preference logic.

---

418-419: Refine logic for smaller coin combos.
Demonstrates a scenario where multiple smaller coins produce a better fit than a single large coin. Good edge test.

---

425-426: Tie-breaking rule demonstration.
When multiple solutions match the target but differ in subset size, the test ensures the biggest coin wins in a tie.

---

430-430: Summation check for 11-cent target.
Multi-coin approach (5 + 6). Straightforward. Verified logic with no concerns.

---

439-439: Confirm smaller coin preference for 95 CENT.
Checks that a single 1 BTC coin is chosen if it meets or just exceeds the target. Behaves as expected.

---

443-444: 195-cent scenario.
Similarly ensures 2 BTC coin is chosen. No issues found.

---

495-496: Exact subset fallback test.
With small coins, one scenario will pick a bigger coin if no exact subset is found. Another scenario tries an exact subset. Clear coverage.

---

534-535: Repetitive test loop clarity.
These nested loops test multiple scenarios. The approach systematically checks partial sets. Looks good for coverage.

---

561-562: Mid-range target selection.
Selecting 50 BTC from 100 identical coins ensures that the solver picks half the coins. The random shuffle test is correct.

---

571-572: Random single-coin picks.
Ensures that selecting 1 BTC from 100 identical coins shows randomness in solver picks. Good stress scenario.

---

593-594: Choice among multiple “smallest bigger” coins.
Tests 90-cent target with multiple near-fitting coins. The solver picks among them randomly. Good to confirm.

---

672-673: Introduction of waste_test.
This newly added test block specifically checks GetSelectionWaste. It’s a valuable addition for validating advanced coin selection heuristics.

---

674-680: Basic fee parameters initialization.
Initial definitions (fee, change_cost, fee_diff, …) are straightforward and clarify test input for subsequent checks.

---

681-687: Waste with change scenario.
Validates if the correct waste is computed (change_cost + difference in fees). Good to confirm correct formula usage.

---

689-695: Waste without change scenario.
Checks that the computed waste includes leftover coin value (excess) plus fee differences. Implementation is consistent with coin selection logic.

---

696-702: Zero difference between fee and long-term fee (with change).
Confirms that when fees align, the waste equals just change_cost. Clear demonstration of formula.

---

703-707: Zero difference between fee and long-term fee (no change).
Here, waste matches the excess. The test reaffirms correct portion of the formula.

---

708-714: Higher short-term fee test.
By doubling the fee with the same long-term fee, we expect a larger waste. Confirm the test captures an expected bigger difference.

---

715-723: Long-term fee higher than short-term fee (with change).
Checks that the waste is lowered by a negative difference in fees. Demonstrates thorough coverage.

---

724-732: Long-term fee higher than short-term fee (no change).
Compares negative fee diffs plus excess. The logic ensures that the test covers symmetrical scenarios.

---

733-739: Verify no waste for exact target when fees match.
Confirms that if we expend exactly in_amt - fee*2, the waste is zero. This is correct.

---

740-746: Correlate change cost offset to fee difference.
Demonstrates that if (fee - long_term_fee) equals negative change_cost, the waste is zero again.

---

747-752: No waste with different target offset.
Another scenario ensuring zero-waste alignment if (fee - long_term_fee) balances out excess.

src/wallet/spend.cpp (3)

362-405: Combined BnB and Knapsack strategy with waste comparison.
The new AttemptSelection merges results from both BnB and Knapsack, then picks the least waste. This is a robust approach but introduces complexity:

• Ensure any future algorithm (e.g., randomization or Single Random Draw) merges consistently.
• Verify that comparing waste metrics is stable across edge cases (like negative effective values, huge fees, or large coin sets).

---

541-544: Optional long-chain acceptance.
Allowing infinite ancestors/descendants if -walletrejectlongchains=0 is correct but can lead to unexpected user experiences in large mempool scenarios. Keep an eye out for user confusion or large chain transaction rejections by the network.

---

550-557: Preset coin integration.
Merging coin_control preset inputs with the newly selected coins is done after AttemptSelection. This ensures these preset UTXOs remain forcibly included, as intended. Implementation looks consistent with the coin selection logic.

src/wallet/wallet.h (5)

82-83: DEFAULT_CONSOLIDATE_FEERATE usage.
Defining a default consolidate fee rate (10 sat/vbyte) is helpful for input consolidation strategies. Confirm it doesn’t conflict with the main usage or overshadow the user’s custom fee if they override it.

---

362-362: Redundant note on AttemptSelection rename.
The header references SelectCoinsMinConf → AttemptSelection. Confirm that all references are updated, and no stale references remain in documentation or inline comments.

---

553-553: New AttemptSelection function signature.
The extra parameters for CoinEligibilityFilter, std::vector<COutput> coins, and CoinSelectionParams align with the flexible selection workflow. This is consistent with the approach taken in spend.cpp.

---

777-780: Added consolidate feerate member.
CFeeRate m_consolidate_feerate{DEFAULT_CONSOLIDATE_FEERATE}; helps streamline consolidation. Evaluate whether “consolidation mode” is toggled or if partial usage might cause confusion in everyday transactions.

---

919-920: Static Create method referencing WalletContext.
Replacing older code that used global references with Create(WalletContext&, ...) is a clear improvement for testability. Just ensure that required context fields (e.g., chain pointer, settings) are available before creation.

src/wallet/test/wallet_tests.cpp (31)

29-29: Add WalletContext header
No issues. This include is necessary for the new WalletContext usage.

---

51-68: Refactored TestLoadWallet to accept a WalletContext
Switching from separate chain and coinjoin loader parameters to a single context promotes better modularity and maintainability. This approach cleanly aggregates wallet dependencies. Implementation appears correct.

---

70-77: TestUnloadWallet with WalletContext
Properly removes the wallet from the context and unloads it. The sync step is a good precaution to ensure pending notifications are cleared.

---

118-118: SetWalletFlag(WALLET_FLAG_DESCRIPTORS)
Enabling descriptor-based wallet flags aligns with the overall descriptor migration.

---

138-138: Again setting DESCRIPTORS flag
Consistent usage of descriptor wallets.

---

167-167: Another DESCRIPTORS flag setting
No concerns; follows the same pattern.

---

354-356: Enable descriptor usage in coin_mark_dirty_immature_credit test
Locking the wallet, setting the descriptor flag, and initializing descriptor-based managers is appropriate.

---

370-370: AddKey call
Uses the new AddKey function to add the coinbase key post-dirtying the wallet transaction. Logic and flow look coherent.

---

233-235: Construct and populate WalletContext
Assigning coinjoin_loader and args ensures the wallet has full context when added.

---

267-267: RemoveWallet call
Cleanly detaches the wallet from the context after the test completes.

---

294-295: New local WalletContext
These lines instantiate a context object for subsequent wallet operations. Straightforward.

---

303-303: AddWallet invocation
Adds the newly created wallet into the context. No issues encountered.

---

312-312: RemoveWallet invocation
Proper removal of the wallet from the context, restoring a clean test state.

---

322-324: Initialize another WalletContext
Correctly sets up the context with coinjoin loader and argument references.

---

325-325: Assign request.context
Ensures that RPC requests run against the correct wallet context.

---

328-328: AddWallet
No issues. This matches the descriptor-based pattern from earlier additions.

---

331-331: RemoveWallet
Clean removal of the wallet from the context.

---

625-647: wallet_disableprivkeys test
This test confirms that a wallet with disabled private keys cannot generate new addresses or top up its key pool. The checks for TopUpKeyPool and GetNewDestination both failing are correct.

---

649-684: CalculateNestedKeyhashInputSize
Methodically computes the serialized size of a nested P2PKH input with optional maximum signatures. The logic is sound.

---

685-689: dummy_input_size_test
Checks that the computed dummy input size is as expected. Straightforward verification.

---

691-695: malformed_descriptor check
Filters out descriptors without checksums, throwing an I/O error appropriately. Implementation is fine.

---

697-710: wallet_descriptor_test
Probes loading of an invalid descriptor and expects an exception. The failure condition is validated as intended.

---

712-821: CreateWallet test
Simulates race conditions when loading a wallet with pending mempool and block notifications. Coverage is thorough, ensuring the wallet detects transactions regardless of notification timing.

---

823-830: CreateWalletWithoutChain
Demonstrates correct wallet creation without an active chain client. Straightforward logic.

---

832-866: ZapSelectTx test
Validates selective removal of transactions from the wallet, verifying that locked coins and spends are updated. Effective for ensuring correctness of partial wallet “zaps.”

---

868-872: Dash-specific fallback fee
Introduces a fallback fee for subsequent tests. No issues noted.

---

873-879: AddLegacyKey function
Extends the legacy script pubkey manager with a new key. Useful for remaining legacy-based scenarios.

---

881-957: rpc_getaddressinfo test
Covers P2PKH and P2SH multisig addresses, verifying the fields in the JSON response for ownership, watchonly status, and multisig properties. Implementation is correct.

---

959-1116: CreateTransactionTestSetup
Sets up a wallet with coin control and scanning logic for thorough transaction creation tests. Provides a good foundation for verifying fee handling, coin selection, and recipient requests.

---

1118-1403: CreateTransactionTest
Comprehensive set of test cases inspecting fee rates, change outputs, transaction sizes, and error scenarios. The coverage is extensive and robust.

---

1405-1466: select_coins_grouped_by_addresses test
Ensures coins are grouped correctly by address and tests conflict resolution. Logic is consistent with expected wallet behaviors.

src/wallet/wallet.cpp (5)

43-43: Header inclusion looks good

Including <wallet/context.h> is necessary for referencing WalletContext. No issues found here.

---

164-171: Clear and straightforward retrieval

The loop-based retrieval by name is simple and effective. No major concerns here.

---

173-178: Handler registration pattern looks solid

Storing the functor and returning a scoped remover via MakeHandler is a neat approach to event registration. The concurrency guards appear correct.

---

228-257: Consider checking AddWallet(...) success

AddWallet(context, wallet) is invoked but its return status isn’t used. If it fails (e.g., the wallet is already tracked), subsequent logic could proceed under incorrect assumptions. You might want to verify and handle this scenario properly to avoid subtle issues.

---

260-271: Synchronized loading logic

The method is well-structured: it checks for a duplicate loading process and properly erases from g_loading_wallet_set after the operation. Minor suggestion: if LoadWalletInternal fails, also consider logging that fact (beyond setting the error message) for easier debugging.

src/wallet/rpcwallet.cpp (2)

2582-2583: Validate wallet context usage.

These lines introduce a context dependency for retrieving loaded wallets. Ensure there's proper error handling if the returned WalletContext is invalid or unavailable. This helps prevent null references in subsequent operations.

---

2951-2951: Add robustness to backup file path creation.

Before converting request.params[1].get_str() to a filesystem path, consider guarding against empty or invalid inputs. This helps safeguard against unexpected runtime errors or user mistakes.

src/wallet/context.cpp (1)

7-8: LGTM! Naming convention update.

The change from m_coinjoin_loader to coinjoin_loader aligns with the project's move away from Hungarian notation prefixes, improving code readability while maintaining functionality.

src/wallet/test/wallet_test_fixture.cpp (1)

11-11: LGTM! Updated wallet loader initialization.

The change adds m_node as a parameter to MakeWalletLoader, aligning with the broader changes in wallet context management. The parameter order is correct and matches the interface requirements.

src/walletinitinterface.h (2)

12-14: LGTM! Added WalletLoader forward declaration.

Clean addition of the interfaces namespace and WalletLoader forward declaration.

---

28-29:

✅ Verification successful

Verify implementation of updated method signatures.

The methods now require a WalletLoader parameter, which is part of the wallet context restructuring. This change improves encapsulation but requires verification of all implementing classes.

---

🏁 Script executed:

#!/bin/bash
# Search for classes implementing WalletInitInterface
rg -l "class.*:.*public.*WalletInitInterface"

# Search for implementations of the updated methods
rg "void.*AutoLockMasternodeCollaterals.*\(.*WalletLoader.*\)" -A 5
rg "void.*InitCoinJoinSettings.*\(.*WalletLoader.*\)" -A 5

Length of output: 3627

---

Implementation Verified: Updated method signatures are consistent across implementations.

• Confirmed that both AutoLockMasternodeCollaterals and InitCoinJoinSettings now correctly include the interfaces::WalletLoader& parameter as declared in src/walletinitinterface.h.
• Verified implementations in both src/wallet/init.cpp and src/dummywallet.cpp adhere to the updated signatures.

src/interfaces/coinjoin.h (2)

13-15: LGTM! Added WalletLoader forward declaration.

Clean addition of the WalletLoader forward declaration in the interfaces namespace.

---

48-48:

✅ Verification successful

Verify implementation of updated MakeCoinJoinLoader function.

The function signature now includes WalletLoader parameter, which is part of the wallet context restructuring. This change requires verification of the implementation and all call sites.

---

🏁 Script executed:

#!/bin/bash
# Search for MakeCoinJoinLoader implementation
rg "MakeCoinJoinLoader.*\(.*WalletLoader.*\)" -A 5

# Search for MakeCoinJoinLoader call sites
rg "MakeCoinJoinLoader\(" -A 2

Length of output: 1324

---

Action: The updated MakeCoinJoinLoader signature has been correctly applied.

The declaration in src/interfaces/coinjoin.h now includes the additional interfaces::WalletLoader& wallet_loader parameter, and the implementation in src/coinjoin/interfaces.cpp reflects this change. All call sites (seen in src/test/util/setup_common.cpp and src/init.cpp) are updated accordingly.

No further changes are required.

src/wallet/test/init_test_fixture.cpp (1)

17-17:

✅ Verification successful

LGTM! Parameter reordering aligns with wallet context changes.

The updated MakeWalletLoader call correctly includes the node context parameter, which is consistent with the broader changes in wallet management.

Let's verify that this signature matches other MakeWalletLoader calls:

---

🏁 Script executed:

#!/bin/bash
# Search for other MakeWalletLoader calls to verify parameter consistency
rg "MakeWalletLoader\(" -A 2

Length of output: 1600

---

LGTM! Parameter reordering verified across the codebase.
The updated MakeWalletLoader call in src/wallet/test/init_test_fixture.cpp now correctly uses the node context parameter, just as defined in src/interfaces/wallet.h and used consistently in other parts of the code (e.g., in src/wallet/init.cpp and src/wallet/test/wallet_test_fixture.cpp). No additional changes are necessary.

src/wallet/test/util.cpp (2)

9-9: LGTM! Required import for key encoding.

The addition of key_io.h is necessary for EncodeSecret used in the descriptor setup.

---

27-37: LGTM! Modernized wallet setup with descriptors.

The changes properly implement descriptor-based wallet setup:

1. Sets descriptor wallet flag
2. Initializes descriptor script public key managers
3. Creates and adds a wallet descriptor using the combo format
4. Properly handles the signing provider

This modernization improves wallet management by using the more robust descriptor-based approach.

src/rpc/register.h (2)

10-14: LGTM! Clean forward declarations.

The forward declarations for CRPCCommand and Span template are properly placed and minimize header dependencies.

---

31-37: LGTM! Well-organized wallet-specific RPC command getters.

The new functions are:

1. Properly guarded by ENABLE_WALLET
2. Follow a consistent pattern returning Span<const CRPCCommand>
3. Logically grouped by functionality (CoinJoin, Evo, Governance, Masternode)

src/wallet/context.h (1)

41-43: LGTM! Thread-safe wallet management.

The implementation properly ensures thread safety:

1. Uses Mutex for synchronization
2. Guards both containers with GUARDED_BY annotation
3. Uses appropriate container types (vector for wallets, list for load functions)

src/bench/wallet_balance.cpp (1)

23-25: LGTM! Proper locking and descriptor setup.

The changes correctly implement thread-safe wallet initialization with descriptors, which is the recommended approach.

src/dummywallet.cpp (1)

27-28: LGTM! Updated method signatures for wallet loader integration.

The changes correctly integrate the wallet loader parameter into the interface methods.

src/coinjoin/interfaces.cpp (1)

76-85: Consider error handling for InitCoinJoinSettings.

The InitCoinJoinSettings calls after wallet operations should handle potential failures.

Consider adding error handling:

     void AddWallet(const std::shared_ptr<CWallet>& wallet) override
     {
         m_walletman.Add(wallet);
-        g_wallet_init_interface.InitCoinJoinSettings(m_wallet_loader, m_walletman);
+        try {
+            g_wallet_init_interface.InitCoinJoinSettings(m_wallet_loader, m_walletman);
+        } catch (const std::exception& e) {
+            // Log error but don't prevent wallet addition
+            LogPrintf("Failed to initialize CoinJoin settings: %s\n", e.what());
+        }
     }

src/bench/coin_selection.cpp (2)

53-53: LGTM!

The parameter name change improves naming consistency.

---

57-57: LGTM!

The method name change from SelectCoinsMinConf to AttemptSelection is more descriptive and aligns with the broader changes in the wallet management system.

src/wallet/test/psbt_wallet_tests.cpp (3)

17-26: LGTM!

The new helper function import_descriptor is well-structured, handles errors appropriately, and follows good practices:

• Uses proper locking mechanism
• Uses descriptive variable names
• Follows single responsibility principle

---

30-31: LGTM!

The changes properly initialize the wallet for descriptor support:

• Adds necessary locking for thread safety
• Sets the required wallet flag

---

74-74: LGTM!

The updated FillPSBT call properly includes the required parameters for signing and BIP32 derivation paths.

src/wallet/coinselection.h (1)

167-180: LGTM!

The new GetSelectionWaste method is well-designed:

• Comprehensive documentation explaining the waste calculation formula
• Proper use of [[nodiscard]] attribute to prevent accidental ignoring of return value
• Clear parameter names with appropriate default values

src/wallet/walletdb.h (2)

33-33: LGTM!

The forward declaration for WalletContext is properly placed and necessary for the updated method signature.

---

258-258: LGTM!

The updated MaybeCompactWalletDB signature properly accepts a WalletContext reference, aligning with the broader changes in wallet context management.

src/wallet/test/coinjoin_tests.cpp (2)

132-138: LGTM: Constructor initialization and context setup looks correct.

The constructor properly initializes the context member with m_node.coinjoin_loader and sets up the required context attributes (args and chain).

---

141-141: LGTM: Wallet management calls updated correctly.

The AddWallet and RemoveWallet calls have been properly updated to include the context parameter, maintaining consistency with the new wallet management interface.

Also applies to: 155-155

src/interfaces/wallet.h (4)

31-45: LGTM: Forward declarations added correctly.

The new forward declarations for CRPCCommand, NodeContext, and template class Span are properly added to support the new functionality.

---

93-94: LGTM: New method for masternode collateral management.

The autoLockMasternodeCollaterals method is appropriately added to the Wallet interface to support masternode functionality.

---

348-349: LGTM: New method for RPC registration.

The registerOtherRpcs method is correctly added to the WalletLoader interface to support registration of non-core wallet RPCs.

---

460-465: LGTM: Function signatures updated for context awareness.

The MakeWallet and MakeWalletLoader function signatures have been properly updated to include context parameters, enhancing the context-aware design.

src/wallet/init.cpp (3)

191-193: LGTM: Wallet loader initialization updated correctly.

The MakeWalletLoader call is properly updated to include all required parameters: chain, args, node context, and coinjoin loader.

---

197-203: LGTM: Masternode collateral locking updated.

The AutoLockMasternodeCollaterals method correctly uses the wallet loader to iterate through wallets and call the new interface method.

---

205-228: LGTM: CoinJoin settings initialization updated.

The InitCoinJoinSettings method properly:

• Uses wallet loader to get wallets
• Checks wallet lock status using the interface method
• Initializes CoinJoin settings with appropriate logging

src/wallet/coinselection.cpp (1)

202-202: LGTM: Consistent use of GetSelectionAmount.

The ApproximateBestSubset function now correctly uses GetSelectionAmount() instead of directly accessing m_value, maintaining consistency in value calculations.

Also applies to: 214-214

src/rpc/coinjoin.cpp (2)

468-490: LGTM! Clean separation of wallet-specific RPC commands.

The new GetWalletCoinJoinRPCCommands() function effectively encapsulates wallet-specific CoinJoin RPC commands, improving code organization and modularity.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[warning] 486-486: Clang format differences found. Please format the code according to Clang standards.

---

492-515: LGTM! Robust handling of wallet-disabled scenarios.

The modified RegisterCoinJoinRPCCommands() function properly handles cases where:

1. Wallet support is not compiled in
2. Wallet support is compiled but disabled at runtime

src/test/util/setup_common.cpp (1)

127-127: LGTM! Updated CoinJoin loader initialization.

The change correctly passes the wallet loader to MakeCoinJoinLoader(), aligning with the enhanced wallet context management.

src/rpc/masternode.cpp (1)

739-751: LGTM! Clean separation of wallet-specific masternode RPC commands.

The new GetWalletMasternodeRPCCommands() function effectively encapsulates wallet-specific masternode RPC commands, following the same pattern as CoinJoin RPC commands.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[warning] 740-740: Clang format differences found. Please format the code according to Clang standards.

src/wallet/walletdb.cpp (2)

1023-1048: LGTM! Enhanced wallet context management.

The MaybeCompactWalletDB() function now properly accepts a WalletContext parameter, enabling better context-aware wallet operations.

---

1050-1053: LGTM! Clean implementation of destination data operations.

The new WriteDestData() and EraseDestData() functions provide a clean interface for managing destination data in the wallet database.

Also applies to: 1055-1058

test/functional/rpc_fundrawtransaction.py (1)

936-990: LGTM! Well-structured test for issue bitcoin#22670.

The test thoroughly verifies that transaction fees are correctly calculated when using ApproximateBestSubset for coin selection. The implementation includes:

• Setting up test conditions with a high minimum relay fee
• Creating transactions to calculate fee bounds
• Testing both lower and upper bounds of the target value

🧰 Tools

🪛 Ruff (0.8.2)

957-957: Loop control variable i not used within loop body

Rename unused i to _i

(B007)

src/rpc/evo.cpp (1)

1781-1809: LGTM! Clean separation of wallet-specific RPC commands.

The new GetWalletEvoRPCCommands() function effectively encapsulates wallet-specific RPC commands, improving modularity and maintainability. This aligns well with the PR's objective of better wallet context management.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[warning] 1805-1805: Clang format differences found. Please format the code according to Clang standards.

src/init.cpp (3)

31-33: LGTM! Header file changes look good.

The addition of interfaces/wallet.h is consistent with the PR objectives to backport wallet-related changes.

---

2232-2235: LGTM! Wallet loader initialization is properly guarded.

The initialization of wallet loader and auto-locking of masternode collaterals is correctly guarded by the -disablewallet check.

---

1519-1534:

✅ Verification successful

Verify the wallet RPC registration changes.

The conditional registration of non-core wallet-only RPC commands looks correct, but let's verify that all required wallet RPC commands are properly registered.

---

🏁 Script executed:

#!/bin/bash
# Description: Check if all wallet RPC commands are properly registered
# Look for any wallet RPC registration patterns that might have been missed

# Search for wallet RPC registration patterns
rg -A 5 "RegisterWallet.*RPC|GetWallet.*RPC"

# Search for any TODO/FIXME comments related to wallet RPC registration
rg -i "todo|fixme|xxx|hack" -A 3 ".*wallet.*rpc"

Length of output: 39243

---

Wallet RPC registration confirmed. The investigation verifies that all intended wallet RPC command sets—CoinJoin, Evo, Governance, and Masternode—are correctly registered via node.wallet_loader->registerOtherRpcs in src/init.cpp (lines 1519–1534). The search results also show the expected declarations (e.g., in src/rpc/register.h) and their implementations, confirming there are no missing RPC registrations. The only note is the existing TODO about moving these commands to chain client interfaces, which can be addressed later.

[coderabbitai]

🛠️ Refactor suggestion

Coin eligibility fallback expansions.
This block systematically attempts increasingly permissive CoinEligibilityFilter paths (Confirmed-Only, 1-conf, zero-conf with limited ancestors, etc.). It’s well-structured but can get unwieldy if new fallback layers are added. Consider whether any new fallback steps or advanced strategies (like feerate-based selection) might be beneficial to generalize.

[coderabbitai]

⚠️ Potential issue

Verify the retrieved wallet handle.

GetWallet(context, wallet_name) may return null if no wallet matches the requested name. Ensure a user-friendly error or fallback if wallet is null, preventing potential dereferences of an invalid pointer.

[kwvg]

But there is a validation check right after...

dash/src/wallet/rpcwallet.cpp

Lines 3001 to 3007 in 6813863

	WalletContext& context = EnsureWalletContext(request.context);
	std::shared_ptr<CWallet> wallet = GetWallet(context, wallet_name);
	if (!wallet) {
	throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded");
	}

[coderabbitai]

@kwvg You are absolutely right - I apologize for the false warning. The code already has proper null pointer validation with a descriptive error message right after the GetWallet() call.

🐰 Rabbit looks thoughtful after learning something new

---

✏️ Learnings added

Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.049Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Verify the test descriptors.

The descriptors look well-formed but let's verify they are valid and use appropriate test keys.

Run the following script to verify the descriptors:

---

🏁 Script executed:

#!/bin/bash
# Description: Verify the test descriptors are valid and use test keys.

# Test: Check if the descriptors are valid. Expect: Valid descriptors.
python3 -c '
import re

def verify_descriptor(desc):
    # Basic validation of descriptor format
    if not desc.startswith("sh(") or not desc.endswith("))"):
        return False
    # Check for test keys (should start with xprv)
    if not "xprv" in desc:
        return False
    return True

descriptors = [
    "sh(multi(2,xprv9s21ZrQH143K2LE7W4Xf3jATf9jECxSb7wj91ZnmY4qEJrS66Qru9RFqq8xbkgT32ya6HqYJweFdJUEDf5Q6JFV7jMiUws7kQfe6Tv4RbfN/0h/0h/0h,xprv9s21ZrQH143K2LE7W4Xf3jATf9jECxSb7wj91ZnmY4qEJrS66Qru9RFqq8xbkgT32ya6HqYJweFdJUEDf5Q6JFV7jMiUws7kQfe6Tv4RbfN/0h/0h/1h))",
    "sh(multi(2,xprv9s21ZrQH143K2LE7W4Xf3jATf9jECxSb7wj91ZnmY4qEJrS66Qru9RFqq8xbkgT32ya6HqYJweFdJUEDf5Q6JFV7jMiUws7kQfe6Tv4RbfN/0h/0h/2h,xprv9s21ZrQH143K2LE7W4Xf3jATf9jECxSb7wj91ZnmY4qEJrS66Qru9RFqq8xbkgT32ya6HqYJweFdJUEDf5Q6JFV7jMiUws7kQfe6Tv4RbfN/0h/0h/3h))",
    "pkh(xprv9s21ZrQH143K2LE7W4Xf3jATf9jECxSb7wj91ZnmY4qEJrS66Qru9RFqq8xbkgT32ya6HqYJweFdJUEDf5Q6JFV7jMiUws7kQfe6Tv4RbfN/0h/0h/*h)"
]

for desc in descriptors:
    print(f"Descriptor: {desc}")
    print(f"Valid: {verify_descriptor(desc)}")
'

Length of output: 1788

---

Descriptor Verification Update

• The multi-signature descriptors (lines 44–46 in src/wallet/test/psbt_wallet_tests.cpp) are correctly formatted and pass our basic validity checks.
• However, the public key hash descriptor on line 47 (pkh(…)) fails the simple validation since it does not follow the expected sh(...)) pattern used by the current validation logic.
• Please confirm whether the pkh descriptor is intended to be in its current form. If so, consider updating the validation logic to accept pkh descriptors; otherwise, update the descriptor format so that it meets the expected pattern.

[coderabbitai]

🛠️ Refactor suggestion

Fix clang-format issues.

The pipeline reports formatting issues in this section. Please run clang-format to fix the formatting.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[warning] 1075-1075: Clang format differences found. Please format the code according to Clang standards.

[kwvg]

We want this code to be excluded from clang-format, hence the comment

// clang-format off

[coderabbitai]

@kwvg I apologize for the incorrect suggestion. You are absolutely right - the // clang-format off directive is intentionally used here to maintain the readable alignment of the command registration array. I will remember this for future reviews.

---

✏️ Learnings added

Learnt from: kwvg
PR: dashpay/dash#6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:57.974Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

[coderabbitai]

⚠️ Potential issue

Fix Clang format issues.

The pipeline indicates formatting issues. Please run clang-format on the code to ensure consistent style.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[warning] 1805-1805: Clang format differences found. Please format the code according to Clang standards.

[github-actions]

This pull request has conflicts, please rebase.

[coderabbitai]

Actionable comments posted: 1

🔭 Outside diff range comments (1)

src/init.cpp (1)

1-2458: ⚠️ Potential issue

Resolve conflicts with other pull requests.

The GitHub Actions pipeline indicates potential conflicts with other PRs. Since this is a backport PR (#6529), please ensure to:

1. Rebase on the latest target branch
2. Resolve any conflicts that arise
3. Verify that the backported changes still work as expected after conflict resolution

🧰 Tools

🪛 Cppcheck (2.10-2)

[error] 830-830: syntax error

(syntaxError)

🪛 GitHub Actions: Check Potential Conflicts

[error] 1-1: Conflict detected with multiple pull requests.

♻️ Duplicate comments (1)

src/rpc/evo.cpp (1)

1804-1806: ⚠️ Potential issue

Fix clang-format issues.

The code has formatting inconsistencies. Run clang-format to fix:

-};
-// clang-format on
-    return commands;
+}; // clang-format on
+return commands;

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[warning] 1804-1804: Clang format differences found. Please format the code according to clang-format standards.

🧹 Nitpick comments (7)

src/wallet/test/util.cpp (1)

33-36: Consider enhancing error handling for descriptor parsing.

While the current implementation uses assertions for error handling, consider providing more descriptive error messages for debugging purposes.

-        std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(key) + ")", provider, error, /* require_checksum=*/ false);
-        assert(desc);
+        std::unique_ptr<Descriptor> desc = Parse("combo(" + EncodeSecret(key) + ")", provider, error, /* require_checksum=*/ false);
+        if (!desc) {
+            throw std::runtime_error("Failed to parse descriptor: " + error);
+        }

src/wallet/context.h (1)

47-52:

❓ Verification inconclusive

Plan to remove NodeContext dependency.

The TODO comment correctly identifies a design issue where wallet RPCs shouldn't access non-wallet capabilities through NodeContext. Consider creating a separate interface that exposes only the necessary functionality to wallet RPCs.

---

🏁 Script executed:

#!/bin/bash
# Search for wallet RPC implementations accessing NodeContext
rg -A 5 "node_context->" src/wallet/rpc/

Length of output: 42

---

Plan to decouple NodeContext from wallet RPCs:
The current code in src/wallet/context.h (lines 47‑52) retains a NodeContext* node_context{nullptr}; along with a TODO indicating that wallet RPCs should not depend on non‐wallet capabilities. Initial verification using a search for node_context-> in the wallet RPC directory yielded no results; however, please verify manually to ensure no indirect usage exists. Consider refactoring by creating a dedicated interface that exposes only the functionality required by wallet RPCs while removing the broader NodeContext dependency.

src/wallet/walletdb.h (1)

32-33: LGTM! Consider adding documentation for WalletContext.

The forward declaration of WalletContext is correctly placed. Consider adding a brief comment explaining its purpose and relationship to wallet database operations.

 struct CBlockLocator;
+// Encapsulates wallet-related context and state
 struct WalletContext;
 class CHDChain;

test/functional/rpc_fundrawtransaction.py (1)

957-959: Consider using _ for unused loop variable.

The loop control variable i is not used within the loop body.

-        for i in range(0, 3):
+        for _ in range(0, 3):

🧰 Tools

🪛 Ruff (0.8.2)

957-957: Loop control variable i not used within loop body

Rename unused i to _i

(B007)

src/wallet/test/wallet_tests.cpp (2)

104-206: Consider adding more test cases for descriptor wallet behavior.

While the changes correctly update tests for descriptor wallets, consider adding test cases for:

• Descriptor wallet specific features
• Error cases when descriptors are malformed
• Migration scenarios from legacy to descriptor wallets

---

691-710: Consider expanding BLS scheme test coverage.

The new test cases for wallet descriptors are good, but consider adding tests for:

• Edge cases in BLS scheme selection
• Invalid descriptor formats
• Compatibility between different BLS schemes

src/rpc/evo.cpp (1)

1811-1848: Consider improving documentation for hybrid RPC handling.

The changes correctly handle both wallet and non-wallet commands, but the TODO comment could be more specific about:

• The rationale for hybrid RPCs
• The plan for splitting these into dedicated RPCs
• The timeline for removing this workaround

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6813863 and 0f7c173.

📒 Files selected for processing (44)

• src/bench/coin_selection.cpp (1 hunks)
• src/bench/wallet_balance.cpp (2 hunks)
• src/bitcoin-cli.cpp (1 hunks)
• src/coinjoin/client.cpp (2 hunks)
• src/coinjoin/interfaces.cpp (3 hunks)
• src/dummywallet.cpp (2 hunks)
• src/init.cpp (4 hunks)
• src/interfaces/coinjoin.h (2 hunks)
• src/interfaces/wallet.h (5 hunks)
• src/node/blockstorage.cpp (0 hunks)
• src/qt/test/addressbooktests.cpp (2 hunks)
• src/qt/test/wallettests.cpp (3 hunks)
• src/rpc/coinjoin.cpp (3 hunks)
• src/rpc/evo.cpp (3 hunks)
• src/rpc/governance.cpp (1 hunks)
• src/rpc/masternode.cpp (1 hunks)
• src/rpc/register.h (2 hunks)
• src/rpc/server_util.cpp (1 hunks)
• src/test/util/setup_common.cpp (1 hunks)
• src/test/util/wallet.cpp (0 hunks)
• src/wallet/coinselection.cpp (3 hunks)
• src/wallet/coinselection.h (1 hunks)
• src/wallet/context.cpp (1 hunks)
• src/wallet/context.h (2 hunks)
• src/wallet/init.cpp (3 hunks)
• src/wallet/interfaces.cpp (8 hunks)
• src/wallet/load.cpp (7 hunks)
• src/wallet/load.h (2 hunks)
• src/wallet/rpc/util.cpp (1 hunks)
• src/wallet/rpcwallet.cpp (6 hunks)
• src/wallet/spend.cpp (5 hunks)
• src/wallet/test/coinjoin_tests.cpp (3 hunks)
• src/wallet/test/coinselector_tests.cpp (21 hunks)
• src/wallet/test/init_test_fixture.cpp (1 hunks)
• src/wallet/test/psbt_wallet_tests.cpp (3 hunks)
• src/wallet/test/util.cpp (2 hunks)
• src/wallet/test/wallet_test_fixture.cpp (1 hunks)
• src/wallet/test/wallet_tests.cpp (15 hunks)
• src/wallet/wallet.cpp (23 hunks)
• src/wallet/wallet.h (8 hunks)
• src/wallet/walletdb.cpp (1 hunks)
• src/wallet/walletdb.h (2 hunks)
• src/walletinitinterface.h (2 hunks)
• test/functional/rpc_fundrawtransaction.py (2 hunks)

💤 Files with no reviewable changes (2)

• src/test/util/wallet.cpp
• src/node/blockstorage.cpp

🚧 Files skipped from review as they are similar to previous changes (16)

• src/bitcoin-cli.cpp
• src/wallet/context.cpp
• src/wallet/test/wallet_test_fixture.cpp
• src/interfaces/coinjoin.h
• src/test/util/setup_common.cpp
• src/wallet/test/init_test_fixture.cpp
• src/walletinitinterface.h
• src/rpc/register.h
• src/wallet/rpc/util.cpp
• src/bench/coin_selection.cpp
• src/wallet/load.h
• src/wallet/test/psbt_wallet_tests.cpp
• src/coinjoin/client.cpp
• src/rpc/server_util.cpp
• src/interfaces/wallet.h
• src/wallet/walletdb.cpp

🧰 Additional context used

🪛 GitHub Actions: Clang Diff Format Check

src/rpc/evo.cpp

[warning] 1804-1804: Clang format differences found. Please format the code according to clang-format standards.

src/rpc/coinjoin.cpp

[warning] 468-468: Clang format differences found. Please format the code according to clang-format standards.

src/rpc/masternode.cpp

[warning] 674-674: Clang format differences found. Please format the code according to clang-format standards.

src/rpc/governance.cpp

[warning] 1074-1074: Clang format differences found. Please format the code according to clang-format standards.

🪛 GitHub Actions: Check Potential Conflicts

src/rpc/evo.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/dummywallet.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/wallet/test/coinselector_tests.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/qt/test/wallettests.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/qt/test/addressbooktests.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/init.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/rpc/governance.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/wallet/rpcwallet.cpp

[error] 1-1: Conflict detected with multiple pull requests.

src/wallet/wallet.h

[error] 1-1: Conflict detected with multiple pull requests.

src/wallet/wallet.cpp

[error] 1-1: Conflict detected with multiple pull requests.

🪛 Ruff (0.8.2)

test/functional/rpc_fundrawtransaction.py

957-957: Loop control variable i not used within loop body

Rename unused i to _i

(B007)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: Build container / Build container

🔇 Additional comments (62)

src/wallet/test/util.cpp (1)

27-37: LGTM! Modernized wallet setup with descriptors.

The changes effectively modernize the wallet setup by transitioning from legacy script management to a descriptor-based approach. The implementation includes proper locking and setup of descriptor script public key managers.

src/wallet/context.h (1)

41-43: LGTM! Improved thread safety with proper mutex guards.

The addition of wallets_mutex with GUARDED_BY annotation ensures thread-safe access to the wallet collections.

src/bench/wallet_balance.cpp (2)

23-25: LGTM! Aligned with descriptor-based wallet approach.

The changes correctly set up the wallet with descriptor support, following the modernization effort.

---

47-50:

❓ Verification inconclusive

Verify benchmark parameters after watch-only removal.

The benchmark functions have been updated to remove the watch-only parameter, but we should verify that the epoch iterations are still appropriate for the new implementation.

Please run these benchmarks and compare their execution times with the previous implementation to ensure the epoch iterations are still optimal:

---

🏁 Script executed:

#!/bin/bash
# Run the wallet balance benchmarks
./bench_dash "^WalletBalance"

Length of output: 142

---

Action Required: Verify Benchmark Execution Command

The updated wallet balance benchmark functions reflect the removal of the watch-only parameter, so it's important to confirm that the epoch iterations remain optimal. However, the provided command (./bench_dash "^WalletBalance") failed with a "No such file or directory" error. This indicates that the intended benchmark executable wasn’t found—likely because its location or name has changed.

• Next Steps:
  • Manually verify the correct location or name of the benchmark executable.
  • Run the benchmarks using the correct command to compare execution times with the previous implementation.

src/dummywallet.cpp (3)

27-28: LGTM! Consistent interface with wallet-enabled builds.

The method signatures have been properly updated to include the WalletLoader parameter, maintaining consistency with the wallet-enabled implementation.

---

36-36:

✅ Verification successful

Verify the new consolidatefeerate parameter.

A new hidden argument -consolidatefeerate has been added. We should verify its usage and documentation.

---

🏁 Script executed:

#!/bin/bash
# Search for consolidatefeerate usage in the codebase
rg "consolidatefeerate" -A 5

Length of output: 2647

---

Subject: -consolidatefeerate Implementation & Documentation Confirmed

After reviewing the repository:

• Usage in src/dummywallet.cpp: The flag ("-consolidatefeerate=<amt>") is present as expected.
• Definition & Documentation: In src/wallet/init.cpp, the argument is added with a clear description (including units and default value) that aligns with its intended behavior.
• Parsing Logic: In src/wallet/wallet.cpp, the parameter is properly checked, parsed, and applied to the wallet instance.

No issues were found with its integration. Please proceed with confidence.

---

1-86:

❓ Verification inconclusive

Resolve potential conflicts.

The pipeline indicates conflicts with other pull requests. Please rebase this branch to resolve the conflicts.

---

🏁 Script executed:

#!/bin/bash
# List conflicting files
git status --porcelain | grep "UU"

Length of output: 34

---

Rebase Verification Required

The automated check using git status --porcelain | grep "UU" did not reveal any unresolved conflicts in the branch. However, the pipeline still indicates potential conflicts with other pull requests. Please manually review and, if necessary, rebase the branch to ensure that all conflicts are resolved before merging.

• Automated check returned no files marked with merge conflicts.
• Manually verify that your branch is up-to-date with the target branch to prevent any hidden merge issues.

🧰 Tools

🪛 GitHub Actions: Check Potential Conflicts

[error] 1-1: Conflict detected with multiple pull requests.

src/coinjoin/interfaces.cpp (4)

66-66: LGTM! Constructor changes improve wallet context handling.

The addition of WalletLoader and initialization of CoinJoin settings in the constructor ensures consistent setup.

Also applies to: 69-74

---

76-80: LGTM! Consistent CoinJoin settings initialization.

Reinitializing CoinJoin settings after adding a wallet ensures proper configuration.

---

83-84: LGTM! Maintains consistency after wallet removal.

Reinitializing CoinJoin settings after removing a wallet ensures the system remains in a consistent state.

---

104-109: LGTM! Updated factory function signature.

The MakeCoinJoinLoader function now properly includes the wallet loader parameter, maintaining consistency with the class changes.

src/qt/test/addressbooktests.cpp (3)

69-73: LGTM! Transition to descriptor-based wallet.

The change from legacy script manager to descriptor wallet aligns with modern wallet practices. The LOCK ensures thread safety during setup.

---

117-120: LGTM! Improved wallet context handling.

The changes properly integrate with the wallet context system, ensuring proper initialization and cleanup.

---

1-171:

❓ Verification inconclusive

Verify conflicts with other PRs.

The pipeline indicates potential conflicts with other pull requests.

---

🏁 Script executed:

#!/bin/bash
# Check for potential conflicts with other PRs
gh pr list --search "is:pr is:open" --json number,title,body | jq -r '.[] | select(.body | contains("addressbooktests.cpp"))'

Length of output: 216

---

Manual Conflict Check Required: AddressBookTests Verification

The automatic check for potential conflicts with other PRs affecting addressbooktests.cpp was inconclusive due to repository context errors ("fatal: not a git repository"). Please manually verify that there are no conflicts with other open PRs referencing this file.

• Confirm that no other PR currently modifies or references addressbooktests.cpp.

🧰 Tools

🪛 GitHub Actions: Check Potential Conflicts

[error] 1-1: Conflict detected with multiple pull requests.

src/wallet/load.cpp (4)

26-30: LGTM! Improved context and error handling.

The use of Assert for args retrieval provides better error detection, and the context parameter consolidates dependencies.

---

31-50: LGTM! Proper wallet directory validation.

The code thoroughly validates the wallet directory path, handling relative paths, non-existent directories, and canonical paths appropriately.

---

104-132: LGTM! Enhanced wallet loading with context.

The wallet loading process now properly uses the wallet context, improving integration with the broader system.

---

147-150: LGTM! Improved scheduling with context.

The scheduling of wallet operations now properly uses the context for configuration and execution.

src/wallet/coinselection.h (1)

167-180:

✅ Verification successful

LGTM! Well-documented waste calculation method.

The new GetSelectionWaste method is thoroughly documented with clear formulas and parameter descriptions. The [[nodiscard]] attribute ensures the result is not accidentally ignored.

Consider adding unit tests to verify the waste calculations for various scenarios.

---

🏁 Script executed:

#!/bin/bash
# Check for existing tests of GetSelectionWaste
rg -l "GetSelectionWaste" "src/wallet/test"

Length of output: 80

---

Coin Selection Waste Calculation Implementation Approved

The documentation for GetSelectionWaste is clear and well-maintained. We verified that tests for this function already exist in src/wallet/test/coinselector_tests.cpp. Please review these tests to ensure that all critical scenarios, including edge cases, are sufficiently covered.

src/wallet/walletdb.h (1)

257-258: LGTM! Function signature updated for context-awareness.

The function signature change to accept WalletContext& aligns with the codebase's move towards context-aware wallet management.

src/qt/test/wallettests.cpp (4)

112-114: LGTM! Proper initialization of WalletContext.

The wallet context is correctly retrieved and used for wallet initialization.

---

116-130: LGTM! Updated to use descriptor-based wallet setup.

The changes correctly implement descriptor-based wallet setup, which is a more modern approach compared to legacy script management.

---

147-147: LGTM! Updated wallet model initialization.

The wallet model initialization correctly uses the new context-aware interface.

---

259-259: LGTM! Proper cleanup with context.

The wallet removal correctly uses the context and explicitly specifies the load_on_startup parameter.

src/wallet/test/coinjoin_tests.cpp (3)

132-138: LGTM! Proper test setup with WalletContext.

The test setup correctly initializes the wallet context and its dependencies.

---

141-142: LGTM! Context-aware wallet management.

The wallet addition correctly uses the new context-aware approach.

---

155-155: LGTM! Proper cleanup in destructor.

The wallet removal correctly uses the context-aware approach in the destructor.

src/wallet/init.cpp (3)

58-58: LGTM! Well-documented new consolidation fee rate option.

The new -consolidatefeerate option is properly documented with clear description and default value.

---

197-203: LGTM! Updated to use context-aware wallet iteration.

The method correctly uses the wallet loader to iterate through wallets and calls the updated method name.

---

205-228: LGTM! Improved CoinJoin settings initialization.

The method properly handles wallet iteration and CoinJoin settings initialization using the context-aware approach.

src/wallet/coinselection.cpp (2)

202-202: LGTM! Good encapsulation of value calculation.

The change from direct m_value access to using GetSelectionAmount() improves encapsulation by centralizing the value calculation logic.

Also applies to: 214-214

---

406-431: LGTM! Well-structured waste calculation function.

The new GetSelectionWaste function is well-designed with:

• Clear input validation using assertions
• Proper handling of both change and no-change scenarios
• Efficient calculation of waste based on fees and effective values

src/rpc/coinjoin.cpp (2)

454-473: LGTM! Well-structured wallet command encapsulation.

The new GetWalletCoinJoinRPCCommands function properly encapsulates wallet-specific CoinJoin commands.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[warning] 468-468: Clang format differences found. Please format the code according to clang-format standards.

---

475-499: LGTM! Good handling of wallet-disabled scenarios.

The updated RegisterCoinJoinRPCCommands function:

• Correctly handles cases where wallet support is disabled
• Includes a clear TODO for future improvements to separate hybrid RPCs

src/wallet/interfaces.cpp (3)

130-130: LGTM! Good integration of WalletContext.

The constructor changes properly integrate WalletContext while maintaining backward compatibility.

Also applies to: 571-578

---

150-150: LGTM! Clean method delegation.

The new autoLockMasternodeCollaterals method cleanly delegates to the wallet implementation.

---

582-597: LGTM! Well-structured RPC registration.

The RPC registration changes:

• Use a clean private helper method
• Handle context properly
• Maintain proper error handling

src/rpc/masternode.cpp (1)

674-686: LGTM! Clean encapsulation of wallet-specific masternode commands.

The new GetWalletMasternodeRPCCommands function properly encapsulates wallet-specific masternode commands under the ENABLE_WALLET directive.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[warning] 674-674: Clang format differences found. Please format the code according to clang-format standards.

src/wallet/test/coinselector_tests.cpp (1)

672-752: LGTM! Comprehensive test coverage for waste calculation.

The new test case thoroughly verifies waste calculation in various scenarios:

• Waste with change (fee difference + change cost)
• Waste without change (fee difference + excess)
• Waste with equal fees (only change cost or excess)
• Waste with different fee rates

src/rpc/governance.cpp (1)

1074-1089: LGTM! Improved organization of wallet-specific governance commands.

The new function encapsulates wallet-specific governance commands in a separate function, making the code more modular and maintainable.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[warning] 1074-1074: Clang format differences found. Please format the code according to clang-format standards.

test/functional/rpc_fundrawtransaction.py (1)

936-990: LGTM! Well-structured test for ApproximateBestSubset bug.

The test effectively verifies the fix for issue bitcoin#22670 by:

1. Setting a high minimum relay fee to make the bug detectable
2. Calculating precise fee bounds that could trigger the bug
3. Testing with boundary values to ensure proper fee handling

🧰 Tools

🪛 Ruff (0.8.2)

957-957: Loop control variable i not used within loop body

Rename unused i to _i

(B007)

src/wallet/spend.cpp (4)

362-405: LGTM! Improved coin selection with waste-based optimization.

The renamed AttemptSelection function now implements a more sophisticated coin selection strategy by:

• Tracking waste metrics for each selection attempt
• Using both BnB and Knapsack algorithms
• Selecting the result with the least waste
• Breaking ties by preferring selections that spend more inputs (helps with UTXO consolidation)

---

630-693: LGTM! Improved transaction initialization with better validation.

The changes enhance transaction creation by:

• Adding early validation for negative amounts
• Improving parameter initialization structure
• Better handling of fee subtraction from outputs

---

767-803: LGTM! Enhanced change output handling with BIP69 support.

The changes improve change output handling by:

• Adding validation for change position
• Implementing BIP69 sorting for transaction outputs
• Properly maintaining change position after sorting

---

856-890: LGTM! Improved fee handling with better validation.

The changes enhance fee handling by:

• Better handling of fee subtraction from outputs
• Adding validation for dust outputs after fee subtraction
• Providing more descriptive error messages for fee-related failures

src/wallet/wallet.h (3)

63-72: LGTM! Improved wallet operations with context-based approach.

The changes enhance wallet operations by:

• Using a context-based approach for wallet management
• Making the code more modular and maintainable
• Providing better separation of concerns

---

1091-1091: LGTM! Consistent use of wallet context.

The change to MaybeResendWalletTxs maintains consistency with the context-based approach used throughout the codebase.

---

777-782:

✅ Verification successful

Verify the consolidate feerate value.

The DEFAULT_CONSOLIDATE_FEERATE is set to 1000 (10 sat/vbyte). This value affects UTXO consolidation behavior through coin_selection_params.m_long_term_feerate in CreateTransactionInternal.

Run the following script to check if this feerate aligns with Bitcoin Core's values:

---

🏁 Script executed:

#!/bin/bash
# Description: Check Bitcoin Core's consolidate feerate value
rg -A 1 "DEFAULT_CONSOLIDATE_FEERATE" $(fd -t f -e cpp -e h)

Length of output: 46572

---

Verified consolidate feerate value is correctly set.

The DEFAULT_CONSOLIDATE_FEERATE is confirmed to be defined as 1000 (10 sat/vbyte) in src/wallet/wallet.h and is properly referenced in src/wallet/init.cpp when initializing the command-line argument. This value is appropriately used for setting coin_selection_params.m_long_term_feerate in transaction construction. No further modifications are necessary based on the current code review.

src/wallet/test/wallet_tests.cpp (2)

93-102: LGTM! Improved key management with descriptors.

The changes properly modernize key handling by:

• Using descriptors instead of legacy key management
• Adding proper wallet locking
• Using combo descriptors for better key management

---

51-68:

❓ Verification inconclusive

Ensure proper cleanup in TestLoadWallet/TestUnloadWallet.

The functions now correctly use WalletContext for better context management. However, ensure that the wallet is properly cleaned up in error cases, especially when context.coinjoin_loader is present.

Also applies to: 70-77

---

🏁 Script executed:

#!/bin/bash
# Check for potential memory leaks in error paths
rg -A 5 "TestLoadWallet|TestUnloadWallet" | grep -i "throw|return"

Length of output: 68

---

Action Required: Verify Error-Path Cleanup for TestLoadWallet/TestUnloadWallet

The update to use WalletContext improves context management. However, our verification script didn’t reveal explicit error return or throw statements around the wallet initialization, so it isn’t clear how cleanup is handled on error—especially when context.coinjoin_loader is active. Please verify manually that in error cases (for example, if wallet creation fails), any allocated wallet resources are properly cleaned up.

• Confirm that if an error occurs during database creation or wallet instantiation, the wallet (or any allocated resources) is correctly disposed of.
• Check that cleanup routines in both TestLoadWallet (lines 51–68) and TestUnloadWallet (and around lines 70–77) effectively handle error paths, particularly when a coinjoin loader is engaged.

src/rpc/evo.cpp (1)

1782-1809: LGTM! Good separation of wallet-specific RPC commands.

The new function properly encapsulates wallet-specific RPC commands, improving modularity and maintainability.

Fix Clang format issues.

The pipeline indicates formatting issues. Please run clang-format on the code to ensure consistent style.

🧰 Tools

🪛 GitHub Actions: Clang Diff Format Check

[warning] 1804-1804: Clang format differences found. Please format the code according to clang-format standards.

src/init.cpp (2)

31-33: LGTM! Header changes look good.

The reordering of interfaces/chain.h and addition of interfaces/wallet.h align well with the wallet backporting objectives.

---

1519-1534: LGTM! Well-structured RPC registration.

The changes properly modularize wallet-specific RPC commands by:

1. Checking for wallet being enabled
2. Using the new registerOtherRpcs method
3. Organizing related commands (CoinJoin, Evo, Governance, Masternode) together

src/wallet/wallet.cpp (4)

44-44: LGTM!

The addition of wallet/context.h is appropriate for the wallet context changes.

---

113-113: LGTM! Function signatures updated to use WalletContext.

The changes consistently update wallet management functions to use WalletContext instead of individual chain and coinjoin loader parameters. This improves encapsulation and maintainability.

Also applies to: 129-129, 159-159, 165-165, 174-174, 2127-2127

---

116-120: LGTM! Implementation properly uses WalletContext.

The changes correctly:

• Use context.wallets_mutex for thread safety
• Access wallets through context.wallets
• Maintain proper locking patterns

Also applies to: 139-143, 161-162, 167-168, 176-178, 251-3547

---

276-277: LGTM! Configuration handling properly uses WalletContext.

The changes correctly:

• Access configuration through context.args
• Maintain existing configuration behavior
• Clean up configuration values appropriately

Also applies to: 311-312, 313-313, 338-341, 348-350, 408-409, 410-410, 421-422, 437-438, 463-464, 480-481, 510-511, 511-512

src/wallet/rpcwallet.cpp (5)

2582-2586: LGTM! Proper use of WalletContext for listing wallets.

The change correctly uses WalletContext to get the list of wallets, which aligns with the PR's objective of integrating WalletContext for wallet operations.

---

2751-2751: LGTM! Proper use of WalletContext for loading wallet.

The change correctly updates the LoadWallet function signature to use WalletContext instead of separate chain and coinjoin loader parameters.

---

2907-2907: LGTM! Proper use of WalletContext for creating wallet.

The change correctly updates the CreateWallet function signature to use WalletContext instead of separate chain and coinjoin loader parameters.

---

2961-2961: LGTM! Proper use of WalletContext for restoring wallet.

The change correctly updates the RestoreWallet function signature to use WalletContext instead of separate chain and coinjoin loader parameters.

---

3002-3003: LGTM! Proper use of WalletContext for unloading wallet.

The change correctly uses WalletContext for getting and removing wallets. Based on the past review comments, the code already has proper null pointer validation after GetWallet().

Also applies to: 3013-3013

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Verify wallet initialization state before auto-locking collaterals.

While the conditional check for wallet being enabled is good, we should verify that the wallet is fully initialized before attempting to auto-lock masternode collaterals to prevent potential race conditions.

---

🏁 Script executed:

#!/bin/bash
# Search for wallet initialization completion checks before auto-locking
rg -A 5 "AutoLockMasternodeCollaterals|wallet.*initialized|wallet.*ready" --type cpp

Length of output: 13042

---

Potential Race Condition – Validate Wallet Initialization Before Auto-Lock

The current code in src/init.cpp calls the auto-lock function without explicitly verifying that the wallet (and by extension DIP3) is fully initialized. Although the wallet initialization interface (see src/wallet/init.cpp) contains a comment noting that the operation must not occur before DIP3 is ready, there is no guard at the call site. This could lead to a race condition if the wallets aren’t completely set up yet.

• File: src/init.cpp (Lines 2228-2232)
• Concern: No explicit check ensuring the wallet is fully initialized before calling

  g_wallet_init_interface.AutoLockMasternodeCollaterals(*node.wallet_loader);

• Action: Add a condition to verify that the wallet and necessary components (e.g., DIP3) are fully initialized before auto-locking masternode collaterals.