From ada85364b5465b59e1dba0e82815bd8b8057f23f Mon Sep 17 00:00:00 2001 From: Matthew Little Date: Fri, 26 Jan 2024 16:19:29 +0100 Subject: [PATCH] fix: sql transactional consistency bug with fetching chaintip in various areas (#1853) --- src/api/controllers/cache-controller.ts | 2 +- src/api/routes/status.ts | 2 +- src/datastore/pg-store.ts | 10 +++++----- src/datastore/pg-write-store.ts | 6 +++--- src/event-stream/event-server.ts | 2 +- src/tests-event-replay/import-export-tests.ts | 8 ++++---- src/tests-event-replay/poison-microblock-tests.ts | 2 +- src/tests/cache-control-tests.ts | 2 +- src/tests/datastore-tests.ts | 6 +++--- src/tests/mempool-tests.ts | 12 ++++++------ src/tests/microblock-tests.ts | 4 ++-- 11 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/api/controllers/cache-controller.ts b/src/api/controllers/cache-controller.ts index 470aeceba7..d660e5e60a 100644 --- a/src/api/controllers/cache-controller.ts +++ b/src/api/controllers/cache-controller.ts @@ -252,7 +252,7 @@ async function calculateETag( switch (etagType) { case ETagType.chainTip: try { - const chainTip = await db.getChainTip(); + const chainTip = await db.getChainTip(db.sql); if (chainTip.block_height === 0) { // This should never happen unless the API is serving requests before it has synced any // blocks. diff --git a/src/api/routes/status.ts b/src/api/routes/status.ts index a56dd3e788..2e3e588146 100644 --- a/src/api/routes/status.ts +++ b/src/api/routes/status.ts @@ -19,7 +19,7 @@ export function createStatusRouter(db: PgStore): express.Router { response.pox_v2_unlock_height = poxForceUnlockHeights.result.pox2UnlockHeight as number; response.pox_v3_unlock_height = poxForceUnlockHeights.result.pox3UnlockHeight as number; } - const chainTip = await db.getChainTip(); + const chainTip = await db.getChainTip(db.sql); if (chainTip.block_height > 0) { response.chain_tip = { block_height: chainTip.block_height, diff --git a/src/datastore/pg-store.ts b/src/datastore/pg-store.ts index 4bb5aa3ea7..8be4f7e6a2 100644 --- a/src/datastore/pg-store.ts +++ b/src/datastore/pg-store.ts @@ -206,8 +206,8 @@ export class PgStore extends BasePgStore { }); } - async getChainTip(): Promise { - const tipResult = await this.sql`SELECT * FROM chain_tip`; + async getChainTip(sql: PgSqlClient): Promise { + const tipResult = await sql`SELECT * FROM chain_tip`; const tip = tipResult[0]; return { block_height: tip?.block_height ?? 0, @@ -609,7 +609,7 @@ export class PgStore extends BasePgStore { async getUnanchoredTxsInternal(sql: PgSqlClient): Promise<{ txs: DbTx[] }> { // Get transactions that have been streamed in microblocks but not yet accepted or rejected in an anchor block. - const { block_height } = await this.getChainTip(); + const { block_height } = await this.getChainTip(sql); const unanchoredBlockHeight = block_height + 1; const query = await sql` SELECT ${unsafeCols(sql, [...TX_COLUMNS, abiColumn()])} @@ -1404,7 +1404,7 @@ export class PgStore extends BasePgStore { sql: PgSqlClient, { includeUnanchored }: { includeUnanchored: boolean } ): Promise { - const chainTip = await this.getChainTip(); + const chainTip = await this.getChainTip(sql); if (includeUnanchored) { return chainTip.block_height + 1; } else { @@ -2144,7 +2144,7 @@ export class PgStore extends BasePgStore { async getStxBalanceAtBlock(stxAddress: string, blockHeight: number): Promise { return await this.sqlTransaction(async sql => { - const chainTip = await this.getChainTip(); + const chainTip = await this.getChainTip(sql); const blockHeightToQuery = blockHeight > chainTip.block_height ? chainTip.block_height : blockHeight; const blockQuery = await this.getBlockByHeightInternal(sql, blockHeightToQuery); diff --git a/src/datastore/pg-write-store.ts b/src/datastore/pg-write-store.ts index 1b78e62cf8..b28983e96b 100644 --- a/src/datastore/pg-write-store.ts +++ b/src/datastore/pg-write-store.ts @@ -183,7 +183,7 @@ export class PgWriteStore extends PgStore { let batchedTxData: DataStoreTxEventData[] = []; await this.sqlWriteTransaction(async sql => { - const chainTip = await this.getChainTip(); + const chainTip = await this.getChainTip(sql); await this.handleReorg(sql, data.block, chainTip.block_height); const isCanonical = data.block.block_height > chainTip.block_height; if (!isCanonical) { @@ -555,7 +555,7 @@ export class PgWriteStore extends PgStore { // Sanity check: ensure incoming microblocks have a `parent_index_block_hash` that matches the // API's current known canonical chain tip. We assume this holds true so incoming microblock // data is always treated as being built off the current canonical anchor block. - const chainTip = await this.getChainTip(); + const chainTip = await this.getChainTip(sql); const nonCanonicalMicroblock = data.microblocks.find( mb => mb.parent_index_block_hash !== chainTip.index_block_hash ); @@ -1792,7 +1792,7 @@ export class PgWriteStore extends PgStore { async updateMempoolTxs({ mempoolTxs: txs }: { mempoolTxs: DbMempoolTxRaw[] }): Promise { const updatedTxIds: string[] = []; await this.sqlWriteTransaction(async sql => { - const chainTip = await this.getChainTip(); + const chainTip = await this.getChainTip(sql); updatedTxIds.push(...(await this.insertDbMempoolTxs(txs, chainTip, sql))); }); if (!this.isEventReplay) { diff --git a/src/event-stream/event-server.ts b/src/event-stream/event-server.ts index 0082604f25..2fb0309e50 100644 --- a/src/event-stream/event-server.ts +++ b/src/event-stream/event-server.ts @@ -888,7 +888,7 @@ export async function startEventServer(opts: { if (ibdHeight) { app.use(IBD_PRUNABLE_ROUTES, async (req, res, next) => { try { - const chainTip = await db.getChainTip(); + const chainTip = await db.getChainTip(db.sql); if (chainTip.block_height > ibdHeight) { next(); } else { diff --git a/src/tests-event-replay/import-export-tests.ts b/src/tests-event-replay/import-export-tests.ts index ed2fe9d03f..cdb8f29a8f 100644 --- a/src/tests-event-replay/import-export-tests.ts +++ b/src/tests-event-replay/import-export-tests.ts @@ -28,7 +28,7 @@ describe('import/export tests', () => { test('event import and export cycle', async () => { // Import from mocknet TSV await importEventsFromTsv('src/tests-event-replay/tsv/mocknet.tsv', 'archival', true, true); - const chainTip = await db.getChainTip(); + const chainTip = await db.getChainTip(db.sql); expect(chainTip.block_height).toBe(28); expect(chainTip.index_block_hash).toBe( '0x76cd67a65c0dfd5ea450bb9efe30da89fa125bfc077c953802f718353283a533' @@ -50,7 +50,7 @@ describe('import/export tests', () => { // Re-import with exported TSV and check that chain tip matches. try { await importEventsFromTsv(`${tmpDir}/export.tsv`, 'archival', true, true); - const newChainTip = await db.getChainTip(); + const newChainTip = await db.getChainTip(db.sql); expect(newChainTip.block_height).toBe(28); expect(newChainTip.index_block_hash).toBe( '0x76cd67a65c0dfd5ea450bb9efe30da89fa125bfc077c953802f718353283a533' @@ -196,14 +196,14 @@ describe('IBD', () => { process.env.IBD_MODE_UNTIL_BLOCK = '1000'; // TSV has 1 microblock message. await expect(getIbdInterceptCountFromTsvEvents()).resolves.toBe(1); - await expect(db.getChainTip()).resolves.toHaveProperty('block_height', 28); + await expect(db.getChainTip(db.sql)).resolves.toHaveProperty('block_height', 28); }); test('IBD mode does NOT block certain API routes once the threshold number of blocks are ingested', async () => { process.env.IBD_MODE_UNTIL_BLOCK = '1'; // Microblock processed normally. await expect(getIbdInterceptCountFromTsvEvents()).resolves.toBe(0); - await expect(db.getChainTip()).resolves.toHaveProperty('block_height', 28); + await expect(db.getChainTip(db.sql)).resolves.toHaveProperty('block_height', 28); }); test('IBD mode covers prune mode', async () => { diff --git a/src/tests-event-replay/poison-microblock-tests.ts b/src/tests-event-replay/poison-microblock-tests.ts index 5bd511ae56..9c403c9c43 100644 --- a/src/tests-event-replay/poison-microblock-tests.ts +++ b/src/tests-event-replay/poison-microblock-tests.ts @@ -25,7 +25,7 @@ describe('poison microblock for height 80743', () => { true ); const poisonTxId = '0x58ffe62029f94f7101b959536ea4953b9bce0ec3f6e2a06254c511bdd5cfa9e7'; - const chainTip = await db.getChainTip(); + const chainTip = await db.getChainTip(db.sql); // query the txs table and check the transaction type const searchResult = await db.searchHash({ hash: poisonTxId }); let entityData: any; diff --git a/src/tests/cache-control-tests.ts b/src/tests/cache-control-tests.ts index 4b1f5e3450..b35b6e6fa4 100644 --- a/src/tests/cache-control-tests.ts +++ b/src/tests/cache-control-tests.ts @@ -321,7 +321,7 @@ describe('cache-control tests', () => { ], }); - const chainTip2 = await db.getChainTip(); + const chainTip2 = await db.getChainTip(db.sql); expect(chainTip2.block_hash).toBe(block1.block_hash); expect(chainTip2.block_height).toBe(block1.block_height); expect(chainTip2.index_block_hash).toBe(block1.index_block_hash); diff --git a/src/tests/datastore-tests.ts b/src/tests/datastore-tests.ts index 54012798ae..e5ac20a2fa 100644 --- a/src/tests/datastore-tests.ts +++ b/src/tests/datastore-tests.ts @@ -4101,7 +4101,7 @@ describe('postgres datastore', () => { const blockQuery1 = await db.getBlock({ hash: block2b.block_hash }); expect(blockQuery1.result?.canonical).toBe(false); - const chainTip1 = await db.getChainTip(); + const chainTip1 = await db.getChainTip(db.sql); expect(chainTip1).toEqual({ block_hash: '0x33', block_height: 3, @@ -4169,7 +4169,7 @@ describe('postgres datastore', () => { const blockQuery2 = await db.getBlock({ hash: block3b.block_hash }); expect(blockQuery2.result?.canonical).toBe(false); // Chain tip doesn't change yet. - const chainTip2 = await db.getChainTip(); + const chainTip2 = await db.getChainTip(db.sql); expect(chainTip2).toEqual({ block_hash: '0x33', block_height: 3, @@ -4220,7 +4220,7 @@ describe('postgres datastore', () => { const blockQuery3 = await db.getBlock({ hash: block3b.block_hash }); expect(blockQuery3.result?.canonical).toBe(true); - const chainTip3 = await db.getChainTip(); + const chainTip3 = await db.getChainTip(db.sql); expect(chainTip3).toEqual({ block_count: 4, block_hash: '0x44bb', diff --git a/src/tests/mempool-tests.ts b/src/tests/mempool-tests.ts index 1436ce9aa5..91073598b0 100644 --- a/src/tests/mempool-tests.ts +++ b/src/tests/mempool-tests.ts @@ -1666,7 +1666,7 @@ describe('mempool tests', () => { // Simulate the bug with a txs being in the mempool at confirmed at the same time by // directly inserting the mempool-tx and mined-tx, bypassing the normal update functions. await db.updateBlock(db.sql, dbBlock1); - const chainTip = await db.getChainTip(); + const chainTip = await db.getChainTip(db.sql); await db.insertDbMempoolTxs([mempoolTx], chainTip, db.sql); await db.updateTx(db.sql, dbTx1); @@ -1828,7 +1828,7 @@ describe('mempool tests', () => { await db.updateMempoolTxs({ mempoolTxs: [mempoolTx] }); - let chainTip = await db.getChainTip(); + let chainTip = await db.getChainTip(db.sql); expect(chainTip.mempool_tx_count).toBe(1); // Verify tx shows up in mempool (non-pruned) @@ -1852,7 +1852,7 @@ describe('mempool tests', () => { expect(mempoolResult2.body.results).toHaveLength(0); const mempoolCount2 = await supertest(api.server).get(`/extended/v1/tx/mempool`); expect(mempoolCount2.body.total).toBe(0); - chainTip = await db.getChainTip(); + chainTip = await db.getChainTip(db.sql); expect(chainTip.mempool_tx_count).toBe(0); // Re-broadcast mempool tx @@ -1865,7 +1865,7 @@ describe('mempool tests', () => { expect(mempoolResult3.body.results[0].tx_id).toBe(txId); const mempoolCount3 = await supertest(api.server).get(`/extended/v1/tx/mempool`); expect(mempoolCount3.body.total).toBe(1); - chainTip = await db.getChainTip(); + chainTip = await db.getChainTip(db.sql); expect(chainTip.mempool_tx_count).toBe(1); // Mine tx in block to prune from mempool @@ -1898,7 +1898,7 @@ describe('mempool tests', () => { expect(mempoolResult4.body.results).toHaveLength(0); const mempoolCount4 = await supertest(api.server).get(`/extended/v1/tx/mempool`); expect(mempoolCount4.body.total).toBe(0); - chainTip = await db.getChainTip(); + chainTip = await db.getChainTip(db.sql); expect(chainTip.mempool_tx_count).toBe(0); // Verify tx is mined @@ -1931,7 +1931,7 @@ describe('mempool tests', () => { expect(mempoolResult5.body.results[0].tx_id).toBe(txId); const mempoolCount5 = await supertest(api.server).get(`/extended/v1/tx/mempool`); expect(mempoolCount5.body.total).toBe(1); - chainTip = await db.getChainTip(); + chainTip = await db.getChainTip(db.sql); expect(chainTip.mempool_tx_count).toBe(1); // Re-broadcast mempool tx diff --git a/src/tests/microblock-tests.ts b/src/tests/microblock-tests.ts index 2f6bfe176d..5d123687cd 100644 --- a/src/tests/microblock-tests.ts +++ b/src/tests/microblock-tests.ts @@ -386,7 +386,7 @@ describe('microblock tests', () => { ], }); - const chainTip1 = await db.getChainTip(); + const chainTip1 = await db.getChainTip(db.sql); expect(chainTip1.block_hash).toBe(block1.block_hash); expect(chainTip1.block_height).toBe(block1.block_height); expect(chainTip1.index_block_hash).toBe(block1.index_block_hash); @@ -549,7 +549,7 @@ describe('microblock tests', () => { ], }); - const chainTip2 = await db.getChainTip(); + const chainTip2 = await db.getChainTip(db.sql); expect(chainTip2.block_hash).toBe(block1.block_hash); expect(chainTip2.block_height).toBe(block1.block_height); expect(chainTip2.index_block_hash).toBe(block1.index_block_hash);