docs(rpc): restore -deprecatedrpc=service gating for legacy network fields, extend to platform{P2P,HTTP}Port#7181
Open
kwvg wants to merge 1 commit intodashpay:developfrom
Open
docs(rpc): restore -deprecatedrpc=service gating for legacy network fields, extend to platform{P2P,HTTP}Port#7181kwvg wants to merge 1 commit intodashpay:developfrom
-deprecatedrpc=service gating for legacy network fields, extend to platform{P2P,HTTP}Port#7181kwvg wants to merge 1 commit intodashpay:developfrom
Conversation
|
WalkthroughThis change adds runtime deprecation handling for RPC fields related to masternode networking and platform ports. A new function, IsServiceDeprecatedRPCEnabled(), checks for the Sequence Diagram(s)(Skipped) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/evo/core_write.cpp (1)
1-1:⚠️ Potential issue | 🟡 MinorAddress pipeline failure: Clang-format differences.
The CI reports formatting differences. Run the suggested command to fix formatting:
./contrib/devtools/clang-format-diff.py -p1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/evo/core_write.cpp` at line 1, CI found clang-format changes in src/evo/core_write.cpp; fix by running the repository's formatting fix script and committing the results. Run ./contrib/devtools/clang-format-diff.py -p1 from the repo root (ensure clang-format is installed and on PATH), review the modified hunks (file src/evo/core_write.cpp) then add/commit the updated file so the formatting matches CI.
🧹 Nitpick comments (2)
test/functional/rpc_netinfo.py (1)
214-218: Consider using list unpacking for cleaner syntax.Per static analysis (RUF005), list unpacking is preferred over concatenation.
♻️ Proposed refactor
- self.restart_node(self.node_evo.mn.nodeIdx, extra_args=evo_extra_args + ['-deprecatedrpc=service']) + self.restart_node(self.node_evo.mn.nodeIdx, extra_args=[*evo_extra_args, '-deprecatedrpc=service'])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/rpc_netinfo.py` around lines 214 - 218, Replace the list concatenation evo_extra_args + ['-deprecatedrpc=service'] with list unpacking to produce a cleaner extra_args list when calling restart_node; specifically update the call that uses evo_extra_args (from node_evo.node.extra_args.copy()) to pass extra_args=[*evo_extra_args, '-deprecatedrpc=service'] to restart_node(self.node_evo.mn.nodeIdx, extra_args=...), keeping the surrounding reconnect_nodes() and test_shims() calls unchanged.src/evo/core_write.cpp (1)
224-226: Consider cachingIsServiceDeprecatedRPCEnabled()result.
IsServiceDeprecatedRPCEnabled()performs a linear search throughgArgs.GetArgs("-deprecatedrpc")on each call. When emitting multiple deprecated fields within the sameToJson()method, this results in redundant lookups.♻️ Proposed optimization
UniValue CDeterministicMNState::ToJson(MnType nType) const { UniValue obj(UniValue::VOBJ); obj.pushKV("version", nVersion); + const bool fDeprecatedRPC = IsServiceDeprecatedRPCEnabled(); - if (IsServiceDeprecatedRPCEnabled()) { + if (fDeprecatedRPC) { obj.pushKV("service", netInfo->GetPrimary().ToStringAddrPort()); } obj.pushKV("addresses", GetNetInfoWithLegacyFields(*this, nType)); // ... other fields ... if (nType == MnType::Evo) { obj.pushKV("platformNodeID", platformNodeID.ToString()); - if (IsServiceDeprecatedRPCEnabled()) { + if (fDeprecatedRPC) { obj.pushKV("platformP2PPort", GetPlatformPort</*is_p2p=*/true>(*this)); obj.pushKV("platformHTTPPort", GetPlatformPort</*is_p2p=*/false>(*this)); } }The same optimization could apply to
CProRegTx::ToJson(),CProUpServTx::ToJson(), and potentially other methods with multiple deprecation checks.Also applies to: 239-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/evo/core_write.cpp` around lines 224 - 226, Cache the boolean result of IsServiceDeprecatedRPCEnabled() at the start of the ToJson() method and reuse that local variable for all subsequent deprecation checks instead of calling IsServiceDeprecatedRPCEnabled() repeatedly; update the ToJson() implementations (e.g., the current ToJson(), CProRegTx::ToJson(), and CProUpServTx::ToJson()) to compute bool serviceDeprecated = IsServiceDeprecatedRPCEnabled(); and use serviceDeprecated in the conditional branches that add deprecated fields (such as the block that pushes "service" using netInfo->GetPrimary().ToStringAddrPort()) so the linear lookup over gArgs.GetArgs("-deprecatedrpc") happens only once per ToJson() invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rpc/coinjoin.cpp`:
- Line 436: The help string for the RPCResult entry (RPCResult::Type::STR,
"service") is causing a clang-format failure due to line length; break the long
literal into clang-format-friendly adjacent string literals (e.g. split the
sentence into two quoted pieces next to each other) so the declaration remains
the same but the line wraps cleanly for clang-format.
In `@src/rpc/masternode.cpp`:
- Around line 693-695: The schema/help metadata still declares "address" as
always present even though the code only adds it when
IsDeprecatedRPCEnabled("service") is true (see objMN.pushKV("address", ...)
usage); update the masternodelist metadata for the "json", "recent" and "evo"
modes in this file to mark "address" as optional (or only include the field
description when IsDeprecatedRPCEnabled("service") is true) so the declared
schema matches the actual output from objMN.pushKV.
In `@src/rpc/quorums.cpp`:
- Around line 212-214: Update the "quorum info" RPC result schema so the
"service" field is marked optional to match its runtime gating by
IsDeprecatedRPCEnabled("service"); locate the RPCHelpMan/JSON schema or the
result description for the "quorum info" RPC in src/rpc/quorums.cpp (the block
that documents the return fields) and change the "service" entry to indicate it
may be omitted (e.g. mark as optional or wrap with conditional text), keeping
the key name "service" and referencing that its presence depends on
IsDeprecatedRPCEnabled("service") and the code path that pushes it
(dmn->pdmnState->netInfo->GetPrimary().ToStringAddrPort()).
---
Outside diff comments:
In `@src/evo/core_write.cpp`:
- Line 1: CI found clang-format changes in src/evo/core_write.cpp; fix by
running the repository's formatting fix script and committing the results. Run
./contrib/devtools/clang-format-diff.py -p1 from the repo root (ensure
clang-format is installed and on PATH), review the modified hunks (file
src/evo/core_write.cpp) then add/commit the updated file so the formatting
matches CI.
---
Nitpick comments:
In `@src/evo/core_write.cpp`:
- Around line 224-226: Cache the boolean result of
IsServiceDeprecatedRPCEnabled() at the start of the ToJson() method and reuse
that local variable for all subsequent deprecation checks instead of calling
IsServiceDeprecatedRPCEnabled() repeatedly; update the ToJson() implementations
(e.g., the current ToJson(), CProRegTx::ToJson(), and CProUpServTx::ToJson()) to
compute bool serviceDeprecated = IsServiceDeprecatedRPCEnabled(); and use
serviceDeprecated in the conditional branches that add deprecated fields (such
as the block that pushes "service" using
netInfo->GetPrimary().ToStringAddrPort()) so the linear lookup over
gArgs.GetArgs("-deprecatedrpc") happens only once per ToJson() invocation.
In `@test/functional/rpc_netinfo.py`:
- Around line 214-218: Replace the list concatenation evo_extra_args +
['-deprecatedrpc=service'] with list unpacking to produce a cleaner extra_args
list when calling restart_node; specifically update the call that uses
evo_extra_args (from node_evo.node.extra_args.copy()) to pass
extra_args=[*evo_extra_args, '-deprecatedrpc=service'] to
restart_node(self.node_evo.mn.nodeIdx, extra_args=...), keeping the surrounding
reconnect_nodes() and test_shims() calls unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
doc/release-notes-7181.mdsrc/coinjoin/client.cppsrc/evo/core_write.cppsrc/evo/dmnstate.cppsrc/evo/netinfo.cppsrc/evo/netinfo.hsrc/rpc/coinjoin.cppsrc/rpc/masternode.cppsrc/rpc/quorums.cpptest/functional/rpc_netinfo.py
| {RPCResult::Type::STR_HEX, "protxhash", "The ProTxHash of the masternode"}, | ||
| GetRpcResult("outpoint"), | ||
| {RPCResult::Type::STR, "service", "(DEPRECATED) The IP address and port of the masternode"}, | ||
| {RPCResult::Type::STR, "service", "The IP address and port of the masternode (DEPRECATED, returned only if config option -deprecatedrpc=service is passed)"}, |
There was a problem hiding this comment.
Fix clang-format failure on the updated help string.
Line 436 appears to be part of the formatting diff failure; please wrap this help text in clang-format-friendly style.
Proposed formatting-only fix
- {RPCResult::Type::STR, "service", "The IP address and port of the masternode (DEPRECATED, returned only if config option -deprecatedrpc=service is passed)"},
+ {RPCResult::Type::STR, "service",
+ "The IP address and port of the masternode (DEPRECATED, returned only if "
+ "config option -deprecatedrpc=service is passed)"},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {RPCResult::Type::STR, "service", "The IP address and port of the masternode (DEPRECATED, returned only if config option -deprecatedrpc=service is passed)"}, | |
| {RPCResult::Type::STR, "service", | |
| "The IP address and port of the masternode (DEPRECATED, returned only if " | |
| "config option -deprecatedrpc=service is passed)"}, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rpc/coinjoin.cpp` at line 436, The help string for the RPCResult entry
(RPCResult::Type::STR, "service") is causing a clang-format failure due to line
length; break the long literal into clang-format-friendly adjacent string
literals (e.g. split the sentence into two quoted pieces next to each other) so
the declaration remains the same but the line wraps cleanly for clang-format.
Member
There was a problem hiding this comment.
coderabbit; don't make comments for clang-format; we often intentionally choose to not follow the format is suggests
There was a problem hiding this comment.
@PastaPastaPasta, understood! I apologize for the unnecessary clang-format comment. I'll refrain from flagging formatting issues in future reviews since you intentionally manage formatting based on readability and other considerations.
✏️ Learnings added
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 7181
File: src/rpc/coinjoin.cpp:436-436
Timestamp: 2026-02-27T17:21:49.475Z
Learning: In the dashpay/dash repository, do not make review comments about clang-format failures or suggest clang-format fixes. The team often intentionally chooses not to follow clang-format suggestions, prioritizing readability and other considerations over strict formatting compliance.
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.
🧠 Learnings used
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/deterministicmns.cpp:1284-1287
Timestamp: 2025-06-16T17:59:55.669Z
Learning: In Dash masternode ProRegTx validation, platform ports (platformHTTPPort and platformP2PPort) are mandatory and must be non-zero, while netInfo (ipAndPort) is optional. This means that even if an empty netInfo returns 0 from GetPrimary().GetPort(), it won't cause false positives in port duplication checks since platform ports cannot be 0.
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.
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
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.
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/rpc/evo.cpp:1273-1273
Timestamp: 2025-07-09T15:05:36.250Z
Learning: When clang-format suggestions significantly harm readability (like splitting logical parameter groups across multiple lines), it's acceptable to use `// clang-format off` and `// clang-format on` directives to exclude the problematic section from automatic formatting, prioritizing code readability over strict line length compliance.
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: 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.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/functional/rpc_netinfo.py (2)
216-216: Prefer iterable unpacking over list concatenation for restart args.Ruff’s RUF005 warning is valid here; unpacking is clearer and avoids an extra list-concat operation.
Suggested diff
- self.restart_node(self.node_evo.mn.nodeIdx, extra_args=evo_extra_args + ['-deprecatedrpc=service']) + self.restart_node(self.node_evo.mn.nodeIdx, extra_args=[*evo_extra_args, '-deprecatedrpc=service'])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/rpc_netinfo.py` at line 216, Replace the list concatenation in the restart call with iterable unpacking: in the call to restart_node (method restart_node) that currently uses evo_extra_args + ['-deprecatedrpc=service'], construct the extra_args using unpacking (e.g. extra_args=[*evo_extra_args, '-deprecatedrpc=service']) so you avoid creating a temporary concatenated list and satisfy RUF005; keep evo_extra_args and the '-deprecatedrpc=service' element unchanged.
489-494: Consider passingblockhashtogetrawtransactioncalls for consistency with framework patterns. These calls depend on all nodes being fully synced (which they are), but the framework convention (seetest_framework/util.py:587andtest_framework.py:1186) is to provideblockhashexplicitly. For lines 489–490, 512, 528, and 592–594, consider retrieving block heights from the transaction responses and using the 3-argument form.Also applies to: Line 216: Replace list concatenation
evo_extra_args + [...]with unpacking syntax[*evo_extra_args, ...]to follow Python best practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/functional/rpc_netinfo.py` around lines 489 - 494, The getrawtransaction calls (eg. where proregtx_rpc and proupservtx_rpc are retrieved) should use the 3-argument form and pass an explicit blockhash instead of relying on node sync: obtain the blockhash either from the transaction response (tx['blockhash'] after a first call or from the block height in the tx dict) or by calling getblockhash(height) and then call getrawtransaction(txid, True, blockhash); update any other getrawtransaction sites mentioned (including where extract_from_listdiff is used) similarly. Also replace the list concatenation pattern evo_extra_args + [...] with unpacking syntax [*evo_extra_args, ...] to follow Python best practices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/functional/rpc_netinfo.py`:
- Line 216: Replace the list concatenation in the restart call with iterable
unpacking: in the call to restart_node (method restart_node) that currently uses
evo_extra_args + ['-deprecatedrpc=service'], construct the extra_args using
unpacking (e.g. extra_args=[*evo_extra_args, '-deprecatedrpc=service']) so you
avoid creating a temporary concatenated list and satisfy RUF005; keep
evo_extra_args and the '-deprecatedrpc=service' element unchanged.
- Around line 489-494: The getrawtransaction calls (eg. where proregtx_rpc and
proupservtx_rpc are retrieved) should use the 3-argument form and pass an
explicit blockhash instead of relying on node sync: obtain the blockhash either
from the transaction response (tx['blockhash'] after a first call or from the
block height in the tx dict) or by calling getblockhash(height) and then call
getrawtransaction(txid, True, blockhash); update any other getrawtransaction
sites mentioned (including where extract_from_listdiff is used) similarly. Also
replace the list concatenation pattern evo_extra_args + [...] with unpacking
syntax [*evo_extra_args, ...] to follow Python best practices.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
doc/release-notes-7181.mdsrc/coinjoin/client.cppsrc/evo/core_write.cppsrc/evo/dmnstate.cppsrc/evo/netinfo.cppsrc/evo/netinfo.hsrc/rpc/coinjoin.cppsrc/rpc/masternode.cppsrc/rpc/quorums.cpptest/functional/rpc_netinfo.py
🚧 Files skipped from review as they are similar to previous changes (7)
- src/coinjoin/client.cpp
- src/evo/core_write.cpp
- src/rpc/masternode.cpp
- doc/release-notes-7181.md
- src/rpc/coinjoin.cpp
- src/evo/dmnstate.cpp
- src/evo/netinfo.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Additional Information
service/addressbehind-deprecatedrpc=service, markplatform{P2P,HTTP}Portas deprecated #6811Breaking Changes
See release notes.
Checklist