[Merged by Bors] - Remove deficit gossipsub scoring during topic transition #4486

ackintosh · 2023-07-09T05:22:00Z

Issue Addressed

This PR closes #3237

Proposed Changes

Remove topic weight of old topics when the fork happens.

Additional Info

Divided NetworkService::start() into NetworkService::build() and NetworkService::start() for ease of testing.

…directly

…gossipsub-scoring # Conflicts: # beacon_node/network/Cargo.toml # beacon_node/network/src/service.rs # beacon_node/network/src/service/tests.rs

…no getter function to obtain `TopicScoreParams`

# Conflicts: # beacon_node/lighthouse_network/src/service/mod.rs # beacon_node/network/src/service/tests.rs

…t update

https://github.com/sigp/lighthouse/actions/runs/6054445319/job/16431877249?pr=4486#step:5:2249

divagant-martian

A couple of grammar corrections and a suggestion to make code a bit clearer, but otherwise I think this looks like what the original issue requested.

beacon_node/lighthouse_network/src/service/mod.rs

divagant-martian · 2023-09-10T02:48:20Z

beacon_node/network/src/service.rs

+
+    #[cfg(test)]
+    #[allow(dead_code)]
+    fn get_topic_params(
+        &self,
+        topic: GossipTopic,
+    ) -> Option<&lighthouse_network::libp2p::gossipsub::TopicScoreParams> {
+        self.libp2p.get_topic_params(topic)
+    }


#[allow(dead_code)] annotations I think are generally confusing. It's not clear if this is used or not in the tests (because if yes then the #[cfg(test)] should be enough).

To better align this with where it's used, you could move it to beacon_node/network/src/service/tests.rs like this

// use tokio::runtime::Runtime; // use types::{Epoch, EthSpec, ForkName, MinimalEthSpec, SubnetId}; impl<T: beacon_chain::BeaconChainTypes> NetworkService<T> { fn get_topic_params( &self, topic: GossipTopic, ) -> Option<&lighthouse_network::libp2p::gossipsub::TopicScoreParams> { self.libp2p.get_topic_params(topic) } } // fn get_logger(actual_log: bool) -> Logger { // .. rest of the code

and it will not require any extra annotation

AgeManning

This looks good to me also. Just pending @divagant-martian comments

Co-authored-by: Divma <[email protected]>

## Issue Addressed This PR closes #3237 ## Proposed Changes Remove topic weight of old topics when the fork happens. ## Additional Info - Divided `NetworkService::start()` into `NetworkService::build()` and `NetworkService::start()` for ease of testing.

bors · 2023-09-15T07:26:51Z

Build failed (retrying...):

release-tests-windows

## Issue Addressed This PR closes #3237 ## Proposed Changes Remove topic weight of old topics when the fork happens. ## Additional Info - Divided `NetworkService::start()` into `NetworkService::build()` and `NetworkService::start()` for ease of testing.

bors · 2023-09-15T08:49:09Z

Build failed:

release-tests-windows

AgeManning · 2023-09-19T05:49:45Z

We are in the process of fixing the CI, so this should be able to merged smoother

michaelsproul · 2023-09-20T23:49:59Z

bors r+

## Issue Addressed This PR closes #3237 ## Proposed Changes Remove topic weight of old topics when the fork happens. ## Additional Info - Divided `NetworkService::start()` into `NetworkService::build()` and `NetworkService::start()` for ease of testing.

bors · 2023-09-20T23:56:55Z

Build failed (retrying...):

release-tests-ubuntu

## Issue Addressed This PR closes #3237 ## Proposed Changes Remove topic weight of old topics when the fork happens. ## Additional Info - Divided `NetworkService::start()` into `NetworkService::build()` and `NetworkService::start()` for ease of testing.

bors · 2023-09-21T00:03:58Z

Build failed (retrying...):

release-tests-ubuntu

beacon_node/network/src/service/tests.rs

michaelsproul · 2023-09-21T00:11:31Z

bors r-

bors · 2023-09-21T00:11:33Z

Canceled.

paulhauner · 2023-10-04T00:43:10Z

bors r+

## Issue Addressed This PR closes #3237 ## Proposed Changes Remove topic weight of old topics when the fork happens. ## Additional Info - Divided `NetworkService::start()` into `NetworkService::build()` and `NetworkService::start()` for ease of testing.

bors · 2023-10-04T01:20:24Z

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

## Issue Addressed This PR closes sigp#3237 ## Proposed Changes Remove topic weight of old topics when the fork happens. ## Additional Info - Divided `NetworkService::start()` into `NetworkService::build()` and `NetworkService::start()` for ease of testing.

This PR closes sigp#3237 Remove topic weight of old topics when the fork happens. - Divided `NetworkService::start()` into `NetworkService::build()` and `NetworkService::start()` for ease of testing.

Remove topic weight from old fork topics

9a20b0d

ackintosh changed the title ~~Remove topic weight from old fork topics~~ Remove deficit gossipsub scoring during topic transition Jul 9, 2023

ackintosh added 10 commits July 9, 2023 14:23

cargo fmt

9aff368

Make clippy happy

c2f002d

WIP: unit test

e82e8b4

Unit test using logs because there's no way to check PeerScoreParams …

c853427

…directly

Manually advance slots instead of sleeping to improve testing time

1009d88

Merge remote-tracking branch 'upstream/unstable' into remove-deficit-…

575917c

…gossipsub-scoring # Conflicts: # beacon_node/network/Cargo.toml # beacon_node/network/src/service.rs # beacon_node/network/src/service/tests.rs

Fix return types

4e97ec7

Fix clippy warning

5623e45

WIP: There is no way to test topic_weight since libp2p-gossipsub has …

152d1f0

…no getter function to obtain `TopicScoreParams`

Fix unexpected change

5589e87

ackintosh mentioned this pull request Jul 22, 2023

Remove deficit gossipsub scoring during topic transition #3237

Closed

ackintosh added 4 commits August 2, 2023 07:06

get_topic_params returns a reference to TopicScoreParams

c488b10

Merge branch 'unstable' into remove-deficit-gossipsub-scoring

c5e6168

# Conflicts: # beacon_node/lighthouse_network/src/service/mod.rs # beacon_node/network/src/service/tests.rs

Check that topic_weight on the old topics has been zeroed

2e26f77

Update how to create beacon processor channels according to the lates…

75ac306

…t update

ackintosh force-pushed the remove-deficit-gossipsub-scoring branch from 4168d1b to 75ac306 Compare August 28, 2023 23:02

ackintosh added 2 commits September 2, 2023 07:15

Compute subnets and add assertions

b144459

Tweak port

1180022

https://github.com/sigp/lighthouse/actions/runs/6054445319/job/16431877249?pr=4486#step:5:2249

ackintosh force-pushed the remove-deficit-gossipsub-scoring branch from 9e54d24 to 1180022 Compare September 3, 2023 04:46

Allow dead code

09d08c7

ackintosh marked this pull request as ready for review September 3, 2023 06:32

ackintosh requested review from jxs, AgeManning and divagant-martian September 3, 2023 06:33

divagant-martian reviewed Sep 10, 2023

View reviewed changes

AgeManning approved these changes Sep 11, 2023

View reviewed changes

ackintosh and others added 2 commits September 13, 2023 06:31

Update beacon_node/lighthouse_network/src/service/mod.rs

71a219d

Co-authored-by: Divma <[email protected]>

Update beacon_node/lighthouse_network/src/service/mod.rs

3c3257e

Co-authored-by: Divma <[email protected]>

michaelsproul requested changes Sep 21, 2023

View reviewed changes

beacon_node/network/src/service/tests.rs Outdated Show resolved Hide resolved

michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-merge This PR is ready to merge. labels Sep 21, 2023

ackintosh added 2 commits September 28, 2023 06:58

Merge branch 'unstable' into remove-deficit-gossipsub-scoring

3636bb1

Update port setting according to the QUIC support

aa4acf5

divagant-martian requested a review from michaelsproul September 28, 2023 03:00

divagant-martian added ready-for-merge This PR is ready to merge. and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 30, 2023

michaelsproul approved these changes Oct 3, 2023

View reviewed changes

bors bot changed the title ~~Remove deficit gossipsub scoring during topic transition~~ [Merged by Bors] - Remove deficit gossipsub scoring during topic transition Oct 4, 2023

bors bot closed this Oct 4, 2023

ackintosh deleted the remove-deficit-gossipsub-scoring branch May 5, 2024 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Merged by Bors] - Remove deficit gossipsub scoring during topic transition #4486

[Merged by Bors] - Remove deficit gossipsub scoring during topic transition #4486

ackintosh commented Jul 9, 2023 •

edited

Loading

Uh oh!

divagant-martian left a comment

Uh oh!

Uh oh!

Uh oh!

divagant-martian Sep 10, 2023

Uh oh!

AgeManning left a comment

Uh oh!

bors bot commented Sep 15, 2023

Uh oh!

bors bot commented Sep 15, 2023

Uh oh!

AgeManning commented Sep 19, 2023

Uh oh!

michaelsproul commented Sep 20, 2023

Uh oh!

bors bot commented Sep 20, 2023

Uh oh!

bors bot commented Sep 21, 2023

Uh oh!

Uh oh!

michaelsproul commented Sep 21, 2023

Uh oh!

bors bot commented Sep 21, 2023

Uh oh!

paulhauner commented Oct 4, 2023

Uh oh!

bors bot commented Oct 4, 2023

Uh oh!

Uh oh!

[Merged by Bors] - Remove deficit gossipsub scoring during topic transition #4486

[Merged by Bors] - Remove deficit gossipsub scoring during topic transition #4486

Conversation

ackintosh commented Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

divagant-martian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

divagant-martian Sep 10, 2023

Choose a reason for hiding this comment

Uh oh!

AgeManning left a comment

Choose a reason for hiding this comment

Uh oh!

bors bot commented Sep 15, 2023

Uh oh!

bors bot commented Sep 15, 2023

Uh oh!

AgeManning commented Sep 19, 2023

Uh oh!

michaelsproul commented Sep 20, 2023

Uh oh!

bors bot commented Sep 20, 2023

Uh oh!

bors bot commented Sep 21, 2023

Uh oh!

Uh oh!

michaelsproul commented Sep 21, 2023

Uh oh!

bors bot commented Sep 21, 2023

Uh oh!

paulhauner commented Oct 4, 2023

Uh oh!

bors bot commented Oct 4, 2023

Uh oh!

Uh oh!

ackintosh commented Jul 9, 2023 •

edited

Loading