Repository navigation
fix(platform): verify signed protocol versions and use one identity proof - #5361
Merged
Merged
Conversation
…nously ContextProvider gains a defaulted fetch_quorum_public_key, which returns a Send + 'static future resolving to the key, to Ok(None) when the provider's trusted source answered and does not know the quorum, or to an error when the provider could not find out. The default returns None, so every existing provider keeps its behaviour. The Arc/Box and Mutex wrappers forward it. The proof verifier reports a key the provider could not supply as Error::QuorumKeyUnavailable, carrying the quorum type, hash and core chain locked height the proof named. Its Display keeps the provider's message, so callers that classify errors by text are unaffected; the variant stays under the retryable Error::Proof in the SDK. ContextProviderError gains QuorumSourceUnavailable for a trusted quorum source that gave no answer. No behaviour change yet: nothing calls fetch_quorum_public_key. Forwarding test: removing the blanket impl's forward makes should_forward_quorum_key_fetches_through_every_wrapper fail (default None), restoring it makes it pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TrustedHttpContextProvider implements fetch_quorum_public_key. A miss joins a refresh of both quorum lists that is still running, or starts one; the starts of two refreshes are at least 1s apart, which bounds the load quorum hashes that do not exist can put on the quorum service whether or not the SDK bans the nodes that send them. The key is looked up in the fetched lists and put back into its cache. The future answers Ok(None), "absent", only when both lists arrived from a refresh that started after the miss; a refresh that started earlier may predate the quorum, so the miss asks for a new one, or reports the source as unavailable while the gap has not passed. A failed list is never read as absent. Shared refreshes run on a copy of the provider that does not hold the refresh slot, so an abandoned refresh cannot keep the provider alive. A panicking refresh becomes two failed lists instead of panicking every waiter. On wasm32 the browser's non-Send fetches run under spawn_local and only their result crosses back. Quorum list requests now time out after 5s on every target. refresh_quorum_caches always starts a refresh and publishes it, so misses that come in while it runs share it. A provider with synchronous refetching on (the default on native) returns None: its lookup already fetched on the miss. The test server's accepted sockets are now blocking: on macOS they inherited the listener's non-blocking mode and could fail to read a request that had not arrived yet. Test would have caught the missing fetch: before this change should_fetch_a_quorum_key_newer_than_the_cache, should_share_one_refresh_between_concurrent_misses, should_report_a_quorum_absent_only_when_both_lists_lack_it, should_not_report_a_quorum_absent_when_a_list_failed and should_let_misses_join_an_explicit_refresh all failed (the trait default returned None); after it all 30 provider tests pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rum key A proof signed by a quorum the context provider has not cached failed on every node: the error was a retryable proof error, so each node was banned in turn until no address was left, and a typed write that executed was reported as failed. Browser SDKs, whose trusted context fetches its quorum keys once at connect, hit this after every rotation. Every Fetch, FetchMany and state-transition wait now verifies through one helper. When verification fails only because the provider has no key for the quorum the proof names, the provider is asked to fetch it and the response already received is verified again, signature included. Nothing is sent again. What the provider's trusted source said decides the outcome: - key fetched: the second verification's result; - quorum absent from lists fetched after the response arrived: the original retryable error, so the node that named it is banned as before; - no answer: ContextProviderError::QuorumSourceUnavailable, which is not retryable, so no node is banned for the source's failure. sync::retry moves such a request to another node with the flat 2s exclusion DPNS failover uses, and stops when banning is off or no other node is live; - provider cannot fetch: the original error, as before. Fetch and FetchMany require their request's response to be Clone, so the response can be verified a second time. Every response in this crate is. Tests would have caught this: before the change should_verify_a_proof_signed_by_a_quorum_the_cache_has_not_seen, should_still_check_the_signature_after_fetching_the_key, should_report_a_quorum_the_trusted_service_does_not_list_against_the_node, should_not_hold_an_unreachable_quorum_service_against_the_node and should_report_the_original_error_when_the_provider_cannot_fetch failed (no fetch was made), and should_fail_over_when_the_quorum_source_gave_no_answer failed with the old retry predicate; after it they pass. They replay the recorded epoch proof through a network SDK whose trusted provider starts with an empty cache. The broadcast wait path goes through the same helper; no recorded wait response with a proof exists to replay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…roid dash_sdk_create_trusted built its TrustedHttpContextProvider with synchronous refetching on, so a proof signed by a quorum newer than the cache blocked inside proof verification on two sequential HTTP fetches. A node could trigger that with every response, concurrent misses each fetched on their own, and a quorum service outage got every node banned. The provider is now built by one build_trusted_provider for all three quorum URL sources, with synchronous refetching off. The SDK fetches a missing key asynchronously and verifies the response it already holds again, as in the browser. The start-up prefetch goes through the provider's shared refresh, so a query issued before it finishes waits for it instead of fetching again. A quorum source that gave no answer maps to NetworkError instead of whatever the message text matched. Tests would have caught this: before the change should_build_a_provider_that_fetches_missing_quorum_keys_asynchronously failed (the provider returned None) and quorum_source_unavailable_maps_to_network_error failed (InternalError); after it both pass. The mainnet default URL branch is not exercised in the unit test because constructing it resolves DNS; it shares the same final with_refetch_if_not_found(false). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fetch WasmTrustedContext fetches its quorum keys once, when the app connects, and cannot refetch inside the synchronous lookup because wasm cannot block. It now forwards fetch_quorum_public_key to its provider, so the SDK fetches a quorum newer than the prefetch and verifies the response again instead of failing every read and banning every node until the page reloads. A quorum source that gave no answer is reported to JS as retriable: no node is banned for it, but it is transient. The test endpoint's accepted sockets are now blocking: on macOS they inherited the listener's non-blocking mode and could fail to read a request that had not arrived yet. Tests would have caught this: before the change should_fetch_a_quorum_newer_than_the_prefetch_through_the_sdk failed (the context returned None) and should_report_an_unavailable_quorum_source_as_retriable failed (not retriable); after it both pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…urce unavailable When a miss joined a refresh that started before the response, and a new refresh could not start yet, the provider answered QuorumSourceUnavailable. That error bans no node, so a node naming a quorum that does not exist escaped its ban whenever someone else had refreshed within the last second, and with banning off every such response failed its request outright. During an outage the error also replaced the real cause with "refreshed less than 1s ago". The miss now waits until a new refresh may start and takes that refresh's answer. QuorumSourceUnavailable only ever means the quorum service failed; a quorum missing from lists fetched after the response is always "absent", which bans the node as before. Refresh starts stay at least 1s apart. Whether a refresh started after the response is now decided by a refresh generation the miss notes when the SDK asks, not by comparing timestamps, which a coarse browser clock could make equal. A finished refresh, failed or not, is reused only within the 1s gap, also by misses that need a newer one, and a running refresh is joined for its requests' timeout plus a margin. A panicking refresh logs its payload; a failed refresh logs its reason once, with the whole error source chain; a key fetched for a quorum newer than the cache is logged at info. Tests: should_look_again_with_a_newer_refresh_when_an_older_one_lacks_the_quorum replaces the test that pinned the old QuorumSourceUnavailable answer and now gets the key; should_judge_a_quorum_absent_from_a_refresh_newer_than_the_response, should_not_refresh_again_within_the_gap_after_a_failed_refresh, should_not_let_an_abandoned_refresh_hold_the_provider and should_join_a_running_refresh_past_the_gap_but_not_a_finished_one are new. The last two fail when detached() is replaced by clone() and when the finished flag is set before the fetch, respectively. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If the provider reported a fetched key but its lookup still missed it on the second verification, the original retryable error came back and the node was banned for the provider's broken contract. That case is now reported as QuorumSourceUnavailable, like any other answer the provider could not give, and logged with the quorum and the reason. Every provider error that is normalized this way keeps its variant in the reason. The failover path in sync::retry and the comments that describe it no longer speak only of DPNS rejections; its stop message logs the error. Tests: should_not_hold_a_provider_that_forgets_a_fetched_key_against_the_node fails without the new mapping (the node would be banned) and passes with it. Also new: a malformed key from the quorum service bans no node; the fetch is asked for exactly the quorum type, hash and core height the proof names; the failover exclusion lasts at most 2s; and the verifier test uses a distinctive core height. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The size was built with NonZeroUsize::new(100).unwrap() at run time; a constant evaluates it at compile time. No behaviour change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In the browser the quorum lists are fetched with fetch and the wait between
two refreshes is a browser timer; neither future is Send, so both run on the
local executor and only their results cross to the SDK's future. The new
wasm32 test runs a failed refresh, then a miss that has to wait out the gap
and refresh again, and checks it reports the unreachable quorum service
after about a second.
Run by hand under Node (CI does not run wasm32 tests):
CC_wasm32_unknown_unknown=<llvm>/clang AR_wasm32_unknown_unknown=<llvm>/llvm-ar \
CARGO_TARGET_WASM32_UNKNOWN_UNKNOWN_RUNNER=wasm-bindgen-test-runner \
cargo test -p wasm-sdk --target wasm32-unknown-unknown --lib
16 passed, including this test (1.03s).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hen it arrived
The staleness check compares a response's height with the highest height
the SDK has accepted. While one response waited for the key of the quorum
that signed it, other requests could raise that mark by more than the
tolerance, so a response that was fresh when it arrived failed as
StaleNode, which is retryable: an honest node was banned for the client's
own wait.
The SDK's proof helper now accepts the verified metadata itself, through
Sdk::accept_verified_metadata, which keeps the rule that the protocol
version ratchets only from verified metadata. A response verified after a
fetch is judged against the height mark read before its first verification;
one verified without a fetch is judged as before. The Fetch, FetchMany and
state-transition wait paths all go through it.
Test would have caught this: before the change
should_judge_a_response_that_waited_for_a_key_as_fresh_as_when_it_arrived
failed with StaleNode { expected_height: 23, received_height: 13 }; after it
the test passes. should_still_reject_a_response_that_was_stale_when_it_arrived
passes before and after: the wait never makes a stale response acceptable.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Validate keys with the same BLS decoder used by proof verification and keep malformed cached keys attributable to the trusted source. Replace synthetic repeated-byte test keys with deterministic valid BLS keys. Test would have caught this in CI: ✖ before fix, ✔ after. should_reject_an_invalid_bls_key_from_the_trusted_source: 0 passed, 1 failed → 1 passed. should_not_hold_a_malformed_key_from_the_quorum_service_against_the_node::invalid_bls_point: retryable InvalidPublicKey → non-retryable source failure. All three malformed-key cases pass, including repeated cached reads; invalid-signature coverage remains green. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Let malformed cached keys enter the existing rate-limited refresh path so a repaired trusted service restores reads without rebuilding the provider. Test would have caught this in CI: ✖ before fix, ✔ after. should_recover_a_malformed_cached_key_after_the_source_is_repaired failed on the retained invalid-hex entry, then passed after the fix. It also checks that the refresh generation stays unchanged within MIN_GAP. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Normalize only QuorumKeyUnavailable wrapping QuorumSourceUnavailable at the shared SDK error conversion. Keep unavailable async hooks with InvalidQuorum retryable and explicitly retain node attribution for authoritative async absence. Test would have caught this in CI: ✖ before fix, ✔ after. should_not_ban_a_node_when_a_synchronous_quorum_source_is_unavailable failed with Proof(QuorumKeyUnavailable) on the default async hook, then passed with non-retryable ContextProviderError. The synchronous missing-key and authoritative-absence-after-source-failure cases remain retryable; all 14 quorum-key tests pass. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Use the same monotonic clock as production and begin measuring before the initial refresh. Time spent in its HTTP requests now counts toward MIN_GAP, avoiding false failures after slow fetches or adjustable wall-clock changes. wasm32 lib suite: 16 passed, 0 failed, including should_fetch_through_the_browser_and_wait_out_the_gap. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Replay the existing signed identity-balance vector through the real withdrawal StateTransitionProofOutcome verifier and broadcast wait call site. Remove the transport expectation during key acquisition so a resend fails, advance the seen height while fetching, and preserve the affected-state guarantee and strict execution rejection. should_verify_the_buffered_broadcast_wait_after_fetching_its_quorum_key: 3 passed, 0 failed (key acquisition, trusted-source failure, already-stale response). The recorded GroveDB bytes, signature, metadata and key are unchanged; no execution evidence is fabricated from the snapshot. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Render provider failures with Display before surfacing them as trusted-source outages.
Test would have caught this in CI: ✖ before fix, ✔ after. should_display_provider_fetch_failures_without_debug_variant_formatting failed with Generic("source offline") in the reason, then passed with Context provider error: source offline.
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Replace the remaining repeated-byte quorum key in the rotated proof-preparation fixture with a deterministic key from a fixed secret. Full native wasm lib suite: 144 passed, 1 failed with the invalid fixture → 145 passed, 0 failed after correction. should_refresh_rotated_quorums_through_proof_preparation now exercises a key accepted by the trusted parser. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Pass the Copy epoch request directly in repeated malformed-key verification and format the deterministic wasm fixture import. cargo clippy -p dash-sdk --all-targets --all-features -- -D warnings passes. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
A decoded BLS identity point is not a usable public key. Reject it at the trusted source boundary so signature verification cannot misattribute the unusable key to an honest node. Test would have caught this in CI: ✖ before fix, ✔ after. should_not_hold_a_malformed_key_from_the_quorum_service_against_the_node::identity_bls_point failed with retryable InvalidSignature for c0 followed by 47 zero bytes, then passed with a source-unavailable error on both initial and cached reads. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Remove the quorum from the other list cache after accepting its repaired key, so the synchronous verification lookup sees the same usable key returned by the fetch. Test would have caught this in CI: ✖ before fix, ✔ after. should_recover_when_the_previous_list_repairs_a_malformed_current_key fetched the repaired key but failed the following lookup on the malformed current entry before the fix, then passed after it. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Normalize InvalidPublicKey during the second verification to QuorumSourceUnavailable, since those key bytes came from the provider. Keep invalid signatures under usable keys retryable and node-attributed. Test would have caught this in CI: ✖ before fix, ✔ after. should_not_blame_a_node_for_an_invalid_key_on_the_second_verification failed with retryable Proof(InvalidPublicKey), then passed with a source error. The existing forged-signature test now explicitly checks retryability; all 20 quorum-key tests pass. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Enable banning and register the actual mock responder. Key acquisition uses two live addresses and a retry, with its buffered transport expectation removed. Source failure is checked directly with no retries; a forged signature must receive a health ban. A source-failover case records only the short exclusion before mock success clears its placeholder address. Test would have caught this in CI: ✖ before fix, ✔ after. should_verify_the_buffered_broadcast_wait_after_fetching_its_quorum_key::key_acquired failed with QuorumKeyUnavailable when broadcast wait temporarily used plain FromProof, then passed after restoring recovery. All five wait cases pass, including forged_signature_bans and source_failover_is_brief. Fixture identity and balance now come from the recorded manifest, and test failures use descriptive expect messages. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Keep the quorum type, hash and Core height when normalizing synchronous source outages, explain the attribution boundary, and log unusable cached keys. Align test diagnostics and imports with repository conventions, and ensure the wasm timer check cannot be satisfied solely by a slow initial refresh. Test would have caught this in CI: ✖ before fix, ✔ after. should_keep_quorum_context_when_normalizing_a_source_failure failed with missing quorum type: offline, then passed with all three quorum fields retained. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Keep usable entries in the other bounded quorum cache and remove only malformed shadows. A test-only barrier forces both opposite-list repairs to insert before either cleans up, exercising the production replacement path without sleeps or a public hook. Test would have caught this in CI: ✖ before fix, ✔ after. should_keep_a_fetched_key_during_opposite_list_cache_repairs failed after both fetches succeeded because neither cache retained the key (0 passed, 1 failed), then passed with synchronous lookup returning the valid key (1 passed, 0 failed). Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Map contextual SDKCallFailed source-unavailable errors to NetworkError while retaining their contextual Display message. Keep every other wrapped SDK failure classified as InternalError and retain consensus-code propagation. Test would have caught this in CI: ✖ before fix, ✔ after. should_preserve_wrapped_quorum_source_failures_as_network_errors failed with InternalError instead of NetworkError (0 passed, 1 failed); after the fix all 17 FFI error tests pass, including direct classification, contextual message preservation, and unchanged classification for other wrapped failures. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Scan matching current and previous snapshot entries until a trusted usable key is found. Preserve the first parse failure if every match is unusable and preserve authoritative absence when no match exists. Test would have caught this in CI: ✖ before fix, ✔ after. should_use_a_valid_previous_key_when_the_current_list_overlaps_with_a_malformed_key failed with QuorumSourceUnavailable for malformed current hex despite a valid previous key (0 passed, 1 failed), then passed with both fetch and synchronous lookup returning the valid previous key (1 passed, 0 failed). Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Capture the local clock before first proof verification alongside the arrival height. Use both anchors when accepting the same response after key acquisition, while ordinary metadata checks retain the current clock. Signed responses already stale on arrival remain node-attributed StaleNode errors. Test would have caught this in CI: ✖ before fix, ✔ after. should_judge_signed_time_when_the_response_arrives_before_key_acquisition::fresh_on_arrival failed with StaleNode(Time) after the trusted provider completed beyond the signed timestamp deadline; stale_on_arrival remained rejected (1 passed, 1 failed). After the fix both genuine recorded-proof cases pass with time checking enabled (2 passed, 0 failed), and the already-stale case remains retryable. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Use one optional MetadataArrival carrying both local height and clock anchors on either proof-verification path. A synchronous trusted-provider refetch can delay the first verification just as asynchronous acquisition delays the second; signed metadata already stale at arrival still fails specifically as StaleNodeError::Time. Test would have caught this in CI: ✖ before fix, ✔ after. should_judge_signed_time_when_the_response_arrives_before_key_acquisition::synchronous_fresh_on_arrival failed with StaleNode(Time) after blocking key lookup crossed the deadline (3 passed, 1 failed across async/sync cases). After applying the anchor to first acceptance, all four real signed-proof cases pass (4 passed, 0 failed), including stale-on-arrival controls with time checking enabled. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Share one typed source-unavailable predicate across direct and contextual SDK errors, traversing NoAvailableAddressesToRetry envelopes. Preserve contextual messages and all other wrapped classifications; tests cover nested retry envelopes and generic retry-wrapped controls. Test would have caught this in CI: ✖ before fix, ✔ after. should_preserve_retry_wrapped_quorum_source_failures_as_network_errors failed with InternalError instead of NetworkError on the contextual retry-exhaustion path (0 passed, 1 failed). After the fix the FFI error module passes all 18 tests, including direct and contextual nested retry wrappers with message preservation. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Use test-only bounded insertion and release channels so an early repair-worker exit fails within the deadline instead of hanging at a barrier. Keep both destination insertions before cleanup, move test-only imports to the module top, and document the cache and fetched-list guarantees. No production behavior delta: red-to-green is not applicable to this test coordination and documentation follow-up. should_keep_a_fetched_key_during_opposite_list_cache_repairs passes with the same controlled interleaving (1 passed, 0 failed); the original behavior failure is recorded in the cache fix commit. Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Contributor
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
thepastaclaw
approved these changes
Oct 11, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Independently reviewed the complete diff at 820b067 and found no actionable in-scope defects. Verifier selection preserves the stored-version floor, metadata learning remains gated by quorum-signature verification, and the prior reserved mock-version panic is fixed. This was a static review; the supplied CI snapshot still had Rust workspace tests, browser shard 1, the platform test suite, and PR Hygiene pending.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); 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: platform-versioning); 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:
normalbygpt-6.1-sol(effort low) — The diff changes security-relevant proof-version selection in sdk.rs and verify_fetching_quorum_key, but the production logic is a contained selector and its propagation rather than a large or intricate cryptographic change; most added lines are tests. - 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 below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - 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 high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); 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.
Contributor
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
3 of 7 tasks
Contributor
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-11T19:40:48.644Z |
shumkov
changed the title
Contributor
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
Merge current v5.0-dev into the signed-version and single-proof integration branch. Keep one copy of the four identical drive-abci dev-dependencies introduced independently by both sides. Validation: cargo metadata --locked --format-version 1 exited 101 with duplicate key before the correction and exits 0 after. All 49 workspace members resolve; dependency features and the merged Cargo.lock remain unchanged. Existing Cargo manifest validation catches this regression; no additional behavior test is needed for identical declaration deduplication.
Contributor
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
thepastaclaw
approved these changes
Oct 11, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Static assessment of the complete diff at a6c9459 found no actionable in-scope defects: signed-version selection preserves the verification floor, and the merged identity proof binds first-after selection and complete identity contents to the signed root. The prior pre-authentication mock-version panic is fixed; the intentional wire/API incompatibility is documented. No builds or tests were run locally; the supplied CI snapshot shows successful JS tests and build checks, with Rust workspace tests still in progress as of 2026-10-11T21:17:27Z.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalby fallback after triage failure (exit 1 timed_out=False: API Error: 400 unknown provider for model gpt-6.1-sol) - 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 below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - 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 high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort high); 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.
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.
- Existing address FFI converters omit ItemWithSumItem balances — The extract_balance_nonce_from_element helpers in packages/rs-sdk-ffi/src/address/queries/branch_state.rs and trunk_state.rs handle Item and SumItem but return None for ItemWithSumItem. Drive stores address funds as ItemWithSumItem with a four-byte nonce and a separate balance, so both conversion loops silently omit these verified rows from the C arrays consumed by Swift. The helpers have identical behavior at the review base, and this PR changes neither their decoding nor the storage format; this is a concrete separate SDK correctness issue, not an in-scope blocker.
- Follow-up: Track a separate issue or maintainer-requested PR to decode ItemWithSumItem in both converters and verify that balances and nonces survive conversion into the foreign result arrays.
Contributor
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
This was referenced Oct 12, 2026
8 of 26 tasks
QuantumExplorer
added a commit
that referenced
this pull request
Oct 13, 2026
Retargets DPNS v3 to protocol version 15. The merge takes the base side of: - the SDK proof version fallback (sdk.rs, quorum_key.rs, fetch.rs, fetch_many.rs): #5361's verifier_version supersedes it; - the dpp parser and meta-schema: #5371 already reads the "$ownerId" param, and DPNS v3 now declares the where entry consume asks for, so the parser needs no relaxation; - the v14 note and the book's refersTo pages, rewritten for PV15 next. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Basic explanation
What this does: for proved Platform responses, the SDK verifies the quorum signature and uses the response metadata to select its proof-verification rules. The signed data includes the protocol version the network ran when it signed. Until now, the SDK checked each proof using the protocol version it had stored locally, and only learned the network's newer version afterwards. Mainnet and testnet SDKs start at version 13. So after PV14 activates, a freshly started SDK would check PV14 proofs with the older PV13 GroveDB rules, which lack two of PV14's proof bindings. With this change, the SDK selects a newer known signed version, or its newest known verification table if the reported version is unknown, while retaining its stored-version floor. The non-unique identity query additionally requires a supported producer version to reconstruct the exact query.
Value: fresh and pinned SDKs use the newer signed verification rules from the first response, including throughout address/nullifier sync. Non-unique public-key-hash identity lookups use one standard proof covering the first matching identity and its complete contents, with the original hash, exclusive cursor and signed root verified together.
Risks: no consensus, stored-state or fee-schedule changes. The non-unique identity endpoint now has a deliberately incompatible wire and low-level Rust/WASM API format: old clients and nodes, including stable v4.1.1, cannot interoperate on proved lookups during a mixed rollout and may temporarily ban incompatible nodes. Coordinate node/client upgrades; there is no old-format fallback. Rust custom response and trunk-query callers also need source changes (see Breaking Changes).
Single-proof identity lookup: this PR also includes #5374. It removes the nested double-proof response completely, including absence responses, and uses the standard proof/metadata/signature path.
Issue being fixed or feature implemented
Found while reviewing protocol-version handling after the Core 24 × Platform 5.0.0-beta.2 devnet rehearsal.
drive_versions/v8.rs:24,v9.rs:24). V4 turns onterminal_non_merk_tree_child_hashandchunk_proof_row_binding(grovedb9791d27,grovedb-version/src/version/v4.rs:565-566).sdk.rs,parse_proof_with_metadata_and_proof) and ratcheted the version only after verification.#5313 is already merged into
v5.0-dev; its sharedverify_fetching_quorum_keyhelper is the foundation for this change. #5374 has been merged into this PR asbe70a485671e250ddac20b1427e61ec4e3b61429, including the reviewed single-proof follow-up fixes. This PR now delivers both changes together tov5.0-dev.What was done?
metadata.protocol_versionis the signedStateId.app_version(rs-drive-proof-verifier/src/verify.rs). A wrong claim fails the quorum signature check.Sdk::verify_fetching_quorum_keycovers Fetch, FetchMany and the state-transition wait. One table is used for both verifications of a response that waited for a quorum key.ProofResponseMetadataexposes response metadata to the selector. The non-unique-key response now participates inVersionedGrpcResponse; all threeIdentityResponsearms delegate to the ordinary metadata accessor. The special nested-response extraction is removed.Proofat field 4, reserve retired field 2, remove double-proof types and regenerate clients. Separate producer query construction from the verification security floor in native and standalone WASM verification. Unknown/future/reserved query versions fail explicitly.rs-sdk-ffi). Branch responses carry no signed metadata, so this path keepsPlatformVersion::latest().ItemWithSumItem), and V4 doesn't change how item rows are encoded. Honest proofs from either version verify, and the newest rules add the row bindings.In-place changes to shipped generations
The modified
prove_full_identity_by_non_unique_public_key_hashproverv0,verify_full_identity_by_non_unique_public_key_hashverifierv0, andidentity_by_non_unique_public_key_hashABCI handlerv0are each selected by feature version 0 in PV1–PV14 and serve read-only queries. Their production callers are the query handler and proof-verifying clients; block execution never calls them. They do not modify persisted indexes/state, state-transition validation, canonical identity serialization, version tables, or fee schedules, so replay and consensus roots are unchanged at every selecting version. Query/wire compatibility changes intentionally apply across these generations. Tests exercise actual historical PV12/PV13 and latest proof construction and verification.How Has This Been Tested?
The signed-version test evidence below predates the #5374 integration. The integrated head
be70a485had the same Git tree as reviewed #5374 head817d8543. Its first combined CI run failed before tests because merging currentv5.0-devduplicated four identicaldrive-abcidev-dependencies. Signed merge commita6c94597dcremoves those four duplicate declarations; dependency features and the merged lockfile are unchanged. The samecargo metadata --locked --format-version 1command failed with duplicate-key exit 101 before the correction and passed afterwards, resolving all 49 workspace members. Opus independently verified the exact commit and metadata resolution. Fresh combined CI is Tests 38168732372, currently queued; no full-CI success is claimed yet.Single-proof regressions from fix(platform): replace non-unique identity double proofs #5374: real tonic wire-format RED→GREEN at PV12/PV13/latest; Drive 24 plus cursor 1, ABCI 8, proof-verifier 6 and SDK 2 targeted tests passed. Coverage includes first/next/exhausted lookup, signed root/version binding, incomplete and dangling-identity proofs, concurrent commits and backend-error preservation.
Direct WASM/API coverage: 4 WASM unit tests, 2 real Node binding cases and 1 mocked wrapper test passed locally. Invalid fifth-version and swapped-version mutations demonstrated meaningful failures. These WASM/wrapper tests are not executed by existing CI. Full-feature wasm32 all-target compilation, targeted Clippy and formatting checks passed; the initial identity-only all-target check failed because unrelated tests require governance features.
Remaining WASM packaging limitation: unrelated facade imports/build-generation problems predate this change; use the directly built binding as documented. The mocked wrapper test does not establish full package-facade importability. No performance or new live-devnet verification is claimed.
Red → green on the committed tests. With the selector temporarily returning the stored table, 6 of the 9 new selection tests fail:
The fix makes them pass.
Unauthenticated warning. A real signed epoch response with its version altered upward used to log the warning before verification failed. It now doesn't.
Address-sync wiring. A real recorded PV12 trunk, read through an SDK pinned at 11, gives branch work version 12. The test fails if address sync drops the trunk version.
Invariants. These passed before the change too and are kept:
protocol_versiontampered upward failsInvalidSignature, and the SDK learns nothing.Suites.
dash-sdk --all-features: 451 passed. That is 302 lib, 3 DPNS unit, 136 recorded-vector tests intests/main.rs, and 10 doc tests; 31 were already ignored.drive-proof-verifier: 349 passed, or 368 withmocks.rs-sdk-ffi --lib: 357 passed, plus 1 live-mainnet test that stays ignored.cargo checkofwasm-sdk; clippy-D warnings;fmt --check.Not covered.
latest()claim for the FFI path rests on the GroveDB and Drive sources cited in the code comment, not on a recorded replay.Breaking Changes
Source-breaking for Rust users:
Fetch::RequestandFetchMany::Requestresponses must now implementProofResponseMetadata, in addition toClone. Every versioned protobuf response gets it through a blanket impl. Custom response types must implement the metadata accessor.TrunkQueryResponsegains the required fieldprotocol_version: the verified trunk metadata version. Struct literals and customTrunkBranchSyncOpsimplementations must fill it in.Behaviour change: a pinned SDK now verifies proofs with a newer signed table when it knows one. The pin still governs construction.
Proofis field 4. Request V0, identity field 1 and metadata field 3 remain unchanged. There is no compatibility fallback for the retired format.(proof, publicKeyHash, startAfter, producerProtocolVersion, verificationProtocolVersion); direct JS callers must update and authenticate the returned root.No consensus behavior is changed. The title omits
!under the repository convention reserving it for consensus-breaking changes; the wire and source/API breaks are documented here.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 ·
a6c9459/self-reviewedWhen 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
Bug Fixes
Compatibility