feat: new NoteProcessor works through all blocks #1404
Conversation
benesjan
force-pushed
the
janb/note-processor-catch-up
branch
from
f831bb2 to
ba176c4
Compare
benesjan
force-pushed
the
janb/note-processor-catch-up
branch
from
b3b439e to
dea49f3
Compare
| if (this.synchedToBlock === 0) { | ||
| // The main sync thread was never started before and for this reason the synchroniser does not have to catch up | ||
| this.noteProcessors.push(noteProcessor); | ||
| } else { | ||
| // The main sync thread was started before and for this reason the synchroniser has to catch up | ||
| this.noteProcessorsToCatchUp.push(noteProcessor); | ||
| } |
There was a problem hiding this comment.
@benesjan sorry for going back to a merged PR, but I think you may have a race condition here: if a noteProcessor is added on the first run of work during await this.updateBlockInfoInBlockTxs(blockContexts), which happens after the note processors are notified but before updating synchedToBlock, then it will be pushed to noteProcessors but will never get the notes from the first iteration.
If the above is correct, maybe you can solve it with a flag isStarted that you set as soon as start is called, and check that instead of synchedToBlock?
There was a problem hiding this comment.
I think @spalladino is right. Perhaps a clean solution is to always add the note processor to noteProcessorsToCatchUp and have workNoteProcessorCatchUp transfer it to noteProcessors in all scenarios.
There was a problem hiding this comment.
@spalladino great catch! Will address
FunctionL2Logs.randomChecklist:
Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.