feat(dpns): unify safe masternode voting operations#901
Conversation
|
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:
📝 WalkthroughWalkthroughThis change introduces a unified DPNS voting workflow with proved vote-state snapshots, durable target journals and locks, coordinated execution, reconciliation for ambiguous results, scheduled-vote migration, and a shared Masternodes Voting Center. ChangesDPNS voting workflow
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
⛔ Blockers found — Sonnet deferred (commit aff18b2) |
|
Heads-up for whoever merges this: a PR-#860 review-feedback cleanup pass just landed a stopgap fix on Since this PR replaces Two related pieces in the same stopgap commit are not superseded and should be preserved through the merge: a 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR establishes a strong shared journal and voting workflow, and all 21 focused DPNS vote tests pass. Four in-scope defects remain: ambiguous post-broadcast errors can release duplicate-prevention locks, cancellation can overwrite active execution, scheduled edits are not crash-atomic, and legacy vote-state migration can cross network boundaries.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 1 suggestion(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 `src/backend_task/contested_names/vote_on_dpns_name.rs`:
- [BLOCKING] src/backend_task/contested_names/vote_on_dpns_name.rs:120-136: Post-broadcast wait errors release the duplicate-prevention lock
The SDK broadcasts at `vote.rs:114-117` and then enters a separate wait path. That wait can return transport, address-exhaustion, proof-verification, stale-metadata, context-provider, or invalid-response errors in addition to `StateTransitionBroadcastError`. This code treats every such variant as a pre-submission failure, so `classify_vote_attempt` records `FailedBeforeSubmission` and releases the target lock even though Platform may already have accepted the vote. Preserve the broadcast phase explicitly, or conservatively classify every error that can arise after the broadcast boundary as `Unconfirmed` until proved reconciliation resolves it.
In `src/context/dpns_vote_operations.rs`:
- [BLOCKING] src/context/dpns_vote_operations.rs:568-605: Cancellation can overwrite a concurrently claimed target
Both cancellation methods load operation snapshots while holding the journal mutex, release it, and later reacquire it through `update_dpns_vote_operation`. During that gap the due scheduler and executor can advance the persisted target from `Scheduled` to `Queued` or `Submitting`. The stale snapshot is then written as `NotApplied`; same-operation writes bypass conflict detection, so the lock is released while submission may be in flight or already broadcasting. Perform each read, status check, and conditional transition under one `dpns_vote_operation_guard` acquisition, and only persist when the currently stored status is still `Scheduled`.
- [BLOCKING] src/context/dpns_vote_operations.rs:264-316: Scheduled replacement is not crash-atomic
Editing a schedule durably changes the old target to `NotApplied` before the replacement operation and index entry are persisted. Each `DetKv::put` is an independent SQLite statement, so the process mutex and error rollback do not protect against termination between writes. A crash in that window leaves the old schedule inactive and the replacement absent or unindexed. The retained legacy row cannot recover it because migration skips any target already present in operation history, including the old `NotApplied` row. Persist the replacement and old-target transition in one storage transaction or one authoritative record mutation.
In `src/context/dpns_vote_state.rs`:
- [SUGGESTION] src/context/dpns_vote_state.rs:63-69: Legacy vote snapshots can be migrated into multiple networks
The legacy snapshot has no network discriminator, but every network missing its v2 entry copies the same retained v1 value into its namespace. Local masternode identities are loaded globally and reinterpreted for the active network, so switching networks can consume a recent snapshot created on another network. Before refresh completes, this can show an incorrect current choice; if it makes a requested choice appear unchanged, operation construction can discard the target as a no-op and return before the backend refresh runs. Since the source network cannot be established, discard or immediately stale the legacy snapshot instead of copying it into an arbitrary network.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
All four prior safety findings are fixed, and focused DPNS tests, formatting, and clippy pass. Four blocking correctness issues remain in cold-cache recovery, scheduled-status reporting, schedule-edit result routing, and unavailable preflight handling.
Validated blockers were found in the Codex precheck. Sonnet 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— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 4 blocking
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 `src/ui/masternodes/voting_center.rs`:
- [BLOCKING] src/ui/masternodes/voting_center.rs:62-68: Voting Center never adopts refreshed contests
The center snapshots `ongoing_contested_names()` only during construction, and this result handler only updates the submitted operation ID. If the contest cache is initially empty, `selected_current_states()` is also empty, so no automatic query runs; even if a later `RefreshedDpnsContests` result arrives, `self.contests` remains empty. Step 2 consequently has neither contests nor an in-center refresh action. Reload the contest list when refresh completes and provide a refresh action for the empty state.
In `src/context/contested_names_db.rs`:
- [BLOCKING] src/context/contested_names_db.rs:245-248: Node cards treat terminal failed schedules as pending
The journal is authoritative, but this summary derives pending state from the legacy mirror's `executed_successfully` flag. That flag is set only after confirmation, so a scheduled target that reaches `Rejected` or `FailedBeforeSubmission` remains false even though its journal lock has been released and it will not be retried. When no contests are open, the node card therefore reports `Vote scheduled` for a terminal failure instead of directing the operator to the failed outcome.
In `src/app.rs`:
- [BLOCKING] src/app.rs:2085-2092: Navigation can strand a successful schedule edit on the temporary ID
A schedule replacement reuses the existing journal record, so the successful result can contain a different operation ID from the submitted draft. `DpnsVotingCenter::updated_submitted_operation` performs that required translation, but this branch sends the result only to the currently visible root screen. If the operator navigates away while the edit completes, the hidden Masternodes center retains the temporary ID; returning makes it poll a nonexistent record and display `Queuing votes…` indefinitely. Route correlated DPNS results to the Masternodes root screen regardless of visibility.
In `src/context/dpns_vote_operations.rs`:
- [BLOCKING] src/context/dpns_vote_operations.rs:528-530: Unavailable preflight state permanently locks an unsent vote
This helper only handles a target whose durable status is `Queued`, before `claim_dpns_vote_target` advances it to `Submitting` and before any nonce or network work. An unavailable current-vote refresh therefore cannot mean the vote was broadcast, yet the code persists `Unconfirmed`. Execution then reloads the operation, finds no queued work, returns success, and allows a scheduled sweep to emit `ScheduledVoteSweepCompleted` and retire its preserved cutoff. Reconciliation leaves an absent or mismatched vote unconfirmed, permanently locking a target DET never submitted. Restore scheduled targets to `Scheduled` or mark immediate targets `FailedBeforeSubmission`, and propagate the preflight failure so deferred schedules remain retryable.
Define authoritative vote state, a shared quick and bulk composer, durable operation coordination, safe post-broadcast recovery, and scheduled-vote consolidation before implementation begins. Co-Authored-By: Codex GPT-5 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Load proved current votes, coordinate durable target-level operations, serialize same-node submissions, reconcile ambiguous results without rebroadcasting, and route quick, bulk, and scheduled voting through one reviewed Voting Center. Co-Authored-By: Codex GPT-5 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Serialize nonce-consuming submissions across independent operations, make the operation journal network-qualified and fail-closed, recover interrupted work conservatively, require fresh proved state, and prevent scheduled terminal outcomes from being rebroadcast. Co-Authored-By: Codex GPT-5 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Carry exact DPNS vote choices through the one-shot operator route so bulk review does not silently replace them with defaults. Co-Authored-By: Codex GPT-5 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Preserve exact routed choices, require explicit node selection, keep healthy targets usable when another node is blocked, render proved-state uncertainty honestly, and consolidate scheduled-vote management under Masternodes with guarded actions and readable review details. Co-Authored-By: Codex GPT-5 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Render and manage scheduled votes from the durable operation journal, retain exact operation correlation for recovery, prevent implicit schedule replacement, report complete mixed outcomes, and preserve explicit unavailable states and target-identifying confirmations. Co-Authored-By: Codex GPT-5 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Durably claim due schedules, preserve ambiguous reconciliation locks, order journal writes before compatibility mirrors, and correlate results and pre-journal failures by network and operation. Co-Authored-By: Codex GPT-5 <noreply@openai.com> <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub>
Show a recovery action when no masternodes are available for the shared voting composer. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Co-Authored-By: OpenAI GPT-5 Codex <noreply@openai.com>
Keep scheduled vote recovery inside the sweep error boundary so every failure clears the per-network in-progress latch through the typed handler. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Re-drive already queued schedules and restore unavailable scheduled targets before propagating the pre-submission failure. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Refresh hidden voting state from correlated results, expose contest refresh, and derive node schedule outcomes from the operation journal. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Claudius-Maginificent
left a comment
Claudius-Maginificent
left a comment
There was a problem hiding this comment.
Consolidated code review — DPNS voting unification (4-agent sweep: security-engineer, project-reviewer, qa-engineer, codex-sol)
Overall assessment. A well-architected, genuinely careful feature whose central safety claim holds — the broadcast gate, lock ordering, crash-recovery direction and fail-closed journal reads are all correct, and no double-broadcast path exists. It is undermined by one systemic state-machine defect family: Unconfirmed is written at points that prove nothing was broadcast, and no code path can ever release it, so routine transient failures can permanently strand a node × contest target with UI copy that promises a recovery the backend cannot deliver. Unconfirmed.holds_lock() is true, Review again is gated on statuses Unconfirmed never reaches, and classify_reconciled_vote (mod.rs:92-97) still has no path to NotApplied — so once a target lands there, it stays there for the life of the contest.
Two further material, independent defects: "Cast Now" is a silent no-op that reports success (the Scheduled → Queued transition is applied to a detached in-memory copy and discarded by the journal reload before dispatch), and cancellation can report success after losing a race to the due-vote sweep, letting an "cancelled" vote proceed to irreversible on-chain submission.
No CRITICAL/HIGH findings — everything is bounded to the DPNS voting subsystem, MEDIUM severity ceiling. 9 of 33 findings are blocking (violate the PR's own shipped requirements or regress base behaviour); the rest are real but non-blocking (per-frame KV scans in the render path, unbounded journal growth, dead fields, tautological tests, i18n fragment concatenation).
Process notes for this review pass
This report was built against 9dc8820 and the PR head has since moved to 541323d3 ("fix: stabilize DPNS voting and SPV reconnect"), touching app.rs, backend_task/error.rs, context/dpns_vote_operations.rs, context/wallet_lifecycle/{spv,tests}.rs, ui/masternodes/{list_screen,voting_center}.rs, wallet_backend/mod.rs, tests/backend-e2e/spv_reconnect.rs. Before posting, every finding whose location fell in a touched file was re-verified against 541323d3 (git diff 9dc8820...541323d3 -- <file> + full-file read):
- 1 finding dropped as already fixed and already threaded: the never-broadcast-target-locks-forever defect in
revalidate_queued_dpns_vote_target(originally reported here as SEC-001) is fixed by the newapply_queued_vote_preflighthelper (Checking/Unavailableon aQueuedtarget now →FailedBeforeSubmission, lock released, covered by a new test). This exact defect was independently caught bythepastaclaw's review (comment3598978018,dpns_vote_operations.rsorigin-line 530) and already confirmed fixed there by reply — replied there to note the residual gap instead of reposting. The systemic root cause survives:classify_reconciled_vote(mod.rs:92-97) still never producesNotApplied, andrecover_interrupted_dpns_vote_operationsstill maps a crash duringSubmitting(i.e. before broadcast) to a permanentUnconfirmed— see the PROJ-001 comment below. - 8 findings had their location shifted (code unchanged, only line numbers moved due to unrelated insertions/refactors in the new commit) — corrected line numbers are posted below, each flagged with a drift note.
- 2 findings excluded from inline posting:
CODE-014(a test-count discrepancy noted during methodology, no source location) andCALL-004(a walk-scope transparency note, "no action") — both informational, no actionable code location. - Dedup against existing threads: cross-checked all 33 surviving findings against the 8 open
thepastaclawreview comments on this PR. No genuine duplicates found beyond the SEC-001/dpns_vote_operations.rs:530 case above —thepastaclaw's vote-classification finding (3596869091) describes the opposite failure direction from this report'sCALL-001and doesn't match the current code structure (its citedupdate_dpns_vote_operationcall path has zero callers in this tree), and its cancellation-race finding (3596869097) describes a different mechanism than this report'sCODE-005(a discarded-return-value bug, directly verified still present) — both are posted below as independent findings; a human may want to reconcile the two cancellation-race write-ups.
30 findings are posted as inline comments below.
🤖 Co-authored by Claudius the Magnificent AI Agent
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 541323d, three prior findings are fixed and the scheduled-status finding remains valid. All six supplied Codex findings are confirmed as blocking correctness issues affecting recovery, scheduled execution, cancellation, and truthful UI state. The focused DPNS suite passed 26 tests and the Masternodes kittest subset passed 16 tests, but several tests explicitly preserve the problematic state transitions.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 6 blocking
🤖 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 `src/context/contested_names_db.rs`:
- [BLOCKING] src/context/contested_names_db.rs:245-248: Node cards treat terminal failed schedules as pending
This summary derives pending state from the legacy mirror's `executed_successfully` flag even though the journal is authoritative. Scheduled targets that become `Rejected` or `FailedBeforeSubmission` are not marked executed, so their legacy rows remain false after the journal releases the target lock and stops retrying them. When no contests are open, the node card consequently reports `Vote scheduled` for a terminal failure instead of exposing that the vote needs attention.
In `src/context/dpns_vote_operations.rs`:
- [BLOCKING] src/context/dpns_vote_operations.rs:552-557: A crash before broadcast can permanently lock an unsent vote
`claim_dpns_vote_target` persists `Submitting` before `submit_dpns_vote` performs the poll query, nonce lookup, signing, or broadcast. A crash during that pre-broadcast window reaches this recovery branch and becomes `Unconfirmed`, which retains the target lock. Reconciliation only changes an exact matching proved vote to `Confirmed`; an absent or different vote leaves the target unconfirmed indefinitely. Recovery needs a durable phase boundary that distinguishes preparation from a transition that may actually have been broadcast.
- [BLOCKING] src/context/dpns_vote_operations.rs:599-604: Cancellation silently succeeds after execution has already claimed the vote
`cancel_scheduled_target` returns false when the due-vote sweep has already transitioned the target from `Scheduled` to `Queued`, but this wrapper discards that result and returns success. The task handler then deletes the legacy schedule row and returns `Refresh` as though cancellation succeeded, while the journaled vote continues toward submission. Propagate the failed conditional cancellation and remove the legacy row only after the journal cancellation actually wins.
- [BLOCKING] src/context/dpns_vote_operations.rs:277-281: Cancellation is recorded as a proved NotApplied outcome
The requirements reserve `NotApplied` for definitive post-broadcast reconciliation, but the only current writers of that status are the scheduled-cancellation helpers. The Scheduled tab special-cases the status as `Cancelled`, while Voting activity calls the same target `Not applied` and the operation detail claims DET proved the vote was not applied. Add a distinct cancellation status and reserve `NotApplied` for an authoritative reconciliation result.
In `src/backend_task/contested_names/mod.rs`:
- [BLOCKING] src/backend_task/contested_names/mod.rs:740-764: A failed scheduled sweep leaves queued votes inert until restart
The sweep durably changes every due target from `Scheduled` to `Queued` before loading voting identities. If that load fails, the task returns with those targets still queued. The one-time recovery latch was already completed before entering the sweep, while subsequent sweeps reconcile only `Unconfirmed` targets and claim only `Scheduled` targets. The UI then displays the queued vote as submitting with cancellation disabled, but no running-process path resumes it; restarting is required.
In `src/ui/masternodes/detail_screen.rs`:
- [BLOCKING] src/ui/masternodes/detail_screen.rs:294-296: The masternode detail view fails open when contest state cannot be read
The list screen maps a failed summary read to `MasternodeContestSummary::unavailable()`, but the detail constructor and `refresh_contests` use `unwrap_or_default()`. That default reports `Ready` with zero open contests, while `load_open_contests` separately converts its read failure to an empty list. The detail page therefore renders a zero-count header and states that no contests are open when storage is actually unavailable, potentially hiding a vote that needs action. Preserve explicit unavailable state for both the summary and contest list.
541323d to
1f186ea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/wallet_backend/mod.rs (1)
2570-2588: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOpen the lock file outside the polling loop.
Opening the lock file inside the polling loop executes a blocking file-system operation on the async executor every 10 milliseconds. Moving the
OpenOptions::new()...open()call outside the loop avoids repeatedly blocking the worker thread while waiting for the release barrier.♻️ Proposed refactor
- loop { - let lock_file = match std::fs::OpenOptions::new() - .read(true) - .write(true) - .create(true) - .truncate(false) - .open(&lock_path) - { - Ok(file) => file, - Err(error) => { - tracing::debug!( - error = %error, - lock_path = %lock_path.display(), - "SPV lock file not openable during release barrier; treating the data directory as unlocked" - ); - return Ok(()); - } - }; + let lock_file = match std::fs::OpenOptions::new() + .read(true) + .write(true) + .create(true) + .truncate(false) + .open(&lock_path) + { + Ok(file) => file, + Err(error) => { + tracing::debug!( + error = %error, + lock_path = %lock_path.display(), + "SPV lock file not openable during release barrier; treating the data directory as unlocked" + ); + return Ok(()); + } + }; + + loop { match lock_file.try_lock() {🤖 Prompt for 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. In `@src/wallet_backend/mod.rs` around lines 2570 - 2588, Move the lock-file OpenOptions/open call out of the polling loop in the teardown logic, preserving its existing error handling and early-proceed behavior. Store the successfully opened file before entering the loop, then reuse it for each lock-status poll so the loop only performs the nonblocking wait/check operation.
🤖 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 `@docs/ai-design/2026-07-16-dpns-voting-experience/01-requirements.md`:
- Around line 3-5: Update the Status section in the requirements document to
identify the specification as implemented or shipped, and remove the statement
that no implementation is authorized. Preserve the surrounding requirements
content.
In `@src/backend_task/contested_names/vote_on_dpns_name.rs`:
- Around line 208-221: Update the mark_dpns_vote_broadcast call in
classify_vote_attempt so its error is converted into
DpnsVoteAttempt::Unconfirmed with the original error attached, rather than
propagated as Err. Preserve the successful path into Vote::wait_for_response and
ensure broadcast-success failures remain eligible for reconciliation instead of
retry.
In `@src/context/dpns_vote_state.rs`:
- Around line 213-225: The cache_confirmed_dpns_vote method must not set the
aggregate snapshot’s available flag or updated_at when confirming a single poll.
Replace this partial update with a full proved-state refresh after confirmation,
or use per-poll availability/freshness tracking so only vote_poll_id is updated
without promoting unrelated cached votes.
In `@src/model/dpns_voting.rs`:
- Around line 104-146: Ambiguous Released votes must not become immediately
resubmittable. In src/model/dpns_voting.rs lines 104-146, update
DpnsVoteTargetStatus::is_reviewable or lock handling so Released remains
non-reviewable until authoritative non-application is proven. Reconcile the
behavior in docs/ai-design/2026-07-16-dpns-voting-experience/01-requirements.md
lines 143-146, 02-ux-spec.md lines 184-222, 03-test-case-spec.md lines 56-66,
04-development-plan.md lines 111-116 and 172-173, and docs/user-stories.md lines
738-742: constrain manual release, define Released/Cancelled safely, add
duplicate-submission coverage, and remove claims that retry is safe while the
outcome is unknown.
In `@src/ui/masternodes/detail_screen.rs`:
- Around line 865-869: Update the contest transformation around
dpns_vote_poll_id so failures no longer discard the contest via filter_map.
Construct an unavailable ContestVoteRow for contests without a poll ID, preserve
the existing refresh action, and provide a user-facing error stating what
happened and how the user can refresh or retry.
In `@src/ui/masternodes/voting_center.rs`:
- Around line 684-686: Update the Err branch in the voting-center journal
handling to stop rendering error.to_string() directly. Show a brief actionable
user-facing message via MessageBanner, and attach the original error as
technical details using BannerHandle::with_details().
- Around line 164-181: Update for_scheduled_edit and the related build_review
flow so editing a scheduled vote preserves the original absolute timestamp
instead of recalculating its offset from a fresh Utc::now(). Store the timestamp
or freeze the offset anchor when initializing node_timing, and ensure
build_review reuses that preserved value without schedule drift during review.
- Around line 125-150: Update DpnsVoteWorkspace::new to handle failures from
load_local_voting_identities and ongoing_contested_names through MessageBanner
rather than unwrap_or_default. Preserve the error details, attach a
MessageBanner for each failed store read, and retain an explicit degraded-state
flag while returning a degraded Self instead of treating failures as valid empty
collections.
---
Nitpick comments:
In `@src/wallet_backend/mod.rs`:
- Around line 2570-2588: Move the lock-file OpenOptions/open call out of the
polling loop in the teardown logic, preserving its existing error handling and
early-proceed behavior. Store the successfully opened file before entering the
loop, then reuse it for each lock-status poll so the loop only performs the
nonblocking wait/check operation.
🪄 Autofix (Beta)
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
Run ID: 9541ec92-bb24-4b54-b679-9859f626a7a9
📒 Files selected for processing (32)
docs/ai-design/2026-07-16-dpns-voting-experience/01-requirements.mddocs/ai-design/2026-07-16-dpns-voting-experience/02-ux-spec.mddocs/ai-design/2026-07-16-dpns-voting-experience/03-test-case-spec.mddocs/ai-design/2026-07-16-dpns-voting-experience/04-development-plan.mddocs/user-stories.mdsrc/app.rssrc/backend_task/contested_names/mod.rssrc/backend_task/contested_names/query_dpns_contested_resources.rssrc/backend_task/contested_names/vote_on_dpns_name.rssrc/backend_task/error.rssrc/backend_task/mod.rssrc/context/contested_names_db.rssrc/context/dpns_vote_operations.rssrc/context/dpns_vote_state.rssrc/context/mod.rssrc/context/wallet_lifecycle/spv.rssrc/context/wallet_lifecycle/tests.rssrc/model/contested_name.rssrc/model/dpns_voting.rssrc/model/mod.rssrc/ui/components/dpns_subscreen_chooser_panel.rssrc/ui/dpns/dpns_contested_names_screen.rssrc/ui/masternodes/card.rssrc/ui/masternodes/detail_screen.rssrc/ui/masternodes/list_screen.rssrc/ui/masternodes/mod.rssrc/ui/masternodes/voting_center.rssrc/ui/state/dpns_vote_workspace.rssrc/ui/state/mod.rssrc/wallet_backend/mod.rstests/backend-e2e/spv_reconnect.rstests/kittest/masternode_tab.rs
💤 Files with no reviewable changes (1)
- src/ui/components/dpns_subscreen_chooser_panel.rs
|
@/tmp/pr901-comment.md |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/dpns/dpns_contested_names_screen.rs (1)
998-1002: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the refresh state when an error clears the banner.
A failed refresh leaves
refreshing_statusasRefreshing, so the empty-state Refresh button ignores subsequent retries.Proposed fix
if matches!(message_type, MessageType::Error | MessageType::Warning) { self.refresh_banner.take_and_clear(); + self.refreshing_status = RefreshingStatus::NotRefreshing; }🤖 Prompt for 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. In `@src/ui/dpns/dpns_contested_names_screen.rs` around lines 998 - 1002, Update display_message so when an Error message clears refresh_banner, it also resets refreshing_status from Refreshing to its idle/non-refreshing state, allowing the empty-state Refresh button to be used for subsequent retries. Preserve the existing warning behavior and banner-clearing side effect.
🤖 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 `@src/backend_task/contested_names/mod.rs`:
- Line 650: Update the recovery sweep around scheduled_vote_is_resumable so
targets left in Submitting or Confirming after a terminal journal persistence
failure are recovered without requiring restart. Retry terminal persistence or
add ownership-aware recovery for these orphaned in-flight targets, while
preserving existing Queued, Unconfirmed, and resumable-vote handling; add
fault-injection coverage for failures after claim and broadcast.
---
Outside diff comments:
In `@src/ui/dpns/dpns_contested_names_screen.rs`:
- Around line 998-1002: Update display_message so when an Error message clears
refresh_banner, it also resets refreshing_status from Refreshing to its
idle/non-refreshing state, allowing the empty-state Refresh button to be used
for subsequent retries. Preserve the existing warning behavior and
banner-clearing side effect.
🪄 Autofix (Beta)
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
Run ID: 33f30cbe-7d0b-4451-8e3b-237326ef4929
📒 Files selected for processing (12)
docs/ai-design/2026-07-16-dpns-voting-experience/01-requirements.mdsrc/app.rssrc/backend_task/contested_names/mod.rssrc/backend_task/contested_names/vote_on_dpns_name.rssrc/backend_task/mod.rssrc/context/dpns_vote_operations.rssrc/context/dpns_vote_state.rssrc/model/dpns_voting.rssrc/ui/dpns/dpns_contested_names_screen.rssrc/ui/masternodes/detail_screen.rssrc/ui/masternodes/list_screen.rssrc/ui/masternodes/voting_center.rs
💤 Files with no reviewable changes (1)
- src/backend_task/mod.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/ai-design/2026-07-16-dpns-voting-experience/01-requirements.md
- src/backend_task/contested_names/vote_on_dpns_name.rs
- src/context/dpns_vote_state.rs
- src/app.rs
- src/model/dpns_voting.rs
- src/ui/masternodes/voting_center.rs
- src/ui/masternodes/detail_screen.rs
- src/ui/masternodes/list_screen.rs
Build a MasternodeContestSummary struct literal directly in the header test instead of default-then-reassign, which the strict-warnings lint gate rejects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/backend_task/contested_names/vote_on_dpns_name.rs (1)
200-207: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftPersist the durable broadcast state before the network call.
Currently,
mark_dpns_vote_broadcastis called afterstate_transition.broadcast(...).await. If the application crashes during the broadcast RPC, the durable state remainsSubmitting. On recovery, an interruptedSubmittingphase is reverted toFailedBeforeSubmission, which releases the target lock and allows the user to cast a duplicate vote that may have already reached the network.To prevent crash-induced duplicate submissions, the operation must be durably advanced to the broadcast phase (
Confirming) before yielding to the RPC. If the RPC fails, the caller's evaluation of the returnedDpnsVoteAttemptvariant will correctly update the durable state to its final outcome.🔒️ Proposed fix to persist state before broadcast
- if let Err(error) = state_transition.broadcast(sdk, Some(settings)).await { - return Ok(classify_broadcast_error(error)); - } if let Err(attempt) = classify_broadcast_journal_result(self.mark_dpns_vote_broadcast(operation_id, key)) { return Ok(attempt); } + + if let Err(error) = state_transition.broadcast(sdk, Some(settings)).await { + return Ok(classify_broadcast_error(error)); + }🤖 Prompt for 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. In `@src/backend_task/contested_names/vote_on_dpns_name.rs` around lines 200 - 207, Update the flow around mark_dpns_vote_broadcast and state_transition.broadcast so the durable broadcast transition to Confirming is completed and classified before awaiting the network RPC. Preserve the existing broadcast error classification afterward, allowing returned DpnsVoteAttempt outcomes to finalize durable state when the RPC fails.
🤖 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.
Duplicate comments:
In `@src/backend_task/contested_names/vote_on_dpns_name.rs`:
- Around line 200-207: Update the flow around mark_dpns_vote_broadcast and
state_transition.broadcast so the durable broadcast transition to Confirming is
completed and classified before awaiting the network RPC. Preserve the existing
broadcast error classification afterward, allowing returned DpnsVoteAttempt
outcomes to finalize durable state when the RPC fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79948085-afb1-45bd-b9e0-d46e560587c0
📒 Files selected for processing (11)
src/app.rssrc/backend_task/contested_names/mod.rssrc/backend_task/contested_names/vote_on_dpns_name.rssrc/backend_task/error.rssrc/context/contested_names_db.rssrc/context/dpns_vote_operations.rssrc/context/identity_db.rssrc/model/dpns_voting.rssrc/ui/masternodes/detail_screen.rssrc/ui/masternodes/list_screen.rssrc/ui/masternodes/voting_center.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- src/backend_task/error.rs
- src/context/contested_names_db.rs
- src/model/dpns_voting.rs
- src/context/dpns_vote_operations.rs
- src/ui/masternodes/detail_screen.rs
- src/app.rs
- src/ui/masternodes/voting_center.rs
- src/ui/masternodes/list_screen.rs
- src/backend_task/contested_names/mod.rs
|
Pushed fixes for all outstanding review findings:
Commit: 🤖 Co-authored by Claudius the Magnificent AI Agent |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 10a3090, all seven prior findings are fixed in their original form. The latest delta introduces two blocking regressions: the durable ambiguous-submission boundary is recorded only after broadcasting, and terminal immediate or mixed-timing operations can no longer be pruned from the journal.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 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 `src/backend_task/contested_names/vote_on_dpns_name.rs`:
- [BLOCKING] src/backend_task/contested_names/vote_on_dpns_name.rs:200-204: Record the ambiguous submission boundary before broadcasting
The target remains durably marked `Submitting` throughout `state_transition.broadcast(...).await` and advances to `Confirming` only after the call returns success. The SDK broadcast path executes the network request with retries, so a process interruption or transport failure can occur after Platform receives the transition but before DET records that possibility. Recovery then treats the target as definitely unsent: immediate targets become `FailedBeforeSubmission`, while scheduled targets return to `Scheduled`. This releases or recycles a target whose vote may already have been accepted, allowing a duplicate broadcast. Persist the ambiguous broadcast phase immediately before entering the network call, after all local preflight, nonce, signing, structural validation, and request-construction checks have succeeded.
In `src/context/dpns_vote_operations.rs`:
- [BLOCKING] src/context/dpns_vote_operations.rs:319-331: Terminal immediate and mixed operations are never pruned
`prune_terminal_operations` now requires every target in a complete operation to use `VoteTiming::Scheduled` and to have a matching successfully removed legacy schedule. Every terminal immediate-vote operation and every operation containing both immediate and scheduled targets therefore fails the predicate permanently. This is the journal's only deletion path, so those records and their operation-index entries accumulate without bound and are reread whenever operation snapshots are refreshed. The legacy-row safeguard should preserve failed or cancelled scheduled targets without making terminal targets that have no legacy mirror permanently ineligible for pruning.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claudius reviews the unified DPNS voting subsystem
I braced myself for a fund-adjacent voting rewrite with a 5-agent verdict of "Critical effort," and I'll admit it: the architecture is genuinely good. Duplicate-broadcast prevention, per-voter serialization, the process-mutex + durable-status-as-CAS crash atomicity, network scoping, secret hygiene, no mutex held across an await — all verified sound by an independent pass. No CRITICAL, no HIGH, no double-vote, no leaked secrets. Credit where due.
Four reviewers (security, project-consistency, and two correctness reviewers split across backend and UI) swept ~8,300 lines over 31 files. The findings cluster into two themes:
1. A fail-closed path that isn't quarantined to the offending row. The flagship finding: one cross-network legacy vote row makes migrate_legacy_operations return before its own index write-back, so the poison id is never drained and the scheduled-vote sweep is permanently bricked on the affected network — complete with a misleading "try again in a moment" banner that recurs once a minute for the life of the app. That one I'd fix before merge.
2. An incomplete migration. The new Voting Center went in; the old bulk-schedule popup (~560 lines) and the legacy scheduled table (~250 lines) did not come out — despite the development plan's Workstream D explicitly calling for their removal. The popup is a re-armable parallel voting path, which is the sort of thing that comes back to haunt you.
Posted inline (4 × MEDIUM)
- Migration poison → permanent scheduled-sweep failure (
dpns_vote_operations.rs) — the one to fix before merge NotAppliedunreachable → a proved-different vote staysUnconfirmedand locked forever, with dead retry UI (contested_names/mod.rs)- Hot-path SQLite read every frame per (voter × contest), contradicting the PR's own "off the hot path" claim (
voting_center.rs) - ~800 lines of dead legacy voting UI left behind by the migration (
dpns_contested_names_screen.rs)
Also in the full report (4 × LOW, not posted inline)
- Unbounded journal growth (completed "Now" votes never pruned)
- Stale per-contest selections can bypass openness filtering and reach Review
DpnsVoteOutcome::transition_hashalwaysNone→ dead "Copy transition hash" buttonContestedName::my_votesalways empty in production, covered only by tautological tests
No CRITICAL or HIGH. Mergeable once the migration-poison path is quarantined; the rest can be batched. A consolidated severity-ranked report (HTML + JSON) was generated alongside this review.
🤖 Co-authored by Claudius the Magnificent AI Agent
github-actions
Bot
removed
the
claudius-review
…e-vote pruning The durable phase-boundary marker crossed into the ambiguous "broadcast may have been sent" state only after broadcast() returned, missing the actual danger window during the RPC call itself (which retries internally). Move the mark-broadcast journal write before the broadcast call, fail closed if it can't be recorded, and reclassify broadcast-call errors: cause-less failures stay retryable (FailedBeforeSubmission), everything past the boundary is Unconfirmed or Rejected. prune_terminal_operations required every target in a complete operation to be Scheduled and legacy-row-cleared, so any immediate- vote-only or mixed operation could never be pruned (regression from this session's earlier legacy-row safeguard). Check per-target instead: immediate targets never needed a legacy row and stay prunable; scheduled targets still require their row confirmed removed. Verified: cargo test vote_on_dpns_name (11 passed), cargo test dpns_vote_operations (26 passed), clippy clean with -D warnings. Co-Authored-By: Codex Sol <noreply@openai.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Masternode detail no longer casts votes inline; voting now lives on the redesigned DPNS Active contests screen (cards, batch, scheduling). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Add Scheduled votes as a persistent 4th DPNS nav tab; it was previously reachable only via a one-shot post-schedule-action link. - Rebuild the Review and cast sheet: candidate names instead of raw Base58 IDs, a Cast now / Schedule for later segmented control instead of the legacy per-identity ComboBox, actionable "Open Masternodes" guidance instead of the stale Identities-screen instruction, and a future-time validation warning for scheduling. - Add the Active-contests summary strip and port homograph-safe filter normalization + explanatory tooltip to the Active-contests filter. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reload the screen's vote-state snapshot from the confirmed vote cache when a voting operation completes, so a changed vote renders immediately instead of showing the pre-change choice until the next full contest refresh. Co-Authored-By: OpenAI Codex <noreply@openai.com>
…d Unconfirmed votes - Migrate valid legacy operations while parking foreign-network rows outside the active legacy index so scheduled sweeps keep making progress. - Corroborate a proved-different vote across two reconciliation passes before marking it NotApplied and releasing the target lock. Fixes PR review threads CMT-4 and CMT-5 (findings by claude[bot]). Co-Authored-By: OpenAI Codex <noreply@openai.com>
…them migrate_legacy_operations previously overwrote the legacy index with an always-empty retained list, silently discarding foreign-network rows instead of keeping them for a subsequent correct-network migration pass. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude GPT-5 Codex <noreply@anthropic.com>
|
Ran a full re-verification pass on the 7 threads that were showing "Unresolved" (independent code reads + tests, not trusting prior diffs):
New regression tests added for both real fixes; 🤖 Co-authored by Claudius the Magnificent AI Agent |
lklimek
added
the
claudius-review
There was a problem hiding this comment.
Claudius reviews PR #901 — feat(dpns): unify safe masternode voting operations
I dispatched four specialists across ~9,200 changed lines in 29 files, then verified every prior-round concern against the current HEAD (76c0ed71) rather than taking the changelog's word for it. Credit where it's due: all three high-priority findings from the last round are genuinely fixed — the fail-closed legacy-journal blast radius now quarantines foreign rows, NotApplied is reachable and releases its lock, and the per-frame render-path DB read is gone in favour of an in-memory snapshot. Double-broadcast safety, network isolation, typed errors, and secret hygiene all hold up. This is a clean, well-tested rewrite, not a broken one.
That said, three MEDIUM items are worth your attention before merge (inline):
- CODE-001 (blocking) —
masternode_contest_summarystill loops the singular per-contest vote-state read on every Masternodes-list reload, reintroducing the exact O(open-contests)-reads pattern that this PR's own requirement VOTE-NFR-007 forbids and that you eliminated everywhere else. The regression test guards the primitive, not this consumer. - SEC-001 — an unreadable journal record fail-closes read-only DPNS browsing (not just voting) with no in-app recovery, and the error copy promises a fix action the UI doesn't offer. Quarantine the bad row the way you already do for cross-network legacy rows.
- CODE-002 —
is_open_for_voteradvertises voter-scoped semantics but ignores itsvoter_idargument, backed by two tautological tests that assert on a field the function never reads.
No CRITICAL or HIGH findings. The remaining 9 LOW / 1 INFO items (vestigial my_votes / transition_hash fields, a missing post-broadcast fault-injection case, a tautological scheduling test, a stale requirements-doc status banner, minor convention nits) are non-blocking cleanups — full breakdown in the review report. Withholding approval solely on the three MEDIUM threads above; clear those and this is ready to ship.
🤖 Co-authored by Claudius the Magnificent AI Agent
| pub fn is_open_for_voter(&self, voter_id: &Identifier) -> bool { | ||
| self.state.state_is_votable() && !self.my_votes.keys().any(|(id, _, _)| id == voter_id) | ||
| /// Whether the contest still accepts this node's initial vote or a change. | ||
| pub fn is_open_for_voter(&self, _voter_id: &Identifier) -> bool { |
There was a problem hiding this comment.
CODE-002 · MEDIUM · misleading API + tautological tests
is_open_for_voter(&self, _voter_id: &Identifier) takes a voter id and its name and doc advertise voter-scoped semantics — but the parameter is discarded (note the tell-tale _ prefix) and the my_votes field it would consult is never populated in production. So the function does a plain contest-level open/closed check while presenting itself as per-voter logic. That's a latent trap: a future caller will reasonably trust the name and pass a voter id expecting it to matter.
Worse, the two tests at lines ~131–152 insert into my_votes and then assert is_open_for_voter — but since the function ignores that field, the assertions pass no matter what you put in it. They're tautological: green, and testing nothing about the voter-scoped behavior they appear to cover. This also collides with checklist item #1 "Current-vote proof results are voter-scoped."
Recommendation: either implement the voter-scoped logic against a genuinely-populated source, or drop the voter_id parameter and rename to reflect the contest-level check it actually performs — then replace the tautological tests with ones that exercise the real decision. (Related: my_votes itself is vestigial — see CODE-003.)
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Assessed as valid, but not auto-fixed this pass — the finding is accurate, yet the remedy is a design/API judgment call that belongs to the author.
I verified the claim independently: my_votes is written only inside #[cfg(test)] (two inserts in this file’s test module), never in production; and is_open_for_voter’s sole production caller is masternode_contest_summary, which only ever needs the contest-level open/closed check. So the _voter_id parameter is genuinely dead and the two tests at ~131–152 that insert into my_votes and then assert is_open_for_voter are indeed tautological — they pass regardless of the field.
Why I am not auto-fixing: your own recommendation is a binary choice — implement genuine voter-scoped logic (a feature/design decision) or drop the voter_id param and rename the public model method (an API-shape change). Either option is the author’s call, and the rename is entangled with the related my_votes-is-vestigial cleanup (CODE-003) — doing it piecemeal here would pre-empt that coordinated change. A test-only patch would also leave the misleading API you are actually objecting to in place, so it would not resolve this finding.
Recommend the author pick one direction and land it together with the my_votes removal.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
Investigated deeply, and the fix landed differently than the letter of this finding asked — explaining why.
is_open_for_voter's only production caller (masternode_contest_summary) genuinely doesn't need per-voter contest-openness beyond contest state — the real per-voter question ("has this masternode already voted, and how many times") is decided by Platform's actual vote-count rule: an initial vote plus 4 changes (5 total) per masternode-per-poll, enforced on-chain via MasternodeVotedTooManyTimesError. We traced whether DET could surface that count client-side to build genuine proactive voter-scoping — it can't, without upstream dashpay/platform changes across three crates (the vote-count field is discarded by Drive's own proof-verification function before it ever reaches a DAPI client; no SDK-facing query exposes it). We decided against pursuing that upstream work for this PR.
Instead, fixed in f60b7ae2: DET now maps Platform's MasternodeVotedTooManyTimesError rejection to a dedicated TaskError::MasternodeVoteLimitReached with a clear, actionable message ("This node has already cast the maximum N votes allowed for this contest and can't vote again. Choose another contest to vote on.") instead of the previous opaque generic-rejection fallback. This directly fixes the user-facing gap (nothing told users why their vote was rejected once they hit the limit) without inventing a proactive check we can't make trustworthy client-side.
is_open_for_voter's misleading _voter_id parameter and the dead my_votes field remain as-is — genuinely can't be made honestly voter-scoped without the upstream work, so renaming/dropping the parameter is the only remaining honest option there, and that's a separate decision we're holding off on for now. Two new regression tests (from_sdk_error_vote_limit_via_consensus_is_specific, from_sdk_error_vote_limit_via_broadcast_is_specific) with distinct count values to catch field-swap mistakes. cargo fmt/clippy --all-features --lib --tests -D warnings clean; backend_task::error module (113 tests) green.
github-actions
Bot
removed
the
claudius-review
…storage read per node `masternode_contest_summary` called the singular `dpns_current_vote_state` once per open contest, each re-reading the same per-node proved snapshot — O(open-contests) KV reads per node on every Masternodes-list refresh, the exact pattern VOTE-NFR-007 / VOTE-TC-005 forbid and that the batched `dpns_current_vote_states` primitive already exists to avoid. Resolve every open contest's vote-poll id up front, then read all states with a single `dpns_current_vote_states` call. A read failure degrades each contest to `Unavailable`, preserving the prior per-contest fallback semantics. Also collapses the duplicated `is_open_for_voter` filter into one pass. Adds a regression test that drives the masternode-card consumer (not just the primitive) and asserts exactly one snapshot read for five open contests. Co-Authored-By: Claude Opus <noreply@anthropic.com>
…unified # Conflicts: # src/backend_task/contested_names/query_dpns_contested_resources.rs # src/context/contested_names_db.rs
Map Platform's typed vote-limit rejection to a dedicated user-facing TaskError while preserving the SDK source for diagnostics. Co-Authored-By: Codex GPT-5.6 <noreply@openai.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head f60b7ae, four prior findings are fixed and the obsolete masternode-detail finding is outdated, but two prior scheduled-vote blockers remain. The Scheduled Votes screen also remains dependent on the non-authoritative legacy mirror, while its cleanup path has two history-retention issues and the new Active-contests renderer performs substantial model cloning and poll-ID recomputation on every frame.
Validated blockers were found in the Codex precheck. Sonnet 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— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 3 suggestion(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 `src/ui/dpns/dpns_contested_names_screen.rs`:
- [BLOCKING] src/ui/dpns/dpns_contested_names_screen.rs:1486-1492: Cancellation silently succeeds after execution has already claimed the vote
The Scheduled Votes Remove control dispatches `DeleteScheduledVote`, which deletes only the legacy mirror. It never calls `CancelScheduledDpnsVote`, so the journal-authoritative target remains `Scheduled` and the periodic sweep can still queue and submit it after the row disappears. The repaired conditional cancellation wrapper therefore does not protect this user-facing path, including the race where the sweep claims the target after the button is enabled but before the backend task runs. Route journal-backed rows through `CancelScheduledDpnsVote` and delete the mirror only after the guarded journal transition succeeds.
- [BLOCKING] src/ui/dpns/dpns_contested_names_screen.rs:2131-2157: Scheduled Votes still treats the legacy mirror as authoritative
Refresh rebuilds the Scheduled Votes table exclusively from `get_scheduled_votes()`, even though scheduling first commits the operation journal and explicitly tolerates failure to write that compatibility mirror. A successfully journaled schedule can therefore remain executable without appearing anywhere the operator can cancel it. The status overlay does not repair this mismatch: `DpnsVoteOperationSnapshot::target_status` indexes only lock-holding states, so a surviving mirror whose journal target is `Rejected`, `FailedBeforeSubmission`, or `Cancelled` is reconstructed as `NotStarted` and shown as Pending after refresh or restart. Build scheduled rows and their typed statuses from the authoritative operation journal, using the legacy table only for migration compatibility.
- [SUGGESTION] src/ui/dpns/dpns_contested_names_screen.rs:537-552: Active contests clone their complete nested model every frame
`render_active_contests` clones every `ContestedName` before filtering or grouping on each egui frame. Each clone copies candidate names, contestant vectors, and the per-contest vote map. Grouping then reconstructs and hashes a Platform poll identifier for every contest, and open cards repeat that computation in availability and proved-vote helpers. This leaves frame-rate heap work proportional to the complete contest payload despite the PR's goal of removing render-path freezes. Cache a lightweight card view model with its poll ID during construction or explicit refresh, or render through stable indices or references without cloning the nested data.
In `src/app.rs`:
- [BLOCKING] src/app.rs:2825-2837: Successful scheduled sweeps leave cached voting views stuck in progress
Sweep completion refreshes only `visible_screen_mut()`. If Active contests received `ScheduledVotesInProgress` and was then covered by a screen on the stack, this refresh targets the covering screen instead of the hidden DPNS root. The hidden-result router handles `DpnsVoteOperationUpdated` and `RefreshedDpnsContests`, but not `ScheduledVoteSweepCompleted`, while the sweep discards each operation's update result. An ordinary `PopScreen` performs no refresh, so it can reveal targets still cached as queued and disabled after execution has finished. Route sweep completion to the hidden Active-contests root or preserve and forward the individual operation updates.
In `src/context/dpns_vote_operations.rs`:
- [SUGGESTION] src/context/dpns_vote_operations.rs:323-332: The scheduled-vote clear action also deletes immediate voting history
The pruning predicate admits every complete operation containing only immediate targets because the non-scheduled side of the condition is always true. This behavior is explicitly exercised by the pruning test, but the only production caller is the Scheduled Votes action whose confirmation says it removes completed scheduled votes while pending votes remain. Activating it therefore also deletes unrelated immediate operations shown under Recent voting activity without telling the user. Either restrict this cleanup to operations associated with the removed scheduled rows or rename and relocate the action so its broader history deletion is explicit.
In `src/context/identity_db.rs`:
- [SUGGESTION] src/context/identity_db.rs:1147-1157: A failed prune leaves completed schedules permanently unprunable
Executed legacy rows are deleted before journal pruning, and the identities of those rows exist only in the local `removed_scheduled_votes` set. If voter-index cleanup, the dirty-marker write, operation deletion, or index rebuilding fails after a mirror was deleted, the method returns with that deletion already persisted. A retry cannot reconstruct the set because the mirror is gone, so the corresponding terminal scheduled operation no longer satisfies the pruning predicate and remains indefinitely. Derive retry eligibility from durable state or persist a cleanup marker before deleting the mirrors.
| ) { | ||
| self.scheduled_vote_recovery_last_attempt.remove(&network); | ||
| } | ||
| self.visible_screen_mut().refresh(); |
There was a problem hiding this comment.
🔴 Blocking: Successful scheduled sweeps leave cached voting views stuck in progress
Sweep completion refreshes only visible_screen_mut(). If Active contests received ScheduledVotesInProgress and was then covered by a screen on the stack, this refresh targets the covering screen instead of the hidden DPNS root. The hidden-result router handles DpnsVoteOperationUpdated and RefreshedDpnsContests, but not ScheduledVoteSweepCompleted, while the sweep discards each operation's update result. An ordinary PopScreen performs no refresh, so it can reveal targets still cached as queued and disabled after execution has finished. Route sweep completion to the hidden Active-contests root or preserve and forward the individual operation updates.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Successful scheduled sweeps leave cached voting views stuck in progress no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| @@ -1815,39 +2157,73 @@ impl ScreenLike for DPNSScreen { | |||
| .collect(); | |||
There was a problem hiding this comment.
🔴 Blocking: Scheduled Votes still treats the legacy mirror as authoritative
Refresh rebuilds the Scheduled Votes table exclusively from get_scheduled_votes(), even though scheduling first commits the operation journal and explicitly tolerates failure to write that compatibility mirror. A successfully journaled schedule can therefore remain executable without appearing anywhere the operator can cancel it. The status overlay does not repair this mismatch: DpnsVoteOperationSnapshot::target_status indexes only lock-holding states, so a surviving mirror whose journal target is Rejected, FailedBeforeSubmission, or Cancelled is reconstructed as NotStarted and shown as Pending after refresh or restart. Build scheduled rows and their typed statuses from the authoritative operation journal, using the legacy table only for migration compatibility.
source: ['codex']
There was a problem hiding this comment.
Resolved in aff18b2 — Scheduled Votes still treats the legacy mirror as authoritative no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| .filter(|operation| { | ||
| operation.is_complete() | ||
| && !operation.targets.is_empty() | ||
| && operation.targets.iter().all(|outcome| { | ||
| !matches!(outcome.target.timing, VoteTiming::Scheduled(_)) | ||
| || removed_scheduled_votes.contains(&( | ||
| outcome.target.key.voter_id.to_buffer(), | ||
| outcome.target.contested_name.clone(), | ||
| )) | ||
| }) |
There was a problem hiding this comment.
🟡 Suggestion: The scheduled-vote clear action also deletes immediate voting history
The pruning predicate admits every complete operation containing only immediate targets because the non-scheduled side of the condition is always true. This behavior is explicitly exercised by the pruning test, but the only production caller is the Scheduled Votes action whose confirmation says it removes completed scheduled votes while pending votes remain. Activating it therefore also deletes unrelated immediate operations shown under Recent voting activity without telling the user. Either restrict this cleanup to operations associated with the removed scheduled rows or rename and relocate the action so its broader history deletion is explicit.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — The scheduled-vote clear action also deletes immediate voting history no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| match kv.get::<StoredScheduledVote>(scope, &key) { | ||
| Ok(Some(stored)) if stored.executed_successfully => { | ||
| kv.delete(scope, &key).map_err(scheduled_vote_err)?; | ||
| removed_scheduled_votes.insert((stored.voter_id, stored.contested_name)); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| prune_vote_voter_if_empty(&kv, voter)?; | ||
| } | ||
| self.prune_terminal_dpns_vote_operations(&removed_scheduled_votes)?; |
There was a problem hiding this comment.
🟡 Suggestion: A failed prune leaves completed schedules permanently unprunable
Executed legacy rows are deleted before journal pruning, and the identities of those rows exist only in the local removed_scheduled_votes set. If voter-index cleanup, the dirty-marker write, operation deletion, or index rebuilding fails after a mirror was deleted, the method returns with that deletion already persisted. A retry cannot reconstruct the set because the mirror is gone, so the corresponding terminal scheduled operation no longer satisfies the pruning predicate and remains indefinitely. Derive retry eligibility from durable state or persist a cleanup marker before deleting the mirrors.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — A failed prune leaves completed schedules permanently unprunable no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| let filter = normalize_dpns_label(&self.active_filter_term); | ||
| let contests = self | ||
| .contested_names | ||
| .lock_recover() | ||
| .iter() | ||
| .cloned() | ||
| .collect::<Vec<_>>(); | ||
| let mut groups = [Vec::new(), Vec::new(), Vec::new()]; | ||
| for contest in contests { | ||
| let index = match self.contest_group(&contest) { | ||
| ActiveContestGroup::NeedsVote => 0, | ||
| ActiveContestGroup::Voted => 1, | ||
| ActiveContestGroup::NotVotable => 2, | ||
| }; | ||
| groups[index].push(contest); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Active contests clone their complete nested model every frame
render_active_contests clones every ContestedName before filtering or grouping on each egui frame. Each clone copies candidate names, contestant vectors, and the per-contest vote map. Grouping then reconstructs and hashes a Platform poll identifier for every contest, and open cards repeat that computation in availability and proved-vote helpers. This leaves frame-rate heap work proportional to the complete contest payload despite the PR's goal of removing render-path freezes. Cache a lightweight card view model with its poll ID during construction or explicit refresh, or render through stable indices or references without cloning the nested data.
source: ['codex']
There was a problem hiding this comment.
Resolved in aff18b2 — Active contests clone their complete nested model every frame no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head aff18b2, the latest merge adds no new in-scope DPNS defect, and five earlier lifecycle findings remain fixed or obsolete. Three blocking issues remain: schedule removal can leave executable journal targets behind, hidden Active-contests state can remain stale after a sweep, and Scheduled Votes still derives its rows from a best-effort compatibility mirror instead of the authoritative journal. Two non-blocking cleanup issues also remain.
Validated blockers were found in the Codex precheck. Sonnet 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— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 3 blocking | 🟡 2 suggestion(s)
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 `src/backend_task/contested_names/mod.rs`:
- [BLOCKING] src/backend_task/contested_names/mod.rs:221-256: Scheduled-vote removal can hide votes that remain executable
The row-level Remove action dispatches `DeleteScheduledVote`, whose handler deletes only the legacy compatibility row and never calls the guarded journal cancellation at lines 239-256. A journal target that is still `Scheduled` therefore remains eligible for the authoritative sweep after disappearing from the UI, including when the sweep queues it between rendering the enabled button and executing the task. Clear All has the related failure mode: it cancels only targets still in `Scheduled`, ignores targets already advanced to `Queued`, and then clears every mirror row, hiding any vote whose execution already started. Removal must be reported from the durable state machine, not independently from its compatibility mirror.
In `src/app.rs`:
- [BLOCKING] src/app.rs:2825-2837: Hidden Active-contests state can remain stale after a scheduled sweep
`cast_due_scheduled_votes` discards each operation's `DpnsVoteOperationUpdated` result at `src/backend_task/contested_names/mod.rs:913-924` and emits only `ScheduledVoteSweepCompleted`. This branch refreshes only the currently visible screen, while `dpns_result_needs_hidden_active_contests_route` does not route sweep completion to the hidden Active-contests root. If Active contests receives the preceding queued state and is then covered by a stack screen or another root before completion, its cached snapshot can retain a lock-holding status after the journal is terminal. A plain `PopScreen` does not refresh the revealed screen, so the user can return to stale, disabled voting controls.
In `src/context/dpns_vote_operations.rs`:
- [SUGGESTION] src/context/dpns_vote_operations.rs:316-332: Clearing completed schedules also deletes immediate voting history
For every immediate target, the predicate's non-scheduled branch is unconditionally true. The Scheduled Votes action labeled `Clear completed scheduled votes` therefore deletes every complete immediate-only operation as well as complete mixed operations whose scheduled rows were removed. Those records provide the Active-contests `Recent voting activity` history, so an action whose confirmation promises to remove completed scheduled votes silently removes unrelated immediate-vote outcomes. Restrict pruning to schedule-only records or rename and relocate the action so its broader history-cleanup behavior is explicit.
In `src/context/identity_db.rs`:
- [SUGGESTION] src/context/identity_db.rs:1143-1157: A partial cleanup can leave completed schedules permanently unprunable
Successfully deleted mirror rows are remembered only in the local `removed_scheduled_votes` set. If voter-index cleanup or journal pruning fails after a mirror deletion but before the corresponding journal record is deleted, the method returns with that deletion already durable. On retry, the missing mirror row can no longer repopulate the set, so the terminal scheduled operation no longer satisfies the pruning predicate and remains indefinitely unless the same node and contest later acquire another removable schedule. Derive pruning eligibility from durable journal state, persist cleanup intent before deleting mirrors, or make the related mutations transaction-like.
| @@ -168,7 +236,592 @@ impl AppContext { | |||
| self.delete_scheduled_vote(voter_id.as_slice(), &contested_name)?; | |||
| Ok(BackendTaskSuccessResult::Refresh) | |||
| } | |||
| ContestedResourceTask::CancelScheduledDpnsVote { | |||
| operation_id, | |||
| key, | |||
| contested_name, | |||
| } => { | |||
| self.cancel_scheduled_dpns_vote_target(operation_id, &key)?; | |||
| if let Err(error) = | |||
| self.delete_scheduled_vote(key.voter_id.as_slice(), &contested_name) | |||
| { | |||
| tracing::warn!( | |||
| ?error, | |||
| operation_id = %operation_id, | |||
| voter_id = %key.voter_id, | |||
| contested_name, | |||
| "Scheduled DPNS vote was cancelled but the legacy mirror could not be cleared" | |||
| ); | |||
| } | |||
| Ok(BackendTaskSuccessResult::Refresh) | |||
There was a problem hiding this comment.
🔴 Blocking: Scheduled-vote removal can hide votes that remain executable
The row-level Remove action dispatches DeleteScheduledVote, whose handler deletes only the legacy compatibility row and never calls the guarded journal cancellation at lines 239-256. A journal target that is still Scheduled therefore remains eligible for the authoritative sweep after disappearing from the UI, including when the sweep queues it between rendering the enabled button and executing the task. Clear All has the related failure mode: it cancels only targets still in Scheduled, ignores targets already advanced to Queued, and then clears every mirror row, hiding any vote whose execution already started. Removal must be reported from the durable state machine, not independently from its compatibility mirror.
source: ['codex']
| ) { | ||
| self.scheduled_vote_recovery_last_attempt.remove(&network); | ||
| } | ||
| self.visible_screen_mut().refresh(); |
There was a problem hiding this comment.
🔴 Blocking: Hidden Active-contests state can remain stale after a scheduled sweep
cast_due_scheduled_votes discards each operation's DpnsVoteOperationUpdated result at src/backend_task/contested_names/mod.rs:913-924 and emits only ScheduledVoteSweepCompleted. This branch refreshes only the currently visible screen, while dpns_result_needs_hidden_active_contests_route does not route sweep completion to the hidden Active-contests root. If Active contests receives the preceding queued state and is then covered by a stack screen or another root before completion, its cached snapshot can retain a lock-holding status after the journal is terminal. A plain PopScreen does not refresh the revealed screen, so the user can return to stale, disabled voting controls.
| ) { | |
| self.scheduled_vote_recovery_last_attempt.remove(&network); | |
| } | |
| self.visible_screen_mut().refresh(); | |
| fn dpns_result_needs_hidden_active_contests_route( | |
| selected: RootScreenType, | |
| screen_stack_is_empty: bool, | |
| result: &BackendTaskSuccessResult, | |
| ) -> bool { | |
| matches!( | |
| result, | |
| BackendTaskSuccessResult::DpnsVoteOperationUpdated { .. } | |
| | BackendTaskSuccessResult::RefreshedDpnsContests | |
| | BackendTaskSuccessResult::ScheduledVoteSweepCompleted { .. } | |
| ) && (selected != RootScreenType::RootScreenDPNSActiveContests || !screen_stack_is_empty) | |
| } |
source: ['codex']
| fn prune_terminal_operations( | ||
| kv: &DetKv, | ||
| network: Network, | ||
| removed_scheduled_votes: &BTreeSet<([u8; 32], String)>, | ||
| ) -> Result<usize, TaskError> { | ||
| let terminal_ids = load_operations(kv, network)? | ||
| .into_iter() | ||
| .filter(|operation| { | ||
| operation.is_complete() | ||
| && !operation.targets.is_empty() | ||
| && operation.targets.iter().all(|outcome| { | ||
| !matches!(outcome.target.timing, VoteTiming::Scheduled(_)) | ||
| || removed_scheduled_votes.contains(&( | ||
| outcome.target.key.voter_id.to_buffer(), | ||
| outcome.target.contested_name.clone(), | ||
| )) | ||
| }) |
There was a problem hiding this comment.
🟡 Suggestion: Clearing completed schedules also deletes immediate voting history
For every immediate target, the predicate's non-scheduled branch is unconditionally true. The Scheduled Votes action labeled Clear completed scheduled votes therefore deletes every complete immediate-only operation as well as complete mixed operations whose scheduled rows were removed. Those records provide the Active-contests Recent voting activity history, so an action whose confirmation promises to remove completed scheduled votes silently removes unrelated immediate-vote outcomes. Restrict pruning to schedule-only records or rename and relocate the action so its broader history-cleanup behavior is explicit.
source: ['codex']
| let mut removed_scheduled_votes = BTreeSet::new(); | ||
| for voter in &voters { | ||
| let scope = DetScope::Identity(voter); | ||
| for key in scheduled_vote_keys(&kv, voter)? { | ||
| match kv.get::<StoredScheduledVote>(scope, &key) { | ||
| Ok(Some(stored)) if stored.executed_successfully => { | ||
| kv.delete(scope, &key).map_err(scheduled_vote_err)?; | ||
| removed_scheduled_votes.insert((stored.voter_id, stored.contested_name)); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| prune_vote_voter_if_empty(&kv, voter)?; | ||
| } | ||
| self.prune_terminal_dpns_vote_operations(&removed_scheduled_votes)?; |
There was a problem hiding this comment.
🟡 Suggestion: A partial cleanup can leave completed schedules permanently unprunable
Successfully deleted mirror rows are remembered only in the local removed_scheduled_votes set. If voter-index cleanup or journal pruning fails after a mirror deletion but before the corresponding journal record is deleted, the method returns with that deletion already durable. On retry, the missing mirror row can no longer repopulate the set, so the terminal scheduled operation no longer satisfies the pruning predicate and remains indefinitely unless the same node and contest later acquire another removable schedule. Derive pruning eligibility from durable journal state, persist cleanup intent before deleting mirrors, or make the related mutations transaction-like.
source: ['codex']
3 participants
Why this PR exists
What was done
SubmitDpnsVoteOperation, typed per-target status, "Check again" / "Review again", the "Do not submit it again" unconfirmed-result guidance).dpns_current_vote_state()was doing a live storage read for every node × contest on every egui frame. It now performs one batched read per node during construction/explicit refresh and serves rendering from an in-memory snapshot, with a regression test asserting 100 poll lookups cost exactly one storage read per node.Mixedstate when loaded nodes disagree) — including immediately after a vote change, not just on first vote or after the next background refresh.User story
Imagine you are a masternode operator managing several voting identities. You open DPNS → Active contests, see at a glance which contests still need your vote, pick your choices, review exactly what will change, and cast or schedule them — with a truthful final or unconfirmed result and no risk of an accidental duplicate broadcast.
Testing
cargo test --test kittest --all-features masternode_tab::— 20 passed (independently re-run against a pre-fix checkout to confirm RED, then against HEAD to confirm GREEN — not a tautological pass)cargo test --lib --all-features ui::dpns::dpns_contested_names_screen::tests::— 9 passed, incl. the no-voting-key state, candidate-name resolution, and proved-vote-on-change regression testscargo test --lib --all-features ui::state::dpns_vote_state::tests::— 1 passedcargo test many_poll_states_use_one_storage_read_per_node --all-features— freeze regression, passedcargo test correlated_vote_result_routes_when_active_contests_is_hidden --all-features— passedcargo clippy --bin dash-evo-tool --all-features -- -D warnings— cleancargo fmt --all -- --check— cleangit diff --check— cleancargo test --all-features --workspaceand clippy sweep left to CI (this PR touches only Rust files already covered bytests.yml/clippy.yml's standard gate).No funded live-network vote was broadcast during verification.
Breaking changes
None.
Checklist
Attribution
🤖 Co-authored by Claudius the Magnificent AI Agent