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
|
⛔ Blockers found — Opus deferred (commit 027764f) |
![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 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 PR adds read-only access to existing masternode, quorum, and InstantSend state without changing existing behavior. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Caller
participant MnEntryImpl
participant LLMQImpl
participant LLMQBlockProcessor
participant InstantSendState
Caller->>MnEntryImpl: request platform HTTPS addresses
MnEntryImpl-->>Caller: return valid addresses
Caller->>LLMQImpl: request platform quorum data
LLMQImpl->>LLMQBlockProcessor: retrieve retained commitments
LLMQBlockProcessor-->>LLMQImpl: return quorum commitments
LLMQImpl-->>Caller: return valid quorum data
Caller->>LLMQImpl: request InstantSend lock by transaction ID
LLMQImpl->>InstantSendState: find matching lock
InstantSendState-->>LLMQImpl: return serialized lock
LLMQImpl-->>Caller: return lock data
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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']
There was a problem hiding this comment.
GUI may have incomplete gateway sets but it won't be empty since the first entry must be CService
There was a problem hiding this comment.
Thanks — that matches what the consumer needs, so I've left the behaviour as-is and documented the constraint instead of widening the API.
For the record on the bot's finding: GetAddrPort() does drop DomainPort entries, so a masternode advertising extra domain-based gateways contributes only its CService ones here. Since the first entry is guaranteed to be a CService, the set is never empty and the GUI always has at least one reachable gateway per evonode — it rotates across many nodes, so an incomplete set per node isn't a functional problem. Resolving domains inside a node-interface getter would also mean DNS in a path that is supposed to be a cheap read of already-synced state.
🤖 Posted autonomously by Claude on behalf of pasta.
| 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']
There was a problem hiding this comment.
Good catch — reworked in d370d90. getPlatformQuorums no longer calls qman->ScanQuorums, so it no longer materializes full CQuorum objects (member lists, contribution/vvec reads) or inserts into mapQuorumsCache. It now reads the mined final commitments directly via quorum_block_processor: GetMinedCommitmentsUntilBlock (or GetMinedCommitmentsIndexedUntilBlock for rotated LLMQ types, matching ScanQuorums' selection) over the same window of max(signingActiveQuorumCount, keepOldKeys) quorums walking back from the tip, then GetMinedCommitment per quorum base block — a single evoDb read each — extracting quorumHash, quorumPublicKey (ToByteVector(specificLegacyScheme=false), unchanged) and the quorum base block height. Invalid public keys are still filtered, null-guard and return semantics are unchanged, and ordering remains newest-first as before.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Avoid constructing full quorums just to export public keys no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| 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']
8b6d630 to
d370d90
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d370d901bd
ℹ️ 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 (!context().llmq_ctx || !context().llmq_ctx->isman) { | ||
| return {}; | ||
| } | ||
| const auto islock{context().llmq_ctx->isman->GetInstantSendLockByTxid(txid)}; |
There was a problem hiding this comment.
When SPORK_2 is disabled, or the node is reindexing/importing, this call returns empty for every txid because CInstantSendManager::GetInstantSendLockByTxid() exits before querying its database (src/instantsend/instantsend.cpp:357-363, with the gate defined at lines 479-482). Consequently, a previously stored islock becomes unavailable to the Platform GUI precisely as it tries to build an asset-lock proof, despite this interface promising emptiness only when the transaction has no islock; use a retained-lock lookup that is independent of whether new InstantSend processing is enabled.
AGENTS.md reference: AGENTS.md:L169-L172
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this is fine; no action needed imo
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The lightweight mined-commitment scan resolves the prior quorum materialization issue without constructing or caching full CQuorum objects. The endpoint export still drops valid DomainPort HTTPS gateways, and the three proof-facing interface contracts remain without targeted tests.
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 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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:138-145: Preserve domain-based Platform HTTPS endpoints
(existing thread: https://github.com/dashpay/dash/pull/7591#discussion_r3772204476)
PLATFORM_HTTPS explicitly accepts both CService and DomainPort entries. ExtNetInfo::ProcessCandidate() allows DomainPort for this purpose, and the unit and functional tests accept internet domains such as example.com:443. GetAddrPort() returns std::nullopt for DomainPort, so this loop silently omits those valid DAPI gateways. Because the public interface returns only std::vector<CService>, the caller cannot recover the omitted hostnames, leaving its gateway set incomplete. Change the interface representation to preserve both endpoint variants, such as validated host-and-port strings or a dedicated endpoint type.
- [SUGGESTION] src/node/interfaces.cpp:138-145: Add targeted coverage for the new interface exports
(existing thread: https://github.com/dashpay/dash/pull/7591#discussion_r3772204486)
The PR introduces three externally consumed interface contracts but adds no tests beyond compilation. Add focused coverage for non-evo and missing-context results, both CService and DomainPort HTTPS endpoints, unsupported LLMQ types, rotated and non-rotated retained-quorum selection, basic-scheme public-key bytes, and exact InstantSend-lock serialization including the missing-lock behavior. These outputs feed Platform proof verification and asset-lock proof construction, so contract-level regressions can reject valid proofs or produce unusable requests.
d370d90 to
08f0b7e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08f0b7ecee
ℹ️ 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".
| std::vector<CService> getPlatformHTTPSAddrs() const override | ||
| { | ||
| std::vector<CService> ret; | ||
| for (const auto& entry : m_dmn->pdmnState->netInfo->GetEntries(NetInfoPurpose::PLATFORM_HTTPS)) { |
There was a problem hiding this comment.
For EvoNodes whose state version predates ProTxVersion::ExtAddr, the Platform endpoint is stored as platformHTTPPort alongside the primary Core address rather than as a PLATFORM_HTTPS netInfo entry (src/evo/dmnstate.h:112-115, with the conversion spelled out in src/evo/specialtxman.cpp:471-473). This loop therefore returns an empty vector for valid legacy EvoNodes that have not submitted an extended-address update, removing those DAPI gateways from the GUI; synthesize the service from the primary address and legacy HTTP port when netInfo cannot store Platform entries.
AGENTS.md reference: AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed at the new head.
Confirmed against the code: CDeterministicMNState only serializes platformP2PPort/platformHTTPPort when nVersion < ProTxVersion::ExtAddr (src/evo/dmnstate.h:112-115), and specialtxman.cpp:471-478 zeroes those scalars from ExtAddr onward precisely because netInfo owns them then. So the netInfo-only loop returned an empty vector for every legacy EvoNode — which today is most of them, meaning the GUI would have seen gateways only from nodes that had already submitted an extended-address update.
getPlatformHTTPSAddrs() now branches on the state version: below ExtAddr it synthesizes the gateway from the primary address and the legacy platformHTTPPort (guarded on MnType::Evo and a non-zero port, using the same CService(addr, port) idiom as dmnstate.cpp:138 and rpc/evo_util.h:41); at ExtAddr and above it reads the PLATFORM_HTTPS netInfo entries as before.
🤖 Posted autonomously by Claude on behalf of pasta.
…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>
08f0b7e to
027764f
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The current head restores legacy EvoNode HTTPS endpoints and avoids constructing full quorum objects, but the endpoint API still drops valid domain-based Platform HTTPS entries and can return an incomplete or empty gateway set. The three new externally consumed interface contracts also remain without targeted tests.
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 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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:152-155: Preserve domain-based Platform HTTPS endpoints
(existing thread: https://github.com/dashpay/dash/pull/7591#discussion_r3772204476)
`PLATFORM_HTTPS` accepts both `CService` and `DomainPort`, but this loop exports only entries for which `GetAddrPort()` succeeds. `ExtNetInfo::ProcessCandidate()` and `ExtNetInfo::Validate()` explicitly permit `DomainPort` for this purpose, while provider-transaction validation requires only that the HTTPS list be nonempty; it does not require its first entry, or any entry, to be a `CService`. A valid EvoNode can therefore advertise only a domain such as `example.com:443`, causing this method to return an empty vector, and mixed lists silently lose their domain gateways. The new header comment's first-entry invariant is not enforced. Change the public representation so it can preserve both endpoint forms, such as validated host-and-port strings or a dedicated endpoint variant.
- [SUGGESTION] src/node/interfaces.cpp:138-158: Add targeted coverage for the new interface exports
(existing thread: https://github.com/dashpay/dash/pull/7591#discussion_r3772204486)
The PR adds three externally consumed interface contracts but no tests exercise them. Add focused coverage for legacy and ExtAddr EvoNodes, non-EvoNodes, missing contexts, both `CService` and `DomainPort` HTTPS endpoints, unsupported LLMQ types, rotated and non-rotated retained-quorum windows, basic-scheme public-key bytes, and exact InstantSend-lock serialization including the missing-lock case. A domain-only HTTPS test would expose the current endpoint loss, while quorum and serialization assertions would protect the Platform proof and asset-lock formats consumed outside this implementation.
3 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.