Repository navigation
fix(sdk): verify quorum proofs without blocking the executor - #5720
Conversation
`bootstrap::start` decodes an envelope and makes every check before the first link; `Verification::verify_link` verifies one link, and `finish` verifies any left, then the target and the records. An async caller can yield between links instead of spending a long chain's BLS work in one poll. `verify` is now `start` followed by `finish`, so both paths make the same checks in the same order. Co-Authored-By: Claude Opus 5.5 <[email protected]>
A relay can answer a proof request with a valid chain through every ChainLock quorum since the state it was asked from: about 960 links on testnet today, 4 s of BLS work natively and more in a browser. The provider verified it in one poll, stalling a Tokio worker (every task on a current-thread runtime, including the fetch's own deadline) or the browser's event loop. The fetch now verifies one link at a time and yields to the executor after every 10 ms of verification: Tokio's `yield_now`, which runs due timers first, natively, and a zero-delay browser timer in wasm, so the event loop's timers, input and rendering run in between. Dropping the fetch stops verification at the next yield, so abandoned fetches leave no work behind. Fixes #5706 Co-Authored-By: Claude Opus 5.5 <[email protected]>
`QuorumProofChain::finish` now verifies any links left before the target, so `verify` and `Verification::finish` share one loop, and the progress carries the network instead of a cloned checkpoint. The checks and their order are unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…in browsers In a browser the proof fetch runs on a local task that outlives its callers, so nothing stopped a long verification, and hidden tabs throttle timers to one wake-up a second or less: a zero-delay timer per link would stretch even an honest proof to seconds and let a hostile one hold a proving place for minutes. Verification now ends at the fetch's deadline without blaming the relay, and the browser yield resumes on a `MessageChannel` message, which is not throttled, falling back to a timer. No yield follows the last link. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Core verification flow packages/rs-core-proof/src/chain.rs, packages/rs-core-proof/src/bootstrap.rs, packages/rs-core-proof/tests/* |
The chain and bootstrap APIs now support starting verification, verifying links individually, and finishing verification. Tests compare stepwise results and errors with one-call verification. |
Provider scheduling and deadline handling packages/rs-sdk-trusted-context-provider/src/proof_fetches.rs, packages/rs-sdk-trusted-context-provider/Cargo.toml, packages/rs-sdk-trusted-context-provider/src/verified_provider.rs |
The provider verifies links in time slices and yields through a native timer or browser MessageChannel. It reports when verification reaches the fetch deadline. Tests cover executor progress, shared verification, and deadline handling. |
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant ProofFetches
participant Bootstrap as bootstrap::start
participant Verification
participant ExecutorYield
ProofFetches->>Bootstrap: start proof verification
Bootstrap-->>ProofFetches: return Verification
loop While links remain
ProofFetches->>Verification: verify_link
ProofFetches->>ExecutorYield: yield after verification slice
ExecutorYield-->>ProofFetches: resume verification
end
ProofFetches->>Verification: finish
Possibly related issues
- feat(sdk): verify Core quorum proof chains (DIP-0175, getquorumproofchain) #5616: This PR changes the chain verification path used by the tracked quorum-proof feature.
Possibly related PRs
- dashpay/platform#5521: This PR extends the core proof verifier and its one-call verification APIs with incremental verification.
- dashpay/platform#5523: This PR changes the proof-fetch verification path introduced by this PR.
Suggested reviewers: thepastaclaw
Merge Risk: 
4a952
-
No actionable issue is established for the incremental verification or provider scheduling changes. The PR is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: adding non-blocking quorum-proof verification for the SDK executor. Linked Issues check The changes satisfy the coding requirements in directly linked issue #5706.bootstrap::start,Verification::verify_link, andfinishenable link-by-link verification.ProofFetcheslimits verifi…Out of Scope Changes check The changed core-proof APIs and implementations support incremental verification required by #5706. The provider scheduling code, deadline handling, documentation, and added tests validate that behavi…Docstring Coverage Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentati… Full details: Linked Issues check
-
Explanation
The changes satisfy the coding requirements in directly linked issue
#5706.bootstrap::start,Verification::verify_link, andfinishenable link-by-link verification.ProofFetcheslimits verification slices, yields between slices on native and wasm targets, and checks the fetch deadline before links and result acceptance. The provider therefore does not block the executor or browser event loop during long proof verification. The regression tests cover timer progress, current-thread execution, shared fetches, deadlines, and browser timers. The optional chain-length bound is not required by#5706.
Full details: Out of Scope Changes check
-
Explanation
The changed core-proof APIs and implementations support incremental verification required by
#5706. The provider scheduling code, deadline handling, documentation, and added tests validate that behavior. The synthetic and fixture-equivalence tests support the required proof-verification changes. No unrelated change is demonstrated.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 88.89% which is sufficient. 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 18 functions across 6 files. (1 skipped: 1 unsupported.)
-
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.
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
✅ Final review complete — no blockers (commit 4a95215) · triage: critical |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The incremental verifier preserves the previous proof checks and their ordering, but native shared-fetch callers can bypass the intended scheduling boundary and keep verification running within one executor poll. The zero-link coverage claim is contradicted by an existing test exercising the refactored synchronous entry point. This was a static verification at the exact requested head; no builds or tests were run, and Rust workspace CI remains pending.
🔴 1 blocking
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate refactor in chain.rs (QuorumProofChain::start, verify_link and finish) and bootstrap.rs changes how cryptographic proof verification maintains and advances trusted signer state across incremental steps. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/src/proof_fetches.rs`:
- [BLOCKING] packages/rs-sdk-trusted-context-provider/src/proof_fetches.rs:1334-1337: Shared fetch callers can resume verification before the executor yields
Native verification runs inside a `Shared` future returned by `start`, and multiple callers can poll its clones within one parent-task poll. In the locked dependencies, Tokio 1.52.3's `yield_now()` returns `Pending` once and then `Ready` on the next poll, without waiting for its deferred wake. Meanwhile, futures-util 0.3.32's `Shared::poll` resets its state to `IDLE` after `Pending`, allowing another clone to immediately poll the inner future. Consequently, `join_all` with 30 callers can process approximately 30 verification slices in one parent-task poll: each subsequent caller completes the preceding yield and starts another slice. That permits hundreds of milliseconds of BLS work without returning control to a current-thread runtime's timers or unrelated tasks. The existing concurrency limits restrict distinct fetches, not callers sharing one fetch. This leaves the executor-stall problem present on the production shared-fetch path; the new native regression test invokes `judge` directly and does not exercise sharing. Gate continuation on a scheduler-driven completion that remains pending under additional polls, and add a regression with multiple shared-fetch callers proving that another task runs before verification finishes.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
github-actions
Bot
added
waiting-self-review
…r turns Tokio's `yield_now` is ready on its second poll whether or not its deferred wake has come. A fetch is `Shared`, and every caller's clone polls it within one turn of the callers' task, so with a few callers a whole proof was verified in one turn again. The native yield now waits for that deferred wake itself and stays pending under further polls. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At ca3670c, the incremental verifier preserves authentication checks and keeps scheduling in the provider, but the shared-fetch yield fix remains incomplete for multithread-runtime entry futures. One blocking scheduling issue and one non-blocking deadline gap remain after independently inspecting the complete diff and locked dependency implementations. Validation was static only; current CI still has Rust workspace runner selection pending, so Rust build/test completion is not established.
🔴 1 blocking | 🟡 1 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate refactor changes cryptographic proof verification control flow in bootstrap::Verification and QuorumProofChain::{verify_link,finish}, while adding shared-future scheduling and cancellation behavior across native and browser executors. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/src/proof_fetches.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/src/proof_fetches.rs:1100-1109: Enforce the verification deadline on paths that do not yield
The only deadline check follows an inter-slice yield. Zero-link and one-link proofs never reach it, nor do longer proofs that fit within one slice. These paths can start verification with an expired deadline or cross it during the final link or finish, then publish the verified key anyway. The surrounding fetch checks its deadline before awaiting a response, not immediately before judging it, and the Proven branch returns without another deadline check. This is a bounded deadline-contract gap rather than an unbounded verification stall. Check expiry before starting verification and before accepting its completed result, independently of whether a yield occurs. Add an already-expired regression with a no-yield slice configuration that asserts OutOfTime and an unchanged cache; the existing regression forces a yield and misses this branch.
- [BLOCKING] packages/rs-sdk-trusted-context-provider/src/proof_fetches.rs:1354-1358: Shared fetch callers can resume verification before the executor yields
(existing thread: https://github.com/dashpay/platform/pull/5720#discussion_r4447444582)
The custom waker fixes the current-thread case covered by the new regressions, but receiving that wake does not always establish an executor turn. In locked Tokio 1.52.3, MultiThread::block_on enters the runtime without installing a scheduler context; runtime::context::defer therefore invokes wake_by_ref synchronously. This includes the normal default #[tokio::main] body. The first poll here consequently sets turn.came before returning Pending, and another Shared clone polled by the same parent immediately resumes verification. Locked futures-util 0.3.32 resets Shared to IDLE after Pending, and join_all with 30 callers polls all 30 within one parent poll, allowing roughly 30 verification slices without returning control. Worker tasks can continue, but sibling futures, cancellation branches, and LocalSet tasks driven on this thread remain blocked. Make verification continuation safe in this entry-future context, for example by giving verification its own cancellable scheduled task and sharing only its result. Add both repeated-poll and shared-fetch regressions under a multithread runtime, leaving the test body on its block_on thread rather than spawning it.
…deadline Outside a scheduler's context, such as a multi-thread runtime's `block_on` future, Tokio does not defer `yield_now`'s wake, so the callers sharing a fetch could still resume verification slice after slice within one turn. The native yield is now a 1 ms timer, which stays pending until the timer driver fires it however often it is polled. The deadline is now checked before every link and before a verified result is accepted, so proofs that never yield are held to it too. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…r needs Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At c994596, the complete diff preserves verification checks and their order while adding cooperative scheduling; both prior findings are fixed. One non-blocking test-coverage gap remains: the deadline regressions start already expired and do not exercise expiration during verification. Validation was static only; current CI still has runner selection and changed-package determination pending, so build and test results are not yet established.
🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate change restructures cryptographic proof verification in QuorumProofChain::{start, verify_link, finish} in packages/rs-core-proof/src/chain.rs and adds shared asynchronous verification, yielding, and deadline handling. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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/src/proof_fetches.rs`:
- [SUGGESTION] packages/rs-sdk-trusted-context-provider/src/proof_fetches.rs:2180-2186: Exercise deadline expiration after verification has started
This test and `should_stop_verifying_at_the_deadline_without_a_yield` both pass `Duration::ZERO`, so the four-link fixture returns at the first per-link deadline check, before verifying any link or yielding. Removing all per-link deadline checks would still let both tests pass because the final check would reject the completed valid proof; removing only the final check would also leave both passing. They therefore do not protect stopping an in-progress proof after expiry or rejecting a result whose final verification step crosses the deadline. Add deterministic coverage that starts with a live deadline, reaches an inter-link yield, expires before resumption, and verifies that subsequent links are not processed. Also exercise expiry during finalization to protect the completed-result guard. A controllable clock or test hook can distinguish these paths without depending on BLS execution speed.
|
Your move: thepastaclaw left review threads unresolved; resolve them. |
`verify` takes its clock as a parameter, `Instant::now` outside tests, so the tests can let the deadline pass at a chosen reading: during the first yield, which must stop the proof before its next link, and at the check of the finished result, which must reject it. 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 |
github-actions
Bot
added
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Reviewed the complete diff at 4a95215 and found no remaining actionable issues. Incremental verification preserves the original proof checks and their order, and all three prior findings are addressed. This was a static review with no builds or tests run locally; current head-matched CI has Rust workspace tests pending and no failed code checks.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The intricate change refactors cryptographic proof acceptance in packages/rs-core-proof/src/chain.rs through start, verify_link and finish, while integrating incremental verification with shared asynchronous fetches and deadlines. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 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.
|
Bots are done — your move: post |
github-actions
Bot
added
the
waiting-self-review
github-actions
Bot
removed
the
waiting-bots
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Only touches experimental opt-in feature; approved; merging
Brings in #5489, #5526 and #5720, which landed on v5.1-dev after this branch started. #5526 adds token-market-contract at 5.0.1; it takes the merged version, 5.0.3, in its package.json and in Cargo.lock, so the lockfile stays current and yarn constraints pass once this merges. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Brings in the rust-dashcore pin c9aeac0d (DGW, the coinbase checks, masternode-sync liveness, the private sync module, the FFI removal), the verified quorum context provider (#5523, #5720) and its SDK opt-in (#5721). Semantic resolutions: - rs-sdk-trusted-context-provider: keeps this branch's `local` module next to v5.1-dev's `proof_fetches` and `proof_transport`. System contracts have one owner, `system_contracts`: `LocalContextResolver::data_contract` calls `system_data_contract`, and the table copied into `local.rs` is gone, so the resolver now also serves the decision-proposals and token-market contracts from protocol 15. - The token-market gate (`is_inactive_system_contract`) holds on every path: in `LocalContextResolver::data_contract` before the known contracts, and in `TrustedHttpContextProvider::get_data_contract` before the resolver and the fallback provider. New tests pin each gate; each was seen red with only its own gate removed. - `platform_activation_height` keeps the dash-context-provider copy; `system_contracts` re-exports it. - rs-sdk: keeps the fixed `context_provider: Arc<dyn ContextProvider>` and the removal of `Sdk::set_context_provider`, and v5.1-dev's `SdkBuilder::build` checks of the provider's network and proof requirement. The two setter tests now test the builder, through both `with_context_provider` and `with_context_provider_arc`; both go red without the build check. The balance checker, `read_contract` example and `quorum_key` test drop the setter calls. - platform-wallet `spv/runtime.rs`: takes v5.1-dev's dash-spv adaptations (`SyncProgress`/`SyncState` from the crate root, infallible `PeerNetworkManager::new`) and keeps this branch's byte-reversed quorum lookup key, `is_ready` (running, headers and masternodes `Synced`), the verified-only lookup, and the rejection of quorum types that name no LLMQ type, now matched as `LlmqtypeUnknown(_)`. - `VerifiedHttpContextProvider`'s docs no longer point at the removed setter. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Issue being fixed or feature implemented
Fixes #5706.
VerifiedHttpContextProviderverified a relay's DIP 175 proof in a single poll of its async fetch. A relay can answer with a valid chain through every ChainLock quorum since the state it was asked from. On testnet today that is about 960 links, roughly 4 s of BLS work natively and about 10 s in wasm. The whole time, the proof stalled one Tokio worker: every task on a current-thread runtime, including the fetch's own deadline timer. In a browser it stalled the event loop. This needs fixing before the provider is exposed to browsers (#5652).What was done?
dash-core-proof: verify one link at a time.bootstrap::startdecodes the envelope and makes every check that comes before the first link.Verification::verify_linkverifies one link.finishverifies any links left, then the target, the coinbase and the records.bootstrap::verifyis nowstart(..)?.finish(), andQuorumProofChain::verifyuses the same steps. Both paths make the same checks in the same order, so they accept and reject exactly the same proofs. The stale-target check still comes before any signature work.Sharedfuture within one turn of its task.tokio::task::yield_nowis ready on its second poll, and outside a scheduler's context, such as a multi-thread runtime'sblock_onfuture, its wake isn't even deferred. With it, a few callers could drive the whole verification in one turn. A timer stays pending until the timer driver fires it, however often it is polled. Slices are 10 ms, so a long proof takes about a tenth longer.MessageChannelmessage: a task of its own, so timers, input and rendering run in between. Hidden tabs don't throttle it the way they throttlesetTimeout. A zero-delay timer is the fallback.How Has This Been Tested?
dash-core-proof:Provider, natively:
should_let_other_tasks_run_while_a_proof_verifies: a current-thread runtime, the real 4-link testnet proof and a slice of zero. Another task runs before verification ends.should_stop_verifying_at_the_fetchs_deadlineandshould_stop_verifying_at_the_deadline_without_a_yield: verification stops and nothing is cached, whether or not it yields.should_let_other_tasks_run_while_callers_share_a_fetch_from_block_on: the 30-caller, one-turn check as a multi-thread runtime'sblock_onfuture.should_let_other_tasks_run_while_many_callers_share_a_verifying_fetch: 30 callers share one verifying fetch. None completes within one turn of their task, and another task runs before verification ends.should_not_end_a_yield_before_the_executor_comes_back: a yield polled twice in one turn stays pending.yield_now, and theblock_onone also fails with a yield that waits for Tokio's deferred wake. The deadline tests fail without the per-link checks. All 121 tests pass.Provider, wasm (
wasm-bindgen-testunder Node):should_let_browser_timers_fire_while_a_proof_verifies: asetTimeout(0)set before judging fires before verification ends. It fails if verification never yields.Timings for the 4-link testnet proof (the debug wasm number includes the yields):
cargo clippy -D warningsis clean natively (--all-features --all-targets) and forwasm32-unknown-unknown.Not in CI yet: the wasm tests still run only locally. feat(sdk): let wasm-sdk and evo-sdk opt into the verified quorum context provider #5652 adds a wasm32 CI job for this crate.
Remaining limit: the chain's header budget (4,096 ancestors) can all sit on one link, which is then verified in one step. That is one BLS check plus up to 4,096 X11 hashes, which I estimate at about 10–20 ms natively and up to roughly 100 ms in wasm. It is bounded and happens at most once per proof.
Breaking Changes
None.
bootstrap::startandbootstrap::Verificationare additions, andbootstrap::verifyis unchanged.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 ·
4a95215/self-reviewedpackages/rs-core-proof/src/bootstrap.rs,packages/rs-core-proof/src/chain.rs,packages/rs-core-proof/tests/synthetic.rsand 4 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