fix(miner): persist intent before broadcast to prevent dest double-send (#296)#299
Open
RUNECTZ33 wants to merge 1 commit intoentrius:testfrom
Open
fix(miner): persist intent before broadcast to prevent dest double-send (#296)#299RUNECTZ33 wants to merge 1 commit intoentrius:testfrom
RUNECTZ33 wants to merge 1 commit intoentrius:testfrom
Conversation
…nd (entrius#296) S0 race: process_swap broadcasts the destination tx via send_dest_funds, then writes the SentSwap record to the in-memory dict + save_sent_cache(). If the miner is killed (SIGKILL/OOM/hardware fault/container shutdown) between the broadcast returning and the persist completing, the destination tx is on-chain but the cache has no record. On restart, sent is None again, and process_swap broadcasts a SECOND destination tx for the same swap. The BTC provider's in-process broadcasted_txids set is also empty after restart, so its dedup is gone. Fix: persist a pending sentinel (SentSwap with empty to_tx_hash) BEFORE calling send_dest_funds. Three outcomes: 1. Crash between sentinel-persist and broadcast — restart sees the pending sentinel, refuses to process this swap, surfaces critical log. 2. Crash after broadcast but before post-broadcast cache update — same outcome: pending sentinel triggers restart-side refusal + critical log, no double-broadcast. 3. Broadcast fails cleanly (send_dest_funds returns falsy) — we drop the sentinel before returning, so the next pass can retry without a stuck pending entry. On restart, load_sent_cache scans for entries with empty to_tx_hash. If any exist, log critical with the swap IDs and tell the operator to scan the dest chain for tx FROM the miner address matching each swap, then update or delete the cache entry. process_swap refuses to operate on pending sentinels to ensure the operator sees the warning before any further send. Trade-off: if the crash window is hit on a real swap, fulfillment for that swap stalls until manual reconciliation. That's strictly better than double-broadcasting funds: a stuck swap can be recovered (look up tx, update cache); a double-spend cannot. Scope: allways/miner/fulfillment.py, +43/-1. No SentSwap dataclass change (empty string in to_tx_hash is the sentinel — backward compatible with the existing 3-field cache schema). No new imports. Closes entrius#296
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #296. S0 race — preventable on-chain double-spend.
process_swapbroadcasts the destination tx viasend_dest_funds(), then writes theSentSwaprecord to the in-memory dict +save_sent_cache():If the miner is killed between the broadcast returning and the persist completing (SIGKILL/OOM/hardware fault/container shutdown), the destination tx is on-chain but the cache has no record. On restart,
sent is None→process_swapbroadcasts a second destination tx for the same swap. The BTC provider's in-processbroadcasted_txidsset is also empty after restart, so its dedup is gone.Fix: persist intent before side effect
Write a pending sentinel (
SentSwapwith emptyto_tx_hash) to the cache before callingsend_dest_funds. Three outcomes are now safe:send_dest_fundsreturns falsy)Trade-off acknowledged: if the crash window is hit on a real swap, that single swap's fulfillment stalls until operator reconciles (look up tx by scanning dest chain for miner-address sends matching the swap, then update or delete the cache entry).
That trade is strictly correct: a stuck swap can be recovered; a double-spend cannot.
Change
allways/miner/fulfillment.py: +43 / -1. NoSentSwapdataclass change (emptyto_tx_hashis the sentinel — backward compatible with the existing 3-field cache schema). No new imports.Plus
load_sent_cachescans for empty-to_tx_hashentries on startup and logsbt.logging.critical(...)with the swap IDs + reconciliation instructions.Plus
process_swapchecks for the sentinel up-front and refuses to operate, surfacing abt.logging.warningso the operator sees the same recovery hint each retry pass.Test plan
python3 -c "import ast; ast.parse(open('allways/miner/fulfillment.py').read())"[to_tx_hash, to_tx_block, marked_fulfilled]array works for sentinels (empty stringto_tx_hash, zero block) and real entries identically.cleanup_stale_sendsstill drops sentinels alongside completed entries when the swap leaves the active set.process_swap+load_sent_cacheflow.test_crash_between_send_and_cache_causes_double_broadcast) — happy to add if reviewer prefers.