fix(walletstore): reconcile local-spend placeholder to the confirmed height (#101)

docs/hardening-backlog.md

@@ -34,7 +34,7 @@ All items below survived an independent skeptic re-reading the cited code.
 | 4 | 🟠 high | small | reliability | `addrpool-monotonic-drain` | ✅ done (#93) |
 | 5 | 🟠 high | small | correctness | `cas-toolarge-misclassified-retryable` | ✅ done (#87) |
 | 6 | 🟠 high | small | resource | `resolution-http-server-no-timeouts-public-bind` | ✅ done (#89) |
-| 7 | 🟠 high | small | data-integrity | `walletstore-localspend-reconcile-untested-and-broken` | open |
+| 7 | 🟠 high | small | data-integrity | `walletstore-localspend-reconcile-untested-and-broken` | ✅ done (#101) |
 | 8 | 🟡 medium | trivial | resource | `kdf-params-unbounded-pre-auth-dos` | ✅ done (#97) |
 | 9 | 🟡 medium | trivial | data-integrity | `indexer-hashat-blockat-toctou` | ✅ done (#95) |
 | 10 | 🟡 medium | small | security | `secrets-on-command-line-no-prompt` | open |

internal/walletstore/bbolt.go

@@ -82,9 +82,11 @@ func (s *boltStore) ApplyBlock(height int32, hash, prevHash string, created []UT
 			}
 		}
 
-		// Apply spends of tracked outpoints (ignore those we do not track).
+		// Apply spends of tracked outpoints (ignore those we do not track). Use the
+		// confirming path so a UTXO previously marked spent at a placeholder height
+		// (MarkSpentLocal) is reconciled to this real block height.
 		for _, op := range spent {
-			if err := markSpent(bUtxos, bSpent, op, height); err != nil {
+			if err := markSpentConfirmed(bUtxos, bSpent, op, height); err != nil {
 				return err
 			}
 		}
@@ -254,6 +256,9 @@ func (s *boltStore) SetBirthday(height int32) error {
 // --- helpers ---
 
 // markSpent flags a tracked outpoint spent at height; a no-op for untracked ones.
+// It does NOT overwrite an already-spent UTXO — used by the optimistic local-spend
+// path (MarkSpentLocal), which records a placeholder height a later confirmation
+// reconciles via markSpentConfirmed.
 func markSpent(bUtxos, bSpent *bolt.Bucket, op OutPoint, height int32) error {
 	v := bUtxos.Get(outpointKey(op))
 	if v == nil {
@@ -277,6 +282,44 @@ func markSpent(bUtxos, bSpent *bolt.Bucket, op OutPoint, height int32) error {
 	return bSpent.Put(spentKey(height, op), outpointKey(op))
 }
 
+// markSpentConfirmed records a tracked outpoint spent at its REAL confirmed block
+// height (the ApplyBlock path). Unlike markSpent, it RECONCILES an existing
+// placeholder spent height to the confirmed one — moving the spent-index entry from
+// spentKey(old,op) to spentKey(height,op) — so the reorg un-spend (which scans the
+// spent index from the fork height up) correctly orphans a spend whose confirmation
+// is rolled back. Without this, a UTXO locally-spent at placeholder X and confirmed
+// at Y>X survives a reorg forking in (X,Y]: the stale index entry at X is below the
+// fork, so the un-spend misses it and the real-BTC funding UTXO is silently burned.
+// A no-op for untracked outpoints and for a spend already recorded at this height.
+func markSpentConfirmed(bUtxos, bSpent *bolt.Bucket, op OutPoint, height int32) error {
+	v := bUtxos.Get(outpointKey(op))
+	if v == nil {
+		return nil // not one of ours
+	}
+	u, err := decodeUTXO(v)
+	if err != nil {
+		return err
+	}
+	if u.Spent && u.SpentHeight == height {
+		return nil // already reconciled (idempotent re-apply)
+	}
+	if u.Spent {
+		// Drop the stale (placeholder or prior-height) index entry before re-keying.
+		if err := bSpent.Delete(spentKey(u.SpentHeight, op)); err != nil {
+			return err
+		}
+	}
+	u.Spent, u.SpentHeight = true, height
+	val, err := encodeUTXO(u)
+	if err != nil {
+		return err
+	}
+	if err := bUtxos.Put(outpointKey(op), val); err != nil {
+		return err
+	}
+	return bSpent.Put(spentKey(height, op), outpointKey(op))
+}
+
 // clearCreatedAtHeight removes every UTXO (and its index entries) created at
 // exactly height — the whole-height-replace step.
 func clearCreatedAtHeight(bUtxos, bCreated, bSpent *bolt.Bucket, height int32) error {

internal/walletstore/sqlite.go

@@ -86,7 +86,13 @@ func (s *sqliteStore) ApplyBlock(height int32, hash, prevHash string, created []
 		}
 	}
 	for _, op := range spent {
-		if _, err := tx.Exec(`UPDATE utxos SET spent=1, spent_height=? WHERE txid=? AND vout=? AND spent=0`,
+		// Reconcile to the REAL confirmed height: no `AND spent=0` guard, so a UTXO
+		// previously marked spent at a placeholder height (MarkSpentLocal) has its
+		// spent_height updated to this block's height. The reorg un-spend is keyed on
+		// spent_height (`WHERE spent=1 AND spent_height >= forkHeight`), so leaving a
+		// stale placeholder below the fork would let a reorg silently burn a real-BTC
+		// UTXO whose spend was orphaned.
+		if _, err := tx.Exec(`UPDATE utxos SET spent=1, spent_height=? WHERE txid=? AND vout=?`,
 			height, op.TxID, op.Vout); err != nil {
 			return err
 		}

internal/walletstore/store_test.go

@@ -224,3 +224,40 @@ func TestParity(t *testing.T) {
 		t.Errorf("spendable count parity: bolt=%d sqlite=%d", len(bsp), len(ssp))
 	}
 }
+
+// TestLocalSpendReconcilesToConfirmedHeight guards #7: a UTXO marked spent at an
+// optimistic placeholder height (MarkSpentLocal at broadcast) must have its spent
+// height reconciled to the REAL confirmed height by ApplyBlock. Otherwise a reorg
+// forking between the placeholder and the confirmation leaves the now-orphaned spend
+// below the un-spend's fork-height scan, silently burning a real-BTC funding UTXO.
+func TestLocalSpendReconcilesToConfirmedHeight(t *testing.T) {
+	eachEngine(t, func(t *testing.T, s Store) {
+		a := mkUTXO(1, 0, 50_000, 100)
+		apply(t, s, 100, []UTXO{a}, nil)
+		fill(t, s, 100, 102)
+
+		// Optimistic local spend at the current tip (placeholder height 102).
+		if err := s.MarkSpentLocal([]OutPoint{a.OutPoint}, 102); err != nil {
+			t.Fatalf("MarkSpentLocal: %v", err)
+		}
+		if bal, _ := s.Balance(1, 102); bal != 0 {
+			t.Fatalf("after local spend, balance = %d, want 0", bal)
+		}
+
+		// The spend confirms on-chain at height 105 (above the placeholder).
+		fill(t, s, 102, 104)
+		apply(t, s, 105, nil, []OutPoint{a.OutPoint})
+		if bal, _ := s.Balance(1, 105); bal != 0 {
+			t.Fatalf("after confirmation, balance = %d, want 0 (still spent)", bal)
+		}
+
+		// A reorg forks at 103 — between the placeholder (102) and the confirmation
+		// (105) — orphaning the spend. The UTXO must become spendable again, NOT burned.
+		if err := s.Rollback(103); err != nil {
+			t.Fatalf("Rollback: %v", err)
+		}
+		if bal, _ := s.Balance(1, 102); bal != 50_000 {
+			t.Errorf("after the reorg orphaned the spend, balance = %d, want 50000 (UTXO must not be burned)", bal)
+		}
+	})
+}