fix(rs-sdk-ffi): build the voter identifier from ProTxHash byte order - #4333
Merged
Conversation
Casting a contested-resource vote failed with Platform's "Internal Error: Protocol error: Public key 0 doesn't exist". `cast_vote` fabricated the signing key with `id: 0`, on the assumption that is "exactly the shape Platform assigns to voter identities". Platform assigns 0 only when it *creates* a voter identity. The voter identifier is `SHA256(pro_tx_hash || voting_address)`, so a voting-key rotation produces a different identity — `update_voter_identity_v0` disables every key on the old one and creates a fresh identity for the new address. And nothing guaranteed the caller's private key corresponded to an identity that exists at all. In each of those cases the fabricated key id was simply wrong, and the caller got an opaque protocol error at broadcast time with nothing to act on. The voting key is now fetched from the voter identity and matched on its own data — `Purpose::VOTING`, `ECDSA_HASH160`, `data == hash160(pubkey)`, not disabled — so the real key id is used whatever it is. The two failure modes that previously hid behind "Public key 0 doesn't exist" now say what happened: - no voter identity for this pro_tx_hash + voting key, naming the expected voter identity, and pointing at the two causes (the key does not match the masternode's registered voting address, or Platform has not created the identity yet); - identity present but holding no enabled matching key, i.e. the voting key was rotated. This mirrors how dash-evo-tool resolves a voting key (`verify_voting_key_exists_on_identity`): derive the identifier, fetch, then match the key by type and data rather than by position. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collaborator
|
🕓 Ready for review — next in queue (commit 54683fa) |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughVote casting now reverses the wire-order transaction hash before identity derivation and transition construction. Failed broadcasts diagnose missing voter identities and missing enabled matching keys. Kotlin and Swift documentation describe the required hash order. ChangesVoting key validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VoteCasting
participant Broadcast
participant Platform
VoteCasting->>Broadcast: Submit vote with local ID-0 key
Broadcast-->>VoteCasting: Return failure
VoteCasting->>Platform: Fetch derived voter identity
Platform-->>VoteCasting: Return identity and voting keys
VoteCasting->>VoteCasting: Select enabled matching ECDSA_HASH160 key
VoteCasting-->>Broadcast: Return specific InvalidParameter or original error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs`:
- Around line 313-349: Add mocked tests covering the identity-resolution logic
in the async closure around Identity::fetch: verify a missing identity returns
the expected InvalidParameter error, a matching enabled ECDSA_HASH160 voting key
is selected, and disabled or mismatched keys are rejected. Exercise the actual
voting-key selection behavior without changing production logic.
- Around line 324-347: Update the FFIError::InvalidParameter messages in the
identity lookup and voting-key matching error paths to remove source-line
indentation from the resulting strings. Preserve the existing wording and
interpolated values while joining wrapped text with normal single spaces so
clients do not receive internal whitespace runs.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b8546cb5-00b0-4137-b78e-9d67d6350ff2
📒 Files selected for processing (1)
packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4333 +/- ##
============================================
- Coverage 87.62% 87.62% -0.01%
============================================
Files 2704 2704
Lines 345206 345206
============================================
- Hits 302474 302473 -1
- Misses 42732 42733 +1
🚀 New features to boost your workflow:
|
thepastaclaw
reviewed
Aug 7, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The fix correctly replaces a fabricated ECDSA_HASH160 key with one resolved via a proof-verified fetch of the actual voter identity, matching by purpose/type/data/enabled-state rather than a hardcoded key id 0. Logic, imports, and identifier derivation are correct and this is a clear net improvement over the previous behavior. Two real defects remain: both new diagnostic error strings (the PR's stated deliverable) contain long runs of literal embedded whitespace from unescaped multi-line format! literals, and the core identity-resolution/key-selection logic has no test coverage. A module-doc nitpick about key-id/rotation semantics is also worth a follow-up. No blocking consensus, security, or FFI-boundary issues were found.
Source: Reviewer backends — gpt-5.6-sol (codex: general, ffi-engineer, security-auditor), claude-sonnet-5 (opus-lane: general, ffi-engineer, security-auditor); final verifier backend — claude-sonnet-5 (final-verifier-sonnet-1-951549e47466468fab7200db619eee5b). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— security-auditor (completed)
🟡 3 suggestion(s) | 💬 1 nitpick(s)
🤖 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-ffi/src/contested_resource/transitions/cast_vote.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs:326: "No voter identity" diagnostic contains literal embedded whitespace runs
This format! literal was written across multiple source lines without a `\` continuation, so the line-wrap indentation became literal characters inside the string. Verified directly in the file: line 326 contains runs of ~22 consecutive spaces before `(expected`, before `masternode's`, and before `voter identity yet.`. This string is exactly the diagnostic this PR exists to add (per the PR description: "Errors are now diagnostic") and crosses the FFI boundary as the `message` field of `DashSDKError`, so a Swift/iOS caller displaying it to the user sees large mid-sentence gaps instead of a clean message.
- [SUGGESTION] packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs:345: "Voting key rotated" diagnostic contains the same literal whitespace defect
Same root cause as the sibling message at line 326: the format! literal contains a run of ~22 literal spaces between "matching this" and "private key." This is the second of the two new diagnostics the PR advertises as its main improvement, and it will render with the same garbled gap to callers and logs.
- [SUGGESTION] packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs:313-349: New identity-fetch/key-selection logic has no test coverage
All three existing tests in this file return before reaching this code (null handle, missing contender, invalid vote choice — all short-circuit before line 307). Nothing exercises: a missing voter identity producing the new `InvalidParameter`, an enabled matching key being correctly selected (propagating its real, possibly non-zero-position id into signing), or a disabled/mismatched key being rejected. This is the core of the fix and the PR description itself flags it as "not yet verified against a live vote" — factoring the lookup into a testable helper (or configuring the mock SDK with an identity fixture) would give real regression coverage for the exact bug this PR fixes.
…rors Both new messages were written as multi-line literals without line continuations, so the source indentation became part of the string. On device they render with blank gaps mid-sentence — "with this voting key [gap] (expected voter identity ...)". Use `\` continuations so the text reads as one line. Verified rendered output contains no double spaces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…onale
Review was right that the module doc misstated Platform's behavior. Every
voter identity's voting key IS id 0: `create_voter_identity_v0` passes 0,
and a rotation creates a *different* identity (the identifier includes the
voting address) whose key is also 0. The id was never the problem.
What actually fails, and what the fetch now catches:
* no voter identity exists for this (pro_tx_hash, voting address) pair
* after a rotation `update_voter_identity_v0` DISABLES the old identity's
keys, so key 0 can exist and be unusable
Both previously surfaced as "Public key 0 doesn't exist". Doc corrected to
say this.
`Identity::fetch` needs a live SDK, so the lookup stays integration-shaped,
but key *selection* decides whether a vote can be signed and is pure —
extracted as `select_voting_key` and covered: the matching enabled key is
selected; a key at a non-zero id is still found (selection is by data, not
position); a disabled key is rejected with the rotation diagnostic; a
different address is rejected; a matching address under the wrong key type
is rejected. `missing_voter_identity` is pinned to name both identifiers
and to contain no whitespace runs, since it crosses the FFI boundary
verbatim.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs`:
- Around line 576-660: Add a test alongside the existing select_voting_key tests
using an AUTHENTICATION key with KeyType::ECDSA_HASH160 and VOTING_ADDRESS, then
assert select_voting_key returns an error for that identity. Keep the test
focused on rejecting the matching address when the key purpose is not
Purpose::VOTING.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 38135101-fa43-427f-8577-71d527a4b4a7
📒 Files selected for processing (1)
packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs
thepastaclaw
reviewed
Aug 7, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This delta (52683dc..04b6edf) is a follow-up cleanup of the previously-reviewed fix: it corrects the two whitespace-mangled diagnostic strings with \ line continuations, extracts key selection into a pure select_voting_key helper backed by nine unit tests, and fixes the module doc's inaccurate key-id/rotation claim. All four prior findings are verified fixed against the current head. Two minor issues remain out of the prior set: the module doc's 'who can actually cast a vote' section still describes the old platform-side rejection path even though a missing voter identity now fails locally before broadcast, and the select_voting_key test suite lacks a case that isolates the Purpose::VOTING predicate (removing it would not currently fail any test). A pre-existing, PR-unrelated Swift-side issue (processVoidResult from PR #3883 discards FFI error codes) is recorded as an out-of-scope follow-up.
Source: reviewer backends — gpt-5.6-sol (Codex general, ffi-engineer, security-auditor), claude-sonnet-5 (Sonnet general, ffi-engineer, security-auditor); final verifier backend — claude-sonnet-5. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— ffi-engineer (completed)
🟡 1 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 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-ffi/src/contested_resource/transitions/cast_vote.rs`:
- [SUGGESTION] packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs:649-660: Cover the voting-purpose predicate independently
`select_voting_key` checks `key.purpose() == Purpose::VOTING`, but no test isolates that condition: the only `Purpose::AUTHENTICATION` fixture (in `selects_by_data_not_by_position`) also uses `OTHER_ADDRESS`, so it's already rejected by the address check. If the purpose check were deleted entirely, every existing test would still pass. Add a fixture with an `AUTHENTICATION` key at the correct `VOTING_ADDRESS` and `ECDSA_HASH160` type to pin that the purpose predicate is actually load-bearing.
`ProTxHash` is declared `#[hash_newtype(forward)]`; `Txid` is not. So for the same transaction, `ProTxHash::to_byte_array()` is display order and `Txid::to_byte_array()` is its reverse. `rpc-json`'s `MasternodeListItem` carries both conventions side by side (`pro_tx_hash: ProTxHash`, `collateral_hash: Txid`), and drive-abci derives the voter identity from `masternode.pro_tx_hash.to_byte_array()`. Callers hand this FFI wire order — what `reg.txid()` yields and what the iOS wallet stores. Passing that straight to `create_voter_identifier` asked Platform for an identity that has never existed, while the real one sat under the reversed hash. The vote was rejected as having no voter identity even with the correct voting key, which is indistinguishable from a genuinely unregistered node. Reverse once at entry so the identifier and the transition's pro_tx_hash cannot drift apart. Verified arithmetically against a live failure: the wallet's key hashes to the registered voting address XgYHAS…, and SHA256(reversed protx ‖ hash160) is the identity Platform holds, where the wire-order hash produced one it does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs`:
- Around line 290-309: Document in the public Swift and Kotlin cast-vote
wrappers that proTxHash must be supplied as 64-character hexadecimal
representing Txid wire-order bytes, matching the Rust reversal before identity
lookup. Update the wrappers’ parameter names and input labels to explicitly
identify this wire-byte order, while preserving the existing conversion and
transition behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ff934ea-5a71-4ae8-a971-4f42a48c9311
📒 Files selected for processing (1)
packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs
thepastaclaw
suggested changes
Aug 8, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Collaborator
There was a problem hiding this comment.
Preliminary review — Codex only
The proof-verified identity fetch and key matching are sound, but the latest unconditional proTxHash reversal silently assumes wire-order input and breaks the shipped Swift path that accepts conventional display-order hex, so changes are required. The two previously verified documentation and test-coverage findings also remain valid at this head.
Source: Reviewer backends — gpt-5.6-sol (Codex general, security-auditor, rust-quality); final verifier backend — gpt-5.6-sol (Codex verifier). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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-ffi/src/contested_resource/transitions/cast_vote.rs`:
- [BLOCKING] packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs:306-310: Unconditional reversal breaks display-order proTxHash callers
This now treats every FFI input as raw `Txid` wire-order bytes and reverses it, but the public Rust, Swift, Kotlin, and JNI contracts only describe a 32-byte proTxHash. The shipped Swift form asks users to paste `pro_tx_hash (64 hex chars)` and decodes those characters verbatim, while `PersistentMasternode.proTxHashHex` explicitly exposes the conventional block-explorer/display-order value. Pasting that value causes this code to reverse it into wire order even though Platform creates the voter identity from the forward/display-oriented `MasternodeListItem.pro_tx_hash.to_byte_array()`, so the proof-verified identity lookup misses and a valid vote cannot be cast. Define one byte-order contract across the public boundary: either accept display-order bytes here without reversing, or retain wire-order input for wallet-internal callers while converting the display-order Swift/Kotlin form input before the FFI call. Pin the chosen conversion with an asymmetric hash and expected voter-identifier regression test.
The identity fetch ran before every cast to pick the voting key. Review established the key id is always 0 — `create_voter_identity_v0` passes 0, and a rotation creates a different identity whose key is also 0 — so the lookup bought no correctness on the happy path, only diagnosis on failure. It cost a Platform round trip per (node, contest): a bulk vote of 6 nodes across 10 names paid 60 fetches for runs that overwhelmingly succeed. Now the key is built locally and broadcast immediately. Only if the broadcast fails does the identity get fetched, to say which of the two indistinguishable causes it was — no voter identity for this (pro_tx_hash, voting address) pair, or a rotation that disabled key 0. Platform reports both as "Public key 0 doesn't exist". A diagnosis replaces the opaque error; anything else — network, fees, an already-closed poll — survives unchanged rather than being recast as a key problem. A diagnostic fetch that itself fails leaves the original error intact. `select_voting_key` and its tests are unchanged: they now serve the diagnosis rather than the signing path, which is what they were really testing. The byte-order fix in this branch is untouched and remains the load-bearing change — the voter identifier and the transition's pro_tx_hash must be built from ProTxHash (display) order, not the wire order callers pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QuantumExplorer
changed the title
…pose check Two review follow-ups. **Byte order is now documented where callers see it.** The Swift and Kotlin wrappers took 32 bytes with no statement of orientation while the Rust side reverses them, which is precisely the ambiguity that caused this bug. All three boundaries — the FFI parameter, `castContestedResourceVote`, and `castVote` — now say the bytes must be WIRE order (what `Txid` stores and a parsed ProRegTx yields), not the reverse that Core displays, and why: Platform identifies masternodes by the opposite orientation because `ProTxHash` is `#[hash_newtype(forward)]` and `Txid` is not. **The voting-purpose check is now load-bearing in tests.** Review was right that nothing isolated it: the only `AUTHENTICATION` fixture also used a different address, so the address check rejected it first and deleting `purpose() == VOTING` would have left every test green. Added a fixture with an AUTHENTICATION key at the CORRECT address and key type, where purpose is the only thing that can reject it. 10/10 in the module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rs`:
- Around line 396-406: Update the error-handling flow around
diagnose_vote_failure so it runs only when broadcast_error identifies a
voter-key failure. Preserve and return the original broadcast_error unchanged
for closed polls, fee errors, network failures, and any other unrelated
broadcast errors; only replace the error when the identity diagnosis applies.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 92b1f41b-a51b-4548-abc8-2b4c6eb2b7a1
📒 Files selected for processing (3)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/voting/VoteCasting.ktpackages/rs-sdk-ffi/src/contested_resource/transitions/cast_vote.rspackages/swift-sdk/Sources/SwiftDashSDK/FFI/PlatformQueryExtensions.swift
…ting key
The fail-soft restructure ran the identity diagnosis on EVERY broadcast
failure and replaced the error whenever the voter identity was absent. So a
vote that arrived after its poll closed, cast by a node with no voter
identity, was reported as "no voting identity exists on Platform" — the
true cause discarded and replaced with a plausible-looking wrong one. My
own comment claimed unrelated errors survived unchanged; the code did not
do that.
Gated on the typed consensus error rather than its rendered text. Only
`SignatureError::{IdentityNotFoundError, MissingPublicKeyError,
PublicKeyIsDisabledError}` are diagnosable — they are exactly what fetching
the identity can explain. Everything else keeps its own error, and the
existing fallback still applies when the identity and key both check out.
Matching text would have silently started diagnosing unrelated failures the
first time a message changed, which is the same class of fragility that
produced the bug.
Tests pin both directions: the three key failures are diagnosable, and a
signature failure that is NOT about the key's existence or state
(`BasicECDSAError`) is not — nor is a broadcast error carrying no consensus
cause at all. 12/12 in the module.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`get_voting_identity_id` derives the voter identity from `voter_pro_tx_hash.as_bytes()` verbatim, and neither `PutVote` method said which orientation that must be. That silence is where this PR's bug came from: a caller holding wire/`Txid` order — what `reg.txid()` yields and what a wallet stores — addresses an identity that has never existed, and Platform rejects the vote as having no voter identity. The FFI now reverses before calling, but that only helps callers who go through the FFI. A native rs-sdk consumer hits the same trap with no guidance, so the contract belongs here: both methods and `get_voting_identity_id` now state that the bytes must be `ProTxHash` order (what Core's RPC hex shows), that `Txid` bytes for the same transaction are its exact reverse because `ProTxHash` is `#[hash_newtype(forward)]` and `Txid` is not, and that `rpc-json`'s `MasternodeListItem` carries both conventions side by side. Documentation only — no behaviour change. Moving the derivation, key construction and failure diagnosis out of rs-sdk-ffi and into rs-sdk is a published-API change and follows separately. Co-Authored-By: Claude Opus 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.
The bug
Masternode votes could not be cast at all. Platform rejected every one, and after the diagnostics in this PR the reason read:
The identity genuinely did not exist — because the FFI asked for the wrong one.
ProTxHashandTxiduse opposite display conventions, declared a few lines apart inhash_types.rs:rpc-json'sMasternodeListItemcarries both side by side (pro_tx_hash: ProTxHash,collateral_hash: Txid), and drive-abci derives the voter identity frommasternode.pro_tx_hash.to_byte_array()— display order. Callers hand this FFI wire order, which is whatreg.txid()yields and what the iOS wallet stores. The two are exact reverses.That broke both consumers of the value:
create_voter_identifier, and thepro_tx_hashtheMasternodeVoteTransitionitself carries. Reversed once at entry so they cannot drift apart.Verified arithmetically against a live device failure: the wallet's key hashes to the node's registered voting address
XgYHAS…, andSHA256(reversed protx ‖ hash160)is the identity Platform holds, where the wire-order hash produced one it does not.Also in here
Diagnosis on the failure path. Platform reports both "no voter identity" and "rotation disabled key 0" as the same opaque
Public key 0 doesn't exist. The identity is now fetched only after a broadcast fails, to say which — the key itself is built locally, sincecreate_voter_identity_v0always assigns id 0 and a rotation creates a different identity whose key is also 0.An earlier revision fetched before every cast. That bought no correctness and cost a round trip per (node, contest) — a bulk vote of 6 nodes across 10 names paid 60 fetches on runs that overwhelmingly succeed. An unrelated failure (network, fees, closed poll) now survives unchanged rather than being recast as a key problem, and a diagnostic fetch that itself fails leaves the original error intact.
Whitespace in the diagnostics. Both new messages were multi-line literals without
\continuations, so source indentation rendered as blank gaps mid-sentence on device.A corrected claim. The original description said the key id was unreliable. It isn't — review was right, and the module doc now states what actually fails.
Testing
9/9 in this module.
select_voting_keyis covered directly: the matching enabled key is selected; a key at a non-zero id behind anAUTHENTICATIONkey at 0 is still found (so selection can't regress to picking by position); a disabled key is rejected with the rotation diagnostic; a wrong address and a matching address under the wrong key type are both rejected. Plus one pinning that the missing-identity message names both identifiers and contains no whitespace runs.Not verified: no vote has been confirmed broadcasting end to end. The byte-order fix is established arithmetically and against a real device failure, but the successful path is untested. The byte orientation itself has no test — the two conventions living in one struct makes that worth adding, and the existing tests all short-circuit before reaching
create_voter_identifier.Depends on nothing; #4338 (merged) fixed a separate derivation bug in the same flow.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation