From da4da3898143be4101ce98b5edf1b44bc410f079 Mon Sep 17 00:00:00 2001 From: Julien Lucca Date: Sat, 8 Aug 2026 15:27:34 +0200 Subject: [PATCH] fix(create-path): never invent objective or action ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #61 removed the serial fallback for claims. The same fallback was still in the objective and action create paths, and it is the same defect: when the chain read failed, the resolver substituted a DB serial that silently names a DIFFERENT row. For an action that is worse than for a claim — claimaction carries action_id, so later claims attach to the wrong action, and the action becomes un-editable. That is the shape of the phantom action ids 399-406 the audit found. Removing the fallback alone would not have been safe, because the reads underneath it were still truncatable: * actionsForObjective used the `byobjective` SECONDARY index. Secondary reads cannot be resumed — nodeos returns the secondary key as next_key — so a short read just looks like "this objective has fewer actions than it does", and the caller then hands the create an id that is already taken. It now pages the whole `action` table through the primary index and filters client-side: 400 rows / 5 calls on prod, on the create path only. * objectivesForCommunity took a single call at limit 2000 and trusted it. It now pages its scope until more=false. Both go through one pager, and assertComplete is gone with them: a lone call is never proof of a complete set at any table size, so there is nothing left for a throw-on-`more` guard to protect. Chain reads are now retried (3 attempts, backing off) before giving up. Without a fallback a failed read costs a skipped create until someone reindexes, and about 2 in 100 calls came back without a rows array while paging the action table on prod — the acceptance check below failed 2 of 105 calls before the retry and passes cleanly after. The node's own message is carried into the error, since Sentry is not running and the log line is the only diagnostic. The claim reader shares the retry for the same reason. Create-path inserts log AND rethrow instead of swallowing. A swallowed insert drops the row while ledgered still records the action as processed, so no reindex revisits it — the likeliest explanation for the two claims found missing on prod. Verified with scripts/verify-create-resolvers.js against the prod chain: replay each parent's creation history (first k ids known, ask for the next) and require the resolver to name id k+1 every time. 14 communities / 95 objective steps and 77 action steps across the busiest objectives all pass, and an exhausted parent throws instead of inventing an id. verify-claim-resolver.js still 636/636. Co-Authored-By: Claude Opus 5 --- scripts/verify-create-resolvers.js | 157 +++++++++++++++++++++++++++++ src/chain.js | 143 ++++++++++++++++---------- src/updaters/community.js | 62 ++++++++---- 3 files changed, 288 insertions(+), 74 deletions(-) create mode 100644 scripts/verify-create-resolvers.js diff --git a/scripts/verify-create-resolvers.js b/scripts/verify-create-resolvers.js new file mode 100644 index 0000000..26c4088 --- /dev/null +++ b/scripts/verify-create-resolvers.js @@ -0,0 +1,157 @@ +// Acceptance check for chain.js/resolveCreatedActionId and +// resolveCreatedObjectiveId against a REAL node. +// +// NODE_ENV=prod BLOCKCHAIN_URL=https://app.cambiatus.io \ +// BLOCKCHAIN_COMMUNITY_CONTRACT=cambiatus.cm BLOCKCHAIN_TOKEN_CONTRACT=cambiatus.tk \ +// node scripts/verify-create-resolvers.js +// +// Both resolvers answer "which id did the contract just generate?" with: the +// smallest chain id for this parent that the DB does not have yet. So replay the +// creation history of every parent — knownIds = the first k ids, ask for the +// next — and require the resolver to name id k+1 every time. Then confirm an +// exhausted parent throws rather than inventing an id. +// +// This is the check that would have caught the failure the serial fallback hid: +// a truncated chain read makes a real id look missing, and the resolver hands +// back an id that belongs to a different action. + +const config = require(`../src/config/${process.env.NODE_ENV || 'dev'}`) +const { resolveCreatedActionId, resolveCreatedObjectiveId } = require('../src/chain') + +const CONTRACT = config.blockchain.contract.community + +async function check (label, ids, resolve) { + let pass = 0 + const failures = [] + + // Replay the parent's creation order: with the first k ids known, the next + // create must resolve to ids[k]. + for (let k = 0; k < ids.length; k++) { + const known = new Set(ids.slice(0, k)) + let got + try { + got = await resolve(known) + } catch (e) { + got = `THREW: ${e.message}` + } + if (got === ids[k]) pass++ + else failures.push({ k, want: ids[k], got }) + } + + // Everything known -> nothing left to create -> must throw. + let exhausted = 'did not throw' + try { + await resolve(new Set(ids)) + } catch (e) { + exhausted = 'threw as required' + } + + const ok = failures.length === 0 && exhausted === 'threw as required' + console.log(` ${ok ? 'PASS' : 'FAIL'} ${label}: ${pass}/${ids.length} steps, exhausted-probe ${exhausted}`) + for (const f of failures.slice(0, 5)) { + console.log(` step ${f.k}: want ${f.want} got ${f.got}`) + } + return ok +} + +async function main () { + console.log(`node: ${config.blockchain.url} contract: ${CONTRACT}`) + + // Communities to exercise, read off chain so this is not hardcoded to muda. + const https = require('https') + const post = (path, body) => new Promise((resolve, reject) => { + const data = JSON.stringify(body) + const url = new URL(path, config.blockchain.url) + const req = https.request({ + hostname: url.hostname, + port: 443, + path: url.pathname, + method: 'POST', + headers: { 'Content-Type': 'application/json', 'Content-Length': Buffer.byteLength(data) } + }, res => { + const chunks = [] + res.on('data', c => chunks.push(c)) + res.on('end', () => { try { resolve(JSON.parse(Buffer.concat(chunks).toString())) } catch (e) { reject(e) } }) + }) + req.on('error', reject) + req.write(data) + req.end() + }) + + const cmm = await post('/v1/chain/get_table_rows', { + json: true, code: CONTRACT, scope: CONTRACT, table: 'community', limit: 1000 + }) + const symbols = (cmm.rows || []).map(c => c.symbol) + console.log(`communities: ${symbols.length}`) + + let allOk = true + + // ---- objectives, per community ----------------------------------------- + let objChecked = 0 + for (const symbol of symbols) { + // The resolver has its own reader; to know the EXPECTED ids we read the + // community's objective scope independently here. + const raw = (() => { + const [precision, code] = symbol.split(',') + let v = BigInt(precision) + for (let i = 0; i < code.length; i++) v |= BigInt(code.charCodeAt(i)) << BigInt(8 * (i + 1)) + return v.toString() + })() + const page = await post('/v1/chain/get_table_rows', { + json: true, code: CONTRACT, scope: raw, table: 'objective', limit: 1000 + }) + const ids = (page.rows || []).map(o => Number(o.id)).sort((a, b) => a - b) + if (ids.length === 0) continue + objChecked++ + const ok = await check( + `objectives of ${symbol}`, + ids, + known => resolveCreatedObjectiveId(CONTRACT, symbol, known) + ) + allOk = allOk && ok + } + console.log(`communities with objectives checked: ${objChecked}`) + + // ---- actions, per objective -------------------------------------------- + const actions = await post('/v1/chain/get_table_rows', { + json: true, code: CONTRACT, scope: CONTRACT, table: 'action', limit: 1000 + }) + // Page the rest (one call is never the whole table). + let all = actions.rows || [] + let more = actions.more + let lb = actions.next_key + while (more) { + const p = await post('/v1/chain/get_table_rows', { + json: true, code: CONTRACT, scope: CONTRACT, table: 'action', lower_bound: Number(lb), limit: 1000 + }) + all = all.concat(p.rows || []) + more = p.more + lb = p.next_key + } + const byObjective = new Map() + for (const a of all) { + const o = Number(a.objective_id) + if (!byObjective.has(o)) byObjective.set(o, []) + byObjective.get(o).push(Number(a.id)) + } + console.log(`actions: ${all.length} across ${byObjective.size} objectives`) + + // Checking every objective means re-reading the action table once per step, + // which is slow; sample the busiest ones plus a spread of the rest. + const objectives = [...byObjective.entries()].sort((a, b) => b[1].length - a[1].length) + const sample = objectives.slice(0, 3).concat(objectives.slice(3).filter((_, i) => i % 20 === 0)) + for (const [objectiveId, ids] of sample) { + ids.sort((a, b) => a - b) + const ok = await check( + `actions of objective ${objectiveId}`, + ids, + known => resolveCreatedActionId(CONTRACT, objectiveId, known) + ) + allOk = allOk && ok + } + + console.log(allOk ? '\nALL CHECKS PASS' : '\nFAILURES PRESENT') + process.exit(allOk ? 0 : 1) +} + +main().catch(e => { console.error(e); process.exit(1) }) diff --git a/src/chain.js b/src/chain.js index e92f023..676cad5 100644 --- a/src/chain.js +++ b/src/chain.js @@ -42,15 +42,69 @@ class ResolveError extends Error {} // Measured against prod on 2026-08-07: the identical bounded `byaction` claim // query returned 31, 35, 38, 51, 60, 67 then 79 rows on seven consecutive calls // (the count tracks page-cache warmth, so it is not even deterministic), always -// with `more: true`; an unbounded read with `limit: 5000` returned 27 rows. +// with `more: true`; an unbounded read with `limit: 5000` returned 27 rows. The +// 400-row `action` table takes 5 calls to read in full. // -// So this guard is only safe for sets small enough that the node reliably walks -// them in one budget — it converts truncation into a throw. Anything that needs -// a COMPLETE set of a potentially large table must page until `more === false` -// instead (see resolveClaimId). -function assertComplete (res, table, context) { - if (!res || !Array.isArray(res.rows)) throw new Error(`get_table_rows(${table}) returned no rows ${context}`) - if (res.more) throw new Error(`get_table_rows(${table}) truncated ${context} (${res.rows.length} rows, more=true) — cannot page safely`) +// So a single call is never proof of a complete set, at any table size. Read +// every table that a resolver depends on through this pager, which follows +// `next_key` until the node says `more: false`. +// +// PRIMARY index only. On a secondary index nodeos returns the secondary key as +// `next_key` (a query bounded to action 389 returns `next_key: 389`), so it does +// not advance and a truncated secondary read cannot be resumed at all. That is +// why the callers below scan a table and filter client-side rather than asking +// the node for "the rows matching key K". +// One get_table_rows call, retried on a transient failure. +// +// Retrying matters more than it looks: the resolvers no longer fall back to a DB +// serial, so a read that fails is a create the indexer skips until someone +// reindexes. A dropped connection or a node hiccup should not cost that. Measured +// against prod 2026-08-08, roughly 2 in 100 calls came back without a `rows` +// array while paging the action table. Reads are pure, and every caller runs +// before its first write, so a retry is free of side effects. +// +// The node's own message is carried into the final error — with Sentry not +// running, the log line is the only diagnostic anyone gets. +async function getTableRows (body, what, attempts = 3) { + let last + for (let attempt = 1; attempt <= attempts; attempt++) { + try { + const res = await post('/v1/chain/get_table_rows', body) + if (res && Array.isArray(res.rows)) return res + last = `node replied without a rows array: ${JSON.stringify(res).slice(0, 300)}` + } catch (e) { + last = e.message + } + if (attempt < attempts) await new Promise(r => setTimeout(r, 250 * attempt)) + } + throw new Error(`get_table_rows for ${what} failed after ${attempts} attempts — ${last}`) +} + +async function pageAllRows (params, what) { + const rows = [] + let lowerBound = 0 + + for (let page = 0; page < 1000; page++) { + const res = await getTableRows({ + json: true, + limit: 1000, + ...params, + lower_bound: lowerBound + }, what) + rows.push(...res.rows) + + if (!res.more) return rows + + const next = Number(res.next_key) + if (!Number.isFinite(next) || next <= lowerBound) { + throw new Error( + `paging ${what}: next_key (${res.next_key}) did not advance past ${lowerBound}` + ) + } + lowerBound = next + } + + throw new Error(`paging ${what} did not terminate`) } // One page of the `claim` table read through the PRIMARY index, ascending from @@ -64,18 +118,14 @@ function assertComplete (res, table, context) { // resolution reads the primary index and filters client-side rather than asking // the node for "the claims of action N". async function claimPage (communityContract, lowerBound, limit) { - const res = await post('/v1/chain/get_table_rows', { + return getTableRows({ json: true, code: communityContract, scope: communityContract, table: 'claim', lower_bound: lowerBound, limit - }) - if (!res || !Array.isArray(res.rows)) { - throw new Error(`get_table_rows(claim) returned no rows from id ${lowerBound}`) - } - return res + }, `claims from id ${lowerBound}`) } // Resolve the real on-chain claim id for the claim a `claimaction` just created. @@ -136,43 +186,32 @@ function symbolRaw (symbolString) { return raw.toString() } -// Fetch every on-chain action for a single objective via the secondary index -// on objective_id (index_position 2). Objectives hold a handful of actions -// (measured on prod 2026-08-07: objective 93 -> 2 rows, `more: false`), well -// inside one walk budget, so assertComplete's throw-on-`more` is the right -// guard here. If an objective ever grows past what the node walks in one -// budget this must move to primary-index paging like resolveClaimId — a -// secondary index cannot be resumed. +// Every on-chain action belonging to one objective. +// +// This reads the WHOLE `action` table through the primary index and filters +// client-side, rather than using the `byobjective` secondary index. The +// secondary index would look cheaper, but it truncates on the same time budget +// as everything else and cannot be resumed (see pageAllRows), so a short read +// silently looks like "this objective has fewer actions than it does" — and the +// caller turns a missing id into the id of the action being created. The whole +// table is 400 rows / 5 calls on prod (2026-08-08), and this runs only on the +// create path, so scanning it is the cheaper mistake. async function actionsForObjective (communityContract, objectiveId) { - const res = await post('/v1/chain/get_table_rows', { - json: true, - code: communityContract, - scope: communityContract, - table: 'action', - index_position: 2, - key_type: 'i64', - lower_bound: objectiveId, - upper_bound: objectiveId, - limit: 5000 - }) - assertComplete(res, 'action', `for objective ${objectiveId}`) - return res.rows + const all = await pageAllRows( + { code: communityContract, scope: communityContract, table: 'action' }, + `actions of objective ${objectiveId}` + ) + return all.filter(a => Number(a.objective_id) === Number(objectiveId)) } -// Fetch every on-chain objective for a single community. Objectives live in a -// per-community scope (the raw symbol value) under the primary index, so a -// plain scan returns exactly this community's objectives. Same truncation -// guard as above. +// Every on-chain objective of one community. Objectives live in a per-community +// scope (the raw symbol value) under the primary index, so paging that scope +// returns exactly this community's objectives and nothing else. async function objectivesForCommunity (communityContract, communitySymbol) { - const res = await post('/v1/chain/get_table_rows', { - json: true, - code: communityContract, - scope: symbolRaw(communitySymbol), - table: 'objective', - limit: 2000 - }) - assertComplete(res, 'objective', `for community ${communitySymbol}`) - return res.rows + return pageAllRows( + { code: communityContract, scope: symbolRaw(communitySymbol), table: 'objective' }, + `objectives of community ${communitySymbol}` + ) } // Resolve the real on-chain id of the action being CREATED (upsertaction with @@ -180,9 +219,9 @@ async function objectivesForCommunity (communityContract, communitySymbol) { // it). `knownIds` is the set of action ids already in the DB for this // objective. Blocks are processed in order, so the earliest chain id we don't // have yet is this create: the created action = the SMALLEST chain id not in -// `knownIds`. Throws if every chain id is already known (chain/DB out of sync -// — e.g. the create hasn't reached the node we query yet), so the block -// retries instead of writing a wrong id. +// `knownIds`. Throws if every chain id is already known (chain/DB out of sync — +// e.g. the create hasn't reached the node we query yet); the caller turns that +// into a ResolveError and skips the action rather than inventing an id. async function resolveCreatedActionId (communityContract, objectiveId, knownIds) { const chainIds = (await actionsForObjective(communityContract, objectiveId)) .map(a => Number(a.id)) @@ -192,7 +231,7 @@ async function resolveCreatedActionId (communityContract, objectiveId, knownIds) if (created === undefined) { throw new Error( `resolveCreatedActionId: all ${chainIds.length} chain actions for objective ${objectiveId} ` + - 'are already in the DB, nothing left to create. Retrying block.' + 'are already in the DB, nothing left to create.' ) } return created @@ -210,7 +249,7 @@ async function resolveCreatedObjectiveId (communityContract, communitySymbol, kn if (created === undefined) { throw new Error( `resolveCreatedObjectiveId: all ${chainIds.length} chain objectives for community ${communitySymbol} ` + - 'are already in the DB, nothing left to create. Retrying block.' + 'are already in the DB, nothing left to create.' ) } return created diff --git a/src/updaters/community.js b/src/updaters/community.js index 178cba1..52115fc 100644 --- a/src/updaters/community.js +++ b/src/updaters/community.js @@ -365,11 +365,15 @@ async function upsertObjective (db, payload, blockInfo, _context) { // insert. Recover the real id from chain: the smallest on-chain id this community // has that we don't have yet is this create (blocks are processed in order). // - // On any failure (chain unreachable, no missing id) we fall back to the serial - // rather than throw — a throw here becomes an unhandledRejection → process exit → - // pm2 crash-loop. The serial was realigned to the chain counter by the 2026-07-02 - // remediation, so the fallback stays correct unless a new drift is introduced; the - // explicit-id path is what keeps that drift from re-opening. + // There is deliberately NO serial fallback (same rule as claimAction). Falling + // back is what let the 2026-08 claim-id drift happen: a chain read that came + // back short was indistinguishable from success, and the serial it substituted + // silently named a different objective. A missing objective row can be + // reindexed; a wrong primary key cannot be undone, and here it also makes the + // objective un-editable and its actions un-creatable. So a resolve failure + // throws ResolveError, which `ledgered` turns into "un-claim this global_seq + // and leave the action for a reindex". Nothing is written before this point, + // which is what makes that safe. try { const known = await db.objectives.find( { community_id: payload.data.community_id }, @@ -381,17 +385,20 @@ async function upsertObjective (db, payload, blockInfo, _context) { new Set(known.map(o => Number(o.id))) ) } catch (e) { - logError('Could not resolve chain objective id, falling back to serial', e) - delete data.id + throw new ResolveError( + `objective id resolution failed for community ${payload.data.community_id} ` + + `at block ${blockInfo.blockNumber}: ${e.message}` + ) } // insert(), not save(): with an id present save() emits an UPDATE (matching - // nothing for a new id); without one the serial assigns it. - return db.objectives - .insert(data) - .catch(e => - logError('Something went wrong while creating objective', e) - ) + // nothing for a new id). Log AND rethrow — swallowing here would drop the + // objective while `ledgered` still recorded the action as processed, which is + // the likeliest way the two claims missing from prod went missing. + return db.objectives.insert(data).catch(e => { + logError('Something went wrong while creating objective', e) + throw e + }) } function upsertAction (db, payload, blockInfo, _context) { @@ -474,11 +481,15 @@ function upsertAction (db, payload, blockInfo, _context) { // the transaction would hold a DB connection open for its whole duration // (claimAction resolves before its insert for the same reason). // - // On any failure (chain unreachable, no missing id) we fall back to the serial - // rather than throw — a throw here becomes an unhandledRejection → process exit → - // pm2 crash-loop. The serial was realigned to the chain counter by the 2026-07-02 - // remediation, so the fallback stays correct unless a new drift is introduced; the - // explicit-id path is what keeps that drift from re-opening. + // There is deliberately NO serial fallback (same rule as claimAction and + // upsertObjective). A substituted serial silently names a DIFFERENT action: + // the action becomes un-editable, and because claimaction carries action_id, + // later claims attach to the wrong action — this is the shape of the phantom + // action ids 399-406 the audit found. A missing action row can be reindexed; + // a wrong primary key cannot be undone. So a resolve failure throws + // ResolveError and `ledgered` leaves the action unprocessed. Nothing is + // written before this point (the withTransaction below is the first write), + // which is what makes that safe. try { const known = await db.actions.find( { objective_id: payload.data.objective_id }, @@ -490,8 +501,10 @@ function upsertAction (db, payload, blockInfo, _context) { new Set(known.map(a => Number(a.id))) ) } catch (e) { - logError('Could not resolve chain action id, falling back to serial', e) - delete data.id + throw new ResolveError( + `action id resolution failed for objective ${payload.data.objective_id} ` + + `at block ${blockInfo.blockNumber}: ${e.message}` + ) } } @@ -532,12 +545,17 @@ function upsertAction (db, payload, blockInfo, _context) { ) ) }) - }).catch(e => + }).catch(e => { + // Log AND rethrow. Swallowing left the action (and its validators) unwritten + // while `ledgered` kept the _processed_actions row, so the create was recorded + // as applied and no reindex would ever revisit it. Rethrowing rolls the block + // back, taking the ledger row with it. logError( 'Something went wrong while executing transaction to create an action', e ) - ) + throw e + }) }) }