Skip to content

fix(sdk-ffi): carry the limit in the proposed-epoch-blocks range request - #4456

Merged
QuantumExplorer merged 1 commit into
v4.2-devfrom
fix/proposed-blocks-range-limit
Aug 22, 2026
Merged

fix(sdk-ffi): carry the limit in the proposed-epoch-blocks range request#4456
QuantumExplorer merged 1 commit into
v4.2-devfrom
fix/proposed-blocks-range-limit

Conversation

@QuantumExplorer

@QuantumExplorer QuantumExplorer commented Aug 22, 2026

Copy link
Copy Markdown
Member

Summary

dash_sdk_evonode_get_proposed_epoch_blocks_by_range ignored its limit parameter and sent the request with limit: None. Drive then built the proof for its default page (100) while the verifier re-derived the query shape from the request (no limit), so every proved call failed with:

Proof verification error: grovedb: invalid proof: Invalid V1 proof verification parameters:
invalid proof error Proof is missing data for query range. Encountered unexpected node type: KVHash(...)

(seen on mainnet from the iOS wallet). The limit now travels in the request; 0 ⇒ Drive's default page.

This commit was pushed to the feat/current-epoch-ffi branch right after #4453 had merged (the branch had been deleted), so it never landed — re-opened here on top of v4.2-dev.

Consumer: dashpay/dashwallet-ios#1036 (Nodes shortcut — blocks proposed this epoch).

Test plan

  • cargo test -p rs-sdk-ffi --lib -- proposed_epoch_blocks, cargo clippy -p rs-sdk-ffi --all-targets -- -D warnings, cargo fmt
  • Mainnet, iOS wallet with 6 evonodes: before — every page failed as above; after — pages verify, 35 blocks tallied for epoch 79.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Enhancements
    • Added support for forwarding page-size limits when retrieving proposed epoch blocks by range.
    • A limit of zero continues to use Drive’s default page size.

`dash_sdk_evonode_get_proposed_epoch_blocks_by_range` ignored its `limit`
parameter and sent the request with `limit: None`. Drive then built the
proof for its default page while the verifier re-derived the query shape
from the request (no limit), so every proved call failed with
`Proof is missing data for query range … KVHash`. The limit now travels
in the request (0 ⇒ Drive's default page).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@thepastaclaw

thepastaclaw commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

🕓 Ready for review — 1 ahead in queue (commit 84690af)
Queue position: 2/3
ETA: start ~20:10 UTC · complete ~20:35 UTC (median 25m across 30 recent reviews; 2 slots)
Queued 6m ago · Last checked: 2026-08-22 20:10 UTC

@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5533150a-ed26-4270-a8e2-96676d8f91bd

📥 Commits

Reviewing files that changed from the base of the PR and between e141368 and 84690af.

📒 Files selected for processing (1)
  • packages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_range.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The proposed epoch blocks query now accepts and stores an optional page limit. Positive limits are forwarded to the gRPC request, while zero selects Drive’s default page size.

Changes

Epoch block query limit

Layer / File(s) Summary
Normalize and store query limit
packages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_range.rs
The helper accepts the limit. Positive values are stored as Some(limit), and zero is stored as None. The API documentation describes this behavior.
Forward limit to gRPC request
packages/rs-sdk-ffi/src/evonode/queries/proposed_epoch_blocks_by_range.rs
The generated request receives the query’s optional limit.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 84690

The change carries the requested page limit so proposed epoch block queries can verify successfully; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: shumkov, lklimek

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: carrying the limit in the proposed epoch blocks range request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/proposed-blocks-range-limit

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@QuantumExplorer
QuantumExplorer merged commit 8f6dce2 into v4.2-dev Aug 22, 2026
18 of 21 checks passed
@QuantumExplorer
QuantumExplorer deleted the fix/proposed-blocks-range-limit branch August 22, 2026 20:17
bfoss765 added a commit that referenced this pull request Aug 23, 2026
Brings the branch up to date with upstream after #4451 (masternode
credit withdrawals), #4452, #4453, #4456, and #4461 landed on v4.2-dev.

One conflict, in packages/rs-platform-wallet/src/wallet/core/mod.rs:
a module-registration collision where this branch adds
'pub mod spend_observer;' and upstream adds
'pub(crate) use sign_message::is_signable_funding_account;' at the same
spot. Resolved as the union — both lines kept, no semantic overlap.

Auto-merged overlaps verified by hand: both error.rs files and
DashSdkError.kt gained disjoint additions (upstream's
MasternodeWithdrawalUnconfirmed / FFI code 42 alongside this branch's
StaleReservation reusing code 34 — codes distinct, both mapping arms
present). generation.rs (the broadcast-fence redesign) was touched by
this branch only.

Verified: cargo check -p platform-wallet -p platform-wallet-ffi clean;
cargo test -p platform-wallet wallet::core::generation — 15/15 passed,
including the settle-boundary and handoff tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bfoss765 added a commit that referenced this pull request Aug 23, 2026
Brings the shielded-invite branch up to date with upstream v4.2-dev
(#4451 masternode credit withdrawals, #4452, #4453, #4456, #4461).

One conflict, in rs-platform-wallet/src/wallet/shielded/operations.rs:
upstream #4451 moved carries_consensus_rejection() and
broadcast_definitely_failed() out of operations.rs into the new shared
crate::broadcast_outcome module (so masternode withdrawals can reuse
them), while this branch had inserted its one-time-key claim machinery
(NullifierSpentStatus, claim-evidence resolution) directly after those
functions. Resolved by dropping the now-local duplicate of
broadcast_definitely_failed() — its body is byte-identical to the moved
copy, and the file already imports both helpers from
crate::broadcast_outcome via upstream's auto-merged use line — and
keeping this branch's one-time-key claim block in place. No semantic
changes to either side.

Verified: cargo check -p platform-wallet -p platform-wallet-ffi
-p rs-unified-sdk-jni clean; cargo test -p platform-wallet
--features shielded wallet::shielded = 205 passed, 0 failed
(includes the one_time_claim_evidence and note_selection suites).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants