Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GossipSub 1.1] Limit the size of PRUNE Peer Exchange list #576

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nashatyrev
Copy link
Contributor

At the moment the ControlPrune.peers list is not limited by any boundary.

This PR suggests to add the MaxExchangePeers parameter for that.
Instead of hardcoding a 'reasonable default' I'd suggest to parametrize it mostly to have the ability to turn off this feature (by setting this parameter to 0).

See also this issue: ethereum/consensus-specs#3500

@Nashatyrev Nashatyrev changed the title [Gossip 1.1] Limit the size of PRUNE Peer Exchange list [GossipSub 1.1] Limit the size of PRUNE Peer Exchange list Sep 14, 2023
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

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

Sounds good to me.

@diegomrsantos
Copy link

If to disable the feature we need to set the parameter to 0, making 16 the default value will make it enabled by default.

@diegomrsantos
Copy link

@Nashatyrev how about making 0 the default value and effectively disabling it in the implementations? Library users who wish to enable the feature should change it to a number greater than 0.

@Nashatyrev Nashatyrev marked this pull request as ready for review October 4, 2023 11:59
@Nashatyrev
Copy link
Contributor Author

@Nashatyrev how about making 0 the default value and effectively disabling it in the implementations? Library users who wish to enable the feature should change it to a number greater than 0.

I don't know actually. There was a reason why this feature was added I believe. And it seems a bit weird having a feature in the spec which is disabled by default.

@diegomrsantos
Copy link

It seems that was the intention as mentioned here #570 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

3 participants