refactor: expose masternode Platform endpoints, quorum keys and islocks via node interface - #7591
refactor: expose masternode Platform endpoints, quorum keys and islocks via node interface#7591PastaPastaPasta wants to merge 1 commit into
Conversation
…ks via node interface Add three read-only exports of already-public node state to the node interface, needed by the Dash Platform GUI train (dashpay#7512): getPlatformHTTPSAddrs() returns the extended netinfo PLATFORM_HTTPS entries for a masternode-list entry, getPlatformQuorums() exports ScanQuorums results over the retained-key window with basic-scheme serialized quorum public keys, and getInstantSendLock() returns the serialized islock for a txid via GetInstantSendLockByTxid. Reference implementation consuming these seams: #67. No consensus surface is touched, no key material is exposed, and everything is compiled unconditionally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
⛔ Blockers found — Opus deferred (commit 8b6d630) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b6d6307e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (const auto service_opt{entry.GetAddrPort()}) { | ||
| ret.push_back(*service_opt); |
There was a problem hiding this comment.
When an EvoNode registers a valid domain endpoint such as example.com:443, GetAddrPort() returns empty because the entry contains a DomainPort, so this loop silently omits it. Platform HTTPS explicitly supports internet domains (as exercised in test/functional/rpc_netinfo.py), meaning the GUI can receive an incomplete or empty DAPI gateway list; expose both service and domain endpoint forms instead of restricting the interface to CService.
AGENTS.md reference: AGENTS.md:L172-L180
Useful? React with 👍 / 👎.
WalkthroughThe node interfaces now expose platform HTTPS addresses for Evo masternodes. Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change adds read-only access to existing node state without a demonstrated production behavior or correctness regression. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/node/interfaces.cpp`:
- Around line 137-146: Add targeted C++ unit tests covering the new interface
exports, including getPlatformHTTPSAddrs() handling invalid platform entries and
non-Evo entries, invalid LLMQ types and retained quorum selection, basic-scheme
public-key bytes, and serialized InstantSend locks. Use existing test fixtures
and assert the changed invariants so valid Platform proofs remain accepted and
asset-lock proofs serialize correctly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5330ff3-c927-4def-bf78-2c489eceafe3
📒 Files selected for processing (2)
src/interfaces/node.hsrc/node/interfaces.cpp
| std::vector<CService> getPlatformHTTPSAddrs() const override | ||
| { | ||
| std::vector<CService> ret; | ||
| for (const auto& entry : m_dmn->pdmnState->netInfo->GetEntries(NetInfoPurpose::PLATFORM_HTTPS)) { | ||
| if (const auto service_opt{entry.GetAddrPort()}) { | ||
| ret.push_back(*service_opt); | ||
| } | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add targeted coverage for the new interface exports.
No test changes cover the new endpoint, quorum, or InstantSend-lock behavior. Add tests for invalid platform entries, non-Evo entries, invalid LLMQ types, retained quorum selection, basic-scheme public-key bytes, and serialized locks. A regression here can cause the GUI to reject valid Platform proofs or build invalid asset-lock proofs.
As per coding guidelines, “Choose and add targeted C++ unit tests for changed behavior,” and quorum changes require small tests that prove the changed invariant.
Also applies to: 552-594
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/node/interfaces.cpp` around lines 137 - 146, Add targeted C++ unit tests
covering the new interface exports, including getPlatformHTTPSAddrs() handling
invalid platform entries and non-Evo entries, invalid LLMQ types and retained
quorum selection, basic-scheme public-key bytes, and serialized InstantSend
locks. Use existing test fixtures and assert the changed invariants so valid
Platform proofs remain accepted and asset-lock proofs serialize correctly.
Source: Coding guidelines
There was a problem hiding this comment.
The coverage request is valid, but endpoint tests must also include a valid DomainPort such as example.com:443. PLATFORM_HTTPS explicitly accepts DomainPort entries, while the current CService-only export silently drops them. The quorum test should also verify that exporting the retained window does not require materializing every full CQuorum object.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Network entry definitions and conversions =='
rg -n -A12 -B8 'GetAddrPort\s*\(|DomainPort|PLATFORM_HTTPS' src --glob '*.{h,cpp}'
printf '%s\n' '== Quorum scan declarations and implementations =='
rg -n -A18 -B10 'ScanQuorums\s*\(' src --glob '*.{h,cpp}'
printf '%s\n' '== Existing interface and quorum-related tests =='
fd -t f . src test 2>/dev/null | rg '(interfaces|quorum|llmq|deterministicmn|netinfo).*(_tests)?\.(cpp|h)$' | head -100
rg -n -A8 -B5 '(GetEntries|PLATFORM_HTTPS|DomainPort|ScanQuorums)' src test --glob '*_tests.cpp' --glob '*.cpp' 2>/dev/null | head -240Length of output: 152096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== ScanQuorums implementation =='
sed -n '177,285p' src/llmq/quorumsman.cpp
printf '%s\n' '== Quorum construction and lightweight commitment retrieval =='
sed -n '168,235p' src/llmq/quorumsman.h
rg -n -A16 -B8 '(BuildQuorumFromCommitment|GetMinedCommitment|GetFinalCommitment|Read.*Commitment|CFinalCommitment)' src/llmq src/evo --glob '*.{h,cpp}' | head -300
printf '%s\n' '== Public interface contract =='
rg -n -A25 -B8 'getPlatformHTTPSAddrs|PlatformQuorum|getPlatformQuorums' src/interfaces/node.hLength of output: 34197
@thepastaclaw, confirmed.
PLATFORM_HTTPS accepts DomainPort. Existing tests validate example.com:443. getPlatformHTTPSAddrs() cannot return this value because its public type is std::vector<CService> and its implementation only uses GetAddrPort(). The fix must change the interface contract and implementation to preserve both CService and DomainPort endpoints. The tests must assert that both endpoint types are exported.
getPlatformQuorums() also currently calls CQuorumManager::ScanQuorums(). That method calls GetQuorum() for every retained result. It therefore materializes full CQuorum objects and their member lists. The fix needs a lightweight retained-commitment query that returns the quorum hash, basic-scheme public key bytes, and base height without constructing CQuorum.
I will keep this finding open with the expanded scope.
🧐🐇
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/dash PR: 7591
File: src/node/interfaces.cpp:137-146
Timestamp: 2026-08-13T03:37:37.236Z
Learning: For Dash Platform endpoint exports, `NetInfoPurpose::PLATFORM_HTTPS` can contain either `CService` or `DomainPort` entries. In particular, `example.com:443` is valid. An `interfaces::MnEntry` accessor for Platform HTTPS endpoints must preserve both types; a `std::vector<CService>` return type silently drops valid `DomainPort` entries.
Learnt from: thepastaclaw
Repo: dashpay/dash PR: 7591
File: src/node/interfaces.cpp:137-146
Timestamp: 2026-08-13T03:37:37.236Z
Learning: In `src/llmq/quorumsman.cpp`, `CQuorumManager::ScanQuorums()` materializes each result through `GetQuorum()`, including full `CQuorum` member data. An interface that exports only retained quorum hash, basic-scheme public key bytes, and height should use a lightweight retained-commitment enumeration path instead of `ScanQuorums()`.
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.
You are interacting with an AI system.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The InstantSend-lock export and basic-scheme quorum-key serialization are consistent with existing node behavior. The Platform endpoint contract omits valid DNS gateways, while quorum enumeration synchronously materializes up to 1,440 full quorum objects; targeted tests for these new interface contracts are also absent.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol; openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
🤖 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 `src/node/interfaces.cpp`:
- [BLOCKING] src/node/interfaces.cpp:140-143: Preserve domain-based Platform HTTPS endpoints
`PLATFORM_HTTPS` entries may contain either a `CService` or a `DomainPort`. `ExtNetInfo::ProcessCandidate()` explicitly permits domains for this purpose, and the netinfo tests accept entries such as `example.com:443`. `GetAddrPort()` returns `std::nullopt` for every `DomainPort`, so this implementation silently drops valid DAPI gateways. Because the interface returns only `std::vector<CService>`, callers cannot recover the omitted hostname, leaving the GUI with an incomplete or empty gateway set for evonodes that advertise DNS endpoints. Expose a representation that preserves both endpoint variants, such as a validated host-and-port string or a dedicated endpoint variant.
- [SUGGESTION] src/node/interfaces.cpp:570-572: Avoid constructing full quorums just to export public keys
Platform quorum parameters retain 24 × 30 × 2 = 1,440 quorum keys. `ScanQuorums()` materializes a `CQuorum` for every uncached commitment: `BuildQuorumFromCommitment()` computes the full member list, reads or rebuilds contribution and verification-vector data, and unconditionally inserts the resulting object into `mapQuorumsCache`, whose per-type capacity is initialized from `keepOldKeys`. The new API only needs the quorum hash, public key, and base height, but its initial GUI-thread invocation can therefore build and retain two months of full quorum objects, and the reference consumer refreshes this context every minute. Add a lightweight quorum-manager export that scans mined commitments and returns these three fields without constructing `CQuorum` instances.
- [SUGGESTION] src/node/interfaces.cpp:137-146: Add targeted coverage for the new interface exports
The PR introduces three externally consumed interface contracts but adds no tests beyond a compile check. Add focused tests for non-evo and missing-context results, valid `CService` and `DomainPort` HTTPS entries, unsupported LLMQ types, the retained-quorum window, basic-scheme public-key bytes, and exact InstantSend-lock serialization with the missing-lock case. In particular, a valid domain-endpoint test would expose the current loss of DNS gateways, while serialization assertions would prevent subtle incompatibilities in Platform proof and asset-lock construction.
| for (const auto& entry : m_dmn->pdmnState->netInfo->GetEntries(NetInfoPurpose::PLATFORM_HTTPS)) { | ||
| if (const auto service_opt{entry.GetAddrPort()}) { | ||
| ret.push_back(*service_opt); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Preserve domain-based Platform HTTPS endpoints
PLATFORM_HTTPS entries may contain either a CService or a DomainPort. ExtNetInfo::ProcessCandidate() explicitly permits domains for this purpose, and the netinfo tests accept entries such as example.com:443. GetAddrPort() returns std::nullopt for every DomainPort, so this implementation silently drops valid DAPI gateways. Because the interface returns only std::vector<CService>, callers cannot recover the omitted hostname, leaving the GUI with an incomplete or empty gateway set for evonodes that advertise DNS endpoints. Expose a representation that preserves both endpoint variants, such as a validated host-and-port string or a dedicated endpoint variant.
source: ['codex']
| const auto quorum_count{static_cast<size_t>(std::max(llmq_params->signingActiveQuorumCount, | ||
| llmq_params->keepOldKeys))}; | ||
| for (const auto& q : context().llmq_ctx->qman->ScanQuorums(type, pindex, quorum_count)) { |
There was a problem hiding this comment.
🟡 Suggestion: Avoid constructing full quorums just to export public keys
Platform quorum parameters retain 24 × 30 × 2 = 1,440 quorum keys. ScanQuorums() materializes a CQuorum for every uncached commitment: BuildQuorumFromCommitment() computes the full member list, reads or rebuilds contribution and verification-vector data, and unconditionally inserts the resulting object into mapQuorumsCache, whose per-type capacity is initialized from keepOldKeys. The new API only needs the quorum hash, public key, and base height, but its initial GUI-thread invocation can therefore build and retain two months of full quorum objects, and the reference consumer refreshes this context every minute. Add a lightweight quorum-manager export that scans mined commitments and returns these three fields without constructing CQuorum instances.
source: ['codex']
| std::vector<CService> getPlatformHTTPSAddrs() const override | ||
| { | ||
| std::vector<CService> ret; | ||
| for (const auto& entry : m_dmn->pdmnState->netInfo->GetEntries(NetInfoPurpose::PLATFORM_HTTPS)) { | ||
| if (const auto service_opt{entry.GetAddrPort()}) { | ||
| ret.push_back(*service_opt); | ||
| } | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Add targeted coverage for the new interface exports
The PR introduces three externally consumed interface contracts but adds no tests beyond a compile check. Add focused tests for non-evo and missing-context results, valid CService and DomainPort HTTPS entries, unsupported LLMQ types, the retained-quorum window, basic-scheme public-key bytes, and exact InstantSend-lock serialization with the missing-lock case. In particular, a valid domain-endpoint test would expose the current loss of DNS gateways, while serialization assertions would prevent subtle incompatibilities in Platform proof and asset-lock construction.
source: ['coderabbit']
2 participants
Issue being fixed or feature implemented
Part of the Dash Platform GUI train (#7512). dash-qt's Platform integration needs three pieces of node state that today have no
interfaces::Nodesurface: evonode Platform HTTPS endpoints (to select DAPI gateways from the locally synced masternode list), locally retained Platform-quorum public keys (to verify Platform quorum signatures against local LLMQ data instead of trusting a remote key service), and InstantSend locks by txid (to build asset-lock proofs).What was done?
Three read-only additions, compiled unconditionally (no Platform types, no feature gate):
MnEntry::getPlatformHTTPSAddrs()— thePLATFORM_HTTPSentries from the extended address list; empty for non-evo masternodes.LLMQ::getPlatformQuorums(uint8_t llmq_type)— quorum hash, serialized basic-scheme BLS public key, and height for locally retained quorums, scanningmax(signingActiveQuorumCount, keepOldKeys)so verification keeps working for proofs signed by recently rotated-out quorums.LLMQ::getInstantSendLock(const uint256& txid)— serialized islock or empty.These are exports of already-public node state (
netInfo->GetEntries,qman->ScanQuorums,isman->GetInstantSendLockByTxid); no consensus surface, no key material, no behavior change for existing code.Reference implementation using these seams: PastaPastaPasta#67.
How Has This Been Tested?
Compiles cleanly (
node/libbitcoin_node_a-interfaces.oon macOS arm64 against depends). No other implementers of these interface classes exist in-tree (verified across src/test, src/qt/test, src/interfaces, src/ipc), so the pure-virtual additions require no stubs.Breaking Changes
None.