docs(rpc): revert gating of service/address behind -deprecatedrpc=service, mark platform{P2P,HTTP}Port as deprecated
#6811
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
This pull request has conflicts, please rebase. |
Due to transitional concerns, while the fields are still considered deprecated, they will currently remain unenforced through gating. Also, don't use the word 'field' and 'key' interchangeably.
WalkthroughThe changes remove the deprecated-RPC gating for the “service”/“address” output across multiple components and make these fields unconditionally present. The helper IsServiceDeprecatedRPCEnabled() is deleted from netinfo.{h,cpp}. JSON serialization for masternodes, proTx-related structures, CoinJoin session info, quorum members, and masternodelist now always includes the service/address derived from netInfo->GetPrimary().ToStringAddrPort(). RPC help text is updated accordingly. Release notes are added/updated to clarify deprecations and replacements with the addresses key. Functional tests are updated: deprecation-focused checks are replaced with field presence checks, and the test topology is reduced from 3 to 2 nodes. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/functional/rpc_netinfo.py (1)
461-463: Destroying the wrong node instance at teardownThis test section operates on
node_two, but destroysnode_evo. Fix to avoid unintended side effects.- self.node_evo.destroy_mn(self) + self.node_two.destroy_mn(self)
🧹 Nitpick comments (5)
doc/release-notes-6811.md (1)
4-8: Minor grammar/clarity nitsConsider:
- “have been deprecated … and have been replaced with the key ‘addresses’.”
- “The deprecated keys are still available … but are liable to be removed …”
doc/release-notes-6665.md (1)
9-12: Wording tweak for consistencySuggest “replaced with the ‘addresses’ key” and pluralize “deprecated keys … are still available …”.
test/functional/rpc_netinfo.py (3)
21-21: Preferdecimalover_decimalfor portabilityUse the standard library import to avoid environment-specific failures.
-from _decimal import Decimal +from decimal import Decimal
123-124: Avoid constructing Decimal from a float literalFloat → Decimal can introduce rounding artifacts; construct from a string.
- {self.mn.fundsAddr: float(address_funds_value - Decimal(0.001))} + {self.mn.fundsAddr: float(address_funds_value - Decimal("0.001"))}
361-418: Good coverage for addresses; consider assertingservicein masternode_statusSince
serviceis now ungated, also assert it via masternode_status to catch regressions in CDeterministicMNState::ToJson().masternode_status = self.node_evo.node.masternode('status') + assert_equal(masternode_status['dmnState']['service'], f"127.0.0.1:{self.node_evo.mn.nodePort}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
-
doc/release-notes-6665.md(1 hunks) -
doc/release-notes-6811.md(1 hunks) -
src/coinjoin/client.cpp(1 hunks) -
src/evo/core_write.cpp(3 hunks) -
src/evo/dmnstate.cpp(2 hunks) -
src/evo/netinfo.cpp(0 hunks) -
src/evo/netinfo.h(0 hunks) -
src/rpc/coinjoin.cpp(1 hunks) -
src/rpc/masternode.cpp(1 hunks) -
src/rpc/quorums.cpp(1 hunks) -
test/functional/rpc_netinfo.py(8 hunks)
💤 Files with no reviewable changes (2)
- src/evo/netinfo.h
- src/evo/netinfo.cpp
🧰 Additional context used
📓 Path-based instructions (4)
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/masternode.cppsrc/evo/dmnstate.cppsrc/evo/core_write.cppsrc/coinjoin/client.cppsrc/rpc/quorums.cppsrc/rpc/coinjoin.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/dmnstate.cppsrc/evo/core_write.cpp
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-6811.mddoc/release-notes-6665.md
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/rpc_netinfo.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#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-06-06T11:53:09.094Z
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/evo/core_write.cpp
📚 Learning: 2025-06-16T17:59:55.669Z
Learnt from: kwvg
PR: dashpay/dash#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.
Applied to files:
test/functional/rpc_netinfo.py
🧬 Code graph analysis (1)
test/functional/rpc_netinfo.py (1)
test/functional/test_framework/util.py (1)
assert_equal(69-74)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/coinjoin.cpp
[error] 1-1: Clang format differences found. Command failed with exit code 1: 'git diff -U0 origin/develop --
🔇 Additional comments (14)
src/rpc/coinjoin.cpp (1)
436-438: Help text deprecation tag for "service" — LGTMConsistent with the PR’s deprecation approach while keeping the field available.
src/rpc/quorums.cpp (2)
1-3: Fix clang-format CI failureCI reports a clang-format diff. Please run the project’s formatter to align with the style check (same command CI uses).
Run locally:
- git fetch upstream (or origin) develop
- ./contrib/devtools/clang-format-diff.py -p1 > diff_output.txt
- Review and apply the suggested formatting
If needed, use git clang-format on the PR range.
206-211: Always include “service” in quorum member JSON — LGTMMatches the un-gated exposure across the codebase; ordering with “addresses” is consistent.
src/coinjoin/client.cpp (1)
1874-1878: Expose “service” in CoinJoin session info — LGTMAligned with the deprecation policy; “addrs_core_p2p” remains for detailed endpoints.
src/rpc/masternode.cpp (1)
616-620: Keep both “address” and “addresses” for compatibility — LGTMMatches the plan: legacy singular field retained; structured “addresses” provided alongside.
src/evo/dmnstate.cpp (2)
38-41: Unconditional “service” in CDeterministicMNState::ToJson — LGTMConsistent with removed gating and mirrors other serializers.
73-76: Include “service” in state diffs when netInfo changes — LGTMAppropriate to surface primary address changes via diffs.
src/evo/core_write.cpp (1)
75-77: Un-gated “service” in ProTx/SMN JSON — LGTMCProRegTx, CProUpServTx and CSimplifiedMNListEntry consistently expose "service" alongside "addresses"; no service-specific gating found.
test/functional/rpc_netinfo.py (6)
184-184: Switch to fields-focused flow: LGTM
432-441: Legacy (basic) path checks: LGTMValidates empty addresses with
[::]:0service and legacy platform ports as expected.
449-460: Extended (empty) addresses behavior: LGTMCorrectly asserts empty addresses and -1 platform ports.
469-469: Comment-only changeNo action needed.
507-508: Unconditionalserviceasserts: LGTMConfirms
servicemirrors coreP2PAddrs[0] post-ungating.
138-142: Topology reduced to 2 nodes — no stale references to a third node foundNo occurrences of self.nodes[2] or any "[2]" index in test/functional/rpc_netinfo.py; reconnect_nodes uses range(1, self.num_nodes) which is correct for 2 nodes.
Additional Information
feat: introduce basic extended addresses implementation, stop using
platform{HTTP,P2P}Port#6666No help text has been updated to mark
platformP2PPortandplatformHTTPPortas deprecated outputs as we do not generate help text that mentions what these fields represent. This gap will be addressed in a future PR and the text "(DEPRECATED)" will be appended there.service, the field is only marked as deprecated in documentation but is accessible without any additional runtime flags.Breaking Changes
Refer to release notes.
Checklist