test: RPC coverage linter + related fixes for listaddressbalances by knst · Pull Request #7279 · dashpay/dash

knst · 2026-04-10T17:50:33Z

Issue being fixed or feature implemented

listaddressbalances returns results that is not matched with its doc and it causes failure on debug builds such as:

 $ listaddressbalances
Internal bug detected: std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [&ret](const RPCResult& res) { return res.MatchesType(ret); }) rpc/util.cpp:530 (HandleRequest) Dash Core v23.1.2-320-ge9b93e3b87be-dirty Please report this issue here: https://github.com/dashpay/dash/issues  (code -1)

Further investigation discovered the bug, that exclude all wallet RPCs from coverage linter.

What was done?

fixes doc for RPC listaddressbalances
implementation of listaddressbalances moved between files to where it is supposed to be
adds functional tests for listaddressbalances
fixes coverage linter to add all wallet RPCs
backport test: fix RPC coverage check bitcoin/bitcoin#33064 (fixes coverage linter and add test for abortrescan RPC)

Backport candidate

only this commit is considerable useful for backporting: fix: RPC doc for listaddressbalances

How Has This Been Tested?

See updates in functional tests

Breaking Changes

N/A

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
I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

…elongs to

8aed477 test: fix RPC coverage check (Brandon Odiwuor) 2630b64 test: add abortrescan RPC test (Brandon Odiwuor) Pull request description: This is bitcoin#27593 cleaned up / rebased, now that the legacy wallet has been dropped. Closes bitcoin#27593. ACKs for top commit: maflcko: lgtm ACK 8aed477 cedwies: ACK 8aed477 Tree-SHA512: 14a28b1ef0c1f63236d04c2ff6c11adddc40642e4a23d30398e8a03fc47f911465af91affc6e66ee2d548515ef4f65fb1cb5d69985c5a771a17b1c9c009f48ad Co-authored-by: merge-script <fanquake@gmail.com>

thepastaclaw · 2026-04-10T17:51:04Z

✅ Review complete (commit c27ba87)

github-actions · 2026-04-10T17:51:16Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2026-04-10T17:56:51Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: f36b8aab-6bb1-4a42-8a8f-438344a1945d

📥 Commits

Reviewing files that changed from the base of the PR and between 53cd792 and c27ba87.

📒 Files selected for processing (1)

src/wallet/rpc/coins.cpp

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

src/wallet/rpc/coins.cpp

Walkthrough

The PR moves the listaddressbalances RPC from src/wallet/rpc/wallet.cpp to src/wallet/rpc/coins.cpp (changing it from a TU-local static definition to an externally declared RPCHelpMan), adds an optional minamount parameter and validation, and implements balance aggregation via GetAddressBalances. Test framework changes introduce a uses_wallet flag to control -disablewallet when starting nodes. Functional tests exercise the new RPC and add rescan assertions. RPC coverage marking for importelectrumwallet was updated.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main changes: fixing the RPC coverage linter and related fixes for listaddressbalances.
Description check	✅ Passed	The description is directly related to the changeset, clearly explaining the bug being fixed, the changes made across multiple files, and testing approach.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

thepastaclaw

Code Review

This backport looks clean on the checked-out SHA. listaddressbalances is moved to the wallet RPC coins module and still registered correctly, and the functional-test coverage plumbing change does what it intends: the cache node now advertises wallet RPCs for coverage reference generation via uses_wallet=True, while tests that never opt into wallet options still run with -disablewallet. I did not find a Dash-specific incompatibility or missing prerequisite on this head.

Reviewed commit: 53cd792

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

thepastaclaw

Code Review

Verified both agents' findings against the checked-out tree at c27ba87. Codex's line-619 finding is a real and high-confidence logic gap: add_dynamically_node() fails to propagate uses_wallet to TestNode, defeating the new flag on that code path. Claude's overlapping-flags observation and backport prerequisite note are both valid but lower-impact suggestions.

Reviewed commit: c27ba87

🟡 2 suggestion(s) | 💬 1 nitpick(s)

1 additional finding

🟡 suggestion: `add_dynamically_node()` does not propagate `uses_wallet`

test/functional/test_framework/test_framework.py (line 619)

The point of uses_wallet (added in this PR) is to let tests opt into wallet support even when options.descriptors is not set, by preventing TestNode.__init__ from appending -disablewallet at test_node.py:117. It is threaded through add_nodes() at line 581 and through _initialize_chain() at line 971, but add_dynamically_node() at line 619 instantiates TestNode without passing uses_wallet=self.uses_wallet. Any node created through this path silently reverts to the old behavior: with uses_wallet=False and descriptors unset, the node is started with -disablewallet even when the enclosing test sets self.uses_wallet = True. Fix by forwarding the framework's flag like the other two call sites do.

💡 Suggested change

        t_node = TestNode(p0, p1, p2, chain=p3, rpchost=p4, timewait=p5, timeout_factor=p6, bitcoind=p7, bitcoin_cli=p8, mocktime=p9, coverage_dir=p10, cwd=p11, extra_conf=p12, extra_args=p13, use_cli=p14, start_perf=p15, use_valgrind=p16, uses_wallet=self.uses_wallet)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `test/functional/test_framework/test_framework.py`:
- [SUGGESTION] line 619: `add_dynamically_node()` does not propagate `uses_wallet`
  The point of `uses_wallet` (added in this PR) is to let tests opt into wallet support even when `options.descriptors` is not set, by preventing `TestNode.__init__` from appending `-disablewallet` at test_node.py:117. It is threaded through `add_nodes()` at line 581 and through `_initialize_chain()` at line 971, but `add_dynamically_node()` at line 619 instantiates `TestNode` without passing `uses_wallet=self.uses_wallet`. Any node created through this path silently reverts to the old behavior: with `uses_wallet=False` and `descriptors` unset, the node is started with `-disablewallet` even when the enclosing test sets `self.uses_wallet = True`. Fix by forwarding the framework's flag like the other two call sites do.
- [SUGGESTION] lines 144-147: Two overlapping wallet flags: `_requires_wallet` and `uses_wallet`
  `BitcoinTestFramework.__init__` now owns two separately-purposed wallet flags: `_requires_wallet` (set by `skip_if_no_wallet()` at line 1071, gates `import_deterministic_coinbase_privkeys()` at lines 466 and 1820) and `uses_wallet` (controls whether `-disablewallet` is appended by `TestNode` at test_node.py:117). The distinction is undocumented and easy to get wrong — e.g. the missed propagation in `add_dynamically_node()` at line 619 is partly enabled by the fact that tests must now remember both flags. Consider either deriving one from the other (`self.uses_wallet = self.uses_wallet or self._requires_wallet`) or adding an explanatory comment at the definition site so future test authors don't set only one and wonder why the wallet is still disabled.

thepastaclaw · 2026-04-23T11:28:11Z

        self._requires_wallet = False
        # Disable ThreadOpenConnections by default, so that adding entries to
        # addrman will not result in automatic connections to them.
+        self.uses_wallet = False


🟡 Suggestion: Two overlapping wallet flags: _requires_wallet and uses_wallet

BitcoinTestFramework.__init__ now owns two separately-purposed wallet flags: _requires_wallet (set by skip_if_no_wallet() at line 1071, gates import_deterministic_coinbase_privkeys() at lines 466 and 1820) and uses_wallet (controls whether -disablewallet is appended by TestNode at test_node.py:117). The distinction is undocumented and easy to get wrong — e.g. the missed propagation in add_dynamically_node() at line 619 is partly enabled by the fact that tests must now remember both flags. Consider either deriving one from the other (self.uses_wallet = self.uses_wallet or self._requires_wallet) or adding an explanatory comment at the definition site so future test authors don't set only one and wonder why the wallet is still disabled.

_{source: unknown}

🤖 Fix this with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed. In `test/functional/test_framework/test_framework.py`: - [SUGGESTION] lines 144-147: Two overlapping wallet flags: `_requires_wallet` and `uses_wallet` `BitcoinTestFramework.__init__` now owns two separately-purposed wallet flags: `_requires_wallet` (set by `skip_if_no_wallet()` at line 1071, gates `import_deterministic_coinbase_privkeys()` at lines 466 and 1820) and `uses_wallet` (controls whether `-disablewallet` is appended by `TestNode` at test_node.py:117). The distinction is undocumented and easy to get wrong — e.g. the missed propagation in `add_dynamically_node()` at line 619 is partly enabled by the fact that tests must now remember both flags. Consider either deriving one from the other (`self.uses_wallet = self.uses_wallet or self._requires_wallet`) or adding an explanatory comment at the definition site so future test authors don't set only one and wonder why the wallet is still disabled.

thepastaclaw · 2026-04-23T11:28:11Z

@@ -114,7 +114,7 @@ def __init__(self, i, datadir, extra_args_from_options, *, chain, rpchost, timew
        if self.mocktime != 0:
            self.args.append(f"-mocktime={mocktime}")

-        if self.descriptors is None:
+        if uses_wallet is not None and not uses_wallet and self.descriptors is None:
            self.args.append("-disablewallet")


💬 Nitpick: Partial backport of bitcoin#28710 uses_wallet infrastructure

bitcoin#33064 relies on the uses_wallet plumbing introduced upstream as part of bitcoin#28710 (legacy wallet removal, commit c847dee). That broader series is not backported to Dash — only the minimum-viable subset needed for bitcoin#33064 was added in follow-up commit 53cd792 (default uses_wallet=False, uses_wallet= kwarg on TestNode.__init__, and the gated -disablewallet branch at line 117). This is sufficient for the current fix, but worth recording: if future backports pull in more of bitcoin#28710's descriptor-only semantics, a more complete port of that series may become warranted.

_{source: ['claude']}

PastaPastaPasta

utACK c27ba87

knst and others added 6 commits April 11, 2026 00:23

fix: RPC doc for listaddressbalances

afa9b91

test: add exception for importelectrumwallet RPC coverage

f6c66c8

test: add coverage for RPC listaddressbalances

4b1a335

refactor: move listaddressbalances to wallet/rpc/coins.cpp where it b…

2b3cdfb

…elongs to

fix: pass uses_wallet correctly to fix coverage bug

53cd792

knst added this to the 24 milestone Apr 10, 2026

knst added the backport-candidate-23.1.x label Apr 10, 2026

knst added the RPC Some notable changes to RPC params/behaviour/descriptions label Apr 10, 2026

thepastaclaw reviewed Apr 10, 2026

View reviewed changes

knst requested review from PastaPastaPasta, UdjinM6 and kwvg April 13, 2026 07:30

UdjinM6 reviewed Apr 19, 2026

View reviewed changes

Comment thread src/wallet/rpc/coins.cpp Outdated

fix: field name in listaddresses

c27ba87

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>

knst requested a review from UdjinM6 April 21, 2026 06:22

thepastaclaw reviewed Apr 23, 2026

View reviewed changes

PastaPastaPasta approved these changes Apr 28, 2026

View reviewed changes

PastaPastaPasta merged commit 2b831c1 into dashpay:develop Apr 28, 2026
44 of 46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: RPC coverage linter + related fixes for listaddressbalances#7279

test: RPC coverage linter + related fixes for listaddressbalances#7279
PastaPastaPasta merged 7 commits intodashpay:developfrom
knst:fix-coverage-check

knst commented Apr 10, 2026 •

edited

Loading

Uh oh!

thepastaclaw commented Apr 10, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 10, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Apr 10, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw left a comment

Uh oh!

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw Apr 23, 2026

Uh oh!

thepastaclaw Apr 23, 2026

Uh oh!

PastaPastaPasta left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

knst commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

Backport candidate

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

thepastaclaw commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai Bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

1 additional finding

Uh oh!

thepastaclaw Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

knst commented Apr 10, 2026 •

edited

Loading

thepastaclaw commented Apr 10, 2026 •

edited

Loading

github-actions Bot commented Apr 10, 2026 •

edited

Loading

coderabbitai Bot commented Apr 10, 2026 •

edited

Loading