Skip to content

fix: wrap spoke pool indexer in a sql transaction #271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

amateima
Copy link
Contributor

@amateima amateima commented Apr 28, 2025

This PR wraps the execution of SpokePoolIndexerDataHandler.processBlockRange() in a SQL transaction. In case of throwing an error, the database should be reverted to the state prior to the block range being processed.

  • fetchEventsByRange is not included in the SQL transaction
  • all queries must be executed using the new transactionalEntityManager
  • to ensure backwards compatibility, in some functions transactionalEntityManager is marked as optional. In such cases the queries are executed using the DataSource instance
    Example:
const repository = transactionalEntityManager
      ? transactionalEntityManager.getRepository(entity)
      : this.postgres.getRepository(entity);
  • managed advisory locks pg_advisory_xact_lock that gets released automatically after the transaction ends get replaced with session lived pg_advisory_lock which needs to be unlocked explicitly
    The following pattern has been using in the SpokePoolProcessor:
try {
     // Acquire a lock to prevent concurrent modifications on the same relayHash
     call pg_advisory_lock(),
} finally {
    // Explicitly call unlock for both successful or failed execution
     call pg_advisory_unlock()
}

Copy link

linear bot commented Apr 28, 2025

@@ -16,6 +21,7 @@ export class SwapBeforeBridgeRepository extends dbUtils.BlockchainEventRepositor
swapBeforeBridgeEvents: SwapBeforeBridgeEvent[],
chainId: number,
lastFinalisedBlock: number,
transactionalEntityManager?: EntityManager,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance we can make a singleton for this EntityManager? Without it we need to pass this variable down to a lot of inner functions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's possible to make a singleton because this instance must be limited to the scope of the callback passed as a parameter to createTransaction() function

Copy link
Contributor

@daywiss daywiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work!

: this.postgres.getRepository(entities.ExecutedRelayerRefundRoot);
const relayHashInfoRepository = transactionalEntityManager
? transactionalEntityManager.getRepository(entities.RelayHashInfo)
: this.postgres.getRepository(entities.RelayHashInfo);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes sense to make it compatible with tests, but i wonder if we should somehow require this when running for real, i dont think we ever want this to run without it in prod

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I need to find a way to make it required. It's easy for someone less familiar with the code to miss using it

@amateima amateima requested a review from james-a-morris April 29, 2025 13:19
@amateima amateima merged commit 296d070 into stage Apr 29, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants