You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(sdk): refresh the embedded quorum-proof checkpoints on every release - #5669
Follows #5523. VerifiedHttpContextProvider proves quorum keys from a checkpoint built into the SDK (checkpoints/{mainnet,testnet}.json). Nothing moved those checkpoints forward: refreshing them was a manual README procedure, so every release would ship an older checkpoint than the last. A proof covers every ChainLock quorum rotation since its starting state, so proofs grow, and relays must keep blocks further back, the longer a checkpoint ages. This PR makes yarn release refresh them.
What was done?
New refresh_checkpoints example in rs-sdk-trusted-context-provider:
It runs on each network, or on the ones named.
It asks the relay's /proofs for two proofs from the current checkpoint and verifies both with bootstrap::verify:
one to the relay's tip;
one to a block just past the oldest active Platform quorum's mining, about a day below the tip.
It writes the lower target as the new checkpoint, together with the evidence that tests/checkpoints.rs checks: the raw header, the coinbase and the coinbase's Merkle branch.
No added trust in the relay: each new checkpoint is proven from the one it replaces.
Freshness doesn't depend on the relay. The relay's /quorums list only suggests which height to ask for. The new checkpoint must be at least half a Platform quorum lifetime (288 blocks, about 12 hours) below two heights, so the Core heights Platform reports after the release are above it:
The verified tip. A relay cannot prove a ChainLock above the real tip, and hiding its tip only makes the checkpoint older.
Platform's own Core height, authenticated. This height stops while Platform halts. It is read from proved getEpochsInfo responses from up to 5 seed evonodes (from dash-network-seeds, not the relay), spread across /24 networks.
How it's verified: the height is part of the Tenderdash StateId that the Platform quorum signs. The tool proves that quorum's key from the current checkpoint with this crate's provider, then checks the GroveDB proof and the signature with drive-proof-verifier.
Why one answer is enough: a verified height is one Platform committed (unless a Platform quorum signed a false block), so no evonode can report a higher one. The tool takes the highest verified height. If no evonode gives one, the network keeps its checkpoint.
Bodies are read with a cap, and a quorum list marked unsuccessful is refused.
Both files are staged before either is replaced, and files already replaced are restored if a later rename fails.
release.sh runs it after the Tenderdash seed refresh.
If a network fails, it keeps its checkpoint. The script prints a warning and flags it again in the summary before the changelog step. An older checkpoint stays valid; proofs from it are only longer.
Each network's result (old → new, block hash and verified tip, or why it was kept) goes into the release PR's description.
A network that keeps its checkpoint is the only failure the release continues past. Anything else stops the release with the committed checkpoints restored: checkpoint files that cannot be written (exit status 4), a failed report write, a crash or build failure, or an interrupt.
After the refresh, the release runs the checkpoints test, so a checkpoint left mismatched with its evidence is never committed.
MAINNET_QUORUM_RELAY / TESTNET_QUORUM_RELAY select another relay. Public mainnet /proofs still returns 403 from CloudFront, so until that is fixed, mainnet needs a self-hosted relay or keeps its checkpoint.
Release PR description: asks reviewers to check each new block hash on a block explorer. A bad checkpoint would carry into every later build, so this keeps an independent check of the chain on the release path.
README: documents the tool and the trust it relies on, and keeps the manual steps as a fallback.
Test: the unit test that pinned the checkpoint heights (2,550,000 and 1,560,000) now checks the network and a floor. Otherwise the first refresh would fail it.
CI path filter: the crate's entry now includes rs-dapi-client, dapi-grpc and rs-drive-proof-verifier, which the example uses.
How Has This Been Tested?
Ran it live, then restored the files; the moved checkpoints belong in a release PR:
Testnet through the public relay: 1,560,000 → 1,581,312 (000000cffd49…58a90), verified tip 1,581,875. The hash matches Insight. Latest run, with the authenticated Platform height: → 1,581,408, with Platform at 1,581,962 against tip 1,581,962.
Mainnet through a self-hosted relay: 2,550,000 → 2,563,009 (000000000000000a56c5…be4cf), verified tip 2,563,568. The hash matches Blockchair. Latest run: → 2,563,081, with Platform at 2,563,651 against tip 2,563,651.
In both latest runs, all 5 evonodes gave a verified height.
Public mainnet failed on the 403 and left mainnet's files unchanged. The report records it as not refreshed.
A lying relay listing only the newest quorum, with /proofs forwarded to the real relay, is refused (within 288 blocks of the verified tip), and no files change.
cargo test -p rs-sdk-trusted-context-provider --all-features: 115 unit tests and the checkpoints test pass on both the refreshed and the original checkpoints. Rerun after rebasing onto v5.1-dev with feat(sdk): add opt-in verified HTTP context provider #5523 merged.
24 tests cover the refresh. Most run it against a loopback relay that serves two real testnet proofs from one checkpoint. They cover:
a successful refresh and its evidence;
a target, or a listed quorum, within 288 blocks of the verified tip;
a relay that hides its tip;
a halted Platform, its exact bound, and the kept case;
an unsuccessful quorum list, an oversized proof, and a relay error;
a proof of undeclared length refused as it streams in;
a staging write that fails partway, a missing destination, and a rename rollback;
checkpoint files that cannot be written, reported apart from relay failures;
Platform-height authentication, using a captured testnet response and the relay proof of its quorum. The authentic height verifies. A height raised by one block fails the signature, and a non-Platform quorum type or a response without a proof is refused.
sampling (highest verified height, one answer enough) and the node spread.
These are the example's tests (test = true), and they run with the crate's.
clippy -D warnings is clean, with and without --all-features. bash -n passes on release.sh.
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have added "!" to the title and described breaking changes in the corresponding section if my code contains any
I have made corresponding changes to the documentation if needed
If I added or changed GroveDB structure, I described it in the area's structure.rs, regenerated grovedb-structure.json, and checked the structure viewer link posted on this pull request
Reviewer requests paused — your 5 review slots are occupied; this PR is excluded from reviewers' queues. Required approvals still count without a slot.
Build green
Approvals
github (.github/package-filters/rs-packages-no-workflows.yml, .github/package-filters/rs-packages.yml) — ktechmidas or shumkov
files with no dedicated owner (Cargo.lock, packages/rs-sdk-trusted-context-provider/Cargo.toml, packages/rs-sdk-trusted-context-provider/README.md and 5 more) — QuantumExplorer or shumkov
When every merge requirement is met, the PR Hygiene check passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.
Summary by CodeRabbit
Improvements
Embedded network checkpoints are refreshed during release preparation, and proof evidence is validated before the release proceeds.
Release notes include checkpoint refresh results. Expected network refresh failures are reported; release preparation may continue with the affected checkpoint unchanged when both networks report.
Documentation
Updated guidance explains automated and manual checkpoint refreshes, verification requirements, and how refresh results affect releases.
No actionable comments were generated in the recent review.
Walkthrough
Adds a release helper that authenticates Platform heights, verifies relay proofs, and updates embedded mainnet and testnet checkpoint state and evidence. Release preparation runs the helper, validates evidence, handles per-network failures, and includes results in release PR text. Documentation, fixtures, and checkpoint tests are updated.
Changes: Checkpoint refresh and release integration
Layer / File(s)
Summary
Refresh inputs and authenticated heights .github/package-filters/*.yml, packages/rs-sdk-trusted-context-provider/Cargo.toml, packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs, packages/rs-sdk-trusted-context-provider/tests/data/platform-epochs-testnet.json
Adds the example target and dependencies, updates package filters, and samples authenticated Platform Core heights from seed evonodes.
Validates relay quorum data and proofs, selects a checkpoint within the height bound, and tests refresh outcomes and proof errors.
Paired checkpoint and evidence updates packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs, packages/rs-sdk-trusted-context-provider/src/verified_provider.rs
Stages checkpoint and evidence files together, restores originals after rename failures, and updates checkpoint tests to allow newer embedded heights.
Release integration and instructions scripts/release/release.sh, packages/rs-sdk-trusted-context-provider/README.md
Runs refresh and evidence validation during release preparation, restores files on failure, and adds checkpoint results to release output and documentation.
sequenceDiagram
participant ReleaseScript as release.sh
participant RefreshTool as refresh_checkpoints
participant SeedNodes as Seed evonodes
participant Relay as Quorum relay
participant CheckpointFiles as Checkpoint files
ReleaseScript->>RefreshTool: Run refresh for mainnet and testnet
RefreshTool->>SeedNodes: Request proved Platform epoch responses
SeedNodes-->>RefreshTool: Return authenticated Core heights
RefreshTool->>Relay: Request quorum list and checkpoint proofs
Relay-->>RefreshTool: Return quorum list and proofs
RefreshTool->>CheckpointFiles: Write checkpoint state and evidence
ReleaseScript->>CheckpointFiles: Validate checkpoint evidence
ReleaseScript-->>ReleaseScript: Append checkpoint results to release PR text
Loading
Possibly related PRs
dashpay/platform#5521: Adds the checkpoint-anchored proof-chain verification used by the refresh helper.
dashpay/platform#5523: Adds the embedded checkpoints and evidence validation that this PR refreshes and integrates into release preparation.
dashpay/platform#5721: Adds continuation support and persisted checkpoint state for the provider whose embedded checkpoints this PR refreshes.
The checkpoint refresh preserves forward-only automatic updates. No demonstrated issue blocks merging after normal checks.
Pre-merge checks: 4 · 1
Failed checks: 1 warning
Check name
Status
Explanation
Resolution
Docstring Coverage
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…
Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed checks: 4 passed
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title "feat(sdk): refresh the embedded quorum-proof checkpoints on every release" matches the main change. The changeset adds a refresh_checkpoints example, wires it into yarn release and `scr…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title "feat(sdk): refresh the embedded quorum-proof checkpoints on every release" matches the main change. The changeset adds a refresh_checkpoints example, wires it into yarn release and scripts/release/release.sh, and updates the embedded mainnet and testnet checkpoints. The title is specific, uses a conventional-commit prefix, and is 73 characters long, which is within a normal title length.
Full details: Docstring Coverage
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 20 functions across 3 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Final validation — Phase 1 + Phase 2
The checkpoint refresh correctly verifies proof chains from the embedded checkpoint, but its freshness policy trusts unauthenticated relay quorum heights. A malicious or incomplete relay can therefore cause a cryptographically valid checkpoint to advance beyond heights currently available from Platform, making released SDKs unable to obtain quorum keys until the network catches up. The remaining findings are release-tool robustness issues; the listed CI checks succeeded, although the snapshot contains no relevant Rust build/test check.
Triage: critical by gpt-6.1-sol (effort low) — The new checkpoint-refresh tool and release integration alter the cryptographic quorum-proof trust boundary and can propagate an invalid embedded checkpoint into every release.
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [BLOCKING] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:93-104: Do not let unauthenticated quorum metadata choose the checkpoint freshness bound
The `/quorums` response is relay-controlled, but its smallest reported `height` directly determines the requested proof height. The later upper-bound check only compares the proof target with that same untrusted value. A malicious or incomplete relay can report an actual platform quorum near the current tip, obtain a valid proof ending near the tip, and pass both checks. `bootstrap::verify` authenticates the proof chain and target state, but does not establish that the relay supplied the oldest active Platform quorum or that the target remains below heights still reported by Platform. The resulting embedded checkpoint can be valid yet too new for the provider to serve quorum keys at currently reported Core heights. Use an independently authenticated or corroborated freshness source, or require an explicit release-time verification gate before writing the checkpoint.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:125-128: Cap relay responses before buffering them
The refresh tool calls `response.bytes()` before `bootstrap::verify` enforces `MAX_PROOF_BYTES`, so a compromised or misconfigured relay can stream an arbitrarily large `/proofs` body for up to the 180-second request timeout and force the release process to allocate it. The `/quorums` response is also deserialized with `.json()` without a body limit. The provider already has `proof_transport::read_capped`, which rejects oversized declared bodies and stops buffering once the limit is reached. Reuse a shared bounded transport or implement the same streaming cap in this release-only path.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:93-97: Reject quorum responses marked as unsuccessful
The parsed `QuorumsResponse.success` flag is ignored before selecting the oldest quorum. A response with `success: false` and non-empty data can still drive the target-height calculation and proof request. The existing `TrustedHttpContextProvider::fetch_current_quorums` rejects this semantic failure, so the refresh tool should apply the same check rather than treating the data as usable.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:182-191: Do not treat sequential renames as an atomic checkpoint pair update
`write_together` stages both files, but renames them independently. If the first rename succeeds and the second fails, or the process is interrupted between the renames, one destination contains the new generation while the other contains the old one. `release.sh` catches the refresh error and continues, so this mixed pair can be committed by the later `git commit -am`; the checkpoint test will then fail, and the documented guarantee that a failed update leaves both files unchanged is false. Add rollback/backup handling for the first rename or use an update mechanism that makes the pair recoverable as one generation.
In `scripts/release/release.sh`:
- [SUGGESTION] scripts/release/release.sh:99-102: Persist partial checkpoint-refresh results in the release PR
When one network refresh fails, the script records only a shell-local flag and prints a warning. The generated PR still uses the static `scripts/release/pr_description.md`, which says the embedded checkpoints were moved forward without identifying which network retained its old checkpoint. A release can therefore contain an unchanged mainnet or testnet checkpoint while the PR body gives reviewers no durable record of that exception. Include per-network refresh results in the generated release description, or require explicit acknowledgment before creating the PR.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
The checkpoint trust, response-size, quorum-success, and release-report issues from the prior review are fixed at this head. Two release-tool robustness issues remain: a crash between the two checkpoint renames can leave a mixed generation, and a failed staging write can leave an artifact that blocks subsequent releases; the new security-sensitive refresh path also lacks direct automated coverage. CI checks covering the change were still pending in the supplied snapshot.
🟡 3 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: normal by gpt-6.1-sol (effort low) — This is a substantial release-tooling and trusted-checkpoint refresh change involving proof verification and remote relay inputs, but it does not alter consensus, cryptographic verification, funds movement, or persistent storage behavior.
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:307-321: Remove the current staging file when staging fails
If `std::fs::write(&staging, text)` fails after creating or truncating the current staging file, the cleanup loop removes only staging files already recorded in `staged`; it does not remove the current `staging` path. A partial `.json.new` can remain in the repository. The current release invocation may continue because the file is untracked, but the next invocation's initial clean-tree check will refuse to run until the artifact is manually removed.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:117-339: Add direct tests for checkpoint refresh trust and failure paths
The PR adds release-critical transport, proof-refresh, report, and two-file replacement logic, but the committed checkpoint test only validates the already-written fixture files. It does not exercise successful refreshes, verified-tip freshness rejection, failed or oversized relay responses, partial network results, staging cleanup, or rename rollback. Add deterministic tests around the helper logic with mocked responses and temporary files, or move the implementation into a testable library module so these failure paths remain regression-tested.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:325-335: Do not treat sequential renames as an atomic checkpoint pair update
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4416126624)
The rollback handles a rename that returns an error, but it cannot recover from a process termination between the two successful renames. It also logs and discards a failure while restoring an already-renamed file. Because release.sh continues after a nonzero refresh status and later commits tracked files with `git commit -am`, an interrupted or incompletely restored update can leave the checkpoint and evidence files from different generations and create a broken release PR. Replace the pair through a recoverable generation/manifest mechanism, or make the release path validate the pair and abort before committing whenever refresh did not complete cleanly.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
The refresh path now authenticates proof targets, bounds relay responses, rejects unsuccessful quorum lists, and records per-network release results. One blocking correctness issue remains: the release tool can publish a checkpoint newer than the Core height currently reported by Platform after a sufficiently long Platform halt. The checkpoint replacement and test coverage also retain non-blocking durability gaps; changed-code CI checks are not reported as failed, although policy reconciliation and runner selection remain pending.
🔴 1 blocking | 🟡 3 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — The large refresh_checkpoints tool changes the SDK's cryptographic trust-anchor and quorum-proof checkpoint pipeline, including relay-boundary validation and release-time persistence of verified chain evidence.
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [BLOCKING] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:149-180: Do not refresh solely from the verified Core tip during a long Platform halt
The candidate checkpoint is bounded only by `tip.height - depth`, where `tip` is a verified Core ChainLock target. If Platform has halted for more than the 288-block depth while Core continues advancing, this bound can be higher than the Core height currently reported by Platform, and the refresh still writes the checkpoint successfully. The provider starts proofs from the embedded checkpoint and cannot prove a quorum for a Platform-reported height below that checkpoint, so the newly released SDK can be unable to fetch the quorum key needed by the live Platform chain. Require an independently verified Platform/Core height to be at or above the candidate checkpoint, or fail closed unless an operator explicitly confirms that Platform has resumed.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:312-353: Do not treat sequential renames as an atomic checkpoint pair update
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4416126624)
`write_together` replaces the evidence and checkpoint files with separate renames. A process interruption between those calls can leave a mixed generation, and when the second rename returns an error, restoration failures are only logged while `release.sh` continues after the refresh command fails and can later commit the tracked files. Use a recoverable generation/manifest or backup protocol, or make the release path stop until both files are confirmed to belong to the same generation.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:493-625: Add direct tests for checkpoint refresh trust and failure paths
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4418381623)
The loopback tests cover the successful refresh and most relay and staging failures, but they still do not exercise the rename-failure rollback branch in `write_together`. The staging-failure test also uses a missing destination, so it fails while reading the second file rather than during `write`, and `unchanged` checks only `checkpoints/testnet.json.new`; a leftover evidence staging file would not be detected. Add a controllable filesystem seam or helper-level test for rename failure and failed writes, and assert cleanup for both staging paths.
In `packages/rs-sdk-trusted-context-provider/Cargo.toml`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/Cargo.toml:36-40: Gate the native-only refresh example out of wasm32 test builds
The crate supports wasm32, but this example is registered unconditionally with `test = true`. Its source uses `#[tokio::main]`, `#[tokio::test]`, `std::net::TcpListener`, and native filesystem/test helpers, while Tokio is declared only for non-wasm targets. A package test targeting wasm32 can therefore try to compile a native-only release tool and fail; gate the example and its tests to native targets or otherwise exclude this release-only target from wasm builds.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
The proof-chain verification, bounded relay reads, and ordinary staging rollback are implemented correctly, and most prior findings are resolved. One blocking issue remains: the Platform freshness bound trusts a majority of successful replies rather than a majority of selected seed nodes, so two malicious responders can raise the bound during a Platform halt when only three nodes answer. Several non-blocking release reliability, test-coverage, and documentation issues also remain; no code-related CI check has failed, though runner selection is still queued.
🔴 1 blocking | 🟡 4 suggestion(s) | 💬 1 nitpick(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — The large checkpoint-refresh implementation changes a cryptographic proof and remote-relay trust boundary in refresh_checkpoints.rs and verified_provider.rs, where an invalid checkpoint could affect every subsequent SDK release.
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [BLOCKING] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:183-188: Do not define the Platform freshness quorum over successful replies only
`platform_core_height` passes only the nodes that answered successfully to `majority_height`, which requires merely three answers. If two compromised or selectively reachable seed nodes report a height near the verified Core tip while one honest node reports the halted Platform height, the median accepts the attacker-controlled value. Because the checkpoint bound is `min(verified_tip, platform_height) - depth`, this can produce a valid Core checkpoint newer than the height actually reached by Platform; the embedded provider then cannot prove quorum keys for Platform requests below that checkpoint. Require support from more than half of the selected nodes, treating missing replies as no support, or use a consensus-authenticated Platform height before writing the checkpoint.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:201-209: Exclude seed nodes that failed the Platform liveness probe
`status_nodes` claims to select evonodes whose Platform probes passed, but the predicate checks only `reachable` and `ssl`. `PlatformLiveness::None` specifically means TLS succeeded but the probe received no HTTP response. Such nodes still consume one of the limited sample slots and can time out during `getStatus`, reducing the number of usable answers below the required threshold even when healthy seed entries are available. Filter on `platform.live == PlatformLiveness::Ok` and apply the same eligibility predicate in the node-selection test.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:156-192: Test the Platform status sampling path end to end
The new release-critical Platform-height path is not exercised end to end. The refresh tests inject `platform_height` directly, `majority_height` is tested only with vectors, and `status_nodes` checks static seed selection without sending a `GetStatus` response through `DapiClient`. A regression in the response nesting, missing `chain` fields, timeout handling, or the minimum-answer failure path could therefore pass the suite while causing releases to omit the Platform bound or fail unexpectedly. Add tests for successful status responses, malformed or missing fields, transport failures, and the answer threshold.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:116-127: Fail closed when writing the checkpoint refresh report fails
The refresh replaces checkpoint files before appending the per-network result to `--report`. If that report write fails, the example returns a failure status, but `release.sh` treats every nonzero status as an expected network-refresh failure, continues through changelog generation and commit creation, and may create a release PR whose report omits a successfully refreshed checkpoint or says the refresh did not run. Distinguish report I/O failures from per-network refresh failures and abort release preparation before creating the PR when the report is incomplete.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:431-475: Do not treat sequential renames as an atomic checkpoint pair update
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4416126624)
`write_together` restores already-replaced files when a later `rename` returns an error, but the real update still consists of two independent renames. A process termination or power loss between them bypasses the rollback and leaves the checkpoint and evidence from different generations. The subsequent checkpoint test can detect the mixed pair, but only after it has been left in the worktree and potentially included in a release commit. Use a recoverable generation or manifest mechanism, or add durable startup recovery, so an interrupted update is completed or rolled back deterministically.
In `packages/rs-sdk-trusted-context-provider/README.md`:
- [NITPICK] packages/rs-sdk-trusted-context-provider/README.md:261-263: Pass example arguments after Cargo's argument separator
The documented command places `[mainnet] [testnet]` directly after the Cargo example selector. Cargo requires example arguments after `--`; copying this command makes Cargo parse the network names as Cargo arguments instead of passing them to `refresh_checkpoints`.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
The current head fixes the earlier relay-bound, response-size, report, staging-cleanup, and majority-denominator issues. Two release-safety defects remain: the Platform-height sample is not an independent majority on the pinned testnet seed set, and checkpoint rollback failures or interruption can leave a mixed checkpoint/evidence pair that release.sh still commits; unexpected Cargo failures are also treated as degraded network refreshes. Several test and documentation improvements remain non-blocking, while the code-related CI checks shown are pending rather than failed.
🔴 2 blocking | 🟡 5 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — This large change adds automated network retrieval, parsing, and cryptographic quorum-proof verification that generates trusted checkpoints used by the SDK, directly affecting a cryptographic trust boundary.
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [BLOCKING] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:223-263: Do not treat a /24-dominated sample as an independent node majority
`status_nodes` takes one node from each /24 only during the first round, then fills the remaining slots from whichever buckets have more nodes. In the pinned testnet seed data, the eligible nodes occupy three /24s: 29 in one block and one in each of the other two, so a 15-node sample can contain 13 nodes from the same block. `majority_height` then accepts eight matching reports, allowing one operator controlling that block to make the Platform bound appear near the Core tip while the honest Platform chain is halted lower. The ordinary `getStatus` response is not consensus-authenticated, so the `/24` heuristic does not establish the independent authority required for a height persisted into every later SDK release. Require sufficient independent groups or an authenticated Platform height, and fail closed when that threshold is unavailable.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:223-244: Align Platform status transport with the seed probe's TLS semantics
`probe_passed` accepts `SslStatus::Valid`, but the pinned `dash-network-seeds` crate documents that this status checks only the leaf certificate format and expiration; it does not verify the certificate chain, trust roots, or hostname. The subsequent `DapiClient` connection uses `https://<IP>:<port>` and tonic configures normal webpki/native-root validation with the IP as the TLS domain. Nodes marked eligible by the seed probe can therefore fail every status request and count as silent, causing otherwise usable refreshes to retain stale checkpoints. Select endpoints whose certificates satisfy the DAPI client's trust model, use matching transport semantics, or add coverage for this actual connection behavior.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:21-24: Header doc still describes the majority over answers, not nodes asked
The module header says the chosen height is the highest one that more than half of the answers report at or above, but `majority_height` now requires more than half of all nodes asked and at least three supporting answers. The README describes the new rule, but an operator reading the example header alone could incorrectly believe that a small number of successful responses can establish the bound. Update the header to match the implementation.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:939-964: Do not require exactly 15 eligible seed nodes in the test
`status_nodes` is documented and implemented as returning up to `STATUS_NODES`, and `sample_platform_height` explicitly handles a smaller sample. The test instead asserts `nodes.len() == STATUS_NODES`, so a legitimate future seed snapshot with fewer than 15 eligible entries will fail the entire crate even though the production behavior is defined for that case. Assert the upper bound and test sampling behavior with synthetic nodes independently of the external seed inventory.
- [BLOCKING] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:499-509: Do not treat sequential renames as an atomic checkpoint pair update
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4416126624)
The rollback added for a later rename failure is still best effort: failures from `std::fs::write(done, old)` are only logged, and the function returns the original rename error. A process interruption between the two renames has the same effect. In either case the checkpoint and its evidence can come from different generations. `refresh_embedded` turns this into the ordinary per-network status 1, and `release.sh` treats that status as an expected refresh failure before continuing to changelog generation, commit, and PR creation. Make the pair update recoverable as one generation, or propagate rollback uncertainty as a fatal status that stops the release.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:153-185: Test the Platform status sampling path end to end
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4419030176)
The new sampling tests inject a closure and the response tests call `status_height` directly; none invokes `platform_core_height` or exercises the actual `DapiClient` request path. The release-critical wiring that constructs an `AddressList` from each seed URI, creates the versioned `GetStatusRequest`, applies timeout and retry settings, and maps `ExecutionResponse.inner` is therefore untested. A regression in that boundary could make all nodes silent while the current tests continue to pass. Add a native transport-boundary test covering a successful response and a request failure.
In `scripts/release/release.sh`:
- [SUGGESTION] scripts/release/release.sh:101-110: Abort the release on unexpected checkpoint refresh failures
The refresh example reserves status 1 for an expected per-network refresh failure and status 2 for a report-write failure, but this branch treats every other nonzero Cargo status as the expected degraded-network case. A compilation failure, panic, signal termination, or other process-level failure can therefore continue into changelog generation, committing, and PR creation with an empty or partial report. Continue only for status 1 and stop for every other status so release.sh, rather than the example, owns the invariant that the refresh attempt completed.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
The new authenticated Platform-height design addresses the prior checkpoint-safety issues, but the current head contains a blocking ownership error: the epoch request is moved into DAPI execution and then reused, so the refresh example cannot compile. Several non-blocking reliability and coverage issues also remain around startup exit-status handling, seed TLS eligibility, release cleanup, active CI path filters, and deterministic transport testing. CI policy and runner checks were still pending; no relevant code-check failure was reported.
🔴 1 blocking | 🟡 7 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Update the active Rust package filter for the refresh example dependencies .github/package-filters/rs-packages-no-workflows.yml:118-123
The dependency paths were added to .github/package-filters/rs-packages.yml, but tests.yml invokes .github/package-filters/rs-packages-no-workflows.yml, whose rs-sdk-trusted-context-provider entry still lists only the crate and its existing dependencies. Changes to rs-dapi-client, dapi-grpc, or rs-drive-proof-verifier therefore will not select this crate's tests even though the release example depends on them. Add the same dependency paths to the active no-workflows filter.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, rust-quality, security-auditor); gpt-6-astra (phase2-reviewer: general, architecture-layering, rust-quality, security-auditor)
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — The large checkpoint-refresh implementation changes a cryptographic trust boundary by verifying quorum proofs, signatures, authenticated Platform state, and selecting embedded checkpoints used by the SDK.
Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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 `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [BLOCKING] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:213-218: Clone the epoch request before executing it
`DapiRequest::execute` takes `self` by value, so line 214 moves `request` into the transport call. The subsequent call at line 218 then reuses that moved value for proof verification. `GetEpochsInfoRequest` is not `Copy`, so the new release example cannot compile and `release.sh` cannot refresh checkpoints. Clone the request for transport execution while retaining the original for authenticated verification.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:265-270: Do not silently treat invalid protocol versions as latest
`PlatformVersion::get` returns an error for protocol version zero and unknown lower values, but `unwrap_or_else` maps every error to the latest version. That conflates malformed or unsupported metadata with the intentional forward-compatibility case and can make proof decoding use the wrong tables instead of failing closed. Preserve the latest-version fallback only when `metadata.protocol_version > LATEST_VERSION`; propagate errors for zero and otherwise unsupported versions.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:1063-1067: Do not require exactly 15 eligible seed nodes in the test
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4419256898)
The test no longer requires the configured maximum, but it still rejects an empty eligible seed snapshot with `!nodes.is_empty()`. Production deliberately handles zero eligible nodes by returning an error and retaining the existing checkpoint, and the synthetic sampling test already covers the empty case. Make the inventory assertion conditional or assert only the upper bound so a valid snapshot with no currently usable seed entries does not make the crate's tests fail.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:350-356: Align Platform status transport with the seed probe's TLS semantics
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4419256888)
`probe_passed` treats `SslStatus::Valid` as sufficient, but the pinned seed-probe implementation explicitly defines that status as a leaf-certificate format and expiry check without chain, trust-root, or hostname validation. The selected endpoint is then `https://<IP>:<port>`, while `DapiClient` configures webpki/native roots and uses that IP as the TLS domain. A seed can therefore pass the inventory filter and fail every authenticated DAPI request. Authentication makes the consequence fail closed rather than accept an unsafe height, but a correlated certificate mismatch can still leave the release with a stale checkpoint. Select endpoints using transport-compatible health information, configure a matching hostname/CA, or retry additional eligible nodes after DAPI failures.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:203-228: Test the Platform status sampling path end to end
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4419030176)
The tests authenticate a captured response and exercise `sample_platform_height` with synthetic futures, but none invokes the production `AddressList`/`DapiClient`/`GetEpochsInfoRequest::execute` closure. Regressions in URI parsing, request serialization, TLS setup, timeout handling, response unwrapping, or transport errors can therefore make every release-time answer fail while the tests still pass. Add a deterministic local DAPI transport/server test or inject the request operation at the DAPI boundary, while retaining the direct proof-verification fixtures.
In `.github/package-filters/rs-packages-no-workflows.yml`:
- [SUGGESTION] .github/package-filters/rs-packages-no-workflows.yml:118-123: Update the active Rust package filter for the refresh example dependencies
The dependency paths were added to `.github/package-filters/rs-packages.yml`, but `tests.yml` invokes `.github/package-filters/rs-packages-no-workflows.yml`, whose `rs-sdk-trusted-context-provider` entry still lists only the crate and its existing dependencies. Changes to `rs-dapi-client`, `dapi-grpc`, or `rs-drive-proof-verifier` therefore will not select this crate's tests even though the release example depends on them. Add the same dependency paths to the active no-workflows filter.
In `scripts/release/release.sh`:
- [SUGGESTION] scripts/release/release.sh:111-133: Keep checkpoint cleanup active until release preparation is committed
The `INT`/`TERM` trap is removed immediately after the checkpoint evidence test, before the interactive prompt and the remaining release preparation. Interrupting at the prompt, or encountering a later failure before the commit, leaves refreshed checkpoint and evidence files in the worktree instead of restoring the committed pair; the next release invocation can then fail its clean-tree check. Keep a lifecycle-aware cleanup trap active until the checkpoint changes are committed, or restore the files on every pre-commit exit path.
- [SUGGESTION] scripts/release/release.sh:113-124: Abort the release on unexpected checkpoint refresh failures
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4419256878)
`release.sh` now stops for statuses other than 1 and 2, but the refresh tool also returns status 1 for startup and usage failures, including invalid command-line input and failure to construct the HTTP client before any network is attempted. The script interprets status 1 as an expected per-network checkpoint miss and proceeds with release preparation, potentially creating a release with an empty report and no attempted refresh. Reserve status 1 for completed per-network failures and use a distinct fatal status for startup or usage errors.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
The checkpoint proof and freshness logic is well protected at this head, but the release wrapper still conflates a global refresh-tool initialization failure with an expected per-network failure and can proceed to create a release PR without running the refresh. Three non-blocking issues remain: the active CI filter omits the refresh example's dependencies, seed TLS eligibility is weaker than the DAPI client's validation, and the real Platform-height transport path lacks deterministic end-to-end coverage. CI code checks were still pending or queued in the supplied snapshot.
🔴 1 blocking | 🟡 3 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Update the active Rust package filter for the refresh example dependencies .github/package-filters/rs-packages-no-workflows.yml:118-123
The workflow uses .github/package-filters/rs-packages-no-workflows.yml, but this entry still lists only the trusted-context-provider package and its existing anchored dependencies. The new paths were added only to rs-packages.yml, which tests.yml does not use for its Rust package filter or workflow-dispatch override. Changes to rs-dapi-client, dapi-grpc, or rs-drive-proof-verifier therefore will not trigger the trusted-context-provider tests even though the release example depends on them. Mirror those three paths into the active rs-packages-no-workflows.yml entry.
Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to release automation and Rust checkpoint tooling that directly handles quorum-proof, ChainLock, and authenticated Platform signature verification, changing a cryptographic trust boundary.
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `scripts/release/release.sh`:
- [BLOCKING] scripts/release/release.sh:118-129: Stop the release when checkpoint refresh setup fails globally
The release script treats every status 1 from `refresh_checkpoints` as an expected per-network failure. However, the example also returns status 1 when `reqwest::Client::builder().build()` fails before the network loop, so no report line is written and no network was attempted. The script then continues through checkpoint validation, changelog generation, commit creation, and PR creation with unchanged checkpoints, contrary to the release invariant that only completed per-network failures may be tolerated. Reserve status 1 for a completed per-network result, return a distinct startup-failure status, or require one report result for every requested network before continuing.
In `.github/package-filters/rs-packages-no-workflows.yml`:
- [SUGGESTION] .github/package-filters/rs-packages-no-workflows.yml:118-123: Update the active Rust package filter for the refresh example dependencies
The workflow uses `.github/package-filters/rs-packages-no-workflows.yml`, but this entry still lists only the trusted-context-provider package and its existing anchored dependencies. The new paths were added only to `rs-packages.yml`, which `tests.yml` does not use for its Rust package filter or workflow-dispatch override. Changes to `rs-dapi-client`, `dapi-grpc`, or `rs-drive-proof-verifier` therefore will not trigger the trusted-context-provider tests even though the release example depends on them. Mirror those three paths into the active `rs-packages-no-workflows.yml` entry.
In `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:313-360: Align Platform status transport with the seed probe's TLS semantics
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4419256888)
`probe_passed` accepts `SslStatus::Valid`, but the pinned `dash-network-seeds` implementation explicitly says that this status checks only the leaf certificate format and expiry; it does not verify the certificate chain, trust roots, or hostname. The selected URI is then passed to `DapiClient` as `https://<IP>:<port>`, whose tonic transport configures webpki/native roots and uses the IP as the TLS domain. A seed can therefore pass selection and fail every authenticated height request, consuming one of the five sample slots and potentially leaving a stale checkpoint even when other usable nodes exist. Select endpoints using DAPI-compatible validation or add bounded replacement/preflight behavior; the authenticated-height check prevents unsafe advancement but does not remove this availability mismatch.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:185-228: Test the Platform status sampling path end to end
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4419030176)
The release-critical `platform_core_height` path constructs `AddressList`, creates `DapiClient`, executes `GetEpochsInfoRequest`, and parses the response before calling the separately tested authentication helper. The tests cover `sample_platform_height` with synthetic futures and `authenticated_height` with a captured response, but no deterministic test exercises the actual DAPI request construction, transport, TLS/address handling, timeout behavior, or response wrapper conversion. A regression at that boundary could make releases silently retain stale checkpoints while the current tests pass. Add a loopback gRPC/transport harness or inject the request operation at that boundary.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
The checkpoint refresh now authenticates Platform heights, bounds updates against both verified Core and Platform state, limits relay responses, and protects release rollback. Two availability and testability gaps remain: seed TLS eligibility is weaker than the DAPI transport's validation, and local checkpoint I/O failures are treated as tolerated network failures; the complete Platform-height transport path is also not covered by deterministic tests. The available CI snapshot has pending policy checks and a queued runner, but no failed code check is shown.
🟡 3 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — This is a large, intricate change to checkpoint and quorum-proof verification, including authenticated Platform height and cryptographic signature/proof validation in refresh_checkpoints.rs and verified_provider.rs.
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:118-130: Do not treat local checkpoint I/O failures as tolerated network failures
Every error from `refresh_embedded` is converted into the tolerated `NETWORK_KEPT` status, including local failures from `write_together` such as inability to read a destination, permission errors, disk-full errors, or failed renames. `release.sh` accepts status 1 after seeing two report lines, so a local checkpoint-update failure can leave the old files in place, record it as a normal network outage, and continue through changelog generation and release commit. Distinguish network or verification unavailability from local checkpoint/setup failures, and reserve status 1 for completed per-network failures that are safe to tolerate.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:360-366: Align Platform status transport with the seed probe's TLS semantics
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4419256888)
`probe_passed` accepts `SslStatus::Valid`, but the pinned `dash-network-seeds` implementation explicitly defines that status as a leaf-certificate format and expiry check without chain, trust-root, or hostname validation. The selected endpoint is then built as `https://<IP>:<port>` and passed to `DapiClient`, whose tonic transport validates against webpki/native roots and uses the IP literal as the TLS domain. A seed can therefore pass eligibility while every authenticated-height request fails, and a seed-snapshot rotation can make all five selected nodes unavailable at once, leaving a stale checkpoint. This cannot forge a checkpoint because heights are authenticated, but the availability policy should be aligned in `dash-network-seeds` or by probing the actual DAPI transport semantics.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:209-232: Test the Platform status sampling path end to end
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4419030176)
The tests exercise `authenticated_height` with captured protobuf data and `sample_platform_height` with injected futures, but no test invokes `platform_core_height` through `AddressList`, `DapiClient`, the real `GetEpochsInfoRequest` execution path, and the `JoinError` conversion. Regressions in URI parsing for `https://<IP>:<port>`, request construction, timeout settings, response nesting, or transport error handling could therefore make every release lose its authenticated Platform bound while the current tests still pass. Add a deterministic local transport or gRPC integration test covering both a successful response and a failed request.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at c726d17 and independently revalidated all 25 prior findings. No blocking issue remains; two suggestions concern local persistence-error classification and partial-write test coverage. Current CI checks pass, but none validates the native checkpoint-refresh example; this review was static, with no builds or tests run.
🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — The large refresh_checkpoints.rs addition implements authenticated Platform height and quorum-proof validation that determines newly embedded trust checkpoints, directly changing a cryptographic trust boundary.
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:1122-1128: Exercise a failed staging write rather than only a failed destination read
The missing destination makes std::fs::read(path) fail before the second staging write is attempted. This exercises cleanup of an earlier staged file, but not the current-file cleanup at lines 564–569: removing that cleanup would leave this test passing. Keep this read-failure case and add an injected write failure that creates partial staging bytes before returning an I/O error, analogous to the existing injected rename failure. Assert that both original destinations remain unchanged and both staging paths are removed.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:123-130: Do not treat local checkpoint I/O failures as tolerated network failures
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4420116787)
This branch maps every refresh error to NETWORK_KEPT, including local destination reads, staging writes, and rename failures propagated by write_together as String. If a checkpoint directory is readable but unwritable, staging can fail without changing either original file; both network report rows can still be written to the separate temporary report, and the checkpoint-evidence test passes on the unchanged pair. release.sh therefore continues despite a local persistence failure, not just an unavailable relay or rejected proof. The evidence gate prevents releasing a mismatched pair, so this is nonblocking. Preserve error provenance with a typed refresh error and return a fatal status for local persistence failures, reserving NETWORK_KEPT for tolerated per-network remote/proof failures.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 86975aa and revalidated all 26 prior findings. No blocking defect remains; three non-blocking suggestions concern local filesystem-error classification and coverage of staging-write and streaming-limit failures. Current CI checks pass, but none provides targeted native Rust validation for the refresh example; no builds or tests were run during this static review.
🟡 3 suggestion(s)
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — The large new packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs implements proof and signature verification orchestration across untrusted relay and evonode responses to select new embedded cryptographic trust checkpoints.
Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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 `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:841-849: Exercise the streaming limit without Content-Length
The loopback relay always sends the body's actual Content-Length, so this oversized-response test exercises only read_capped's early declared-length rejection. It would still pass if the chunk-reading loop stopped enforcing its cap for responses without Content-Length. The implementation currently checks each chunk correctly, but proof_transport.rs tests a separate helper and cannot protect this one from regression. Add a response without Content-Length that crosses the cap over multiple chunks, preferably remaining open afterward to verify rejection before EOF, and an exact-limit success case.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:1122-1128: Exercise a failed staging write rather than only a failed destination read
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4430489634)
The second destination is missing, so write_together returns from std::fs::read(path) before attempting that destination's staging write. This verifies cleanup of the first staging file, but not the branch that removes the current staging file after a write partially succeeds and then fails. Removing the current-file cleanup at lines 567–569 would leave this test passing. Inject the staging writer, analogous to the existing injected rename operation, and make its second invocation write partial contents before returning an I/O error. Assert that both destinations retain their original contents and both staging paths are absent.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:118-130: Do not treat local checkpoint I/O failures as tolerated network failures
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4420116787)
This arm maps every refresh_embedded error to NETWORK_KEPT, including destination-read, staging-write, and rename errors propagated by write_together through the same String error channel as remote failures. A denied staging write can leave the original checkpoint pair intact, produce both report entries, and pass the subsequent checkpoint-evidence test, so release preparation continues after a local persistence failure. That test protects pair consistency; it does not detect or reject an unsuccessful local write. Preserve an error category through the refresh helpers and return a fatal status for local persistence failures, while retaining NETWORK_KEPT for the intended remote acquisition or verification failures.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
Verified the complete diff at 9ac26d4. The earlier checkpoint-authentication and release-blocking issues are addressed; three non-blocking suggestions remain concerning failure-path coverage and local persistence-error classification. This was a static review with no local builds or tests: current Rust workspace, Kotlin, and Swift checks pass, while PR Hygiene remains pending.
🟡 3 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Triage: critical by gpt-6.1-sol (effort low) — The large new packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs implements security-critical proof and signature verification orchestration that decides which remotely supplied state becomes an embedded SDK trust checkpoint.
Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 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 `packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:841-849: Exercise the streaming limit without Content-Length
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4430700466)
The loopback relay always supplies the body's exact Content-Length at lines 674–678. This oversized-proof test therefore returns through read_capped's declared-length check before exercising the incremental limit at lines 530–538. Removing that streaming guard would leave the test passing, even though unknown-length responses would no longer be bounded. Add a chunked or connection-delimited response without Content-Length whose combined chunks cross the cap, and assert rejection with both checkpoint files unchanged. The production guard is present; this is a regression-coverage gap, not a current size-limit bypass.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:1122-1128: Exercise a failed staging write rather than only a failed destination read
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4430489634)
The missing second destination causes std::fs::read(path) at line 561 to fail before its staging write runs. This checks cleanup of the earlier staging file, but cannot detect a regression in the branch that removes the current staging file after a partial write. Keep both destinations present and inject a staging writer, analogous to the existing rename seam, that writes a prefix and then returns an I/O error. Assert that both destinations retain their original contents and that both staging paths are removed.
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/examples/refresh_checkpoints.rs:123-130: Do not treat local checkpoint I/O failures as tolerated network failures
(existing thread: https://github.com/dashpay/platform/pull/5669#discussion_r4420116787)
Every refresh_embedded error becomes NETWORK_KEPT, including destination-read, staging-write, and rename failures propagated by write_together. If the original pair remains intact or rollback succeeds, both report entries can be written and the checkpoint consistency test passes, so release.sh continues despite a local persistence failure. The setup/report exit statuses and evidence gate address different failure modes: they do not distinguish an unavailable relay from an unwritable checkout. Preserve a typed distinction between remote refresh refusals and local checkpoint persistence failures, and return a non-tolerated status for the latter. This does not expose an authentication bypass, but it makes local release-preparation failures visible instead of silently accepting an unchanged checkpoint.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at dcb6e02 and found no actionable in-scope defects: checkpoint advancement uses existing cryptographic verification owners, authenticated freshness bounds, bounded response reads, and fail-closed release handling. All 27 prior findings were independently reconciled: 23 fixed, one outdated, and three remaining withdrawn. This was a static review with no builds or tests run; current CI still has “Determine changed packages” and “Select compatible runner image” pending, so current-head Rust validation remains unconfirmed.
Triage: critical by gpt-6.1-sol (effort low) — The large new examples/refresh_checkpoints.rs implements a security-critical proof-authentication and checkpoint-selection pipeline that turns untrusted network responses into embedded trust anchors shipped in subsequent SDK releases.
Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
Out-of-scope follow-up suggestions (1)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
Gate the native-only refresh example out of wasm32 test builds — REFUTED: The new dev-dependencies are declared under cfg(not(target_arch = "wasm32")), and the supported library-only wasm invocation does not select this example. The base already contains the native Tokio with_fallback example, so unrestricted wasm example compilation is not a newly broken supported path. Native release orchestration outside the library dependency graph does not violate the book's verify-only or transport-free boundaries.
Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
…ider
A network testnet SDK holding a cold VerifiedHttpContextProvider verifies a
captured, proved getEpochsInfo response through
parse_proof_with_metadata_and_proof: the first verification misses the
quorum key, verify_fetching_quorum_key has the provider prove it through a
loopback relay (asked once, for the response's Core height 1,581,959), and
the same response verifies. The verifier reports that height to the
provider the SDK holds (lowest_verified_core_height), continuation()
anchors on the checkpoint more than one DKG interval below it, and a
provider restored from the continuation's JSON proves the key again from
the anchor and verifies the same response at the same height.
The test fails when verify_tenderdash_signature stops calling
core_height_verified.
It enters at parse_proof_with_metadata_and_proof rather than fetch_many: a
mock SDK verifies through the mock (as regtest) without
verify_fetching_quorum_key, and a network SDK's transport would need a gRPC
server.
Fixture: rs-sdk-trusted-context-provider/tests/data/platform-epochs-testnet.json
is real testnet data from PR #5669 (feat/release-refreshes-quorum-checkpoints,
dcb6e02), copied unchanged: a proved getEpochsInfo response (start_epoch
0, count 1, ascending, prove) at Core height 1,581,959, and the relay's
DIP 175 proof of its signing quorum from the proof-testnet.json checkpoint
(1,574,768), whose target is that height.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
`yarn release` now moves the verified provider's embedded checkpoints
forward. A new example, `refresh_checkpoints`, asks each network's relay
for a proof from the current checkpoint to a block just past the oldest
active Platform quorum's mining (about a day below the tip), verifies it,
and writes the target as the new checkpoint along with the header,
coinbase and Merkle branch the `checkpoints` test checks. Each checkpoint
is proven from the one it replaces.
A network whose relay fails keeps its checkpoint and the release warns;
`MAINNET_QUORUM_RELAY` / `TESTNET_QUORUM_RELAY` select another relay while
public mainnet `/proofs` returns 403. The unit test that pinned the
checkpoint heights now checks the network and a floor instead.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…orum heights
The refresh chose and checked the new checkpoint's height against the
relay's `/quorums` heights, which nothing authenticates, so a relay
could get a valid checkpoint near the tip, above the Core heights
Platform still reports. It now first verifies a proof to the relay's
tip and requires the new checkpoint to be at least half a Platform
quorum lifetime (288 blocks) below it. A relay cannot prove a ChainLock
above the real tip, and hiding its tip only makes the checkpoint older.
Also from review:
- read `/quorums` and `/proofs` bodies with a cap instead of buffering
them whole;
- reject a quorum list marked unsuccessful;
- put back already-replaced files if a later rename fails;
- `--report` records each network's result, and `release.sh` puts it in
the release PR's description instead of a fixed line.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
The refresh tool's logic had no direct tests. `refresh` now takes the
relay, the current checkpoint and the output directory, so its tests run
it against a loopback relay serving two real testnet proofs from one
checkpoint (to 1,576,768 and to a later tip). They cover a successful
refresh and its evidence, a target or listed quorum within 288 blocks of
the verified tip, a relay hiding its tip, an unsuccessful quorum list,
an oversized proof, a relay error, and staging that fails partway. The
example's tests run with the crate's (`test = true`).
Staging now also removes the file whose write failed, so a partial
`.json.new` cannot block the next release's clean-tree check.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
During a Platform halt Core keeps moving, so a checkpoint bounded only
by the verified Core tip could land above the Core height Platform
reports, and released SDKs could not prove any key Platform asks for
until it caught up. The refresh now also asks up to 15 seed evonodes
(from dash-network-seeds, not the relay) for Platform's Core height.
The evonodes have passed their Platform probes and are spread across
/24 networks in random order. The refresh uses the highest height that
more than half of at least 3 answers are at or above, and the checkpoint
must be 288 blocks below both that height and the verified tip.
`write_together` takes its rename function so the rollback path is
tested. Tests cover a halted Platform, its exact bound, the kept case,
the majority rule, and the node spread. The CI path filter for the crate
now includes the DAPI crates the example uses.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…e height
The Platform-height majority was taken over the evonodes that answered,
so with only three of fifteen answering, two could set it. A height now
counts only if more than half of the evonodes asked (and at least three)
report at least it; one that does not answer supports nothing, and if
too few answer the network keeps its checkpoint.
Also from review:
- skip seeds whose Platform liveness probe failed;
- split the `getStatus` parsing and the sampling out of the transport,
and test missing fields, silent nodes and the threshold;
- exit with status 2 when the `--report` file cannot be written, and stop
`release.sh` on it, since the report would no longer match the files;
- put the README's example arguments after `--`.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…points
The refresh bounded the new checkpoint by a Core height that seed
evonodes reported through `getStatus`, unauthenticated, so an operator
holding most of a sample could raise it during a Platform halt. It now
reads the height from proved `getEpochsInfo` responses: the height is part
of the state a Platform quorum signs, and the quorum's key is proven from
the current checkpoint by this crate's provider before drive-proof-verifier
checks the GroveDB proof and the Tenderdash signature. A verified height is
one Platform committed, so the highest is used and one answer is enough.
Each node is verified in its own task, so a panic only loses that answer.
`release.sh` now continues only when a network kept its checkpoint
(status 1). A failed report, any other failure or an interrupt stops the
release with the committed checkpoints restored, and after the refresh it
runs the `checkpoints` test, so a pair left mismatched by a crash between
the two renames is never committed.
Tests use a captured testnet response and the relay proof of its quorum:
the authentic height verifies, while a raised height, another quorum type,
or a response without a proof is refused.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ints until the release commit
- The refresh reads a Platform response with the latest version tables
only when its protocol version is newer than this build's, and refuses
an unknown older one instead of guessing.
- The epoch request is built twice instead of being reused after
`execute` takes it (it is `Copy` today, but that is incidental).
- `release.sh` keeps restoring the embedded checkpoints on any exit,
including an interrupt at the changelog prompt or a later failure, until
the release commit is made.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
The refresh tool exited with status 1 for bad arguments or a failed HTTP
client build, before any network ran, and `release.sh` treats 1 as "a
network kept its checkpoint" and continues. Those now exit with 3, which
stops the release, and `release.sh` also requires a report line for both
networks before continuing past a 1. The refresh example's dependencies
are added to the package filter the tests workflow actually reads.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…uorums
The verified provider now refuses a key cache smaller than the active
Platform quorums (24 on mainnet and testnet). The refresh built its
provider with 16 keys and its tests with 4, so the release step and five
of its tests failed with CacheTooSmall. Both now use 32, the size the
provider's docs recommend.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
A failure to write a network's checkpoint files was reported like a relay
failure, status 1, which the release continues past with the old
checkpoint. It is a local problem with the checkout, so the refresh now
reports it with its own status, 4, and the release stops with the
committed checkpoints restored.
Also covers two paths the tests missed: a proof of undeclared length
refused as it streams in, and a staging write that fails partway, through
a write seam beside the existing rename one.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…fails
Under `set -e` a failed `git checkout` in the restore exited before the
reason for stopping was printed; it is now reported instead. Also lists
status 4 in the refresh tool's header and pins its new tests to the exact
failure.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post /skip-bots to proceed without them; blocking findings still need addressing.
Full checklist in the description.
The reason will be displayed to describe this comment to others. Learn more.
Re-review — Final validation — Phase 1 + Phase 2
Static verification of the complete diff at 8715a97 found no actionable in-scope defects: checkpoint advancement remains authenticated, freshness bounds are independently verified, and local failures stop release preparation. All 27 prior findings were revalidated: 23 are fixed, one is outdated, and three remain withdrawn. No builds or tests were run; current GitHub checks still show Rust workspace runner selection and Kotlin SDK build/tests pending, with no failed code checks reported.
Triage: critical by gpt-6.1-sol (effort low) — The large new refresh_checkpoints.rs implementation establishes replacement trust anchors by authenticating Platform heights and verifying relay proofs, directly changing security-critical proof acceptance and checkpoint selection logic.
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
…ider
A network testnet SDK holding a cold VerifiedHttpContextProvider verifies a
captured, proved getEpochsInfo response through
parse_proof_with_metadata_and_proof: the first verification misses the
quorum key, verify_fetching_quorum_key has the provider prove it through a
loopback relay (asked once, for the response's Core height 1,581,959), and
the same response verifies. The verifier reports that height to the
provider the SDK holds (lowest_verified_core_height), continuation()
anchors on the checkpoint more than one DKG interval below it, and a
provider restored from the continuation's JSON proves the key again from
the anchor and verifies the same response at the same height.
The test fails when verify_tenderdash_signature stops calling
core_height_verified.
It enters at parse_proof_with_metadata_and_proof rather than fetch_many: a
mock SDK verifies through the mock (as regtest) without
verify_fetching_quorum_key, and a network SDK's transport would need a gRPC
server.
Fixture: rs-sdk-trusted-context-provider/tests/data/platform-epochs-testnet.json
is real testnet data from PR #5669 (feat/release-refreshes-quorum-checkpoints,
dcb6e02), copied unchanged: a proved getEpochsInfo response (start_epoch
0, count 1, ascending, prove) at Core height 1,581,959, and the relay's
DIP 175 proof of its signing quorum from the proof-testnet.json checkpoint
(1,574,768), whose target is that height.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
`--tests` builds every target with tests enabled, which now includes the
native-only `refresh_checkpoints` example from #5669, so the wasm32 job
failed to compile. `--test '*'` selects the integration tests alone.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
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
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.
thepastaclaw
left a comment
github-actions
Bot
added
the
waiting-self-review
Issue being fixed or feature implemented
Follows #5523.
VerifiedHttpContextProviderproves quorum keys from a checkpoint built into the SDK (checkpoints/{mainnet,testnet}.json). Nothing moved those checkpoints forward: refreshing them was a manual README procedure, so every release would ship an older checkpoint than the last. A proof covers every ChainLock quorum rotation since its starting state, so proofs grow, and relays must keep blocks further back, the longer a checkpoint ages. This PR makesyarn releaserefresh them.What was done?
refresh_checkpointsexample inrs-sdk-trusted-context-provider:/proofsfor two proofs from the current checkpoint and verifies both withbootstrap::verify:tests/checkpoints.rschecks: the raw header, the coinbase and the coinbase's Merkle branch./quorumslist only suggests which height to ask for. The new checkpoint must be at least half a Platform quorum lifetime (288 blocks, about 12 hours) below two heights, so the Core heights Platform reports after the release are above it:getEpochsInforesponses from up to 5 seed evonodes (fromdash-network-seeds, not the relay), spread across /24 networks.drive-proof-verifier.release.shruns it after the Tenderdash seed refresh.checkpointstest, so a checkpoint left mismatched with its evidence is never committed.MAINNET_QUORUM_RELAY/TESTNET_QUORUM_RELAYselect another relay. Public mainnet/proofsstill returns 403 from CloudFront, so until that is fixed, mainnet needs a self-hosted relay or keeps its checkpoint.rs-dapi-client,dapi-grpcandrs-drive-proof-verifier, which the example uses.How Has This Been Tested?
Ran it live, then restored the files; the moved checkpoints belong in a release PR:
000000cffd49…58a90), verified tip 1,581,875. The hash matches Insight. Latest run, with the authenticated Platform height: → 1,581,408, with Platform at 1,581,962 against tip 1,581,962.000000000000000a56c5…be4cf), verified tip 2,563,568. The hash matches Blockchair. Latest run: → 2,563,081, with Platform at 2,563,651 against tip 2,563,651.A lying relay listing only the newest quorum, with
/proofsforwarded to the real relay, is refused (within 288 blocks of the verified tip), and no files change.cargo test -p rs-sdk-trusted-context-provider --all-features: 115 unit tests and thecheckpointstest pass on both the refreshed and the original checkpoints. Rerun after rebasing ontov5.1-devwith feat(sdk): add opt-in verified HTTP context provider #5523 merged.The refresh builds its provider with a 32-key cache, since feat(sdk): add opt-in verified HTTP context provider #5523 now refuses a cache smaller than the 24 active Platform quorums.
24 tests cover the refresh. Most run it against a loopback relay that serves two real testnet proofs from one checkpoint. They cover:
These are the example's tests (
test = true), and they run with the crate's.clippy
-D warningsis clean, with and without--all-features.bash -npasses onrelease.sh.Breaking Changes
None.
Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Generated with Claude Code
PR Hygiene ·
8715a97/self-reviewedgithub(.github/package-filters/rs-packages-no-workflows.yml,.github/package-filters/rs-packages.yml) — ktechmidas or shumkovCargo.lock,packages/rs-sdk-trusted-context-provider/Cargo.toml,packages/rs-sdk-trusted-context-provider/README.mdand 5 more) — QuantumExplorer or shumkovWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.Summary by CodeRabbit