Skip to content

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

Open
Nashatyrev wants to merge 1 commit intolibp2p:masterfrom
Nashatyrev:gossip-prune-px-limit
Open

[GossipSub 1.1] Limit the size of PRUNE Peer Exchange list#576
Nashatyrev wants to merge 1 commit intolibp2p:masterfrom
Nashatyrev:gossip-prune-px-limit

Conversation

@Nashatyrev
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown

@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
Copy Markdown
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
Copy Markdown

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