diff --git a/docs/hardening-backlog.md b/docs/hardening-backlog.md index c236f93..8ff26e9 100644 --- a/docs/hardening-backlog.md +++ b/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 | diff --git a/internal/walletstore/bbolt.go b/internal/walletstore/bbolt.go index 4a66a7e..5fb6297 100644 --- a/internal/walletstore/bbolt.go +++ b/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 { diff --git a/internal/walletstore/sqlite.go b/internal/walletstore/sqlite.go index 05d089c..708b953 100644 --- a/internal/walletstore/sqlite.go +++ b/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 } diff --git a/internal/walletstore/store_test.go b/internal/walletstore/store_test.go index e887058..c034933 100644 --- a/internal/walletstore/store_test.go +++ b/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) + } + }) +}