Repository navigation
fix(sdk): read the proved vote of a masternode vote that already exists - #5628
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Validate vote poll requests and decode results packages/rs-drive-proof-verifier/src/proof.rs |
The verifier requires an inclusive starting poll ID and selects the vote keyed by that ID. Tests cover missing or exclusive starts and update a malformed-identity request. |
Exercise vote proofs through the platform packages/rs-drive-abci/tests/masternode_vote_proof.rs |
Tests build and sign proofs for three votes, decode each requested vote, and check that a poll below the fixture’s vote IDs returns None. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix · Severity of issue fixed: Medium
Possibly related PRs
- dashpay/platform#5558: Adds the pre-send
FromProof::check_requesthook used by this change’s vote-poll request validation.
Suggested reviewers: lklimek, quantumexplorer, thepastaclaw
Merge Risk: 
2cfa0
-
The change is mergeable with no actionable current-head risk identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "fix(sdk): read the proved vote of a masternode vote that already exists" matches the main change. The PR changes Vote proof decoding in drive-proof-verifier so it selects the vote keyed by … Linked Issues check Issue [ #5443] requires a regression test and a fix for proof decoding when an existing masternode vote is returned.packages/rs-drive-proof-verifier/src/proof.rsnow selects the vote by the requeste…Out of Scope Changes check The changes remain within issue [ #5443]. Request validation prevents ambiguous single-vote decoding, and the unit and integration tests cover that validation and the existing-vote proof path. No unrel…Full details: Title check
-
Explanation
The title "fix(sdk): read the proved vote of a masternode vote that already exists" matches the main change. The PR changes Vote proof decoding in drive-proof-verifier so it selects the vote keyed by the requested vote-poll ID instead of comparing it with the identity ID, returns None when no vote matches, and rejects non-vote-poll start requests before sending. The added tests in masternode_vote_proof.rs cover reading existing votes and the None case, and the objectives say the change addresses the already-existing-vote decoding failure in issue
#5443. The title is specific and concise, and it uses the conventional fix(scope) prefix.
Full details: Linked Issues check
-
Explanation
Issue [
#5443] requires a regression test and a fix for proof decoding when an existing masternode vote is returned.packages/rs-drive-proof-verifier/src/proof.rsnow selects the vote by the requested vote-poll ID and returnsNonewhen that poll has no vote.packages/rs-drive-abci/tests/masternode_vote_proof.rsexercises the real query handler, signed proof, andFromProofdecoding for both cases.
Full details: Out of Scope Changes check
-
Explanation
The changes remain within issue [
#5443]. Request validation prevents ambiguous single-vote decoding, and the unit and integration tests cover that validation and the existing-vote proof path. No unrelated production behavior or protocol data changes appear in the reviewed diff.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 2 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
|
Waiting for bot review — thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The core fix correctly selects a proved vote by vote-poll identifier while retaining identity binding through the verified GroveDB path. Two non-blocking hardening gaps remain: the new request gate permits offsets and zero limits that can cause a valid proof page to be interpreted as absence, and the malformed starting-poll identifier error path lacks direct test coverage. CI reports the Rust workspace and relevant package checks successful; only the non-code PR Hygiene check remains pending.
🟡 2 suggestion(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: 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 substantial change to Vote proof decoding and request validation alters interpretation of verified network proofs, making proof-system soundness a critical security surface. - 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— 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-drive-proof-verifier/src/proof.rs`:
- [SUGGESTION] packages/rs-drive-proof-verifier/src/proof.rs:2375-2393: Reject vote requests with offsets or a zero limit
The single-vote decoder selects the requested poll from the proved page, but `requested_vote_poll_id` only requires an inclusive start. A request with `offset: Some(1)` passes `check_request`, causes the query to skip the requested poll, and then returns `Ok(None)` even when that poll has a vote. Likewise, a zero limit cannot include the requested vote. The current `VoteQuery` caller uses `offset: None` and `limit: Some(1)`, but `FromProof` and `LimitQuery<VoteQuery>` expose these request shapes, so the single-vote contract should reject them before transmission.
- [SUGGESTION] packages/rs-drive-proof-verifier/src/proof.rs:6395-6455: Cover malformed starting vote-poll identifiers
The new `requested_vote_poll_id` maps an invalid starting poll identifier to `Error::RequestError`, but the refusal test only covers a missing start and an excluded start with a valid 32-byte identifier. Add a malformed start identifier case to ensure corrupt poll IDs are rejected before proof processing and that this error mapping remains stable.
|
Bots are done — your move: post |
github-actions
Bot
added
waiting-self-review
|
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
The PR correctly fixes single-vote proof decoding by selecting the requested vote-poll identifier after the identity-bound proof is verified. The new preflight validation and malformed-identifier coverage are appropriate, and no in-scope correctness, security, consensus, or layering issues were found. Rust workspace tests, JS builds, and Kotlin tests were still in progress in the supplied CI snapshot; no completed failure was reported.
🔴 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: rust-quality); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 10: 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 change is an intricate proof-decoding update inFromProof<GetContestedResourceIdentityVotesRequest> for Votethat affects the cryptographic proof-verification trust boundary and could alter which authenticated vote is returned. - 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— 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— 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
waiting-self-review
When a masternode vote broadcast is answered with "already exists", the SDK fetches the stored vote with `VoteQuery`: the voter's votes from the vote poll on, at most one. The proof verifier returns those votes keyed by vote poll id, but `Vote`'s decoder compared that key with the request's identity id, so every existing vote failed with "expected vote for identity <voter>, got vote for identity <vote poll>". The decoder now takes the vote poll the request starts at (inclusively) and reads the vote keyed by it. When the voter has not voted on that poll, the proof carries its votes on the following polls instead, and the decoder reports no vote rather than one of those. A request that does not start at its vote poll has no vote to pick, so `Vote` refuses it in `check_request`, before it is sent, instead of failing the answer of a node that answered correctly. The node's proof was right; the fix is client-side decoding only and changes no protocol version's behaviour. Rebuilt on v5.0-dev: here `ResourceVotesByIdentity` holds a plain `ResourceVote` per vote poll (v5.1 holds `ResourceVoteWithCount`), so the proved vote is wrapped with `Vote::ResourceVote` directly. Test would have caught this in CI: masternode_vote_proof (drive-abci integration suite: real query handler, proof signed by a test quorum, decoded as `Vote`) ✖ before the fix — both tests fail with the ResponseDecodeError above — ✔ after, on the v5.0-dev base. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 40c235c)
…l id Covers the remaining refusal path of the vote request check: a start of 5 bytes instead of a 32-byte vote poll id is refused before the request is sent, like a missing or excluded start. Test-only coverage of an error path this branch added; there is no earlier behaviour for it to have failed on. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit 2cfa0d1)
2cfa0d1 to
4f36710
Compare
|
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
The proof decoder correction is valid and the malformed-start test is now covered. However, the already-exists recovery path still queries votes using the masternode ProTxHash instead of the derived voter identity, while the node stores and queries votes under that derived identity; this leaves the primary retry-confirmation path broken. Rust workspace tests and evo-sdk tests are still queued in CI.
🔴 1 blocking
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Use the derived voter identity for the confirmation query
packages/rs-sdk/src/platform/transition/vote.rs:145-146
The retry confirmation passes voter_pro_tx_hash as the first argument to VoteQuery, which becomes the request's identity_id. This is not the identity under which the vote is stored: the function already derives voting_identity_id at line 111, the transition uses that same (ProTxHash, voting key) derivation, and the node creates voter identities with Identifier::create_voter_identifier(pro_tx_hash, voting_address). The proof verifier therefore validates a query for the wrong identity and finds no vote, causing the recovery branch to return vote was proved to not exist but was said to exist. The new proof tests construct requests directly and do not exercise this PutVote recovery path.
let vote =
Vote::fetch(sdk, VoteQuery::new(voting_identity_id, vote_poll_id)).await?;
source: gpt-6.1-sol (phase2-reviewer: general)
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: rust-quality); reviewer 6: 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 change modifiesFromProof<GetContestedResourceIdentityVotesRequest> for Voteinpackages/rs-drive-proof-verifier/src/proof.rs, a peer-facing proof deserialization and verification boundary where incorrect key selection could affect the soundness of authenticated vote data. - 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— 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/src/platform/transition/vote.rs`:
- [BLOCKING] packages/rs-sdk/src/platform/transition/vote.rs:145-146: Use the derived voter identity for the confirmation query
The retry confirmation passes `voter_pro_tx_hash` as the first argument to `VoteQuery`, which becomes the request's `identity_id`. This is not the identity under which the vote is stored: the function already derives `voting_identity_id` at line 111, the transition uses that same `(ProTxHash, voting key)` derivation, and the node creates voter identities with `Identifier::create_voter_identifier(pro_tx_hash, voting_address)`. The proof verifier therefore validates a query for the wrong identity and finds no vote, causing the recovery branch to return `vote was proved to not exist but was said to exist`. The new proof tests construct requests directly and do not exercise this `PutVote` recovery path.
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
github-actions
Bot
added
waiting-self-review
|
@thepastaclaw On the blocking finding "Use the derived voter identity for the confirmation query" (
The suggested change would query The |
|
@shumkov on it — I've queued this as job |
|
Thanks for the clarification. reviewsys handles this review thread; it will answer on the finding’s thread and re-review on push (and draft again once the PR is marked ready). |
No code change. The bot re-reviews only on push; the outside-diff finding (use the derived voter identity in the already-exists vote query) was declined with evidence in the PR conversation: Drive stores masternode votes under the ProTxHash. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Your move: thepastaclaw requested changes on this head; dismiss the review or push a fix. |
Issue being fixed or feature implemented
Closes #5443.
When a masternode vote broadcast is answered with "already exists" (Tenderdash's answer when the same vote transition is broadcast again, for example after a retry),
PutVote::put_to_platform_and_wait_for_responseconfirms the vote by fetching it withVoteQuery. That fetch always failed to decode:Root cause:
VoteQueryasks for the voter's votes starting at the vote poll, inclusively, limit 1. The proof verifier (verify_identity_votes_given_proof) has always keyed the proved votes by vote poll id.Vote'sFromProofdecoder compared that key with the request's identity id, so it rejected every vote that exists. The node's proof is correct: the query handler and the verifier build the same path query.This PR targets
v5.0-dev, so the fix ships in 5.0.x. It reachesv5.1-devthrough the next forward merge.What was done?
FromProof<GetContestedResourceIdentityVotesRequest> for Vote(rs-drive-proof-verifier) now reads the vote poll id the request starts at and takes the vote keyed by it. If the voter has not voted on that poll, the proof carries its votes on the following polls instead, and the decoder returns no vote rather than one of those.Vote::check_requestrefuses a request that does not start at its vote poll inclusively. Such a request has no vote to pick, so it is refused before it is sent (Error::Configin the SDK). Refusing it only after a node answered would count against a node that answered correctly.identity_id.Adaptations for v5.0
The branch was first written on
v5.1-devand has been rebuilt onv5.0-devby cherry-picking its two commits. One adaptation was needed:ResourceVotesByIdentitymaps each vote poll id to a plainResourceVote. On v5.1 it maps toResourceVoteWithCount. The decoder therefore wraps the vote as.map(Vote::ResourceVote)instead of.map(|vote| Vote::ResourceVote(vote.resource_vote)).Nothing else changed. The test suite's helpers have the same interface on v5.0:
tests/signed_response(signed,Provider), the shared voting fixturers-drive/tests/support/voting_default_limits.rs,TestPlatformBuilder::with_latest_protocol_version, andFromProof::check_request.In-place changes to shipped generations
None. On v5.0 every protocol generation that exists has shipped: the latest, PV14, is in the stable 5.0 releases and active on testnet. This PR edits no
vNmodule and no version table. The change is confined to client-sideFromProofdecoding indrive-proof-verifier, which no protocol version selects. The node's query handler, Drive's proof generation and verification, and every protocol version's tables are unchanged, so nothing is consensus-relevant.How Has This Been Tested?
New integration suite
packages/rs-drive-abci/tests/masternode_vote_proof.rs. It stores a masternode's votes, has the node's realquery_contested_resource_identity_voteshandler prove the exact requestVoteQuerysends, signs the proof with the test quorum (tests/signed_response), and decodes it asVotethrough the full proof and signature verification. It runs atPlatformVersion::latest(), which is PV14 on v5.0:should_read_the_proved_vote_of_a_masternode_vote_that_already_exists: the vote on each voted poll is read back.should_prove_no_vote_on_a_vote_poll_the_masternode_has_not_voted_on: a poll the masternode did not vote on yieldsNone, not its vote on the next poll. A fix that only deleted the comparison would fail this test.Red → green on the v5.0 base:
origin/v5.0-dev, both suite tests fail withResponseDecodeError { "expected vote for identity GgBaCs3N…, got vote for identity 82trM6AV…" }(the "no vote" test with… got vote for identity 6QMHZEhK…, the masternode's vote on the next poll).should_refuse_a_vote_request_that_does_not_start_at_its_vote_pollfails against v5.0's unfixedVoteimpl, which accepts such a request because it does not overridecheck_request. It passes with the fix.Also run on the v5.0 base:
RUST_MIN_STACK=4194304 CARGO_PROFILE_DEV_DEBUG=0 CARGO_PROFILE_DEV_CODEGEN_UNITS=256 cargo test -p drive-proof-verifier -p drive-abci -p dash-sdk --all-features --locked --lib --bins --tests --no-fail-fast -- --skip shield) passes: 5,274 tests, 0 failed, 48 ignored. That includes 349drive-proof-verifierlib tests, among them the newshould_refuse_a_vote_request_that_does_not_start_at_its_vote_polland an updatedvote_identity_vote_rejects_bad_identity_id_length, which now checks the identity id with a valid start.cargo clippy -p drive-proof-verifier -p drive-abci -p dash-sdk --all-targets --locked -- --no-deps -D warningsis clean, both with--all-featuresand with default features.cargo check -p drive-proof-verifier --locked(the transport-free cut) is clean.cargo fmt --all -- --checkis clean.Breaking Changes
None in practice.
Vote::fetchwith a request that does not start at a vote poll (for example a bareProTxHash) is now refused before sending. Before this change, such a request returnedOk(None)when the identity had no votes and failed to decode whenever it had any. The only caller in the repo usesVoteQuery, which always sets the start.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 ·
365cb1e/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