Repository navigation
fix(sdk): verify no-limit proposer, distribution and group queries against the page Drive proves - #5485
Conversation
…ainst the page Drive proves Drive answers a proved request that names no limit with its default page (default_query_limit, 100) and proves that page. The verifiers of four paged queries passed the missing limit to Drive's verify functions, so a proof of a full page was checked against an unlimited range and failed with "Proof is missing data for query range": - ProposerBlockCounts (GetEvonodesProposedEpochBlocksByRangeRequest) - TokenPreProgrammedDistributions (GetTokenPreProgrammedDistributionsRequest) - Groups (GetGroupInfosRequest) - GroupActions (GetGroupActionsRequest) A new limit_drive_proves helper defaults the limit to DEFAULT_QUERY_LIMIT in those FromProof impls; requests are unchanged. Mainnet's current epoch already has more than 200 proposers, so the no-limit proposer query fails there today. Each query gets a drive-abci test that writes 101 results, proves the no-limit request through the real handler and runs the real FromProof with a provider that refuses quorum keys: QuorumKeyUnavailable means the Drive part verified, GroveDBError without the default. The rs-sdk-ffi proposer query comment that said a no-limit request fails verification is updated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
|
Waiting for bot review — coderabbitai ✓ · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
|
⏹️ Not reviewed — PR is closed (merged) (commit a9c5fe3) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The changes align all four verifiers with standard 100-result server pages, but introduce a compatibility regression for previously valid complete responses from nodes configured with larger defaults. Static verification against the exact head and pinned GroveDB source confirms one blocker; focused helper-level test coverage is also missing. Current CI reports successful Rust workspace tests and SDK builds, with only PR Hygiene pending; no builds or tests were run locally.
🔴 1 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Add direct unit test coverage for limit_drive_proves in rs-drive-proof-verifier
packages/rs-drive-proof-verifier/src/verify.rs:739-740
The four new handler-to-FromProof regressions exercise omitted limits, but do not exercise this helper's explicit-limit passthrough behavior. Add a focused test in the existing verify.rs unit-test module covering None and representative explicit limits, including a value above DEFAULT_QUERY_LIMIT. This gives the verifier crate's standalone tests coverage of both branches and protects explicitly requested page sizes from being normalized accidentally.
source: gemini-3.8-flash-high (phase1-reviewer: rust-quality)
Review provenance
Source: reviewer 1: gemini-3.8-flash-high (agent: phase1-reviewer, role: general); reviewer 2: gemini-3.8-flash-high (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); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The behavioral change is a small, shared default-limit substitution in four client-side proof verifiers, with most of the diff devoted to regression tests and no consensus or FFI behavior changes. - Phase 1 reviewers:
gemini-3.8-flash-high— general (completed, effort high); agentphase1-reviewer,gemini-3.8-flash-high— rust-quality (completed, effort high); agentphase1-reviewer - Phase 1 model:
gemini-3.8-flash-high— antigravity quota: weekly 71% left, 5h 100% left - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort medium); 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-drive-proof-verifier/src/verify.rs`:
- [BLOCKING] packages/rs-drive-proof-verifier/src/verify.rs:17-22: Preserve complete responses from nodes with a larger default
Unconditionally converting an omitted limit to 100 rejects responses that verified successfully before this change. For example, configure `DriveConfig.default_query_limit = 200` and store 150 epoch proposers. The handler accepts the omitted limit and generates a proof containing all 150 entries. Previously, `ProposerQueryType::ByRange(None, ...)` verified that complete range; this helper now supplies `Some(100)`, and the pinned GroveDB Merk verifier rejects the additional matching entries with `Proof returns more data than limit` (`merk/src/proofs/query/verify.rs`, lines 471–477). The handlers read the configurable default directly, so this is a supported configuration, not an invalid request. The PR's acknowledged limitation therefore does not merely preserve existing failures: complete responses containing more than 100 entries newly fail. Preserve unlimited verification for complete omitted-limit responses while also accepting the standard default page—for example, attempt strict unlimited verification first and use strict default-page verification as a fallback only for omitted limits. Add a regression with a configured default of 200 and 150 results.
- [SUGGESTION] packages/rs-drive-proof-verifier/src/verify.rs:739-740: Add direct unit test coverage for `limit_drive_proves` in `rs-drive-proof-verifier`
The four new handler-to-FromProof regressions exercise omitted limits, but do not exercise this helper's explicit-limit passthrough behavior. Add a focused test in the existing `verify.rs` unit-test module covering `None` and representative explicit limits, including a value above `DEFAULT_QUERY_LIMIT`. This gives the verifier crate's standalone tests coverage of both branches and protects explicitly requested page sizes from being normalized accidentally.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix; thepastaclaw left review threads unresolved; resolve them. |
…rger default Drive's default page is node configuration (DriveConfig.default_query_limit), not protocol. Verifying a missing limit only as DEFAULT_QUERY_LIMIT broke a node configured with a larger default: its complete proof of, say, 150 results verified as no limit before, but checked against 100 it reads as "Proof returns more data than limit". verify_page_drive_proves replaces limit_drive_proves: a named limit is verified as is; a missing limit is verified as the standard default page and, if that is refused, as no limit, returning the default page's error when both are refused. Either reading only returns proved results. The four drive-abci tests now run a node left at 100 with 101 results and a node configured with 200 with 150 results; helper unit tests cover a named limit (above the default too), the default page first, the fallback and the error. setup_platform builds through a shared setup_platform_with_config, and setup_platform_with_default_query_limit configures the node. The rs-sdk-ffi comment edit is reverted: that crate needs its own code owners' review, so the stale comment is left for a separate change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
…ge_drive_proves #5485 added `verify::verify_page_drive_proves` for the proposer, distribution and group queries, with the same policy this PR gave the vote queries: a request naming a limit is verified against it, and one naming none as the standard default page and then as the whole range. The five vote verifiers now use it instead of their own copy. Each one validates the proof envelope and resolves the contract once, outside the retry, and attaches the proof to a Drive error only for the final result, so a failed first attempt no longer clones the proof buffer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Basic explanation
What this does: Some Platform queries return their answers in pages. When an app asks one of them without saying how many answers it wants, Platform sends back at most 100 answers together with a cryptographic proof of those 100. For four of these queries the SDK then checked that proof as if it should cover every answer, not just the first 100, so whenever there were more than 100 it rejected a correct answer as an invalid proof. This PR makes the SDK check the proof against the page Platform actually proved: first as the standard 100-answer page, and if that does not fit, as every answer, which is what a node whose operator configured a larger page sends. It is the same bug #5483 fixes for the five vote queries.
Value: These four queries now work with no limit named once more than 100 results exist:
ProposerBlockCounts): mainnet's current epoch already has more than 200 proposers, so this fails there today;It is fixed in the Rust SDK's proof verifier, which the C FFI and the wasm SDK build on.
Risks: Low. Only client-side proof checking changes; nothing a node runs or stores changes, and requests are sent exactly as before. Every answer that verified before still verifies: a no-limit request is checked first as the standard page of 100 (
DEFAULT_QUERY_LIMIT, what every node set up with dashmate uses) and, if that is refused, as the complete answer it was checked as before. Each reading only accepts results the proof proves. The cost is a second proof check, only for no-limit requests whose proof is not a standard page. A node with a non-standard default that sends a full page of its own size with more answers left still fails, as it does today. The new drive-abci tests together run in under a second.Issue being fixed or feature implemented
Follow-up to #5483, which listed these four verifiers as having the same code pattern and left them for later.
Each of these Drive handlers replaces a missing limit with
config.default_query_limit(100) and proves the query withlimit: Some(100):packages/rs-drive-abci/src/query/)packages/rs-drive-proof-verifier/src/)GetEvonodesProposedEpochBlocksByRangeRequestvalidator_queries/proposed_block_counts_by_range/v0proof.rs,ProposerBlockCountsGetTokenPreProgrammedDistributionsRequesttoken_queries/token_pre_programmed_distributions/v0proof/token_pre_programmed_distributions.rsGetGroupInfosRequestgroup_queries/group_infos/v0proof/groups.rs,GroupsGetGroupActionsRequestgroup_queries/group_actions/v0proof/groups.rs,GroupActionsThe matching
FromProofimpls passed the request's missing limit (None) to the Drive verify function, so GroveDB verified the proof of a 100-item page against an unlimited range and failed with "Proof is missing data for query range". Results of 100 or fewer were unaffected, which is why the existing tests and SDK test vectors never hit it.The default page is node configuration (
DriveConfig.default_query_limit), not protocol. Verifying a missing limit only as 100 would turn a node with a larger default, say 200, into a new failure: its complete proof of 150 results verified as no limit before, but checked against 100 it reads as "Proof returns more data than limit". So the verifier accepts either reading.All four can pass 100 results, so all four are fixed:
startAfterthe last one) another full 100. Testnet has 73 evonodes, so it cannot reach a full page there yet.validate_groupsonly checks the positions are contiguous); a group of two members is under 100 bytes, so more than 100 fit in one contract.I found no open PR or issue for these (searched "missing data for query range", "default query limit", "ProposerBlockCounts", "pre-programmed distributions limit", "group infos limit", "group actions limit", "default_query_limit", "proof limit"; only #5483 and #5482 come up).
What was done?
packages/rs-drive-proof-verifier/src/verify.rs: a crate helper,verify_page_drive_proves(limit, verify), runs a Drive verify function against the page Drive proved. A request's limit is verified as is. A missing limit is verified asDEFAULT_QUERY_LIMITand, if that is refused, as no limit; when both are refused the error is the standard page's. Either reading only returns proved results: a proof verifying as no limit proves the whole range, and a proof verifying as the default page proves its first page.The four verifiers verify through it:
ProposerBlockCounts(proof.rs),TokenPreProgrammedDistributions(proof/token_pre_programmed_distributions.rs),GroupsandGroupActions(proof/groups.rs). The distributions verifier keeps its start as copyable parts so the start can be rebuilt for each reading.Requests the SDK sends are unchanged, so the recorded SDK test vectors still match.
Unit tests of the helper in
verify.rs(verify::tests::verify_page_drive_proves): a named limit, including one above the default, is verified once as is; a missing limit is tried as the default page first, falls back to no limit, and returns the default page's error when both are refused.New drive-abci tests (test-only change in drive-abci), one per query, each next to its handler's tests:
proposed_block_counts_by_range::v0::tests::should_verify_the_default_page_of_proposers_requested_with_no_limittoken_pre_programmed_distributions::v0::tests::should_verify_the_default_page_of_distributions_requested_with_no_limitgroup_infos::v0::tests::should_verify_the_default_page_of_groups_requested_with_no_countgroup_actions::v0::tests::should_verify_the_default_page_of_group_actions_requested_with_no_countEach runs two nodes (
query::tests::NO_LIMIT_PROOF_CASES): one left at the default page of 100 storing 101 results, and one configured with a default of 200 storing 150. It proves the request with no limit through the real handler, checks the proof holds the node's page, then runs the realFromProofwith a context provider that refuses quorum keys (NoQuorumKeysContextProvider).FromProofchecks the GroveDB proof before the Tenderdash signature, soQuorumKeyUnavailablemeans the Drive part verified. The sharedquery::testshelpers gainsetup_platform_with_default_query_limit;setup_platformnow builds through a commonsetup_platform_with_config, with the same steps as before.Relation to #5483
That PR verifies a missing limit only as
DEFAULT_QUERY_LIMIT(vote_query_limit_drive_provesinproof.rs) and has the same open review finding about nodes configured with a larger default.verify_page_drive_provesanswers it for both: once either PR lands, the five vote verifiers can verify through it. The two PRs touch different lines and merge in either order without conflicts.Before and after
The same SDK call against mainnet (current epoch, more than 200 proposers):
From a node whose operator set
default_query_limit = 200, with 150 groups in a contract:Likewise for
Group::fetch_many,GroupAction::fetch_manyandTokenPreProgrammedDistributions::fetchwithlimit: Noneonce more than 100 results exist. The C FFI'sdash_sdk_evonode_get_proposed_epoch_blocks_by_rangewithlimit = 0(documented as "Drive's default page") sends no limit, so it failed the same way and works now. Its comment inrs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_range.rsstill says a request without a limit fails proof verification; it is left for a separate change, since that crate needs its own code owners' review.How Has This Been Tested?
cargo test -p drive-abci --lib -- should_verify_the_default_page_of: 4 passed.DEFAULT_QUERY_LIMIT(no fallback), all four fail atFromProofwithGroveDBErroron the node configured with 200 ("Proof returns more data than limit Some(100)", or for distributions "V1 proof contains extra lower layer ... not required by query");cargo test -p drive-proof-verifier --lib -- verify group pre_programmed proposer u32_to_u16: 67 passed.cargo test -p drive-abci --lib -- query::validator_queries::proposed_block_counts_by_range query::token_queries::token_pre_programmed_distributions query::group_queries::group_infos query::group_queries::group_actions: 75 passed.setup_platformhelper changed:cargo test -p drive-abci --lib -- query::: 807 passed, 1 ignored (already#[ignore]).cargo test -p dash-sdk --test main -- group_actions token_pre_programmed: 9 passed. (There is no recorded vector for proposers by range.)cargo clippy -p drive-proof-verifier -p drive-abci --lib --testsclean;cargo fmt --all -- --checkclean.git merge-treeof this branch with fix(sdk): verify no-limit vote queries against the page Drive proves #5483's branch: clean.getEvonodesProposedEpochBlocksByRangewith grpcurl against two evonodes): no limit returns a full page of 100, and the next page another 100, so the proved no-limit query exceeds Drive's page there. I did not run the proved query against mainnet; the drive-abci test covers the verification.Breaking Changes
None. No public API or request changes; only how the client checks the proofs of these four no-limit queries.
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 ·
a9c5fe3/skip-botsproceeds without the ones not yet reported/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
Documentation