Skip to content

Use the mesh_n value from NetworkLoad for PeerScoreSettings#5013

Merged
mergify[bot] merged 3 commits intosigp:unstablefrom
jimmygchen:peer-score-settings
Apr 11, 2024
Merged

Use the mesh_n value from NetworkLoad for PeerScoreSettings#5013
mergify[bot] merged 3 commits intosigp:unstablefrom
jimmygchen:peer-score-settings

Conversation

@jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Dec 18, 2023 •
edited
Loading

Issue Addressed

We're using the libp2p default gs_config here for PeerScoreSettings:

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

Line 180 in 6c0c41c

let score_settings = PeerScoreSettings::new(ctx.chain_spec, &config.gs_config);

and then we build a Lighthouse specific one here and use it for Gossipsub

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

Lines 266 to 273 in 6c0c41c

let snappy_transform = SnappyTransform::new(config.gs_config.max_transmit_size());
let mut gossipsub = Gossipsub::new_with_subscription_filter_and_transform(
MessageAuthenticity::Anonymous,
config.gs_config.clone(),
gossipsub_metrics,
filter,
snappy_transform,
)

The only value that is used for initialising PeerScoreSettings from the gs_config appears to be the mesh degree mesh_n, which is taken from NetworkLoad.

Proposed Changes

Build the correct gs_config and use the mesh_n value for PeerScoreSetttings.
Note that the score settings needs a bit of rework, so this would just be a temporary fix.

The impact of this fix is that scoring will now use the mesh_n from NetworkLoad (default to 4) rather than the libp2p2 hard coded value (6).

@jimmygchen jimmygchen added Networking work-in-progress PR is a work-in-progress labels Dec 18, 2023
@jimmygchen
Copy link
Member Author

jimmygchen commented Dec 18, 2023

Marking this as work-in-progress as this PR is mainly for tracking and need some review from @AgeManning

@AgeManning AgeManning self-assigned this Jan 10, 2024
@jimmygchen
Merge branch 'unstable' into peer-score-settings
1961929 
# Conflicts:
#	beacon_node/lighthouse_network/src/config.rs
#	beacon_node/lighthouse_network/src/service/gossipsub_scoring_parameters.rs
#	beacon_node/lighthouse_network/src/service/mod.rs
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Apr 11, 2024
pawanjay176
pawanjay176 approved these changes Apr 11, 2024
View reviewed changes
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much cleaner!

@pawanjay176 pawanjay176 added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Apr 11, 2024
@realbigsean
Copy link
Member

realbigsean commented Apr 11, 2024

@Mergifyio queue

@mergify
Copy link

mergify bot commented Apr 11, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at d30ba97

mergify bot added a commit that referenced this pull request Apr 11, 2024
@mergify
Merge of #5013
f2af3d0
This was referenced Apr 11, 2024
Closed
Merged
@mergify mergify bot merged commit d30ba97 into sigp:unstable Apr 11, 2024
@jimmygchen jimmygchen deleted the peer-score-settings branch April 12, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AgeManning AgeManning AgeManning approved these changes

@pawanjay176 pawanjay176 pawanjay176 approved these changes

Assignees

@AgeManning AgeManning

Labels

Networking ready-for-merge This PR is ready to merge.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jimmygchen @realbigsean @AgeManning @pawanjay176