Feature/known gaps #217

2022-03-23T18:08:03Z

abdulrabbani00 commented

2022-03-23 18:08:03 +00:00

(Migrated from github.com)

Overview

This PR contains does the following:

Contains a function for updating known_gaps to the DB - FindAndUpdateGaps.
- This function has a unit-test: TestKnownGapsUpsert
Triggering the function FindAndUpdateGaps throughout the codebase to check for gaps.

Future Cases

processingKey - A field that can be used to differentiate the block calculation logic between geth nodes. We should be able to calculate the expectedDiferrence from the processingKey (perhaps through a mapping).
expectedDifference - Indicates what the difference between blocks should be.

# Overview This PR contains does the following: * Contains a function for updating `known_gaps` to the DB - `FindAndUpdateGaps`. * This function has a unit-test: `TestKnownGapsUpsert` * Triggering the function `FindAndUpdateGaps` throughout the codebase to check for gaps. # Future Cases 1. `processingKey` - A field that can be used to differentiate the block calculation logic between geth nodes. We should be able to calculate the `expectedDiferrence` from the `processingKey` (perhaps through a mapping). 2. `expectedDifference` - Indicates what the difference between blocks should be.

i-norden reviewed

2022-03-28 21:33:16 +00:00

i-norden left a comment

Looking good! Added some comments, the main issue I see is with respect to the race conditions we will run into when we have multiple writeLoop workers (how we run it in prod) which you've already noted. But I also very strongly think we should decouple these gap r/w methods from the Indexer interface.

i-norden reviewed

2022-03-28 21:35:44 +00:00

i-norden reviewed

2022-03-28 22:15:32 +00:00

abdulrabbani00 commented

2022-03-29 12:41:03 +00:00

(Migrated from github.com)

Looking good! Added some comments, the main issue I see is with respect to the race conditions we will run into when we have multiple writeLoop workers (how we run it in prod) which you've already noted. But I also very strongly think we should decouple these gap r/w methods from the Indexer interface.

Thank you for the thorough review, a few notes:

I strongly agree with decoupling the known gaps with the indexer. Honestly, I have no idea why I tried to jam known gaps in there so strongly, it gave me nothing but trouble (and pain). I will work on refactoring.
The race condition for multiple workers is certainly worrisome. I was considering the best way to proceed considering the write to lastProcessedBlock. There is no simple fix in my mind just yet.
- Let's consider the only case in my mind where this would actually be needed -
  - We processed blocks 10, 11, 12.
  - For some reason, geth did not receive blocks 13, 14, 15.
  - We receive blocks 16, 17 18,.... and processed them
- In this scenario, we would want to add blocks 13-15 to the known gaps table. The question I have is:
  - Is the above-mentioned scenario even possible?
  - Does geth have any build-in mechanism that would realize we jumped from block 12 to 16, and request block 13-15? Because if it does, then we do not need to handle this use case.
- If geth does allow us to jump from blocks and have these gaps then we might need to consider an implementation.
- It's also important to note that if we have any errors when processing 13-15, those blocks are added to known gaps.
- If geth simply errors out and panics when we see we went from block 12 to 16, that's okay too, because when geth restarts it will see the last processed block was block 12, and we are currently on block 16+n, it will capture 13-16+n in the known gaps table.
Depending on the above, testing will vary. We should talk about the possibility of undetected gaps within geth.

> Looking good! Added some comments, the main issue I see is with respect to the race conditions we will run into when we have multiple writeLoop workers (how we run it in prod) which you've already noted. But I also very strongly think we should decouple these gap r/w methods from the Indexer interface. Thank you for the thorough review, a few notes: * I strongly agree with decoupling the known gaps with the indexer. Honestly, I have no idea why I tried to jam known gaps in there so strongly, it gave me nothing but trouble (and pain). I will work on refactoring. * The race condition for multiple workers is certainly worrisome. I was considering the best way to proceed considering the write to `lastProcessedBlock`. There is no simple fix in my mind just yet. * Let's consider the only case in my mind where this would actually be needed - * We processed blocks 10, 11, 12. * For some reason, `geth` did not receive blocks 13, 14, 15. * We receive blocks 16, 17 18,.... and processed them * In this scenario, we would want to add blocks 13-15 to the known gaps table. The question I have is: * Is the above-mentioned scenario even possible? * Does `geth` have any build-in mechanism that would realize we jumped from block 12 to 16, and request block 13-15? Because if it does, then we do not need to handle this use case. * If `geth` does allow us to jump from blocks and have these gaps then we might need to consider an implementation. * It's also important to note that if we have any errors when processing 13-15, those blocks are added to known gaps. * If `geth` simply errors out and panics when we see we went from block 12 to 16, that's okay too, because when `geth` restarts it will see the last processed block was block 12, and we are currently on block 16+n, it will capture 13-16+n in the known gaps table. * Depending on the above, testing will vary. We should talk about the possibility of undetected gaps within geth.

❤️ 1

abdulrabbani00 (Migrated from github.com) reviewed

2022-03-29 20:33:20 +00:00

i-norden reviewed

2022-03-31 14:16:11 +00:00

i-norden left a comment

LGTM just some nitpicky comments, and if we decide to use mod_block_number + offset columns in the DB we'll need to update that here.

There is also some linting issue which I think is left over from my previous work, if you can fix that so that the linting CI test passes that'd be great.

LGTM just some nitpicky comments, and if we decide to use `mod_block_number` + `offset` columns in the DB we'll need to update that here. There is also some linting issue which I think is left over from my previous work, if you can fix that so that the linting CI test passes that'd be great.

i-norden approved these changes

2022-03-31 18:37:26 +00:00

i-norden left a comment

:shipit:

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/go-ethereum#217