-
-
Notifications
You must be signed in to change notification settings - Fork 282
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
Merge single and multi block processors #4317
Merge single and multi block processors #4317
Conversation
} | ||
} catch (e) { | ||
// above functions should only throw BlockError | ||
const err = getBlockError(e, blocks[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the block error be for the block that errored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After completing this work everything is processed in batch so you don't know which block caused the error. Each function should take care of adding the right block to BlockError which is already happening, but here, at this line you have no clue which block caused this. blocks[0] is a placeholder, it shouldn't really happen. In future PRs we can review what to do in this case
if (res.err instanceof ChainSegmentError && res.err.importedBlocks > 0) { | ||
this.advanceChain(batch.startEpoch); | ||
} | ||
// TODO: Disabled for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make an issue for this? I guess it's a small thing, an optimization to avoid reprocessing up to batch.length - 1 blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do an issue but I don't see this ever being implemented judging for our priorities in the last 2 years. i.e. to help process sync we have tens higher priority items to go after
Performance Report✔️ no performance regression detected Full benchmark results
|
Motivation
Description
Merge single and multi block processors