fix: don't treat arrays/objects as string literals for composite methods

[kwvg]

Additional Information

thephez identified a bug where attempting to specify arrays as expected by coreP2PAddrs, platformHTTPSAddrs or platformP2PAddrs using dash-cli or the Qt wallet resulted in errors (see below)

$ ./src/dash-cli --testnet protx register_prepare "6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000" 1 '["1.1.1.1:19999"]' "yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ" "93746e8731c57f87f79b3620a7982924e2931717d49540a85864bd543de11c43fb868fd63e501a1db37e19ed59ae6db4" "yTretFTpoi3oQ3maZk5QadGaDWPiKnmDBc" 0 "yNbNZyCiTYSFtDwEXt7jChV7tZVYX862ua"
error code: -8
error message:
Error setting coreP2PAddrs[0] to '["1.1.1.1:19999"]' (invalid input)

This was not caught by functional tests as they send fully formed JSON messages and do not require JSON client interpretation, which both dash-cli and Dash Qt require, this pull request adds additional handling to ensure that arrays and objects are interpreted if they are likely to be so (starting and ending with curly or square brackets) or otherwise passed as-is as a string.

What this pull request isn't is full fledged conversion support for composite RPC arguments to other non-string types like integers, the scope here is specifically to allow for the backwards compatibility logic for coreP2PAddrs, platformHTTPSAddrs or platformP2PAddrs to work as intended.

With this pull request, the following result is expected (see below)

$ ./src/dash-cli --testnet protx register_prepare "6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000" 1 '["1.1.1.1:19999"]' "yhq7ifNCtTKEpY4Yu5XPCcztQco6Fh6JsZ" "93746e8731c57f87f79b3620a7982924e2931717d49540a85864bd543de11c43fb868fd63e501a1db37e19ed59ae6db4" "yTretFTpoi3oQ3maZk5QadGaDWPiKnmDBc" 0 "yNbNZyCiTYSFtDwEXt7jChV7tZVYX862ua"
error code: -32603
error message:
No funds at specified address yNbNZyCiTYSFtDwEXt7jChV7tZVYX862ua

Breaking Changes

None expected.

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]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[kwvg]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Walkthrough

The RPC parameter conversion was refactored to support conditional parsing of non-string JSON values. CRPCConvertTable now stores conversion rules in maps keyed by (method, index) and (method, name) with a boolean preserve_str flag per entry. New helpers MaybeUnquoteString and LikelyJSONType detect quoted JSON-like inputs. ArgToUniValue and RPC conversion entry points consult the maps and preserve_str to decide whether to ParseNonRFCJSONValue or keep arguments as strings. RPCConvertValues/RPCConvertNamedValues adjust method names for composite subcommands using a new IsDefined method. Tests and RPC-help parsing were updated for composite commands.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant RPCConvert as RPCConvertValues()/RPCConvertNamedValues()
    participant CRPCTable as CRPCConvertTable
    participant ArgToUni as ArgToUniValue
    participant LikelyJSON as LikelyJSONType
    participant MaybeUnq as MaybeUnquoteString

    Caller->>RPCConvert: strMethod, strParams[]
    Note over RPCConvert,CRPCTable: Determine whether a composite subcommand exists
    RPCConvert->>CRPCTable: IsDefined(candidateMethod, named?)
    CRPCTable-->>RPCConvert: true/false
    alt composite found
        RPCConvert->>RPCConvert: adjust strMethod to "method subcmd"
    end

    loop for each param
        RPCConvert->>CRPCTable: lookup preserve_str for (strMethod, idx/name)
        CRPCTable-->>RPCConvert: preserve_str (bool)
        RPCConvert->>ArgToUni: paramStr, preserve_str
        ArgToUni->>MaybeUnq: MaybeUnquoteString(paramStr)
        MaybeUnq-->>ArgToUni: possibly_unquoted
        ArgToUni->>LikelyJSON: LikelyJSONType(possibly_unquoted)
        LikelyJSON-->>ArgToUni: isJSON
        alt preserve_str == false AND isJSON == true
            ArgToUni->>ArgToUni: ParseNonRFCJSONValue(possibly_unquoted)
        else
            ArgToUni->>ArgToUni: return string value
        end
        ArgToUni-->>RPCConvert: UniValue
    end

    RPCConvert-->>Caller: UniValue array/object result

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Files needing extra attention:
  • src/rpc/client.cpp — map-based refactor, preserve_str propagation, MaybeUnquoteString/LikelyJSONType, and subcommand resolution logic.
  • src/rpc/client.h — signature changes for RPCConvertValues/RPCConvertNamedValues and public API implications.
  • src/test/rpc_tests.cpp — new composite-command tests and edge cases.
  • test/functional/rpc_help.py — regex change and filtering of composite commands.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 38.46% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main fix: preventing arrays/objects from being treated as string literals for composite methods.
Description check	✅ Passed	The description is directly related to the changeset, explaining the bug, the issue with existing tests, the specific scope of the fix, and expected behavior changes.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 50f6697 and 5978985.

📒 Files selected for processing (4)

• src/rpc/client.cpp (4 hunks)
• src/rpc/client.h (1 hunks)
• src/test/rpc_tests.cpp (1 hunks)
• test/functional/rpc_help.py (2 hunks)

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

• test/functional/rpc_help.py
• src/test/rpc_tests.cpp

🧰 Additional context used

📓 Path-based instructions (1)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit inference engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

• src/rpc/client.h
• src/rpc/client.cpp

🧠 Learnings (2)

📓 Common learnings

Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.

Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.

Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.

Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.

Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.

Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.783Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.

Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.

Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.

Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.

📚 Learning: 2025-08-08T07:01:47.332Z

Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.

Applied to files:

• src/rpc/client.h
• src/rpc/client.cpp

🧬 Code graph analysis (1)

src/rpc/client.h (1)

src/rpc/client.cpp (4)

• RPCConvertValues (349-368)
• RPCConvertValues (349-349)
• RPCConvertNamedValues (370-408)
• RPCConvertNamedValues (370-370)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

• GitHub Check: mac-build / Build source
• GitHub Check: linux64_tsan-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64_fuzz-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: arm-linux-build / Build source
• GitHub Check: Lint / Run linters

🔇 Additional comments (10)

src/rpc/client.h (1)

15-18: LGTM: Pass-by-value enables subcommand detection.

The change from const std::string& to std::string is necessary because the implementation now modifies strMethod locally to support composite subcommands (e.g., "protx register"). With C++20 move semantics, this approach is efficient and cleaner than creating a mutable copy inside the function.

src/rpc/client.cpp (9)

15-22: LGTM: New preserve_str flag enables selective parsing.

The preserve_str field with default false maintains backward compatibility while allowing specific RPC methods (protx composite commands) to opt into conditional parsing based on input format.

---

257-275: LGTM: Protx entries correctly configured for backward compatibility.

The 19 new entries for composite protx methods (register, register_evo, register_fund, etc.) correctly enable preserve_str=true for the address parameters (coreP2PAddrs, platformP2PAddrs, platformHTTPSAddrs) mentioned in the PR objectives. This allows both string and array/object inputs to work correctly.

---

282-283: LGTM: Map-based storage enables per-parameter metadata.

Changing from std::set to std::map with bool values allows storing the preserve_str flag per conversion rule while maintaining efficient O(log n) lookups.

---

285-297: Heuristic-based JSON detection is reasonable for this use case.

The LikelyJSONType helper uses a heuristic (checking for [] or {} brackets) to identify JSON-like inputs. While this could theoretically misidentify strings like "{not valid}", such cases would fail gracefully during parsing with clear error messages. The heuristic is appropriate for the CLI/Qt wallet use case described in the PR.

---

303-318: LGTM: Conditional parsing logic correctly implements backward compatibility.

The ArgToUniValue overloads correctly implement the logic:

• When preserve_str=false (default): Always parse (maintains existing behavior)
• When preserve_str=true: Only parse if LikelyJSONType returns true, otherwise treat as string

This allows both '["1.1.1.1:19999"]' (parsed as array) and "single-string-value" (kept as string) to work correctly for composite methods.

---

320-326: IsDefined implementation is straightforward and sufficient.

The linear search through map entries is O(n), but acceptable given:

• Called only once per RPC invocation during subcommand detection
• Typical conversion table size is small
• Readability and simplicity are prioritized

If performance becomes a concern, consider maintaining a separate index of method names.

---

329-335: LGTM: Constructor correctly populates maps with preserve_str flags.

Using try_emplace ensures that duplicate entries (if any exist in vRPCConvertParams) don't overwrite earlier ones. The preserve_str flag is correctly stored as the map value for both positional and named parameter lookups.

---

349-368: LGTM: Subcommand detection elegantly handles composite methods.

The lambda IIFE pattern cleanly implements subcommand detection:

1. Checks if method has no space (not already composite)
2. Constructs candidate by appending first parameter
3. Uses IsDefined to verify candidate exists in conversion table
4. Updates strMethod only if candidate is found

This enables "protx register" entries in the conversion table to match CLI invocations of protx register ....

---

370-408: LGTM: Named parameter conversion correctly mirrors positional logic.

The subcommand detection for named parameters adds an appropriate check (strParams[0].find('=')) to ensure the first parameter is positional (the subcommand name) rather than a named parameter assignment. The IsDefined call with named=true correctly queries the membersByName map.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[UdjinM6]

utACK 5978985