fix(platform): preserve tracked masternode ABI and persistence - #4473
fix(platform): preserve tracked masternode ABI and persistence#4473QuantumExplorer wants to merge 2 commits into
Conversation
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
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 |
|
✅ Final review complete — no blockers (commit 874859d) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The ABI restoration, callback size gating, persistence rollback, and SwiftData migration changes are coherent, but one blocking concurrency defect remains in tracked-masternode refresh persistence. Concurrent refreshes can commit stale whole snapshots and discard metadata learned and persisted by another refresh.
Source: reviewer backend gpt-5.6-sol (Codex general, FFI engineer, Rust quality, and security auditor lanes); final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 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 `packages/rs-platform-wallet/src/masternode/tracked.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/masternode/tracked.rs:784-787: Concurrent refreshes can overwrite newly learned snapshot data
Each refresh clones the current row at line 673 before awaiting several network requests, while this mutation later replaces the entire live snapshot. Two refreshes of the same masternode can therefore start from the same state: refresh A can learn and persist a registration, while refresh B receives `Ok(None)` and retains its original `registration == None`. If B finishes last, this assignment replaces A's snapshot with B's stale clone and persists the loss. The same race affects list and Platform metadata. The service is explicitly cloneable and `Send + Sync`, and the FFI dispatches refreshes through a multithreaded runtime, so callers are not serialized. Serialize refreshes per masternode, merge only fields actually learned by each refresh into the live snapshot, or use a revision/CAS retry, and add an interleaved-refresh regression test.
| let label = self.mutate_and_persist(|guard| match guard.get_mut(pro_tx_hash) { | ||
| Some(live) => { | ||
| live.snapshot = tracked.snapshot.clone(); | ||
| Ok(live.label.clone()) |
There was a problem hiding this comment.
🔴 Blocking: Concurrent refreshes can overwrite newly learned snapshot data
Each refresh clones the current row at line 673 before awaiting several network requests, while this mutation later replaces the entire live snapshot. Two refreshes of the same masternode can therefore start from the same state: refresh A can learn and persist a registration, while refresh B receives Ok(None) and retains its original registration == None. If B finishes last, this assignment replaces A's snapshot with B's stale clone and persists the loss. The same race affects list and Platform metadata. The service is explicitly cloneable and Send + Sync, and the FFI dispatches refreshes through a multithreaded runtime, so callers are not serialized. Serialize refreshes per masternode, merge only fields actually learned by each refresh into the live snapshot, or use a revision/CAS retry, and add an interleaved-refresh regression test.
source: ['codex']
There was a problem hiding this comment.
Confirmed and fixed.
The race was real and as described: refresh cloned the row up front, then the final mutate_and_persist assigned the whole live.snapshot, so two passes over one node could start from the same state and the one finishing last would put its pre-read clone back — losing the other's registration, Platform key hashes, list/ever_listed and refreshed_at — and the whole-set write right after made the loss durable. Nothing serialized the callers: each FFI call builds its own TrackedMasternodes from the manager and dispatches through the shared multithreaded runtime.
Fix — serialize per masternode. The manager now holds a TrackedMasternodeRegistry (rows + a per-proTxHash gate map, std::sync::Mutex<BTreeMap<[u8; 32], Arc<tokio::sync::Mutex<()>>>>) behind one Arc, so every handle it hands out shares the gates as well as the rows. refresh_row_and_persist takes the node's gate FIRST, then reads the row, runs the network half, and writes back: the gate spans the read and the write, so a second refresh of the same node waits for the one in flight (and skips the network work it already did) instead of racing it. The std lock is held only long enough to clone the Arc — released before the await, so the future stays Send and nothing blocks the runtime; gates whose only reference is the map are swept on the next acquisition, so it stays bounded. The network steps moved into TrackedMasternodes::learn, which merges into the snapshot it is handed rather than into a clone it took earlier, so "came back empty" now means "keep what is there".
Regression test: interleaved_refresh_passes_keep_what_the_other_learned — two real refresh_row_and_persist passes over one node with scripted network halves, interleaved so the pass that learns nothing writes last; it asserts the registration the other pass learned survives both in the registry and in the persisted set. It fails on the unserialized ordering (drop the gate line and registration comes back None) and passes with it.
🤖 Addressed by Claude Code
There was a problem hiding this comment.
Resolved in 874859d — Concurrent refreshes can overwrite newly learned snapshot data 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.
…nt stale-snapshot overwrite A refresh cloned the tracked row before its network round-trips and later replaced the live snapshot wholesale, so two refreshes of one masternode could start from the same state: the one that finished last put its own pre-read clone back over whatever the other had learned (registration, Platform key hashes, list entry) and persisted the loss. The service is cloneable and the FFI dispatches refreshes through a multithreaded runtime, so nothing serialized the callers. The registry now carries a per-node refresh gate alongside the rows, and one pass (read row, learn, write back) holds it end to end: the network half always starts from everything earlier passes learned, and a second refresh of the same node waits instead of repeating the work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4473 +/- ##
============================================
- Coverage 87.39% 86.71% -0.69%
============================================
Files 2735 2735
Lines 347804 350211 +2407
============================================
- Hits 303980 303681 -299
- Misses 43824 46530 +2706
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The prior blocking stale-snapshot race is fixed: refreshes now share a per-masternode gate that spans snapshot read, learning, mutation, and persistence, and the targeted regression test passes. Two non-blocking test-coverage gaps remain around freezing the new V2 ABI layout and deterministically exercising the refresh interleaving.
Source: reviewer backend gpt-5.6-sol (Codex general, FFI engineer, Rust quality, and security auditor lanes); final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— ffi-engineer (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— security-auditor (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 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 `packages/rs-platform-wallet-ffi/src/core_wallet_types.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet_types.rs:1551: The new V2 C ABI layout is not pinned
The layout regression test freezes V1 but only verifies that V2 begins with its nested V1 member. The stride test constructs and reads the array through the same current Rust type, so it remains green if `source` or `label` is reordered, padding changes, or another field is inserted. Because `MasternodeEntryV2FFI` is now a public versioned C array element, pin its 64-bit size, alignment, and additive-field offsets so incompatible changes require a V3 API.
In `packages/rs-platform-wallet/src/masternode/tracked.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/masternode/tracked.rs:1346-1347: The refresh regression test does not guarantee the intended interleaving
The regression test relies on one `yield_now()` polling the spawned blind refresh before the learner is released, but Tokio does not guarantee which runnable task is polled after a yield. If the test task or learner proceeds before the blind refresh is first polled, the learner can persist its registration before the blind pass reads the row; the test then passes even if the per-node gate is removed because the blind pass starts from the updated snapshot. Add explicit first-poll synchronization so the blind refresh is known to have reached a pending point before releasing the learner.
| std::mem::offset_of!(MasternodeEntryFFI, platform_ownership_checked), | ||
| 288 | ||
| ); | ||
| assert_eq!(std::mem::offset_of!(MasternodeEntryV2FFI, v1), 0); |
There was a problem hiding this comment.
🟡 Suggestion: The new V2 C ABI layout is not pinned
The layout regression test freezes V1 but only verifies that V2 begins with its nested V1 member. The stride test constructs and reads the array through the same current Rust type, so it remains green if source or label is reordered, padding changes, or another field is inserted. Because MasternodeEntryV2FFI is now a public versioned C array element, pin its 64-bit size, alignment, and additive-field offsets so incompatible changes require a V3 API.
| assert_eq!(std::mem::offset_of!(MasternodeEntryV2FFI, v1), 0); | |
| assert_eq!(std::mem::size_of::<MasternodeEntryV2FFI>(), 312); | |
| assert_eq!(std::mem::align_of::<MasternodeEntryV2FFI>(), 8); | |
| assert_eq!(std::mem::offset_of!(MasternodeEntryV2FFI, v1), 0); | |
| assert_eq!(std::mem::offset_of!(MasternodeEntryV2FFI, source), 296); | |
| assert_eq!(std::mem::offset_of!(MasternodeEntryV2FFI, label), 304); |
source: ['codex']
| // Let it get as far as it can while the learner still holds the row. | ||
| tokio::task::yield_now().await; |
There was a problem hiding this comment.
🟡 Suggestion: The refresh regression test does not guarantee the intended interleaving
The regression test relies on one yield_now() polling the spawned blind refresh before the learner is released, but Tokio does not guarantee which runnable task is polled after a yield. If the test task or learner proceeds before the blind refresh is first polled, the learner can persist its registration before the blind pass reads the row; the test then passes even if the per-node gate is removed because the blind pass starts from the updated snapshot. Add explicit first-poll synchronization so the blind refresh is known to have reached a pending point before releasing the learner.
source: ['codex']
2 participants
Issue being fixed or feature implemented
Follow up on #4465 to preserve the established masternode C ABI and make tracked-masternode persistence safe across failures, concurrency, SwiftData migration, and unrelated changeset rollbacks.
What was done?
MasternodeEntryFFIlayout and legacy wallet-only behavior; addedMasternodeEntryV2FFIplus versioned list/free APIs for provenance and labels.DashSchemaV2with a lightweight V1 migration forPersistentTrackedMasternode.How Has This Been Tested?
cargo test -p platform-wallet --lib— 730 passed.cargo test -p platform-wallet-ffi --lib— 277 passed.swift test— 364 passed, 12 skipped, 0 failed../build_ios.sh --target sim --profile dev— simulator XCFramework generated and SwiftExampleApp warnings-as-errors build succeeded.cargo fmt --all -- --checkandgit diff --check— passed.Breaking Changes
None. The pre-#4465 C ABI and behavior remain available through the unversioned entry points; additive fields use V2 APIs.
Checklist:
For repository code-owners and collaborators only