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

kwvg · 2025-01-11T02:16:23Z

Additional Information

Dependent on backport: merge bitcoin#25497, #17331, #22155 (wallet backports: part 1) #6517
Dependent on refactor: polish CoinJoin interface, use WalletLoader in Dash wallet initialization code, introduce new RPC coinjoin status #6594
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).

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 · 2025-01-15T17:02:25Z

This pull request has conflicts, please rebase.

github-actions · 2025-01-30T17:15:21Z

This pull request has conflicts, please rebase.

…tcoin#22155 (wallet refactoring) 2c3d9cf merge bitcoin#21207: CWallet transaction code out of wallet.cpp/.h (Kittywhiskers Van Gogh) a21691a partial bitcoin#22155: Add test for subtract fee from recipient behavior (Kittywhiskers Van Gogh) 829510c merge bitcoin#15596: Ignore sendmany::minconf as dummy value (Kittywhiskers Van Gogh) Pull request description: ## Additional Information As [dash#6517](#6517) and [dash#6529](#6529) both introduce behavior changes and workarounds that, changes from both PRs that do not affect behavior have been extracted and spun-off into this PR and they will be subsequently rebased on this PR upon merger into `develop`. This should let us at least get some of the larger (mostly move-only) diffs merged in and ease review for the aforementioned PRs. [bitcoin#21207](bitcoin#21207) can be reviewed using `git log -p -n1 --color-moved=dimmed_zebra` ## Breaking Changes None expected, no changes in behavior. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 2c3d9cf PastaPastaPasta: utACK 2c3d9cf Tree-SHA512: 3e5bb5869793b389aefbe0e97b1cedd13d06abce2ad06d1c16c0f8ecd4e0fa7f148fa5d89653c76ac97171787b35525ec20ece1ea753fb97eebec95a456411e2

(wallet backports: part 1) 153bdc2 merge bitcoin#22155: Add test for subtract fee from recipient behavior (Kittywhiskers Van Gogh) 0185847 fix: correct fee calculations in `CreateTransactionInternal` (Kittywhiskers Van Gogh) 445f489 merge bitcoin#17331: Use effective values throughout coin selection (Kittywhiskers Van Gogh) 7e54bd9 wallet: copy and sort `vecSend` if BIP69 sorting enabled, enable sorting (Kittywhiskers Van Gogh) 9e9e66f partial bitcoin#17331: Use effective values throughout coin selection (Kittywhiskers Van Gogh) 66fe2d4 merge bitcoin#25497: more accurate target for large transactions (Kittywhiskers Van Gogh) 6e4d789 wallet: add back missing `CoinSelectionParams` assignments (Kittywhiskers Van Gogh) bd35042 wallet: move `CoinSelectionParams` to positions expected upstream (Kittywhiskers Van Gogh) 0711e67 wallet: shuffle transaction inputs if we aren't using BIP69 (Kittywhiskers Van Gogh) 1cf1c58 refactor: move selected coin and txin sorting to the end of the scope (Kittywhiskers Van Gogh) ab756ba wallet: Fail if maximum weight is too large (Kittywhiskers Van Gogh) 05c319e refactor: move oversized transaction check to tail end of scope (Kittywhiskers Van Gogh) 6ca51df wallet: Use existing feerate instead of getting a new one (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6543 * Dependency for #6529 * [bitcoin#17331](bitcoin#17331) logically partially reverts [dash#3368](#3668) as Dash Core implemented a calculate-before approach (compared to _then_ Bitcoin Core's calculate-and-adjust approach) and it is being replaced with the current upstream calculate-after approach done in a single-pass instead of iteratively (like the former two). * As the changes are non-trivial, they have been split into a "partial" and a "merge" commit, the first half dedicated just to the logical partial revert and the latter half dedicated to using effective values in coin selection. * BIP69 sorting is disabled in the former half to allow the fix to be in a separate commit while allowing validation of the remaining set of changes. The fix re-enables BIP69 sorting. * Due to the changes introduced in [dash#3368](#3668), a lot of then-redundant code was removed and changes to it upstream were not mirrored in Dash Core. To allow [bitcoin#17331](bitcoin#17331) to work properly, a lot of that unmirrored code was reintroduced and existing code readjusted to match upstream. * `coin_selection_params.tx_noinputs_size` is said to have a size (sans output count) of `9` instead of `10` as we don't have a SegWit field (referred to as `1 witness overhead (dummy, flag, stack size)` in a code comment) on account of not having SegWit. * To allow for backporting [bitcoin#17331](bitcoin#17331), portions of [bitcoin#21083](bitcoin#21083) (1a6a0b0) and [bitcoin#20536](bitcoin@51e2cd3) (3e69939) were backported. * [bitcoin#17331](bitcoin#17331) seems to have silently broken `CreateTransactionInternal` as functional tests fail (see below) despite the backport not intending to change behavior. This was caught due to unit tests introduced in [dash#3667](#3667). The aberration seems be remedied by portions of [bitcoin#25647](bitcoin#25647) and [bitcoin#26643](bitcoin#26643) and they have been incorporated into this pull request in a separate commit. **Special thanks to UdjinM6 for figuring this out!** 🎉 <details> <summary>Error log:</summary> ``` dash@479e0aa4ebbf:/src/dash$ ./src/test/test_dash -t coinselector_tests,wallet_tests Running 21 test cases... wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 2 - 5 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 4 - 4 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 4 - 5 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 0 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 2 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 4 wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true] CreateTransactionTest failed at: 6 - 5 *** 7 failures are detected in the test module "Dash Core Test Suite" ``` </details> ## How Has This Been Tested? 153bdc2 was tested on Debian 12 (`bookworm`) mixing ~2 tDASH on default settings. ![CoinJoin run](https://github.com/user-attachments/assets/da1f13e7-dd83-4211-8d42-0cd4c770bbf1) ## Breaking Changes * If a transaction isn't shuffled using BIP69 (i.e. if an explicit position for the change txout is specified), it will be randomly shuffled (mirroring upstream behavior, [source](https://github.com/bitcoin/bitcoin/blob/51a3ac242c92e69b59df26f8f9e287b31e5c3b0f/src/wallet/wallet.cpp#L3048)). This deviates from earlier behavior where no shuffling would be done at all if BIP69 isn't applied. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 153bdc2 PastaPastaPasta: utACK 153bdc2 Tree-SHA512: 709b77dce3cea2bbf09eab42c7e70171c3bc6527ff4c387a4db75994da5c0d59025b641d90f734b87a62cdfa8e422d09513697a6e875635de2765a1c9141233e

github-actions · 2025-02-12T16:31:46Z

This pull request has conflicts, please rebase.

PastaPastaPasta · 2025-02-12T17:58:57Z

src/wallet/wallet.h

+    /** When the actual feerate is less than the consolidate feerate, we will tend to make transactions which
+     * consolidate inputs. When the actual feerate is greater than the consolidate feerate, we will tend to make
+     * transactions which have the lowest fees.
+     */
+    CFeeRate m_consolidate_feerate{DEFAULT_CONSOLIDATE_FEERATE};


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?

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 · 2025-02-14T15:13:42Z

Walkthrough

The changes refactor various components of the wallet and RPC systems. Multiple coin selection functions have been updated by renaming and enhancing their logic, including the renaming of SelectCoinsMinConf to AttemptSelection and the introduction of waste calculation. Legacy wallet behaviors such as the setup of legacy script public key management and address import methods have been removed in favor of descriptor-based setups. RPC command registration for features like CoinJoin, Evo, governance, and masternode functionalities has been reorganized so that wallet-specific commands are now grouped into dedicated functions returning spans of RPC commands. Across wallet interfaces, initialization, transaction creation, and test cases, function signatures have been updated to include new context objects (WalletContext and NodeContext) and new parameters (such as -consolidatefeerate) to better manage state. These modifications are consistently applied across benchmarks, wallet methods, RPC utilities, and test suites.

✨ 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

🪧 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:

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.

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.

-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