diff --git a/docs/REMEDIATION_PLAN.md b/docs/REMEDIATION_PLAN.md index c79c495..addc9b3 100644 --- a/docs/REMEDIATION_PLAN.md +++ b/docs/REMEDIATION_PLAN.md @@ -19,7 +19,7 @@ Key: `S` ≤ half day · `M` ~1–3 days · `L` ~1 week+ · 🔴 launch-blocker - [x] **0.1 🔴 S — Verify amount received ≥ expected before confirming.** ✅ Gate on `status.complete` in `paymentHandlers.js`; underpaid → stay pending + `payment_underpaid` warning (once). `received_amount` now recorded for single-game + credits. Regression test added (`payment_flow.integration.test.js`). _113 tests green._ - [x] **0.2 🔴 M — Make provably-fair real.** ✅ Per-game seeded RNG (`seedToInt`) threaded through `dungeon.js` (Digger reseed + all `Math.random()` → `rng`), `monster.js` (`seededShuffle`), `game.js` (chase + generation). `DungeonGenerator.regenerateFromSeed` + `layoutFingerprint`; `/api/verify` regenerates & returns fingerprint. Determinism test suite added (`provablyFair.test.js`). _Verifier can now reproduce the exact dungeon from the seed._ -- [ ] **0.3 🔴 M — Fix batch-payout `tx_hash` collision + double-pay.** Stop writing one `tx_hash` to every row (`src/game/gameModeManager.js:1306`). Use per-destination hashes or a `batch_id`; ensure on-chain rows are never re-sent by retry. *Test: 3-dest batch → all completed, retry finds nothing.* +- [x] **0.3 🔴 M — Fix batch-payout `tx_hash` collision + double-pay.** ✅ Migration 015 drops the payouts `tx_hash` unique index (batches legitimately share a tx_hash) and extends the one-per-game guard to cover `processing`. Batch completion now happens in ONE transaction via `id = ANY` (no row stranded mid-batch); BIGINT stat math (no float). Ambiguous batch failures → `needs_review` (retry service won't re-send) + operator alert. New `batchPayout.test.js` covers both paths. - [ ] **0.4 🔴 S — Harden identity transport.** Move `anon_token` from query string to handshake `auth` (`src/network/sessionManager.js:24`); call `rotateToken` on resume. - [ ] **0.5 🟠 S — Single-game disconnect recovery.** Extend `recoverPendingPayments` (`src/network/sessionManager.js:158`) to handle `single_game`; stop silently deleting in-progress paid games (`src/network/socketHandlers.js:787`). - [ ] **0.6 🟠 S — Client + admin XSS and CSP.** Escape chat/admin fields (`html/js/network/socketHandlers.js:291,323`; `html/admin.html`); add CSP headers. diff --git a/src/game/gameModeManager.js b/src/game/gameModeManager.js index 71e0872..de4360e 100644 --- a/src/game/gameModeManager.js +++ b/src/game/gameModeManager.js @@ -1300,37 +1300,52 @@ class GameModeManager { } } else { // Multiple payouts — batch via transfer_split with multiple destinations + const ids = pending.rows.map(p => p.id); try { const result = await this.walletService.processBatchPayout(destinations); - // All destinations in one transfer_split typically produce 1 tx - const txHash = result.tx_hash_list[0]; + // transfer_split sends ONE on-chain tx to all destinations; every payout + // row in this batch legitimately shares that tx_hash (see migration 015). + const txHash = (result.tx_hash_list && result.tx_hash_list[0]) || null; const feePerPayout = result.totalFee ? Math.ceil(result.totalFee / pending.rows.length) : null; - // Store tx_hash + status + user stats atomically for each payout - for (const p of pending.rows) { - await this.db.withTransaction(async (client) => { + // Mark the ENTIRE batch completed in a SINGLE transaction so we never + // leave some rows completed and others stranded mid-batch (which the old + // per-row loop did when the shared tx_hash hit the unique index). + await this.db.withTransaction(async (client) => { + await client.query( + `UPDATE payouts SET tx_hash = $1, fee = $2, batch_id = $3, status = 'completed', processed_at = NOW() WHERE id = ANY($4)`, + [txHash, feePerPayout, batchId, ids] + ); + // Per-row stat increments inside the same transaction. Amounts are passed + // as DB values so Postgres does the BIGINT arithmetic (no JS float). + for (const p of pending.rows) { await client.query( - `UPDATE payouts SET tx_hash = $1, fee = $2, batch_id = $3, status = 'completed', processed_at = NOW() WHERE id = $4`, - [txHash, feePerPayout, batchId, p.id] - ); - await client.query( - `UPDATE users SET total_amount_won = total_amount_won + $1, total_payouts_received = COALESCE(total_payouts_received, 0) + 1 WHERE id = $2`, + `UPDATE users SET total_amount_won = total_amount_won + $1::bigint, total_payouts_received = COALESCE(total_payouts_received, 0) + 1 WHERE id = $2`, [p.amount, p.user_id] ); - }); - } + } + }); console.log(`💸 Batch payout completed: ${pending.rows.length} payouts in tx ${txHash}`); } catch (err) { console.error(`❌ Batch payout failed (${pending.rows.length} payouts):`, err.message); - // Mark all as failed — retry service will pick them up individually - for (const p of pending.rows) { - await this.db.query( - `UPDATE payouts SET status = 'failed', last_error = $1, last_retry_at = NOW() WHERE id = $2`, - [err.message, p.id] - ).catch(() => {}); + // CRITICAL: a batch failure is ambiguous — transfer_split may have already + // broadcast on-chain even though the RPC response errored. These rows have no + // tx_hash, so the retry service's blockchain guard can't protect them and an + // auto-retry could DOUBLE-PAY. Mark 'needs_review' (which the retry service + // does NOT pick up) and alert an operator instead. + await this.db.query( + `UPDATE payouts SET status = 'needs_review', last_error = $1, last_retry_at = NOW() WHERE id = ANY($2)`, + [String(err.message).slice(0, 500), ids] + ).catch(() => {}); + if (this.alertService && typeof this.alertService.sendAlert === 'function') { + this.alertService.sendAlert('batch_payout_failed', { + subject: '⚠️ Batch payout failed — manual review required', + html: `
A batch of ${pending.rows.length} payout(s) failed and was marked needs_review to avoid a possible double-payout.
` + + `Payout IDs: ${ids.join(', ')}
Error: ${err.message}
` + }).catch(() => {}); } } } diff --git a/src/migrations/015_payout_batch_tx_hash.sql b/src/migrations/015_payout_batch_tx_hash.sql new file mode 100644 index 0000000..e2f0a29 --- /dev/null +++ b/src/migrations/015_payout_batch_tx_hash.sql @@ -0,0 +1,20 @@ +-- Migration 015: Allow batched payouts to share a tx_hash; tighten one-per-game guard +-- +-- `transfer_split` sends ONE on-chain transaction to many destinations, so multiple +-- payout rows in a batch legitimately share the same tx_hash. The old UNIQUE index on +-- payouts.tx_hash (migration 008) made the 2nd+ row in a batch fail to record its +-- tx_hash, leaving funds already sent but rows stranded in 'processing' — and at risk +-- of being re-sent by the retry service (a double-payout vector). +-- +-- Payout idempotency does NOT depend on tx_hash uniqueness; it is enforced by +-- idx_payouts_one_per_game (one active payout per game). So we drop the payouts +-- tx_hash unique index. (The payments.tx_hash unique index stays — incoming payments +-- are 1:1 with a transaction.) +DROP INDEX IF EXISTS idx_payouts_tx_hash_unique; + +-- Extend the one-active-payout-per-game guard to also cover 'processing', so a second +-- payable row cannot be inserted for a game while its payout is mid-flight. +DROP INDEX IF EXISTS idx_payouts_one_per_game; +CREATE UNIQUE INDEX IF NOT EXISTS idx_payouts_one_per_game +ON payouts(game_id) +WHERE game_id IS NOT NULL AND status IN ('pending', 'processing', 'completed'); diff --git a/test/batchPayout.test.js b/test/batchPayout.test.js new file mode 100644 index 0000000..7a06efc --- /dev/null +++ b/test/batchPayout.test.js @@ -0,0 +1,111 @@ +/** + * Batch payout dispatch tests (Phase 0.3). + * + * transfer_split sends ONE on-chain tx to many destinations, so every payout row in a + * batch shares that tx_hash. These tests guard: + * - the whole batch is marked completed with the shared tx_hash in one transaction + * (no row stranded mid-batch by a unique-index collision), and + * - an ambiguous batch failure is marked 'needs_review' (NOT 'failed'), so the retry + * service won't re-send and double-pay. + */ + +const GameModeManager = require('../src/game/gameModeManager'); + +const createMockDb = () => { + const mockClient = { query: jest.fn().mockResolvedValue({ rows: [] }) }; + return { + query: jest.fn().mockResolvedValue({ rows: [] }), + withTransaction: jest.fn().mockImplementation(async (cb) => cb(mockClient)), + _mockClient: mockClient + }; +}; + +const createMockPaymentConfig = () => ({ + getConfig: () => ({ + paymentsEnabled: true, + currency: { symbol: 'WOW', decimals: 11 }, + modes: { + direct: { enabled: true, price: 100000000000n }, + credits: { enabled: true, creditsPerGame: 1, packages: [{ id: 'small', credits: 10, price: '500000000000', bonus: 0 }] } + }, + payouts: { + rules: { + direct: { enabled: true, multipliers: { escape: 2, escapeWithTreasure: 3 } }, + credits: { enabled: false, multipliers: { escape: 1.5, escapeWithTreasure: 2 }, baseValue: 50000000000n } + } + }, + preferences: { preferCreditsFirst: true }, + earlyEntry: { enabled: false } + }), + getLegacyGameMode: () => 'PAID_SINGLE', + eventBus: { on: () => {} } +}); + +function buildGmm(walletService) { + const db = createMockDb(); + const debugManager = { CONSOLE_LOGGING: false, getCurrentBlockHeight: () => 1 }; + const gmm = new GameModeManager(db, walletService, debugManager, createMockPaymentConfig()); + return { gmm, db }; +} + +const TWO_PENDING = [ + { id: 10, user_id: 1, game_id: 'g1', payout_address: 'addrA', amount: '200000000000', multiplier: 2, reason: 'escape' }, + { id: 11, user_id: 2, game_id: 'g2', payout_address: 'addrB', amount: '300000000000', multiplier: 3, reason: 'escape' } +]; + +describe('Batch payout dispatch', () => { + test('marks the whole batch completed with the shared tx_hash in one transaction', async () => { + const walletService = { + processPayout: jest.fn(), + processBatchPayout: jest.fn().mockResolvedValue({ tx_hash_list: ['txSHARED'], totalFee: 200 }) + }; + const { gmm, db } = buildGmm(walletService); + + // First withTransaction = gather: SELECT returns 2 pending rows, then UPDATE ... processing. + db._mockClient.query + .mockResolvedValueOnce({ rows: TWO_PENDING }) // SELECT ... FOR UPDATE SKIP LOCKED + .mockResolvedValue({ rows: [] }); // everything else + + await gmm._processPendingPayouts(); + + // The batch RPC was called once with both destinations. + expect(walletService.processBatchPayout).toHaveBeenCalledTimes(1); + expect(walletService.processBatchPayout.mock.calls[0][0]).toHaveLength(2); + + // The completion UPDATE set the shared tx_hash on the whole batch via id = ANY(...). + const calls = db._mockClient.query.mock.calls.map(c => c[0]); + const batchUpdate = calls.find(sql => /UPDATE payouts SET tx_hash/i.test(sql) && /id = ANY/i.test(sql)); + expect(batchUpdate).toBeDefined(); + expect(batchUpdate).toMatch(/status = 'completed'/); + + const batchUpdateCall = db._mockClient.query.mock.calls.find(c => /UPDATE payouts SET tx_hash/i.test(c[0]) && /id = ANY/i.test(c[0])); + expect(batchUpdateCall[1][0]).toBe('txSHARED'); // tx_hash param + expect(batchUpdateCall[1][3]).toEqual([10, 11]); // ids array + + // Per-user stats incremented for each winner inside the same transaction (BIGINT, no float). + const statUpdates = calls.filter(sql => /UPDATE users SET total_amount_won/i.test(sql)); + expect(statUpdates.length).toBe(2); + }); + + test('an ambiguous batch failure is marked needs_review, not failed', async () => { + const walletService = { + processPayout: jest.fn(), + processBatchPayout: jest.fn().mockRejectedValue(new Error('RPC timeout')) + }; + const { gmm, db } = buildGmm(walletService); + + db._mockClient.query + .mockResolvedValueOnce({ rows: TWO_PENDING }) + .mockResolvedValue({ rows: [] }); + + await gmm._processPendingPayouts(); + + // Failure handling uses db.query (outside a transaction) to mark the batch needs_review. + const failCall = db.query.mock.calls.find(c => /needs_review/i.test(c[0])); + expect(failCall).toBeDefined(); + expect(failCall[0]).toMatch(/id = ANY/i); + // Must NOT mark them 'failed' (which the retry service would auto-resend). + const failedCall = db.query.mock.calls.find(c => /status = 'failed'/i.test(c[0])); + expect(failedCall).toBeUndefined(); + }); +});