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 docstrings
🧪 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 513af26) |
|
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
…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
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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
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.
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.
…el check Rename the voter-scoped-looking helper to is_votable, remove its unused voter parameter, update the sole caller, and replace voter-map-dependent tests with exhaustive contest-state coverage. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
…tests root dpns_result_needs_hidden_active_contests_route() was missing ScheduledVoteSweepCompleted from its matches!, so the general hidden-route dispatcher (route_dpns_vote_result_to_hidden_active_contests, called unconditionally for every Success result) never refreshed a hidden RootScreenDPNSActiveContests after a scheduled-vote sweep completed. A user who navigated away or had a screen pushed on the stack while a sweep was in flight could return to stale, disabled voting controls after a plain PopScreen. Also narrows the sweep-completion branch's direct visible_screen_mut() refresh so it only fires when Active-contests is actually the visible screen (no screen-stack, no other root selected) instead of unconditionally refreshing whatever screen happens to be visible. Adds scheduled_vote_sweep_completion_routes_when_active_contests_is_hidden covering both the hidden and visible cases. Co-Authored-By: Codex Sol <noreply@openai.com>
Makes the operation journal authoritative over the legacy scheduled-vote KV mirror for removal, Clear All, and terminal-operation pruning: - remove_scheduled_dpns_vote(): guarded row removal that changes a Scheduled target to Cancelled and persists before touching the mirror, refuses to touch the mirror while a target is Queued/Submitting/ Confirming/Unconfirmed (returns DpnsScheduledVoteAlreadyStarted), and only allows mirror-only deletion when no journal operation still holds the target lock. - clear_all_scheduled_dpns_votes(): guarded Clear All that cancels Scheduled targets, retains mirror rows for anything still in flight, and returns a typed per-target DpnsScheduledVoteClearOutcome instead of unconditionally wiping every mirror row. - insert_dpns_vote_operation_with_scheduled_mirror(): serializes the journal write and the best-effort compatibility-mirror write under the same guard, closing the race where a concurrent Clear All could cancel and prune a schedule before its mirror row ever landed. - prune_terminal_dpns_vote_operations(): now takes no removed-set parameter and derives eligibility from a durable raw-key enumeration of surviving mirror rows instead of an in-memory BTreeSet, so a crash or I/O failure partway through cleanup no longer permanently strands a terminal operation as unprunable. The predicate also no longer treats "not scheduled" as automatic pruning grounds, so immediate-only operations backing Recent voting activity survive a scheduled-only clear. - clear_executed_scheduled_votes(): rewritten around the durable raw-key helper, propagates row-read errors instead of silently treating them as absent, and calls the new no-arg pruning method. - operation_for_scheduled_vote(): prefers the lock-holding operation, falling back to the newest terminal one, instead of first-match. New model types (src/model/dpns_voting.rs): DpnsScheduledVoteKey, DpnsScheduledVoteClearDisposition, DpnsScheduledVoteClearOutcome. New BackendTaskSuccessResult::ScheduledVotesCleared variant. UI wiring (journal-first Scheduled Votes table, button dispatch, Active contests render cache) is a separate follow-up commit — this pass is scoped to the backend_task/context layer only, per the DET module placement policy. Co-Authored-By: Codex Sol <noreply@openai.com>
Builds on 4058b04b's guarded backend layer to close out the UI-side gaps: - Scheduled Votes table rows are now built from the newest journal outcome per (voter, contested_name) via DpnsVoteOperationSnapshot:: scheduled_vote_rows(), falling back to the legacy mirror row only when no journal pair exists. A journaled-but-unmirrored schedule is no longer invisible, and a terminal journal status (Rejected/ FailedBeforeSubmission/Cancelled) is no longer misdisplayed as Pending. - Row Remove dispatches CancelScheduledDpnsVote (guarded journal cancellation) for journal-backed rows, and DeleteScheduledVote (legacy-only deletion) only for true fallback rows with no journal entry. - Cast-now/Remove availability follows the journal's DpnsVoteTargetStatus instead of the old mirror-derived ScheduledVoteCastingStatus; pending Cast-now clicks are deduplicated locally until the journal catches up. - ScheduledVotesCleared now gets a real result-handling arm: routes through the hidden-Active-contests mechanism when appropriate, shows a success/information MessageBanner summarizing cleared vs. still-in- flight targets, and rebuilds the Scheduled Votes rows immediately instead of silently doing nothing (previously swallowed by a wildcard match arm on both app.rs and the screen's display_task_result). - Active-contests render path now builds an ActiveDpnsContestSnapshot once per construction/refresh (Arc-wrapped contests, poll ID computed once each) instead of cloning every ContestedName and rehashing its poll ID from three separate call sites every egui frame. QA follow-ups from independent review of 4058b04b: - remove_scheduled_dpns_vote(None, ...) — the actual production path used by DeleteScheduledVote — now has test coverage for both the unlocked (mirror deleted) and locked (refused) cases; the locked case now correctly refuses deletion when a journal lock exists. - Removed the redundant durable mirror-key scan in prune_terminal_operations that computed and immediately discarded a duplicate KV enumeration. Co-Authored-By: Codex Sol <noreply@openai.com>
DPNSScreen::display_task_error() cleared the progress overlay and the pending operation id when a vote submission failed, but left bulk_vote_handling_status on CastingVotes/SchedulingVotes. That status is what show_review_and_cast_window() uses as operation_in_progress, and it disables the Submit button *and* the Cancel button; the egui::Window has no close control either. Only display_task_result() ever moved the status out of the in-flight state, so a failed submission left the modal frozen on "Submitting votes…" with no way to retry, cancel, or dismiss it — and the state survives navigating away and back, since the screen instance is kept in AppState::main_screens. Reproduces whenever the backend task returns Err after dispatch, e.g. DpnsCurrentVoteUnavailable: the screen's cached proved-vote snapshot is fresh enough to build targets, then the backend's pre-submission refresh_dpns_vote_states() fails against an unreachable DAPI. Move the status to Failed(<error text>) alongside the existing overlay cleanup, gated on the same clear_vote_overlay_on_error ownership check so an unrelated task error cannot disturb a submission still in flight. The window already renders Failed inline and re-enables both buttons, and TaskError's Display is the user-facing text by convention. Adds failed_submission_releases_the_review_window (covers both in-flight statuses) and unrelated_error_keeps_the_pending_submission_in_progress. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head fcbc1dc, four of the five required prior findings are fixed, but mixed immediate/scheduled batches still lose their immediate history during scheduled-vote cleanup. The journal-first changes also leave row-level Remove unable to remove journal-backed rows, which is a blocking regression in a core Scheduled Votes action; manual cast correlation and per-frame activity cloning remain non-blocking issues.
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)
🔴 1 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/context/dpns_vote_operations.rs`:
- [BLOCKING] src/context/dpns_vote_operations.rs:991-1041: Remove cannot remove a journal-backed scheduled vote
For a `Scheduled` target, Remove persists `Cancelled` and deletes only the compatibility mirror. For an already-terminal target, it leaves the journal outcome unchanged and again deletes only the mirror. The journal-first projection in `DpnsVoteOperationSnapshot::scheduled_vote_rows` includes every scheduled-timing outcome, including `Cancelled`, `Confirmed`, and failed outcomes, and prefers that outcome over the mirror. The refresh returned by the removal task therefore reconstructs the same row immediately, and clicking Remove again reaches the terminal branch without changing that result. Add a durable per-target dismissal/removal state, or prune only the selected journal target while preserving unrelated targets and history in the same operation.
In `src/ui/dpns/dpns_contested_names_screen.rs`:
- [SUGGESTION] src/ui/dpns/dpns_contested_names_screen.rs:2126-2174: Unrelated vote activity can finish a manual scheduled cast
`CastScheduledVote` maps to `BackendTaskContext::Other`, so a manual Cast now overlay has no `pending_vote_operation`. The fallback at lines 2127-2129 consequently treats any visible task error without a DPNS operation ID, including a periodic `ScheduledVoteSweep` error, as the owner of that overlay. The success fallback at lines 2170-2171 similarly accepts any `DpnsVoteOperationUpdated`, including unattributed reconciliation updates emitted by an overlapping sweep, and both paths clear all pending scheduled-cast keys. This can lower the blocker and re-enable Cast now while the original cast is still running. Give manual scheduled casts a context containing their exact voter/contest key or another dispatch identity, and clear only the matching pending entry.
- [SUGGESTION] src/ui/dpns/dpns_contested_names_screen.rs:863-871: Recent activity deep-clones and sorts all operation history every frame
`render_voting_activity` calls `to_vec()` on the complete operation snapshot before filtering to five entries. Each clone recursively copies all targets and their owned aliases and contested-name strings, and the full cloned history is then sorted on every egui frame. Immediate-only terminal operations are intentionally retained, so this render cost grows with normal voting history even though only five records are displayed. Select and sort references or lightweight indices, then render only the newest five operations without cloning the stored records.
| let journal_is_authoritative = match selected { | ||
| Some((operation_id, DpnsVoteTargetStatus::Scheduled)) => { | ||
| if lock_index | ||
| .get(key) | ||
| .is_some_and(|owner| *owner != operation_id) | ||
| { | ||
| return Err(TaskError::DpnsScheduledVoteAlreadyStarted); | ||
| } | ||
| if !cancel_scheduled_target(&kv, self.network, operation_id, key)? { | ||
| return Err(TaskError::DpnsScheduledVoteAlreadyStarted); | ||
| } | ||
| true | ||
| } | ||
| Some(( | ||
| _, | ||
| DpnsVoteTargetStatus::Queued | ||
| | DpnsVoteTargetStatus::Submitting | ||
| | DpnsVoteTargetStatus::Confirming | ||
| | DpnsVoteTargetStatus::Unconfirmed, | ||
| )) => return Err(TaskError::DpnsScheduledVoteAlreadyStarted), | ||
| Some((operation_id, _)) => { | ||
| if lock_index | ||
| .get(key) | ||
| .is_some_and(|owner| *owner != operation_id) | ||
| { | ||
| return Err(TaskError::DpnsScheduledVoteAlreadyStarted); | ||
| } | ||
| true | ||
| } | ||
| None => { | ||
| if lock_index.contains_key(key) { | ||
| return Err(TaskError::DpnsScheduledVoteAlreadyStarted); | ||
| } | ||
| false | ||
| } | ||
| }; | ||
|
|
||
| let voter = key.voter_id.to_buffer(); | ||
| if let Err(error) = delete_scheduled_vote_in(&kv, &voter, contested_name) { | ||
| if !journal_is_authoritative { | ||
| return Err(error); | ||
| } | ||
| tracing::warn!( | ||
| ?error, | ||
| expected_operation_id = ?expected_operation_id, | ||
| voter_id = %key.voter_id, | ||
| contested_name, | ||
| "Scheduled DPNS vote journal was updated but its compatibility mirror remains" | ||
| ); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
🔴 Blocking: Remove cannot remove a journal-backed scheduled vote
For a Scheduled target, Remove persists Cancelled and deletes only the compatibility mirror. For an already-terminal target, it leaves the journal outcome unchanged and again deletes only the mirror. The journal-first projection in DpnsVoteOperationSnapshot::scheduled_vote_rows includes every scheduled-timing outcome, including Cancelled, Confirmed, and failed outcomes, and prefers that outcome over the mirror. The refresh returned by the removal task therefore reconstructs the same row immediately, and clicking Remove again reaches the terminal branch without changing that result. Add a durable per-target dismissal/removal state, or prune only the selected journal target while preserving unrelated targets and history in the same operation.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Remove cannot remove a journal-backed scheduled vote 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.
| fn display_backend_task_error(&mut self, context: &BackendTaskContext, _error: &TaskError) { | ||
| self.clear_vote_overlay_on_error = match self.pending_vote_operation { | ||
| Some(operation_id) => dpns_operation_id(context) == Some(operation_id), | ||
| None => self.vote_overlay.is_some() && dpns_operation_id(context).is_none(), | ||
| }; | ||
| } | ||
|
|
||
| fn display_task_error(&mut self, error: &TaskError) -> bool { | ||
| let handled = scheduled_vote_sweep_is_quiet(error); | ||
| if matches!( | ||
| error, | ||
| TaskError::ScheduledVoteRejected { .. } | ||
| | TaskError::ScheduledVoteAllAddressesExhausted { .. } | ||
| | TaskError::ScheduledVoteResultUnavailable | ||
| | TaskError::ScheduledVoteSweepFailed { .. } | ||
| | TaskError::ScheduledVoteSweepAllAddressesExhausted { .. } | ||
| ) { | ||
| self.scheduled_vote_cast_in_progress = false; | ||
| if let Ok(mut guard) = self.scheduled_votes.lock() { | ||
| for vote in guard.iter_mut() { | ||
| if vote.1 == ScheduledVoteCastingStatus::InProgress { | ||
| vote.1 = ScheduledVoteCastingStatus::Failed; | ||
| } | ||
| } | ||
| self.pending_scheduled_casts.clear(); | ||
| if self.clear_vote_overlay_on_error { | ||
| self.vote_overlay.take_and_clear(); | ||
| self.pending_vote_operation = None; | ||
| self.clear_vote_overlay_on_error = false; | ||
| // The review window disables both Submit and Cancel while a | ||
| // submission is in flight, so a failed submission must leave that | ||
| // state here. Otherwise the window stays on "Submitting votes…" | ||
| // with no way to retry or close it. | ||
| if matches!( | ||
| self.bulk_vote_handling_status, | ||
| VoteHandlingStatus::CastingVotes | VoteHandlingStatus::SchedulingVotes | ||
| ) { | ||
| self.bulk_vote_handling_status = VoteHandlingStatus::Failed(error.to_string()); | ||
| } | ||
| } | ||
| handled | ||
| if let Err(refresh_error) = self.vote_operations.refresh(&self.app_context) { | ||
| tracing::warn!( | ||
| ?refresh_error, | ||
| "Could not refresh DPNS voting state after a task error" | ||
| ); | ||
| } | ||
| self.rebuild_scheduled_vote_rows(); | ||
| scheduled_vote_sweep_is_quiet(error) | ||
| } | ||
|
|
||
| fn display_task_result(&mut self, backend_task_success_result: BackendTaskSuccessResult) { | ||
| match backend_task_success_result { | ||
| // If immediate cast finished, see if we have pending to schedule next | ||
| BackendTaskSuccessResult::DPNSVoteResults(results) => { | ||
| let errors: Vec<String> = results | ||
| .iter() | ||
| .filter_map(|(_, _, r)| r.as_ref().err().map(|e| e.to_string())) | ||
| .collect(); | ||
| let successes: Vec<String> = results | ||
| .iter() | ||
| .filter_map(|(name, _, r)| r.as_ref().ok().map(|_| name.clone())) | ||
| .collect(); | ||
|
|
||
| if !errors.is_empty() { | ||
| let errors_string = errors.join("\n\n"); | ||
| if !successes.is_empty() { | ||
| // partial success | ||
| self.bulk_schedule_message = Some(( | ||
| MessageType::Error, | ||
| format!( | ||
| "Successes: {}/{}\n\nErrors:\n\n{:?}", | ||
| successes.len(), | ||
| successes.len() + errors.len(), | ||
| errors_string | ||
| ), | ||
| )); | ||
| } else { | ||
| // all failed | ||
| self.bulk_schedule_message = | ||
| Some((MessageType::Error, format!("Errors:\n\n{}", errors_string))); | ||
| BackendTaskSuccessResult::DpnsVoteOperationUpdated { operation_id, .. } => { | ||
| self.pending_scheduled_casts.clear(); | ||
| if let Err(error) = self.vote_state.reload(&self.app_context) { | ||
| tracing::warn!( | ||
| ?error, | ||
| "Could not reload proved DPNS vote state after an operation update" | ||
| ); | ||
| } | ||
| let owns_result = self.pending_vote_operation == Some(operation_id) | ||
| || (self.pending_vote_operation.is_none() && self.vote_overlay.is_some()); | ||
| if owns_result { | ||
| self.vote_overlay.take_and_clear(); | ||
| self.pending_vote_operation = None; |
There was a problem hiding this comment.
🟡 Suggestion: Unrelated vote activity can finish a manual scheduled cast
CastScheduledVote maps to BackendTaskContext::Other, so a manual Cast now overlay has no pending_vote_operation. The fallback at lines 2127-2129 consequently treats any visible task error without a DPNS operation ID, including a periodic ScheduledVoteSweep error, as the owner of that overlay. The success fallback at lines 2170-2171 similarly accepts any DpnsVoteOperationUpdated, including unattributed reconciliation updates emitted by an overlapping sweep, and both paths clear all pending scheduled-cast keys. This can lower the blocker and re-enable Cast now while the original cast is still running. Give manual scheduled casts a context containing their exact voter/contest key or another dispatch identity, and clear only the matching pending entry.
source: ['codex']
| fn render_voting_activity(&mut self, ui: &mut Ui) { | ||
| let mut operations = self.vote_operations.operations().to_vec(); | ||
| operations.sort_by_key(|operation| operation.created_at); | ||
| let operations = operations | ||
| .into_iter() | ||
| .rev() | ||
| .filter(|operation| !operation.targets.is_empty()) | ||
| .take(5) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
🟡 Suggestion: Recent activity deep-clones and sorts all operation history every frame
render_voting_activity calls to_vec() on the complete operation snapshot before filtering to five entries. Each clone recursively copies all targets and their owned aliases and contested-name strings, and the full cloned history is then sorted on every egui frame. Immediate-only terminal operations are intentionally retained, so this render cost grows with normal voting history even though only five records are displayed. Select and sort references or lightweight indices, then render only the newest five operations without cloning the stored records.
source: ['codex']
…unified # Conflicts: # src/ui/masternodes/detail_screen.rs
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
At exact head 32032a7, all four indexed prior findings remain valid. The cumulative PR also contains two blocking Active-contests defects that contradict its voting-capability and exact-review requirements, plus two additional vote-state correctness and diagnostic issues; the latest base-branch merge does not resolve these DPNS paths.
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)
4 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/ui/dpns/dpns_contested_names_screen.rs`:
- [BLOCKING] src/ui/dpns/dpns_contested_names_screen.rs:2350-2354: Loaded nodes without voting keys bypass the actionable empty state
This gate treats any entry in `voting_identities` as voting-capable, but `load_local_voting_identities()` includes every non-User identity without requiring an associated voter identity or signing key. Read-only masternodes therefore bypass the promised "None of your loaded nodes has a voting key" state and enter the composer. Submission later reaches `submit_dpns_vote`, which rejects identities whose `associated_voter_identity` is absent. Filter the composer to identities that can satisfy the submit path, and show the load action when that filtered set is empty.
- [BLOCKING] src/ui/dpns/dpns_contested_names_screen.rs:1597-1607: Review and cast presents no-op nodes as votes that will be cast
The review sheet says it will cast on behalf of all loaded nodes but lists each contest choice only once. It does not display the required node × contest targets, each node's current choice, or which targets already match the requested choice. Those no-op targets are removed only after Submit by `DpnsVoteOperation::new`, so a mixed-current-state review claims that every node will vote even though some will do nothing. This violates VOTE-FR-024/025 and the PR's stated exact-review behavior. Build and filter the effective target set before rendering, list current choice, requested choice, and timing for every retained target, and explain suppressed no-ops before submission.
In `src/context/dpns_vote_state.rs`:
- [SUGGESTION] src/context/dpns_vote_state.rs:182-245: Concurrent snapshot writers can erase a newly confirmed vote
Each refresh performs network I/O and then replaces the complete per-voter snapshot, while `cache_confirmed_dpns_vote` performs an unsynchronized read-modify-write of the same record. Backend tasks are spawned independently, and scheduled sweeps execute several operations concurrently, so a refresh that fetched state before a successful vote can finish afterward and overwrite the confirmed cache entry. A subsequent UI reload can present the proved vote as absent, and concurrent read-modify-write updates can also lose entries. Serialize per-voter snapshot writes or attach generations to fetches and reject or merge stale completions; cover the stale-refresh-after-confirmation interleaving with a test.
- [SUGGESTION] src/context/dpns_vote_state.rs:166-230: Best-effort refresh drops the error that blocked vote preflight
`refresh_dpns_vote_states` logs SDK query failures, stores an unavailable snapshot, and returns `()`. That is suitable for an ambient refresh, but `execute_dpns_vote_operation` also uses it as a mandatory submission preflight. In that path the typed source is discarded and the task returns only `DpnsCurrentVoteUnavailable`. Because that fieldless error contains no DAPI reachability source, the outer contextualizer and operation diagnostics cannot report the transport or exhausted-address failure that actually blocked submission. Return a typed per-voter refresh report or add a strict preflight API that preserves the source while ambient callers explicitly choose best-effort behavior.
| let has_any = !self.active_contests.is_empty(); | ||
| if self.voting_identities.is_empty() { | ||
| inner_action |= self.render_no_voting_nodes(ui); | ||
| } else if has_any { | ||
| self.render_active_contests(ui); |
There was a problem hiding this comment.
🔴 Blocking: Loaded nodes without voting keys bypass the actionable empty state
This gate treats any entry in voting_identities as voting-capable, but load_local_voting_identities() includes every non-User identity without requiring an associated voter identity or signing key. Read-only masternodes therefore bypass the promised "None of your loaded nodes has a voting key" state and enter the composer. Submission later reaches submit_dpns_vote, which rejects identities whose associated_voter_identity is absent. Filter the composer to identities that can satisfy the submit path, and show the load action when that filtered set is empty.
source: ['codex']
There was a problem hiding this comment.
Resolved in cbb4afa — Loaded nodes without voting keys bypass the actionable empty state 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.
| ui.label(format!( | ||
| "Casting on behalf of all my nodes ({count}).", | ||
| count = self.voting_identities.len() | ||
| )); | ||
| ui.separator(); | ||
| ui.heading(format!("Votes to cast ({}):", self.selected_votes.len())); | ||
| for vote in &self.selected_votes { | ||
| let candidate_name = self.candidate_name(&vote.contested_name, vote.vote_choice); | ||
| let choice = review_vote_choice_label(vote.vote_choice, candidate_name.as_deref()); | ||
| ui.label(format!("• {name}.dash → {choice}", name = vote.contested_name)); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Review and cast presents no-op nodes as votes that will be cast
The review sheet says it will cast on behalf of all loaded nodes but lists each contest choice only once. It does not display the required node × contest targets, each node's current choice, or which targets already match the requested choice. Those no-op targets are removed only after Submit by DpnsVoteOperation::new, so a mixed-current-state review claims that every node will vote even though some will do nothing. This violates VOTE-FR-024/025 and the PR's stated exact-review behavior. Build and filter the effective target set before rendering, list current choice, requested choice, and timing for every retained target, and explain suppressed no-ops before submission.
source: ['codex']
There was a problem hiding this comment.
Resolved in cbb4afa — Review and cast presents no-op nodes as votes that will be cast 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.
| stream::iter(voters) | ||
| .map(|voter| { | ||
| let sdk = sdk.clone(); | ||
| let kv = kv.clone(); | ||
| let network = self.network; | ||
| async move { | ||
| let voter_id = voter.identity.id(); | ||
| match fetch_votes_for_voter(&sdk, voter_id).await { | ||
| Ok(votes) => { | ||
| let snapshot = StoredCurrentVotes { | ||
| available: true, | ||
| updated_at: now_ms(), | ||
| votes, | ||
| }; | ||
| if let Err(error) = save_snapshot(&kv, network, &voter_id, &snapshot) { | ||
| tracing::warn!( | ||
| ?error, | ||
| voter_id = %voter_id, | ||
| "Could not save proved DPNS vote state" | ||
| ); | ||
| } | ||
| } | ||
| Err(error) => { | ||
| let snapshot = StoredCurrentVotes { | ||
| available: false, | ||
| updated_at: now_ms(), | ||
| votes: BTreeMap::new(), | ||
| }; | ||
| if let Err(storage_error) = | ||
| save_snapshot(&kv, network, &voter_id, &snapshot) | ||
| { | ||
| tracing::warn!( | ||
| ?storage_error, | ||
| voter_id = %voter_id, | ||
| "Could not save unavailable DPNS vote state" | ||
| ); | ||
| } | ||
| tracing::warn!( | ||
| ?error, | ||
| voter_id = %voter_id, | ||
| "Proved DPNS vote-state query was unavailable" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| .buffer_unordered(4) | ||
| .collect::<Vec<_>>() | ||
| .await; | ||
| } | ||
|
|
||
| /// Update the proved-state cache after a confirmed target. | ||
| pub(crate) fn cache_confirmed_dpns_vote( | ||
| &self, | ||
| voter_id: Identifier, | ||
| vote_poll_id: Identifier, | ||
| choice: ResourceVoteChoice, | ||
| ) -> Result<(), TaskError> { | ||
| let kv = self.det_kv()?; | ||
| let mut snapshot = load_snapshot(&kv, self.network, &voter_id)?.unwrap_or_default(); | ||
| snapshot.available = true; | ||
| snapshot.updated_at = now_ms(); | ||
| snapshot.votes.insert(vote_poll_id.to_buffer(), choice); | ||
| save_snapshot(&kv, self.network, &voter_id, &snapshot) |
There was a problem hiding this comment.
🟡 Suggestion: Concurrent snapshot writers can erase a newly confirmed vote
Each refresh performs network I/O and then replaces the complete per-voter snapshot, while cache_confirmed_dpns_vote performs an unsynchronized read-modify-write of the same record. Backend tasks are spawned independently, and scheduled sweeps execute several operations concurrently, so a refresh that fetched state before a successful vote can finish afterward and overwrite the confirmed cache entry. A subsequent UI reload can present the proved vote as absent, and concurrent read-modify-write updates can also lose entries. Serialize per-voter snapshot writes or attach generations to fetches and reject or merge stale completions; cover the stale-refresh-after-confirmation interleaving with a test.
source: ['codex']
| pub(crate) async fn refresh_dpns_vote_states(&self, sdk: &Sdk) { | ||
| let voters = match self.load_local_masternode_identities() { | ||
| Ok(voters) => voters, | ||
| Err(error) => { | ||
| tracing::warn!(?error, "Could not load nodes for DPNS vote-state refresh"); | ||
| return; | ||
| } | ||
| }; | ||
| let kv = match self.det_kv() { | ||
| Ok(kv) => kv, | ||
| Err(error) => { | ||
| tracing::warn!(?error, "Could not open DPNS vote-state storage"); | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| stream::iter(voters) | ||
| .map(|voter| { | ||
| let sdk = sdk.clone(); | ||
| let kv = kv.clone(); | ||
| let network = self.network; | ||
| async move { | ||
| let voter_id = voter.identity.id(); | ||
| match fetch_votes_for_voter(&sdk, voter_id).await { | ||
| Ok(votes) => { | ||
| let snapshot = StoredCurrentVotes { | ||
| available: true, | ||
| updated_at: now_ms(), | ||
| votes, | ||
| }; | ||
| if let Err(error) = save_snapshot(&kv, network, &voter_id, &snapshot) { | ||
| tracing::warn!( | ||
| ?error, | ||
| voter_id = %voter_id, | ||
| "Could not save proved DPNS vote state" | ||
| ); | ||
| } | ||
| } | ||
| Err(error) => { | ||
| let snapshot = StoredCurrentVotes { | ||
| available: false, | ||
| updated_at: now_ms(), | ||
| votes: BTreeMap::new(), | ||
| }; | ||
| if let Err(storage_error) = | ||
| save_snapshot(&kv, network, &voter_id, &snapshot) | ||
| { | ||
| tracing::warn!( | ||
| ?storage_error, | ||
| voter_id = %voter_id, | ||
| "Could not save unavailable DPNS vote state" | ||
| ); | ||
| } | ||
| tracing::warn!( | ||
| ?error, | ||
| voter_id = %voter_id, | ||
| "Proved DPNS vote-state query was unavailable" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| .buffer_unordered(4) | ||
| .collect::<Vec<_>>() | ||
| .await; |
There was a problem hiding this comment.
🟡 Suggestion: Best-effort refresh drops the error that blocked vote preflight
refresh_dpns_vote_states logs SDK query failures, stores an unavailable snapshot, and returns (). That is suitable for an ambient refresh, but execute_dpns_vote_operation also uses it as a mandatory submission preflight. In that path the typed source is discarded and the task returns only DpnsCurrentVoteUnavailable. Because that fieldless error contains no DAPI reachability source, the outer contextualizer and operation diagnostics cannot report the transport or exhausted-address failure that actually blocked submission. Return a typed per-voter refresh report or add a strict preflight API that preserves the source while ambient callers explicitly choose best-effort behavior.
source: ['codex']
…es blocked Three blocking defects in the unified DPNS voting flow. Review and cast lied about what it would send. It printed one bullet per contest and a raw node count, so a multi-node batch never showed its real node x contest targets, the choice already on chain, the per-target timing, or the targets DpnsVoteOperation::new silently drops as no-ops after Submit. The sheet and the submit click now share one resolved plan, so the sheet cannot promise something other than what is sent: every retained target is listed with node, contest, requested choice, current choice and timing, the skipped no-op count is stated, the headline counts the effective targets, and Submit is disabled when nothing would be submitted. No-op suppression now has a single definition, DpnsVoteTarget::is_no_op, shared by the review and the operation; the operation still receives the unfiltered list so its no_op_count keeps driving the post-submit feedback. Remove on a scheduled vote did not remove it. The cancellation write was durable and correct, but scheduled_vote_rows projected every scheduled-timing outcome back into the table including Cancelled, so the row returned on the next refresh with a second Remove button that did nothing. The projection now honours the cancellation and drops the row, while keeping the target's pair in the legacy-suppression set so a mirror row that outlived a best-effort delete cannot resurrect it. Pruning the journal could not have fixed this: a bulk schedule is one operation with many targets, so removing one row leaves the operation incomplete and unprunable. Clear All keeps its bulk prune. A masternode loaded without its voting key reached the composer and only failed at submit time with NoVotingIdentity, because the gate filtered on identity type alone. The DPNS surfaces now keep only identities that satisfy the submit path (QualifiedIdentity::can_cast_masternode_vote), so a read-only node lands on the actionable "no voting key" state with Load a masternode before any vote is composed. VOTE-FR-024, VOTE-FR-025 and VOTE-TC-013 of the DPNS voting experience design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The limited-vote-change note belongs to VOTE-FR-015, which is outside the scope of this blocking-fix pass. The review sheet still lists each target's current choice, so the operator sees what a vote replaces; only the extra advisory line and the ReviewPlan predicate behind it are removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
762c66cf -> 5931df74 for dash-sdk, rs-sdk-trusted-context-provider, platform-wallet and platform-wallet-storage. Cargo.lock moves only the platform git source revs; no other dependency drifts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed an update covering three things since the last 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 513af26, the keyless-node gate and exact node-by-contest review findings are fixed. Three blocking defects remain: terminal scheduled rows cannot actually be removed, the required limited-vote-change warning was removed, and an absolute UTC schedule drifts while Review remains open. Six additional in-scope suggestions remain around mixed-history pruning, result correlation, unbounded frame work, vote-state races and diagnostics, and swallowed identity-loading errors.
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 | 🟡 1 suggestion(s)
6 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/ui/dpns/dpns_contested_names_screen.rs`:
- [BLOCKING] src/ui/dpns/dpns_contested_names_screen.rs:1724-1726: Review no longer warns that changing a vote is limited
The review lists current and requested choices but no longer labels a changed existing vote as consuming a limited vote change. This directly contradicts VOTE-FR-015 and VOTE-TC-006 in the PR's authoritative requirements, and commit a9d5a2c4 removed the predicate, warning, and test that previously enforced the behavior. Restore a warning whenever any effective target replaces an existing proved choice, without claiming a remaining count.
- [BLOCKING] src/ui/dpns/dpns_contested_names_screen.rs:2035-2040: Absolute UTC schedules drift while the review remains open
`simple_schedule_option()` converts the absolute `Cast on (UTC)` value into a relative day/hour/minute offset only when the option is applied or the field changes. `build_review_plan()` then recreates the persisted timestamp as `Utc::now() + offset` on every frame and again on Submit. If 12:00 is selected at 11:00 and submission happens ten minutes later, the vote is scheduled around 12:10 rather than the chosen time; that can also move it beyond a contest deadline. Preserve the selected absolute timestamp separately, while keeping the advanced `Schedule after` controls relative if intended.
- [SUGGESTION] src/ui/dpns/dpns_contested_names_screen.rs:198-204: Voting identity load failures masquerade as a keyless-node state
`loaded_voting_identities` converts every typed error from `load_local_voting_identities()` into an empty vector. Both the Active-contests gate and Review window then show `None of your loaded nodes has a voting key` and direct the operator to load another masternode, even when identity storage was unreadable or unavailable. Return the `Result` and surface the actual `TaskError` through a `MessageBanner`; use an empty fallback only after recording the load failure for the user.
| if plan.no_op_count() > 0 { | ||
| ui.label(review_skipped_line(plan.no_op_count())); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Review no longer warns that changing a vote is limited
The review lists current and requested choices but no longer labels a changed existing vote as consuming a limited vote change. This directly contradicts VOTE-FR-015 and VOTE-TC-006 in the PR's authoritative requirements, and commit a9d5a2c removed the predicate, warning, and test that previously enforced the behavior. Restore a warning whenever any effective target replaces an existing proved choice, without claiming a remaining count.
| if plan.no_op_count() > 0 { | |
| ui.label(review_skipped_line(plan.no_op_count())); | |
| } | |
| if plan.no_op_count() > 0 { | |
| ui.label(review_skipped_line(plan.no_op_count())); | |
| } | |
| if plan | |
| .effective() | |
| .any(|entry| entry.target.current_choice.is_some()) | |
| { | |
| ui.colored_label( | |
| DashColors::warning_color(dark_mode), | |
| "Changing a vote uses one of this node's limited vote changes. Review the current and requested choices before submitting.", | |
| ); | |
| } |
source: ['codex']
| let offset = chrono::Duration::days(*days as i64) | ||
| + chrono::Duration::hours(*hours as i64) | ||
| + chrono::Duration::minutes(*minutes as i64); | ||
| let scheduled_time = (now + offset).timestamp_millis() as u64; | ||
|
|
||
| for sv in &self.selected_votes { | ||
| let new_vote = ScheduledDPNSVote { | ||
| contested_name: sv.contested_name.clone(), | ||
| voter_id: identity.identity.id(), | ||
| choice: sv.vote_choice, | ||
| unix_timestamp: scheduled_time, | ||
| executed_successfully: false, | ||
| }; | ||
| scheduled_list.push(new_vote); | ||
| } | ||
| ( | ||
| VoteTiming::Scheduled((Utc::now() + offset).timestamp_millis() as u64), | ||
| format!("Scheduled in {days} d {hours} h {minutes} min"), |
There was a problem hiding this comment.
🔴 Blocking: Absolute UTC schedules drift while the review remains open
simple_schedule_option() converts the absolute Cast on (UTC) value into a relative day/hour/minute offset only when the option is applied or the field changes. build_review_plan() then recreates the persisted timestamp as Utc::now() + offset on every frame and again on Submit. If 12:00 is selected at 11:00 and submission happens ten minutes later, the vote is scheduled around 12:10 rather than the chosen time; that can also move it beyond a contest deadline. Preserve the selected absolute timestamp separately, while keeping the advanced Schedule after controls relative if intended.
source: ['codex']
| fn loaded_voting_identities(app_context: &AppContext) -> Vec<QualifiedIdentity> { | ||
| app_context | ||
| .load_local_voting_identities() | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .filter(QualifiedIdentity::can_cast_masternode_vote) | ||
| .collect() |
There was a problem hiding this comment.
🟡 Suggestion: Voting identity load failures masquerade as a keyless-node state
loaded_voting_identities converts every typed error from load_local_voting_identities() into an empty vector. Both the Active-contests gate and Review window then show None of your loaded nodes has a voting key and direct the operator to load another masternode, even when identity storage was unreadable or unavailable. Return the Result and surface the actual TaskError through a MessageBanner; use an empty fallback only after recording the load failure for the user.
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.is_open_for_votercheck — which silently ignored the voter it was named after — tois_votable, and replaced its tautological tests with ones that actually exercise the state-based logic it performs.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 test --all-features dpns_result_routing_tests— 5 passed (incl. sweep-completion and Clear-All hidden-route regressions)cargo test --all-features dpns_vote_operations::tests::— 36 passed (guarded removal/Clear All, durable resumable pruning, immediate-history preservation)cargo test --all-features identity_db::tests::— 33 passedcargo test --all-features contested_name::— 13 passedcargo test --all-features contested_names::— 26 passedcargo test failed_submission_releases_the_review_window unrelated_error_keeps_the_pending_submission_in_progress --all-features— 2 passed (stuck Review-and-cast window regression)cargo check --all-features --bin dash-evo-tool— cleancargo 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