-
Notifications
You must be signed in to change notification settings - Fork 582
refactor: atomic reorg handling #19364
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
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| // Update the header to the last block. | ||
| const newHeader = await this.node.getBlockHeader(event.block.number); | ||
| if (!newHeader) { | ||
| this.log.error(`Block header not found for block number ${event.block.number} during chain prune`); |
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.
Just logging out an error here if the new header is not found seemed incorrect as this would result in an inconsistent PXE state. For this reason I now throw an error here.
3ab2740 to
cefb7ca
Compare
cefb7ca to
eba9c04
Compare
eba9c04 to
e5f9c22
Compare
mverzilli
left a comment
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.
LGTM, just consider using some Promise.all where possible/sensible
83a369a to
1e1c8b8
Compare
nventuro
left a comment
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.
Approving to unblock, but you should probably address my feedback. Thanks!
yarn-project/stdlib/src/kernel/hints/build_note_hash_read_request_hints.ts
Show resolved
Hide resolved
e5f9c22 to
44d8dd8
Compare
Flakey Tests🤖 says: This CI run detected 2 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
1a8468f to
a61be37
Compare
As Martin mentioned [here](#19327 (comment)) makes sense to have the reorg handling be atomic. In this PR I tackle that. Note that this PR doesn't need to be in stack with #19443 but #19451 depends on both so putting this one into stack was the only way to unblock myself working on #19451.
a61be37 to
911b2b3
Compare

As Martin mentioned here makes sense to have the reorg handling be atomic. In this PR I tackle that.
Note that this PR doesn't need to be in stack with #19443 but #19451 depends on both so putting this one into stack was the only way to unblock myself working on #19451.