Race condition fix + Reliability improvements around forks pruning #1132

adaszko · 2020-05-11T13:02:05Z

paulhauner · 2020-05-13T01:38:03Z

I'm still seeing some pruning errors when syncing Schlesi:

May 13 11:36:29.566 WARN Block pruning failed: BeaconStateError(MissingBeaconBlock(SignedBeaconBlockHash(0x48983f73d7f1e11806fbf02d3db2b50af3611b7b65debed084d915d3658b5d69))), service: freezer_db

EDIT: For clarity, I am using this branch to sync.

paulhauner · 2020-05-13T01:55:24Z

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 :)

paulhauner

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 :)

paulhauner · 2020-05-13T02:00:27Z

beacon_node/store/src/hot_cold_store.rs

+                .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 {


Nit: potentially a nice place for the recently-added map_or function.

Almost. filter passes the iterator item to its closure by reference whereas map_or consumes self.

You can use result.as_ref().map_or(true, |block| ...)

You can use result.as_ref().map_or(..) to avoid consuming self.

beacon_node/beacon_chain/src/migrate.rs

paulhauner · 2020-05-13T02:11:19Z

beacon_node/store/src/leveldb_store.rs

@@ -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> {


This is cool!

adaszko · 2020-05-13T13:44:09Z

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 :)

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.

michaelsproul

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/lib.rs

michaelsproul · 2020-05-15T00:11:20Z

beacon_node/store/src/iter.rs

        } 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)? {


This could use Option::ok_or{_else}, like:

self.store.get_block(&block_root)?.ok_or(Error::BlockNotFound(block_root))?

beacon_node/beacon_chain/src/migrate.rs

beacon_node/store/src/hot_cold_store.rs

michaelsproul · 2020-05-15T00:22:48Z

beacon_node/store/src/hot_cold_store.rs

+                .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 {


You can use result.as_ref().map_or(true, |block| ...)

beacon_node/store/src/hot_cold_store.rs

michaelsproul · 2020-05-15T00:43:17Z

Also, looks like CI is failing on the head_tracker tests due to the API change: https://github.com/adaszko/lighthouse/runs/674828986

paulhauner

Happy with the substantive content.

I'll give this an approval pending @michaelsproul's comments are addressed and don't change the actual logic.

beacon_node/beacon_chain/src/migrate.rs

paulhauner · 2020-05-15T00:47:57Z

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -1522,6 +1524,9 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                );
            });

+        self.head_tracker


This was a great pickup, nice work.

An invariant was violated: For every block hash in head_tracker, that block is accessible from the store.

michaelsproul

Perfect! Let’s get this merged! (I’ll let @paulhauner choose a merge ordering)

paulhauner · 2020-05-16T03:23:26Z

Thanks for squashing a killer bug @adaszko!

adaszko force-pushed the pruning-bugs branch from 68650c7 to 2955e6b Compare May 11, 2020 13:24

paulhauner added the ready-for-review The code is ready for review label May 12, 2020

paulhauner reviewed May 13, 2020

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels May 13, 2020

adaszko force-pushed the pruning-bugs branch 2 times, most recently from 7dc9ffd to fac0bab Compare May 13, 2020 12:29

adaszko requested a review from paulhauner May 14, 2020 09:25

adaszko added 2 commits May 14, 2020 16:34

Improve error handling in block iteration

bda60ef

Introduce atomic DB operations

7dcb6f0

adaszko force-pushed the pruning-bugs branch from fac0bab to d9c0a71 Compare May 14, 2020 14:35

adaszko changed the title ~~Improve error handling in block iteration~~ Race condition fix + Reliability improvements around forks pruning May 14, 2020

michaelsproul requested changes May 15, 2020

View reviewed changes

paulhauner approved these changes May 15, 2020

View reviewed changes

adaszko force-pushed the pruning-bugs branch 2 times, most recently from d275911 to 48d89e6 Compare May 15, 2020 07:42

Fix race condition

f1daadd

An invariant was violated: For every block hash in head_tracker, that block is accessible from the store.

adaszko force-pushed the pruning-bugs branch from 48d89e6 to f1daadd Compare May 15, 2020 10:11

michaelsproul approved these changes May 16, 2020

View reviewed changes

michaelsproul added ready-to-squerge and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels May 16, 2020

paulhauner merged commit 59ead67 into sigp:master May 16, 2020

adaszko mentioned this pull request Jun 5, 2020

Add error handling to iterators #1243

Merged

dapplion mentioned this pull request Jun 10, 2024

Store linear blocks in freezer db #5905

Draft

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Race condition fix + Reliability improvements around forks pruning #1132

Race condition fix + Reliability improvements around forks pruning #1132

adaszko commented May 11, 2020 •

edited

Loading

paulhauner commented May 13, 2020 •

edited

Loading

paulhauner commented May 13, 2020

paulhauner left a comment

paulhauner May 13, 2020

adaszko May 13, 2020

michaelsproul May 15, 2020

paulhauner May 15, 2020 •

edited

Loading

paulhauner May 13, 2020

adaszko commented May 13, 2020

michaelsproul left a comment

michaelsproul May 15, 2020

adaszko May 15, 2020

michaelsproul May 15, 2020

michaelsproul commented May 15, 2020 •

edited

Loading

paulhauner left a comment

paulhauner May 15, 2020

adaszko May 15, 2020

michaelsproul left a comment

paulhauner commented May 16, 2020

Race condition fix + Reliability improvements around forks pruning #1132

Race condition fix + Reliability improvements around forks pruning #1132

Conversation

adaszko commented May 11, 2020 • edited Loading

paulhauner commented May 13, 2020 • edited Loading

paulhauner commented May 13, 2020

paulhauner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulhauner May 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adaszko commented May 13, 2020

michaelsproul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsproul commented May 15, 2020 • edited Loading

paulhauner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelsproul left a comment

Choose a reason for hiding this comment

paulhauner commented May 16, 2020

adaszko commented May 11, 2020 •

edited

Loading

paulhauner commented May 13, 2020 •

edited

Loading

paulhauner May 15, 2020 •

edited

Loading

michaelsproul commented May 15, 2020 •

edited

Loading