-
Notifications
You must be signed in to change notification settings - Fork 738
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
Race condition fix + Reliability improvements around forks pruning #1132
Conversation
I'm still seeing some pruning errors when syncing Schlesi:
EDIT: For clarity, I am using this branch to sync. |
It also seems like it's failing store-related tests on your end. I'm guessing you know this and are looking for a preliminary review :) |
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.
Ok after reviewing I think I understand that perhaps this PR isn't necessarily supposed to solve the block pruning error.
I like the atomic operations stuff but think the iterator needs to be guarded against running forever.
I'll just leave this as a comment since I haven't done a super through review since the branch is failing tests and I assume will need further changes.
@michaelsproul I know you're busy but it might be worth just quickly throwing your eyes across the atomics here :)
.map(|result| result.map(|(_, block)| block)) | ||
// Include the block at the end slot (if any), it needs to be | ||
// replayed in order to construct the canonical state at `end_slot`. | ||
.filter(|result| match result { |
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.
Nit: potentially a nice place for the recently-added map_or function.
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.
Almost. filter
passes the iterator item to its closure by reference whereas map_or
consumes self
.
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.
You can use result.as_ref().map_or(true, |block| ...)
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.
You can use result.as_ref().map_or(..)
to avoid consuming self.
@@ -145,6 +146,37 @@ impl<E: EthSpec> Store<E> for LevelDB<E> { | |||
) -> Self::ForwardsBlockRootsIterator { | |||
SimpleForwardsBlockRootsIterator::new(store, start_slot, end_state, end_block_root) | |||
} | |||
|
|||
fn do_atomically(&self, atomic_batch: Vec<StoreOp>) -> Result<(), Error> { |
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.
This is cool!
7dc9ffd
to
fac0bab
Compare
I created this PR to eliminate stuff that might have been causing problems around forks pruning since I couldn't reproduce it on my end. The tests are now passing and the PR is ready for review and merging. |
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.
Looks great! I'm syncing Schlesi now to confirm that the race condition is fixed, happy to merge once that works and comments are addressed
beacon_node/store/src/iter.rs
Outdated
} else { | ||
let block_root = self.next_block_root; | ||
let block = self.store.get_block(&block_root).ok()??; | ||
let block = match self.store.get_block(&block_root)? { |
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.
This could use Option::ok_or{_else}
, like:
self.store.get_block(&block_root)?.ok_or(Error::BlockNotFound(block_root))?
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.
👍
.map(|result| result.map(|(_, block)| block)) | ||
// Include the block at the end slot (if any), it needs to be | ||
// replayed in order to construct the canonical state at `end_slot`. | ||
.filter(|result| match result { |
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.
You can use result.as_ref().map_or(true, |block| ...)
Also, looks like CI is failing on the |
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.
Happy with the substantive content.
I'll give this an approval pending @michaelsproul's comments are addressed and don't change the actual logic.
@@ -1522,6 +1524,9 @@ impl<T: BeaconChainTypes> BeaconChain<T> { | |||
); | |||
}); | |||
|
|||
self.head_tracker |
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.
This was a great pickup, nice work.
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.
Thanks :)
d275911
to
48d89e6
Compare
An invariant was violated: For every block hash in head_tracker, that block is accessible from the store.
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.
Perfect! Let’s get this merged! (I’ll let @paulhauner choose a merge ordering)
Thanks for squashing a killer bug @adaszko! |
Fixes #1109