live-store: owner-tagged atomic single-spend domain (off-chain + on-chain), part of #1159 - #1162

This PR has been open for 9 days without review. @bitcoin-coder-bob is anyone looking at this? (Part of the #1159 VTXO live-store stack — #1160 and #1161 also awaiting review.)

Review — #1162

live-store: owner-tagged atomic single-spend domain

Core change

Replaces the per-process offchainTxMu mutex + recheck-then-add pattern with an owner-tagged atomic claim at the store layer. The inmemory implementation uses its own write lock; the Redis implementation uses Lua scripts, making the conflict check + registration a single server-side atomic step that is safe across multiple arkd processes.

Lua scripts

The scripts are correct:

• claimScript and addScript check all outpoints for different-owner conflicts first (returns immediately without registering anything on conflict), then check if the owner already holds all of them, then register the batch. The two-pass structure is correct for Lua atomicity: since the script runs atomically on Redis, no interleaving between the conflict check and the HSET is possible.
• removeScript and releaseScript only delete outpoints whose owner matches — correct scoped deletion.
• The anyNew logic correctly distinguishes ClaimAlreadyOwned (all already owned by same owner) from ClaimFresh (at least one new).

Key rename

offChainInputsSetKey (Redis SET) → offChainInputsHashKey (Redis HASH) with a new key name. This avoids a WRONGTYPE error on a mixed-version rolling deploy since the old SET is left intact until it expires. Well handled.

ClaimAlreadyOwned (idempotent retry)

When Add returns ClaimAlreadyOwned, the service skips applying the Accepted event (changes = nil) and returns the accepted result. This correctly handles a concurrent or retried submit of the same arkTxid without emitting a duplicate event.

ClaimOutpoints / ReleaseOutpoints

New methods for the on-chain Arkade cosign path (#1159). The same conflict domain — good, the mutual exclusion is enforced at the store layer regardless of the path.

Notes

• The inmemory Remove previously deleted inputs unconditionally; it now only deletes inputs owned by the arkTxid. This is stricter and correct.
• The Redis Remove loads the tx first to get inputs — if the tx is absent (already removed), it still calls removeScript with no outpoints (no-op). Safe.
• parseClaimResult parsing from the Lua return array was not shown; worth confirming the error handling is correct for unexpected return shapes.

⚠️ Protocol-critical — touches the single-spend enforcement path for offchain VTXOs. Flagged for human review regardless.

Verdict: Design is sound, atomicity is correct, and the idempotent-retry case is well handled. Looks ready to merge.

Arkana review — owner-tagged atomic single-spend domain

🔒 PROTOCOL-CRITICAL — please ensure a second human reviews before merging. This touches the core double-spend prevention for off-chain transactions.

What this does: Replaces a per-process mutex + read-check-then-add pattern in SubmitOffchainTx with a single atomic owner-tagged claim step. The new OffChainTxStore.Add returns a ClaimStatus tri-state (Fresh/AlreadyOwned/Conflict) and accepts no external lock. The same conflict domain is extended for on-chain spends via ClaimOutpoints/ReleaseOutpoints.

Design correctness:

• The old pattern had a TOCTOU window: read (Includes), release lock, concurrent submit claims the same VTXO. The new atomic claim eliminates that window.
• In-memory backend: the sync.RWMutex wrapping the map makes the claim atomic at the process level. ✓
• Redis backend: a Lua script (not shown in diff, but referenced via addScript, claimScript, etc.) makes the claim atomic across processes. The design note in the port interface makes this explicit. Assuming the Lua script itself is correct, the approach is sound.
• ClaimAlreadyOwned returning the accepted result without re-applying the event is correct idempotency for same-arkTxid retries.
• Removing offchainTxMu from the service struct is safe given the store is now the single authority — make sure no other call site depended on that mutex outside SubmitOffchainTx (the diff removes it from test structs as well, which suggests it was only used here).

Things to verify before merging:

1. The Lua scripts (addScript, claimScript, removeScript, releaseScript) — not in this diff but critical to correctness. Ensure they are also reviewed.
2. Confirm Remove (called on finalization) correctly releases inputs so they don't permanently block the outpoints.
3. The ClaimAlreadyOwned path sets changes = nil, which drops any new event. Verify there are no side effects from the earlier part of SubmitOffchainTx that need reverting when we take this path.

Test coverage: The e2e double-spend test is updated with distinct randomized destinations (fixing a pre-existing ambiguity where same-destination txids would be identical). Good improvement.

Verdict: Approach is sound and a clear security improvement over the mutex pattern. Human review of the Lua scripts and the change = nil path is essential before merge.

This PR has been open 28 days without review. @bitcoin-coder-bob is anyone looking at the single-spend domain stack (#1159)?

Arkana review — sha ad348b9

This PR converts OffChainTxStore.Add from a plain write to an owner-tagged atomic claim, and adds ClaimOutpoints/ReleaseOutpoints for on-chain Arkade spends. It also removes the per-process offchainTxMu mutex in SubmitOffchainTx in favour of the store's atomic barrier.

Correctness of the core atomic claim logic:
The inmemory claimLocked function is a clean single-lock implementation:

1. First pass: reject if any outpoint is held by a different owner (returns ClaimConflict and the conflicting outpoint, registers nothing).
2. After conflict pass: if all present outpoints are owned by owner, check for any new ones. If none, ClaimAlreadyOwned. Otherwise register all and ClaimFresh.

This is correct and all-or-nothing within the lock.

SubmitOffchainTx service change:

• Removes the explicit re-check loop (was prone to TOCTOU between process-local lock and the store) and the offchainTxMu mutex.
• ClaimAlreadyOwned path correctly clears changes to avoid emitting a duplicate Accepted event — good.
• ClaimConflict returns the conflicting outpoint in the error, which is an improvement for diagnostics.

One question: The Remove path now only deletes inputs owned by this arkTxid. If a concurrent Add by a different owner races with Remove (e.g. after a partial round failure), is there a scenario where inputs stay claimed beyond their intended lifetime? Specifically: can ClaimAlreadyOwned on a re-submit prevent the original owner's Remove from cleaning up? Looking at the logic: Remove deletes only entries where owner == arkTxid, so a different-owner conflict would never be cleaned up by the original owner's Remove. But by design, a conflict returns ClaimConflict and the new owner never gets registered — so there's no cross-owner input left dangling. This looks correct.

Test coverage is good: disjoint claim, idempotent re-claim, all-or-nothing conflict, owner-scoped release, and a concurrent stress test.

Looks ready. Not protocol-critical in the tree-signing sense, but double-spend prevention is critical.

First review — #1162 (owner-tagged atomic single-spend domain)

This PR introduces an important architectural change: the OffChainTxStore becomes the canonical cross-process single-spend conflict domain, shared by both off-chain and on-chain (Arkade) spends, with owner-tagged outpoint registration.

Design assessment:

• The ClaimStatus enum (ClaimFresh / ClaimAlreadyOwned / ClaimConflict) is a clear, well-named three-way outcome. The semantics are well-documented in the interface comment.
• Moving from a per-process offchainTxMu + recheck pattern to an atomic store operation (Redis Lua / in-memory lock) is the correct progression — it's the only option that works across processes.
• ClaimAlreadyOwned (idempotent retry by the same owner) is handled in SubmitOffchainTx by setting changes = nil so no duplicate Accepted event is written. This is correct.
• The new ClaimOutpoints / ReleaseOutpoints methods for on-chain Arkade UTXOs (issue #1159) extend the same domain cleanly.

Questions / observations:

1. The in-memory store's snapshot in Get (maps.Clone) is a good race fix, but SigningContext.AggregatedNonces is a slice and isn't cloned. If callers mutate it concurrently, there's still a data race — worth checking if that path exists.
2. When ReleaseOutpoints is called for an owner, outpoints held by a different owner are left untouched — this is the documented behaviour. Confirm that the on-chain flow that calls this never races with an off-chain claim of the same outpoint in a way that could release the wrong owner's claim.
3. The PR description mentions this is part of #1159 — dependent PRs (#1161, #1160) should be reviewed in sequence before this lands.

Overall the design is sound. The atomic store semantics are correct. ⚠️ PROTOCOL-CRITICAL — flagging for human sign-off before merge.

Arkana review — #1162 (sha bf4c653, updated from ad348b9)

live-store: owner-tagged atomic single-spend domain

Protocol-critical — changes the SubmitOffchainTx double-spend barrier. Human sign-off required.

Key changes since sha ad348b9

• offchainTxMu per-process mutex removed from service struct
• SubmitOffchainTx drops the per-process recheck-under-mutex pattern; the store's Add() is now the single barrier
• Add() returns a status enum: ClaimFresh, ClaimConflict, ClaimAlreadyOwned
• Retry path for idempotent submits: ClaimAlreadyOwned returns success without re-emitting Accepted
• releaseClaimsOfIntents / releaseClaimsOfIntentIds added; DeleteIntents now calls release before delete
• Comprehensive test in intent_claims_test.go

Looks correct

• The claim tag being the intent/tx owner id (not global) is the right design: cross-owner release would clobber valid claims
• ClaimAlreadyOwned path sets changes = nil before returning — no duplicate event in projections
• The "re-read before claiming" comment and reference to ArkLabsHQ/arkd#71 give good context
• Tests verify per-owner keying, lookup-by-id path, boarding-only intent (no inputs = no claims), lookup failure isolation, and delete-all claim release

One concern to verify

The script_watch_test.go change removes offchainTxMu: &sync.Mutex{} from service initialization. Confirm no other test or code path in the file initializes service with a mutex expectation that would now panic or dereference nil.

Question

In the ClaimConflict branch, conflict.String() is used in the error — does this always produce a useful outpoint string, or can it return an empty/zero value for non-vtxo input types (e.g. boarding inputs)?

Verdict: Looks ready. The atomicity move from per-process mutex to store-level claim is the right architectural direction for a multi-process deployment.

PROTOCOL-CRITICAL: human review required.

Follow-up pass — 1 commit since 562db18 (1058a24). All 3 prior issues are now closed. No new findings.

Prior-issue closure

1. Blocking — Duplicate release path (service vs. admin) → Fixed (562db18, unchanged)
Shared helpers in intent_claims.go mechanically enforce the invariant. Closed.

2. Minor — Misleading rebuild log count → Fixed (562db18, unchanged)
Counter increments only when added > 0. Closed.

3. Non-blocking — 2-minute rebuild timeout operator documentation → Fixed (1058a24)
NewOffChainTxStore now emits a timeout-specific error:
"failed to rebuild offchain tx inputs from stored txs within %s: %v" (with rebuildTimeout interpolated) when ctx.Err() != nil after rebuildInputs returns, and falls through to the generic message when the context is live. A docstring on NewOffChainTxStore also explains why the constructor fails fatally rather than degrading (partial rebuild = ClaimFresh answers for every in-flight tx it didn't reach). This is the right place to put it. Closed.

Incremental review of 1058a24

This commit is documentation and error-message only. No logic was changed.

internal/core/ports/live_store.go:87–95
The new comment on Add correctly states the arkTxid invariant: because an arkTxid is the txid of the ark transaction committing to the checkpoint outputs it spends, a different input set implies a different arkTxid, so the same-owner idempotence path can only be reached with identical inputs. The documented failure mode ("inputs only the previous body named would stay claimed") is accurate and useful — it describes exactly what Remove would miss if the invariant were ever broken. No issue.

internal/infrastructure/live-store/redis/offchain_txs.go:149–159
The ctx.Err() != nil check is correct for distinguishing deadline expiry from other errors. One edge case is benign: if rebuildInputs returns a non-context error that coincides with the deadline having just passed, the operator sees the "within timeout" message rather than the underlying error — but %v in the message still includes the wrapped error text, so it is not silenced. Acceptable.

No protocol, security, or correctness findings in this increment. The single-spend enforcement path reviewed in prior passes is unchanged.

Arkana review — live-store: updated (claim release on DeleteIntents)

Since my last review (sha ad348b9), the key addition is intent_claims.go and the rework of DeleteIntents in the admin service.

The bug fixed: the previous DeleteIntents implementation wiped the store directly or called Delete without releasing claims first. If an intent holding vtxo claims was deleted without releasing them, those vtxos would remain claimed indefinitely — locked out of future rounds with no recovery path.

Fix is correct:

• Snapshot intents first, then release their claims, then delete exactly that snapshot by id. This avoids the TOCTOU risk of releasing an intent that was registered after the snapshot (whose claim was never released and which round-start reconcile can't see since it was never popped).
• releaseClaimsOfIntents logs failures rather than returning them, because the delete path proceeds regardless, and a lost release is a stale claim rather than a caller-visible error.
• The test "the service and the admin service release identically" is particularly valuable: it pins the contract that both code paths release through the same helper, preventing drift.

Redis init: the NewLiveStore error is now surfaced — a silent init failure would produce a broken live store.

Test coverage is comprehensive. Looks ready to merge. ✓

Arkana follow-up review — b0ca2a6

Status: One cosmetic commit since the last pass; 3 of 3 prior code findings are fixed, 1 ops item remains open. No new code findings.

What changed

Commit b0ca2a6b0b ("live-store: drop colon syntax and an issue reference from the comments") rewrites comment text across seven files — grammar, punctuation, and prose style. No logic is touched in any of those 20 changed lines.

Prior findings — close-out

Fixed — wire.OutPoint.String() vs domain.Outpoint.String() format mismatch
internal/infrastructure/live-store/redis/offchain_txs.go:420–425
checkpointInputs now constructs a domain.Outpoint{Txid: ..., VOut: ...} and calls .String() on it, with an explicit comment tying it to ClaimOutpoints, ReleaseOutpoints, and Includes. Fixed at the baseline commit (1058a24003) by the "fail closed on a failed rebuild, parse checkpoint inputs all or nothing" commit, predating this increment.

Fixed — offchainTxMu removal, no nil-deref survivors
Answered by @bitcoin-coder-bob: the struct field, every initialisation (including script_watch_test.go), and every call site are gone. The only remaining mention is a comment in service.go describing what the claim replaced. Confirmed clean.

Fixed / No longer applicable — conflict.String() shape on boarding inputs
Answered by @bitcoin-coder-bob: claims are only taken over VTXO inputs. A boarding-only intent takes no claims and is covered by its own test. The ClaimConflict branch is therefore unreachable for non-VTXO input types; no zero-value or empty conflict outpoint can surface there.

Still open (ops) — Redis key-type migration window
The key rename from offChainTxStore:inputs (SET) to offChainTxStore:inputsByOwner (HASH) avoids a WRONGTYPE error on a mixed-version rolling deploy, but there is a brief window during the upgrade where in-flight off-chain txs that accepted under the old version retain their stored body without re-registration in the new hash until the first restart of an upgraded instance. The PR description explains the rebuildInputs startup path that closes this, and the commit logs describe the fallback. This needs ops sign-off on the upgrade sequencing (quiesce or accept the window) — it is not a code defect, and nothing in this increment changes the situation.

No new findings from the increment. Protocol-critical flag from the prior pass stands — human sign-off before merge is still the right call.

Arkana review — live-store: owner-tagged intent claim release

Protocol-critical (vtxo claim management). Flagging for human sign-off.

This PR was reviewed at sha 49ec1a83 on 2026-09-04 (questions about AggregatedNonces clone race and ReleaseOutpoints race). Re-reviewing the updated diff.

What changed since last review:

• DeleteIntents (admin) now snapshots intents, releases their claims, then deletes exactly the snapshot. This fixes the race where a delete-all could wipe an intent that registered after the snapshot, leaving its vtxo claims stranded.
• releaseClaimsOfIntents is now a shared helper used by both the service and adminService paths, preventing them from drifting.
• intent_claims.go + comprehensive tests in intent_claims_test.go covering: per-owner release, lookup-by-id, boarding-only intents (nothing to release), lookup failures, round-start selected-intent release, delete-by-proof, and the delete-all snapshot race.
• Redis live store constructor now returns an error (NewLiveStore → err checked in config).

Looks solid.

• The snapshot-then-delete pattern in admin DeleteIntents correctly handles the race. The test "admin delete-all keeps an intent registered after the snapshot" directly exercises it with an afterViewAll hook — well done.
• releaseClaimsOfIntents is idempotent: failed releases are logged rather than surfaced, which is correct since the caller drops the intent regardless.
• The "the service and admin service release identically" test pins the two paths against each other, which is exactly the right regression guard.

Previously raised questions:

1. AggregatedNonces clone race — is that concern addressed in this diff or a follow-up?
2. ReleaseOutpoints concurrency — the owner-tagging design means concurrent releases for different owners are independent; a concurrent release for the same owner on the same outpoint is idempotent. Looks addressed by the design.

Looks ready pending confirmation that the AggregatedNonces concern from the previous review has been resolved.

Arkana follow-up — #1162 (d860341, incremental from b0ca2a6)

Status: 2 commits since last pass, both comment-only (no logic changes). 3 prior issues fixed, 1 still open, 1 silently dropped in earlier passes now acknowledged.

Prior issue tracking

1. offchainTxMu nil-dereference concern — Fixed
Confirmed by @bitcoin-coder-bob: the field and every initialisation site are gone. No code path can dereference it. Closed.

2. conflict.String() for non-vtxo input types — Fixed
Confirmed by @bitcoin-coder-bob: boarding-only intents take no claims and cannot reach the ClaimConflict branch. The conflict value is always a typed *domain.Outpoint. Closed.

3. Comment colon style — Fixed (commits e94baf3, d860341)
These two commits are the entire incremental diff. They fix the remaining mid-sentence and end-of-line colons in redis/offchain_txs.go and live_store_test.go that earlier passes missed. All three comment-only commits collectively address the prose style feedback. No logic changed.

4. Rolling-upgrade / Redis key-type quiescing — Still open
The old offChainInputsSetKey (Redis SET) is not migrated to the new offChainInputsHashKey (Redis HASH). A rolling deploy leaves a window where an in-flight tx retains its stored body but loses input-claim coverage when the old key expires. @bitcoin-coder-bob explicitly flagged this as needing ops sign-off rather than code review. That sign-off has not appeared in this thread. This must be resolved — via a documented upgrade runbook, a one-time migration step, or explicit acceptance — before the PR lands.

5. SigningContext.AggregatedNonces slice not deep-copied — Acknowledged as silently dropped
I raised this in the first pass ("the AggregatedNonces slice inside the maps.Clone snapshot isn't cloned; concurrent mutation is still a data race if that path exists") and then dropped it from subsequent reviews without closure. The incremental diff does not touch that code, so I cannot close it here. Before merge, confirm whether any call site mutates the nonce slice after Get returns, and if so add a slices.Clone or equivalent. If no call site mutates it, note that explicitly so future reviewers do not re-raise it.

Incremental diff assessment

No logic was changed in these two commits. The substantive review remains as filed. The single blocking item is rolling-upgrade ops sign-off (item 4 above).

Arkana review — #1162 (sha d860341)

live-store: owner-tagged atomic single-spend domain (updated)

This is an update since my previous review. The new intent_claims.go / intent_claims_test.go files are the main addition.

What changed / review

releaseClaimsOfIntents helper (intent_claims.go):

• Centralises the claim-release logic that previously had to be duplicated between service and admin paths.
• Release is per-owner and idempotent — a claim already gone or taken by someone else is left alone. Correct.
• Failure is logged-not-returned on purpose: the caller always drops the intent regardless, so a stale claim (bad but survivable) is better than surfacing an error that hides the real operation.

DeleteIntents fix (admin.go):

• Old code: DeleteAll when no IDs passed, which wiped the store atomically without releasing claims.
• New code: snapshot → release claims → delete by ID. Correctly avoids deleting an intent registered after the snapshot (whose claim was never released).
• Test admin delete-all keeps an intent registered after the snapshot exercises this race explicitly — good.

config.go fix:

• NewLiveStore now returns (LiveStore, error), and the error is propagated. This was a latent bug (unchecked init failure for Redis live store).

Tests (intent_claims_test.go):
Very comprehensive: covers boarding-only intents (no release), lookup failure (no panic, no wrong release), round-start selected-intent release (only non-queued ones), delete-by-proof, admin delete-all with concurrent registration, and the symmetry test that proves the service and admin service use the same release path. Coverage is excellent.

Minor note

The releaseClaimsOfSelectedIntents and releaseClaimsOfIntentIds methods on service — these appear in tests but weren't in the diff snippet. Assuming they call through to releaseClaimsOfIntents as the tests imply.

Looks ready to merge. The original review concern (unchecked DeleteAll leaking claims) is addressed.

Review — #1162 live-store: owner-tagged atomic single-spend domain

⚠️ PROTOCOL-CRITICAL (double-spend prevention) — flag for human review before merge.

The DeleteIntents fix and releaseClaimsOfIntents consolidation look correct.

DeleteIntents fix (admin.go). The old code called DeleteAll (wipe the store) or Delete(ids) without releasing claims first. The new path snapshots with ViewAll, releases claims for each intent under its own id, then deletes exactly the snapshotted set. The comment explaining the race is accurate: a wipe would also drop an intent registered after the snapshot whose claims were never released, leaving vtxos claimed forever. This is the right fix.

releaseClaimsOfIntents (intent_claims.go). Extracting the release logic to a package-level helper shared by all delete paths is the right call. The comment on "two copies of this that drifted apart" explains the motivation clearly. Error on release is logged rather than returned — intentional, the comment gives the right reasoning.

Tests (intent_claims_test.go). The table covers: per-owner release, id-only lookup path, boarding-only intent (no claims), lookup failure, round-start selected-intent release, and delete-by-proof. The claimIntentStore.selected vs all test for round start is the tricky one and it's correct: only intents in selected but not in all (i.e., those not re-queued) should release.

One small observation: the recordingOffchainTxStore mock accumulates releases per-owner in a map. A test that calls release twice with the same owner would not detect duplicate releases. Not a blocking concern, but worth keeping in mind if new callers are added.

Arkana review · sha d860341f

Summary: Fixes a claim-leak bug in DeleteIntents and centralises the intent-claim-release logic into a single helper that both the service and admin service share.

Correctness of the fix:
The old DeleteAll() path wiped the store atomically, which could delete an intent registered after the snapshot started, leaving its vtxo claims held with no round cycle to release them. The fix snapshots first (ViewAll), releases claims against that snapshot, then deletes only the snapshotted IDs. This is the correct ordering.

The afterViewAll injection in the test ("admin delete-all keeps an intent registered after the snapshot") cleanly demonstrates the race that was possible and verifies that post-snapshot intents survive.

Observations:

• releaseIntentClaims logs rather than returns an error on release failure. The comment explains why: the caller drops the intent regardless, and a failed release leaves a stale claim rather than a caller-visible error. This is a deliberate trade-off and the rationale is clear.
• The releaseClaimsOfSelectedIntents logic for round-start is a nice fix: it only releases intents that were selected but are no longer queued, not all selected intents, preventing double-release of a re-pushed intent.
• releaseClaimsOfIntents / intentsOf are well-decomposed. The cross-test ("the service and the admin service release identically") is an excellent guard against drift between the two paths.
• Removal of offchainTxMu from service: this field disappears from the struct, initialisation, and tests. Confirm that no other branch still touches it.

Verdict: Looks ready. This is an important correctness fix with excellent test coverage. The shared claim-release helpers are the right architecture.

Arkana review — sha d860341 ⚠️ PROTOCOL-CRITICAL (claim management) — flag for human review before merge.

What this does: three related correctness fixes in one PR:

1. DeleteIntents claim leak fix (admin): Previously with no ids would call , dropping intents without releasing their vtxo claims. Now it snapshots via , releases claims from the snapshot, then deletes by the snapshot's ids. A registration racing between the snapshot and the delete is not deleted (correct — its claim is still active). The test verifies this precisely.

2. ** RWMutex:** A new R/W mutex brackets registrations (R-locked for duration of each registration) and is write-locked while a finished batch's claims are released. This closes the window where a registration could read an input as unspent after the batch settled but before its projection landed. The naming is clear and the locking pattern is correct.

3. ** → :** Generalises from waiting on a single batch id to waiting on all currently-tracked batches. A batch that doesn't land its projection within the timeout stays tracked (correct — the next call covers it). Previously a single missed projection could permanently drop tracking.

4. Remove : The field is removed from struct and test fixtures. The protection it provided is now handled by the RWMutex above.

Tests are comprehensive — the suite covers every path including the race-condition scenario and the 'service and admin service release identically' invariant test, which is exactly the right thing to pin.

One nit: in , logs rather than returning the error 'since the caller is always on a path that drops the intent regardless'. The comment is a multi-line block with an awkward line break ('since the caller\n// is always…'). Minor style issue only.

Overall: this is a careful correctness fix with excellent tests. Looks ready. Human sign-off required before merge.

Arkana review — sha d860341 — PROTOCOL CRITICAL

⚠️ Flagging for mandatory human review — this changes how intent claims are released on admin delete.

DeleteIntents fix:
The old path called DeleteAll / Delete directly, which wiped the store without releasing conflict-domain claims. The new path: (1) snapshots intents via ViewAll, (2) releases their claims, (3) deletes exactly that snapshot. This closes the claim-leak on admin delete.

The comments in the code correctly identify the two failure modes of the old approach:

• An intent registered after the snapshot would have its claims freed but never be deleted.
• The selected-intent set read by reconcile would be dropped, leaking claims of round-in-progress intents.

New intent_claims.go:

• releaseIntentClaims is idempotent and owner-tagged — a release under the wrong id is a no-op, not a corruption.
• releaseClaimsOfIntents is package-level so every claim-dropping path shares one implementation — drift between paths was the root cause of prior leaks.
• Logging instead of returning errors on release failure is correct: the intent is being dropped anyway, a lost release is recoverable, and returning an error would block the delete.

Test coverage:
Tests exercise: correct owner tagging, id-based lookup, boarding intents (no vtxo inputs → no release), store-down path, and the round-start reconcile case. Good.

Looks ready to merge. ✅ (human sign-off for protocol-critical change)

Review — arkade-os/arkd #1162 — live-store: owner-tagged atomic single-spend domain (sha d860341)

PROTOCOL-ADJACENT — intent claim management, liveness of vtxo spend paths — human review recommended.

Summary: The core change is a correctness fix in DeleteIntents and the centralisation of claim release. Looks ready to merge.

The bug being fixed:
The old DeleteAll path wiped the intent store, which could delete an intent registered after the snapshot and whose vtxo claims were never released. Those claims would then be permanently held, making the vtxos unspendable. The new path: snapshot → release claims by id → delete exactly the snapshot. A registration racing between snapshot and delete is left intact.

New intent_claims.go:

• releaseClaimsOfIntents is shared by both service and adminService, eliminating the risk of the two paths drifting.
• releaseIntentClaims logs rather than returns errors (correct — the caller is always dropping the intent anyway; a lost release is a stale claim, not a fatal error, and reporting it as fatal would be worse).

Tests are comprehensive: covers the main paths (by-id, by-proof, delete-all, delete-all keeps post-snapshot registration, error handling, cross-service consistency). The "service and admin service release identically" test is particularly valuable for catching future divergence.

Stale redis live store error handling: the diff shows liveStoreSvc, err = redislivestore.NewLiveStore(...) where the original ignored the error. Good fix.

No concerns. Looks ready.

Live-store correctness fix for intent deletion.

What changed and why it matters

The old DeleteAll path wiped the store, which would:

1. Drop intents registered after the snapshot was taken, leaving their vtxos claimed with no round to release them.
2. Remove the selected-intent set that round-start reconcile reads, leaking claims from the running round.

The fix snapshots intents first, releases their claims (owner-tagged and idempotent), then deletes the exact snapshot by id. Any intent registered between snapshot and delete survives untouched.

Tests are excellent

TestIntentClaimRelease covers the full surface area: delete-by-proof, delete-all safety (intent registered after snapshot is kept), the service-vs-admin-service equivalence test, and the round-start selected-intent release. The equivalence test in particular is a good guard against the two paths drifting.

One comment on error handling

releaseIntentClaims logs rather than returning the error, with the rationale that a failed release leaves a stale claim (bad but recoverable) while turning it into a hard error would block intent deletion entirely (worse). That's a reasonable trade, but the comment should note what "stale claim" means for the operator — specifically that the vtxo stays locked until the claim TTL expires or the server restarts, whichever applies to the live-store backend.

Minor nit: intentsOf strips timing metadata to get to the domain type. It would be clearer as a method on TimedIntent rather than a package-level function, but that is a style preference.

Looks ready to merge.

Arkana review — 2026-09-18

Summary

Fixes a claim-leak bug in DeleteIntents: the old code called Intents().DeleteAll() or Delete() without releasing the vtxo claims those intents held, leaving their inputs permanently claimed. The fix snapshots the intents first, calls releaseClaimsOfIntents, then deletes exactly that snapshot.

What looks good

• The fix correctly handles the race: intents registered after the snapshot are not in the delete list, so their claims are never released — they remain safe.
• The new intent_claims.go helper is package-level, shared by both the service and the admin service, with a test that verifies both paths produce identical release behaviour. This is exactly the right way to prevent drift.
• Extensive test coverage: boarding-only intents (no claims to release), lookup failure, round-start selected-intent release, delete-by-proof, and the critical service-vs-admin equivalence test.
• The ViewAll-then-Delete pattern is correctly atomic in spirit: if Delete fails, claims are already released (idempotent), which is the safe direction.

Minor

• The comment in admin.go explaining why DeleteAll was replaced (would silently drop running-round state) is excellent.

Looks ready to merge. The test that verifies service and admin service release identically is particularly valuable.

Arkana review — #1162 (sha d860341) — catchup pass

Prior reviews at ad348b9 and bf4c653 flagged the protocol-critical nature of this PR. This pass covers changes since bf4c653.

New in this diff

1. NewLiveStore returns error — NewOffChainTxStore now returns an error and NewLiveStore propagates it. The caller in config.go handles it correctly. This closes the gap where a store-init failure was silently swallowed.

2. DeleteIntents snapshot-then-release-then-delete — Replaces the old wipe-all path. The key invariant: the intent list is snapshotted first, claims are released under each intent's own owner-id, then only the snapshotted ids are deleted. A new intent registered after the snapshot isn't deleted and its claims aren't freed spuriously. ✅

3. intent_claims.go — releaseClaimsOfIntents and releaseIntentClaims are correctly owner-tagged and idempotent. Moving this to a package-level helper ensures all intent-drop paths use the same release logic. ✅

4. e2e test — Destinations are now randomly generated, avoiding the same-txid-idempotent problem in the double-spend test. Good fix.

Previously raised issues — status

• Redis Lua atomic check-and-claim: addressed in prior commits.
• offchainTxMu removal: done.
• The prior CHANGES_REQUESTED concerned issues in those earlier commits. Nothing in the current diff reintroduces them.

Remaining concern

Confirm that releaseClaimsOfIntents is also called from the round-start reconcile path when intents are evicted, not only from DeleteIntents. If a round ends and intents are pruned without going through DeleteIntents, claims could leak.

⚠️ PROTOCOL-CRITICAL — still requires human sign-off before merge. The atomic single-spend domain is the core double-spend barrier. Current code looks correct; the main remaining question is the reconcile path above.