[Merged by Bors] - Alternative (to BeaconChainHarness) BeaconChain testing API #1380

adaszko · 2020-07-22T06:28:25Z

The PR:

Adds the ability to generate a crucial test scenario that isn't possible with BeaconChainHarness (i.e. two blocks occupying the same slot; previously forks necessitated skipping slots):

New testing API: Instead of repeatedly calling add_block(), you generate a sorted Vec<Slot> and leave it up to the framework to generate blocks at those slots.
Jumping backwards to an earlier epoch is a hard error, so that tests necessarily generate blocks in a epoch-by-epoch manner.
Configures the test logger so that output is printed on the console in case a test fails. The logger also plays well with --nocapture, contrary to the existing testing framework
Rewrites existing fork pruning tests to use the new API
Adds a tests that triggers finalization at a non epoch boundary slot
Renamed BeaconChainYoke to BeaconChainTestingRig because the former has been too confusing
Fixed multiple tests (e.g. block_production_different_shuffling_long, delete_blocks_and_states, shuffling_compatible_simple_fork) that relied on a weird (and accidental) feature of the old BeaconChainHarness that attestations aren't produced for epochs earlier than the current one, thus masking potential bugs in test cases.

michaelsproul

This looks good, and definitely like an improvement, but I'm a little hesitant about maintaining two very similar harnesses in-tree. Do you think it would be feasible to rewrite the existing harness tests in terms of the yoke? Maybe even with compatibility shims that defer to the yoke's functions. We don't have to do it all at once, and I'd be happy to help with the effort.

Having just one harness would solve the naming issue too 😅 I mistook yoke for yolk when I first saw it and had to look up the definition... I guess we could also choose another name? Driver? Runner?

beacon_node/beacon_chain/src/test_utils.rs

beacon_node/beacon_chain/tests/store_tests.rs

paulhauner

Arr, I wrote up a response to this but it's not here. I must have forgot to submit :(

Agree with @michaelsproul about maintaining two harnesses and also the "yoke" name (perhaps I'm not just familiar with the word). Suggestion: BeaconChainTestRig (not in love with it.. this thing is hard name lol).

I think it would be ideal to have "one beacon chain harness to rule them all", but whether or not we do that in this PR is the question at hand, I assume. I'm happy to see this merged with two harnesses, with the intention of eventually have one.

I noticed that this new harness rewinds the slot clock. I think we might eventually run into issues if we wind back too much, especially around the "seen" caches (that check for already-seen blocks, attestations). If we come across these I think we should fix them, since we should ideally support an arbitrary slot clock.

Approved pending Michael's comments are addressed :)

beacon_node/beacon_chain/src/test_utils.rs

beacon_node/beacon_chain/tests/store_tests.rs

michaelsproul

I'm sorry it's taken me so long to re-review this. I still can't sign off on it with two almost identical harnesses present, as this is adding technical debt rather than alleviating it. I'd like to see:

One harness type in test_utils.rs, it may as well be called BeaconChainHarness
The ability to run with and without a disk DB (two harness types)
All tests currently in beacon_chain/tests passing, and straight-forward to verify their meaning hasn't changed (refactoring all the tests is not desirable, as it would be very time consuming to do and review)

I think the best way to achieve this would be to fuse the new parts of your testing rig into the BeaconChainHarness, while preferring the original function names (to avoid too much more refactoring). AFAICT the main new things are:

add_block_at_slot does the trick with the graffiti and the slot clock
There are some new useful utility methods like get_finalized_checkpoints and cold_state_exists
The harness holds an RNG and some validator counts (straight-forward)

I realise that's a lot of work, in which case we could expedite the block pruning fix and the get_state change in a separate smaller PR.

beacon_node/beacon_chain/src/test_utils.rs

Extracted from #1380 because merging #1380 proves to be contentious. Co-authored-by: Michael Sproul <[email protected]>

…chain test case

@adaszko

## Issue Addressed Closes #1488 ## Proposed Changes * Prevent the pruning algorithm from over-eagerly deleting states at skipped slots when they are shared with the canonical chain. * Add `debug` logging to the pruning algorithm so we have so better chance of debugging future issues from logs. * Modify the handling of the "finalized state" in the beacon chain, so that it's always the state at the first slot of the finalized epoch (previously it was the state at the finalized block). This gives database pruning a clearer and cleaner view of things, and will marginally impact the pruning of the op pool, observed proposers, etc (in ways that are safe as far as I can tell). * Remove duplicated `RevertedFinalizedEpoch` check from `after_finalization` * Delete useless and unused `max_finality_distance` * Add tests that exercise pruning with shared states at skip slots * Delete unnecessary `block_strategy` argument from `add_blocks` and friends in the test harness (will likely conflict with #1380 slightly, sorry @adaszko -- but we can fix that) * Bonus: add a `BeaconChain::with_head` method. I didn't end up needing it, but it turned out quite nice, so I figured we could keep it? ## Additional Info Any users who have experienced pruning errors on Medalla will need to resync after upgrading to a release including this change. This should end unbounded `chain_db` growth! 🎉

michaelsproul

This PR is so close. I've merged master back in to it and fixed up the pruning tests that were added as part of #1564

michaelsproul · 2020-08-26T05:46:49Z

beacon_node/beacon_chain/tests/store_tests.rs

+    loop {
+        let epoch = slot.epoch(E::slots_per_epoch());
+        let epoch_start_slot: u64 = epoch.start_slot(E::slots_per_epoch()).into();
+        let epoch_end_slot: u64 = epoch.end_slot(E::slots_per_epoch()).into();
+        let this_epoch_slots: HashSet<Slot> =
+            (epoch_start_slot..epoch_end_slot).map(Into::into).collect();
+
+        let this_epoch_fork1_slots: Vec<Slot> = fork1_slots
+            .iter()
+            .filter(|slot| this_epoch_slots.contains(slot))
+            .copied()
+            .collect();
+        let this_epoch_fork2_slots: Vec<Slot> = fork2_slots
+            .iter()
+            .filter(|slot| this_epoch_slots.contains(slot))
+            .copied()
+            .collect();
+
+        if this_epoch_fork1_slots.is_empty() && this_epoch_fork2_slots.is_empty() {
+            break;
+        }
+
+        let (_, _, honest_head_, fork1_state_) = harness.add_attested_blocks_at_slots(
+            fork1_state,
+            &this_epoch_fork1_slots,
+            &honest_validators,
+        );
+        honest_head = honest_head_.into();
+        fork1_state = fork1_state_;
+
+        let (_, _, faulty_head_, fork2_state_) = harness.add_attested_blocks_at_slots(
+            fork2_state,
+            &this_epoch_fork2_slots,
+            &faulty_validators,
+        );
+        faulty_head = faulty_head_.into();
+        fork2_state = fork2_state_;
+
+        slot = epoch.end_slot(E::slots_per_epoch()) + 1;
+    }


I had a go at abstracting this pattern (or something similar to it) into add_blocks_on_multiple_chains, which you can see an example of in the updated pruning_test. Would you mind refactoring this block in terms of the new primitive, and then we can merge?

I was thinking of abstracting it that way too. Just wanted to have something working first, then clean up. Notice how much easier it is to abstract things with the new harness.

I did appreciate that! I was a bit dubious of whether it would work, but it worked almost as soon as I ran it :O

beacon_node/beacon_chain/tests/store_tests.rs

michaelsproul

I know it's been a hard slog, thanks for persevering and taking my suggestions on board. I think we've ended up with a really good test harness that will be useful for many future tests! Definitely a good legacy ☺️

Without further ado...

bors r+

🤯 🎉 ✨

The PR: * Adds the ability to generate a crucial test scenario that isn't possible with `BeaconChainHarness` (i.e. two blocks occupying the same slot; previously forks necessitated skipping slots): ![image](https://user-images.githubusercontent.com/165678/88195404-4bce3580-cc40-11ea-8c08-b48d2e1d5959.png) * New testing API: Instead of repeatedly calling add_block(), you generate a sorted `Vec<Slot>` and leave it up to the framework to generate blocks at those slots. * Jumping backwards to an earlier epoch is a hard error, so that tests necessarily generate blocks in a epoch-by-epoch manner. * Configures the test logger so that output is printed on the console in case a test fails. The logger also plays well with `--nocapture`, contrary to the existing testing framework * Rewrites existing fork pruning tests to use the new API * Adds a tests that triggers finalization at a non epoch boundary slot * Renamed `BeaconChainYoke` to `BeaconChainTestingRig` because the former has been too confusing * Fixed multiple tests (e.g. `block_production_different_shuffling_long`, `delete_blocks_and_states`, `shuffling_compatible_simple_fork`) that relied on a weird (and accidental) feature of the old `BeaconChainHarness` that attestations aren't produced for epochs earlier than the current one, thus masking potential bugs in test cases. Co-authored-by: Michael Sproul <[email protected]>

bors · 2020-08-26T10:21:11Z

Pull request successfully merged into master.

Build succeeded:

adaszko force-pushed the pr/beacon-chain-yoke branch from 5144364 to 6bcfff3 Compare July 22, 2020 15:29

adaszko marked this pull request as ready for review July 22, 2020 15:36

adaszko requested review from michaelsproul and paulhauner July 22, 2020 15:36

adaszko added the ready-for-review The code is ready for review label Jul 22, 2020

adaszko force-pushed the pr/beacon-chain-yoke branch from 6bcfff3 to c5c4c45 Compare July 23, 2020 08:17

michaelsproul reviewed Jul 24, 2020

View reviewed changes

beacon_node/beacon_chain/src/test_utils.rs Outdated Show resolved Hide resolved

beacon_node/beacon_chain/tests/store_tests.rs Show resolved Hide resolved

paulhauner approved these changes Jul 25, 2020

View reviewed changes

michaelsproul added under-review A reviewer has only partially completed a review. and removed ready-for-review The code is ready for review labels Jul 25, 2020

adaszko force-pushed the pr/beacon-chain-yoke branch 3 times, most recently from 85c0fd1 to c60d3ba Compare July 31, 2020 13:50

adaszko added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed under-review A reviewer has only partially completed a review. labels Jul 31, 2020

paulhauner mentioned this pull request Aug 3, 2020

Tracking: October security review #1436

Closed

8 tasks

paulhauner added the sept-sec-review Required for the September 1st security review label Aug 3, 2020

adaszko force-pushed the pr/beacon-chain-yoke branch from 9b0911b to d9e63db Compare August 3, 2020 11:15

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

adaszko force-pushed the pr/beacon-chain-yoke branch from d9e63db to c29c4da Compare August 3, 2020 13:12

michaelsproul reviewed Aug 4, 2020

View reviewed changes

beacon_node/beacon_chain/src/test_utils.rs Outdated Show resolved Hide resolved

beacon_node/beacon_chain/tests/store_tests.rs Outdated Show resolved Hide resolved

adaszko force-pushed the pr/beacon-chain-yoke branch from 96a9cf8 to 29ee2d1 Compare August 4, 2020 10:05

adaszko mentioned this pull request Aug 4, 2020

Fix non-canonical chain access in DB get_state #1420

Closed

adaszko requested a review from michaelsproul August 10, 2020 10:32

michaelsproul requested changes Aug 11, 2020

View reviewed changes

beacon_node/beacon_chain/src/test_utils.rs Outdated Show resolved Hide resolved

beacon_node/beacon_chain/src/test_utils.rs Outdated Show resolved Hide resolved

beacon_node/beacon_chain/src/test_utils.rs Outdated Show resolved Hide resolved

michaelsproul 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 Aug 11, 2020

adaszko mentioned this pull request Aug 11, 2020

[Merged by Bors] - Fix a bug in fork pruning #1507

Closed

bors bot pushed a commit that referenced this pull request Aug 12, 2020

Fix a bug in fork pruning (#1507)

8a1a405

Extracted from #1380 because merging #1380 proves to be contentious. Co-authored-by: Michael Sproul <[email protected]>

adaszko added 8 commits August 24, 2020 09:38

Rewrite pruning_does_not_touch_abandoned_block_shared_with_canonical_…

9ebc4fa

…chain test case

Style fixes

3c0f5cf

Rewrite prunes_fork_running_past_finalized_checkpoint test case

ac1c1d9

Rewrite prunes_skipped_slots_states

d778b7e

Add finalizes_noncheckpoint_block test case

e33e7e6

Remove previous version of the test harness

c08994e

Review feedback

31544cf

Implement old harness in terms of the new one

da3ae3e

adaszko force-pushed the pr/beacon-chain-yoke branch from e98e42d to da3ae3e Compare August 24, 2020 07:38

Fail early if a test misbehaves

d9a75cc

adaszko force-pushed the pr/beacon-chain-yoke branch from ad83aeb to 465ad9d Compare August 24, 2020 12:43

michaelsproul mentioned this pull request Aug 25, 2020

[Merged by Bors] - Fix bug in database pruning #1564

Closed

Rewrite multi_epoch_fork_valid_blocks_test()

d94aacf

adaszko force-pushed the pr/beacon-chain-yoke branch from 7536e88 to d94aacf Compare August 25, 2020 13:47

adaszko added 2 commits August 25, 2020 16:10

All but one tests pass

a7f4b4e

All tests pass

da45778

michaelsproul added 4 commits August 26, 2020 10:26

Fix fork choice harness tests

0accd31

Some clean-ups and formatting fixes

75da227

Merge remote-tracking branch 'origin/master' into pr/beacon-chain-yoke

d93c407

Fix new pruning tests and add multi-chain builder

e723c91

michaelsproul reviewed Aug 26, 2020

View reviewed changes

michaelsproul and others added 3 commits August 26, 2020 16:27

Resolve warning

f14b44a

DRY delete_blocks_and_states

f6319ed

DRY multi_epoch_fork_valid_blocks_test

d4704f0

michaelsproul approved these changes Aug 26, 2020

View reviewed changes

bors bot changed the title ~~Alternative (to BeaconChainHarness) BeaconChain testing API~~ [Merged by Bors] - Alternative (to BeaconChainHarness) BeaconChain testing API Aug 26, 2020

bors bot closed this Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Alternative (to BeaconChainHarness) BeaconChain testing API #1380

[Merged by Bors] - Alternative (to BeaconChainHarness) BeaconChain testing API #1380

adaszko commented Jul 22, 2020 •

edited

Loading

michaelsproul left a comment

paulhauner left a comment

michaelsproul left a comment

michaelsproul left a comment

michaelsproul Aug 26, 2020

adaszko Aug 26, 2020 •

edited

Loading

michaelsproul Aug 26, 2020

michaelsproul left a comment

bors bot commented Aug 26, 2020

[Merged by Bors] - Alternative (to BeaconChainHarness) BeaconChain testing API #1380

[Merged by Bors] - Alternative (to BeaconChainHarness) BeaconChain testing API #1380

Conversation

adaszko commented Jul 22, 2020 • edited Loading

michaelsproul left a comment

Choose a reason for hiding this comment

paulhauner left a comment

Choose a reason for hiding this comment

michaelsproul left a comment

Choose a reason for hiding this comment

michaelsproul left a comment

Choose a reason for hiding this comment

michaelsproul Aug 26, 2020

Choose a reason for hiding this comment

adaszko Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

michaelsproul Aug 26, 2020

Choose a reason for hiding this comment

michaelsproul left a comment

Choose a reason for hiding this comment

bors bot commented Aug 26, 2020

adaszko commented Jul 22, 2020 •

edited

Loading

adaszko Aug 26, 2020 •

edited

Loading