Event-based block lookup tests #5534

dapplion · 2024-04-08T11:40:41Z

Issue Addressed

Extends @jimmygchen refactor by

Applying the changes to all existing block lookup tests
Injects events directly on handle_message to avoid async machinery :)

This PR does not:

Change the syntatic style of existing tests
Modify or extend the tested scenarios. However it adds helper functions to make them more succint

Switching to event-based tests is very valuable to generalize tests, allow to test more diverse edge cases, and allow refactors without being too tied to block lookup internals.

This PR is an alternative to

Block lookup tests with SyncTester #5530

Proposed Changes

All tests now interface with sync through this event channels

                     +-----------------+
                     | BeaconProcessor |
                     +---------+-------+
                            ^  |
                            |  |
                  WorkEvent |  | SyncMsg
                            |  | (Result)
                            |  v
+--------+            +-----+-----------+             +----------------+
| Router +----------->|  SyncManager    +------------>| NetworkService |
+--------+  SyncMsg   +-----------------+ NetworkMsg  +----------------+
          (RPC resp)  |  - RangeSync    |  (RPC req)
                      +-----------------+
                      |  - BackFillSync |
                      +-----------------+
                      |  - BlockLookups |
                      +-----------------+

Additional Info

RE: style we can discuss as follow-up to this PR, I see as the contentious options:

Use declarative "state-less" API (pre-deneb tests)
Use a chained builder-stype API for tests with internal implicit state (post-deneb)

Some notes to inform the above:

chained API doesn't play well with backtrace

Consider this code, and a panic in action_a

10: rig
11:   .action_a()
12:   .action_b()
13:   .action_a()

The backtrace line for action_a will declare line 10. You can't know which of the two action_a invocations caused the error

internal implicit state succinctness

Post-deneb test are shorter at the expense of clarity. There's machinery in the back that makes assumptions about what request is doing what. For straightforward cases it's fine but it can be restrictive for more complex cases. Pre-deneb tests with proper helper functions should look as succint as those with a chained API. We can remane rig to r and it almost looks like chained, which allowing conditional flow and storing variables.

Comments

Any sync tests are quite difficult to understand for those that have not written them. Having comments explaining the case is very important for either style. In that direction I think we should focus much more on having the test easy to understand and well documented with comments besides succintness

…e extra beacon chain harness instance created in tests.

mergify · 2024-04-08T11:41:15Z

⚠️ The sha of the head commit of this PR conflicts with #5530. Mergify cannot evaluate rules on this PR. ⚠️

beacon_node/network/src/sync/block_lookups/mod.rs

realbigsean

Love the move to event based testing, thanks @jimmygchen and @dapplion !

The test do already look more straightforward and looks like there are some logging improvements in here.

stateless vs. builder-ish API

This PR is worth merging regardless of where we land there.

Any sync tests are quite difficult to understand for those that have not written them. Having comments explaining the case is very important for either style. In that direction I think we should focus much more on having the test easy to understand and well documented with comments besides succintness

Agree! My fault for lack of comments on a lot of the existing ones.

beacon_node/network/src/sync/manager.rs

beacon_node/network/src/sync/block_lookups/tests.rs

realbigsean · 2024-04-08T22:55:06Z

beacon_node/network/src/sync/block_lookups/tests.rs

+    }
+
+    fn after_deneb(&self) -> bool {
+        matches!(self.current_fork(), ForkName::Deneb | ForkName::Electra)


Here's a place fork order would be useful, so we don't forget to enable these in future forks

beacon_node/network/src/sync/block_lookups/tests.rs

jimmygchen · 2024-04-09T00:59:46Z

beacon_node/network/src/sync/manager.rs

+            range_sync: RangeSync::new(beacon_chain.clone(), log.clone()),
+            backfill_sync: BackFillSync::new(beacon_chain.clone(), network_globals, log.clone()),
+            block_lookups: BlockLookups::new(
+                beacon_chain.data_availability_checker.clone(),
+                log.clone(),
+            ),


Unrelated to this PR but since you're touching this I thought it might be worth mentioning - perhaps we could create child logger for each of these individual components so we know where the log is coming from?

log.new(slog::o!("service" => "range_sync")

ps. i'm not suggesting a change in this PR

Does this label then apply to all logs? That's a good suggestion, can add to a future PR def

dapplion · 2024-04-09T01:15:55Z

Moved the logging changes into their own PR

Improve block lookup logging #5535

jimmygchen · 2024-04-09T01:24:35Z

beacon_node/network/src/sync/block_lookups/tests.rs

    rng: XorShiftRng,
    harness: BeaconChainHarness<T>,
+    sync_manager: SyncManager<T>,
+    beacon_processor: Arc<NetworkBeaconProcessor<T>>,


Any reasons to have beacon_processor in the TestRig? I don't see it being used other than accessing network_globals.

I kept it out because the test rig should not need to directly interact with the beacon_processor directly in any way, otherwise it could interfere testing of the SyncManager.

Good point! Removed beacon_processor for network_globals, and harness for fork_name

jimmygchen · 2024-04-09T01:27:18Z

beacon_node/network/src/sync/block_lookups/tests.rs

    rng: XorShiftRng,
    harness: BeaconChainHarness<T>,
+    sync_manager: SyncManager<T>,
+    beacon_processor: Arc<NetworkBeaconProcessor<T>>,


Could we have some of the field documentation back as I think it would help people writing the tests understand what each of these fields are for, without reading through the implementation.

https://github.com/sigp/lighthouse/pull/5530/files#diff-13439a1c05bab56ff7fb8b91e2d4f569076456e8a909000e1d9d3b5df8afb1b0R59-R81

…-p2p-tests

realbigsean · 2024-04-09T16:55:56Z

@Mergifyio queue

mergify · 2024-04-09T16:56:05Z

queue

🛑 The pull request rule doesn't match anymore

The pull request has been synchronized by a user.

realbigsean · 2024-04-09T17:03:05Z

@Mergifyio refresh

mergify · 2024-04-09T17:03:13Z

refresh

✅ Pull request refreshed

realbigsean · 2024-04-09T17:19:51Z

@Mergifyio requeue

mergify · 2024-04-09T17:20:18Z

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

mergify · 2024-04-09T17:20:19Z

queue

🛑 The pull request has been removed from the queue `default`

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

jimmygchen · 2024-04-10T09:22:20Z

@mergify requeue

mergify · 2024-04-10T09:22:27Z

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

mergify · 2024-04-10T09:22:29Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at b1f9751

jimmygchen and others added 10 commits April 4, 2024 17:48

WIP

86b77a0

Initial working version of new sync tests.

6bfeaba

Remove sync traits and fix lints.

5ba47b6

Reduce internal method visibility and make test method instead. Remov…

857f94f

…e extra beacon chain harness instance created in tests.

Improve SyncTester api.

de109d0

Fix lint.

0fa3153

Test example

3c3a475

Lookup tests using rig

241343b

Tests should interface with events only

3690d54

lint

032d9f7

dapplion added 2 commits April 8, 2024 22:13

Skip deneb test pre-deneb

4988da9

Add more assertions

90cddc8

dapplion commented Apr 8, 2024

View reviewed changes

beacon_node/network/src/sync/block_lookups/mod.rs Outdated Show resolved Hide resolved

realbigsean reviewed Apr 9, 2024

View reviewed changes

beacon_node/network/src/sync/block_lookups/tests.rs Outdated Show resolved Hide resolved

Remove logging changes

16d7d9b

jimmygchen reviewed Apr 9, 2024

View reviewed changes

dapplion mentioned this pull request Apr 9, 2024

Improve block lookup logging #5535

Merged

jimmygchen reviewed Apr 9, 2024

View reviewed changes

dapplion and others added 3 commits April 9, 2024 11:35

Address @jimmygchen comments

8b8b624

Merge branch 'unstable' of https://github.com/sigp/lighthouse into bn…

f39268c

…-p2p-tests

remove unused assertions

5714e8e

realbigsean approved these changes Apr 9, 2024

View reviewed changes

realbigsean added the ready-for-merge This PR is ready to merge. label Apr 9, 2024

mergify bot added a commit that referenced this pull request Apr 9, 2024

Merge of #5534

124ebc2

This was referenced Apr 9, 2024

merge queue: embarking unstable (b74da14) and [#5534 + #5531] together #5540

Closed

Impl Ord on ForkName for ChainSpec usage #5531

Merged

fix lint

bca10a3

jimmygchen mentioned this pull request Apr 10, 2024

Block lookup tests with SyncTester #5530

Closed

dapplion mentioned this pull request Apr 10, 2024

Block lookup sync technical debt #5549

Closed

mergify bot added a commit that referenced this pull request Apr 10, 2024

Merge of #5534

1f75cc9

This was referenced Apr 10, 2024

merge queue: embarking unstable (ced6538) and [#5420 + #5352 + #5534] together #5551

Closed

Move processing cache out of DA #5420

Merged

Skip calculation of boosted_relay_value when builder_boost_factor pro… #5352

Merged

mergify bot added a commit that referenced this pull request Apr 10, 2024

Merge of #5534

c02a500

mergify bot mentioned this pull request Apr 10, 2024

merge queue: embarking unstable (ced6538) and [#5352 + #5534] together #5553

Closed

8 tasks

mergify bot merged commit b1f9751 into sigp:unstable Apr 10, 2024

dapplion mentioned this pull request Nov 27, 2024

Write range sync tests in external event-driven form #6618

Merged

dapplion deleted the bn-p2p-tests branch January 24, 2025 17:57

Event-based block lookup tests #5534

Event-based block lookup tests #5534

Uh oh!

Conversation

dapplion commented Apr 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

mergify bot commented Apr 8, 2024

Uh oh!

Uh oh!

realbigsean left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dapplion commented Apr 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

realbigsean commented Apr 9, 2024

Uh oh!

mergify bot commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛑 The pull request rule doesn't match anymore

Uh oh!

realbigsean commented Apr 9, 2024

Uh oh!

mergify bot commented Apr 9, 2024

✅ Pull request refreshed

Uh oh!

realbigsean commented Apr 9, 2024

Uh oh!

mergify bot commented Apr 9, 2024

✅ This pull request will be re-embarked automatically

Uh oh!

mergify bot commented Apr 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛑 The pull request has been removed from the queue default

Uh oh!

jimmygchen commented Apr 10, 2024

Uh oh!

mergify bot commented Apr 10, 2024

✅ This pull request will be re-embarked automatically

Uh oh!

mergify bot commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ The pull request has been merged automatically

Uh oh!

Uh oh!

dapplion commented Apr 8, 2024 •

edited

Loading

mergify bot commented Apr 9, 2024 •

edited

Loading

mergify bot commented Apr 9, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

mergify bot commented Apr 10, 2024 •

edited

Loading