prysmaticlabs / prysm

Go implementation of Ethereum proof of stake
https://www.offchainlabs.com
GNU General Public License v3.0
3.47k stars 1.01k forks source link

Fix Mesh Size in libp2p-pubsub #14516

Closed zhaochonghe closed 1 month ago

zhaochonghe commented 1 month ago

Describe the bug

https://github.com/prysmaticlabs/prysm/blob/cfbfccb203eba6c41d21276dc88ef348dcb993ab/beacon-chain/p2p/pubsub.go#L180-L189 This is the function that sets the mesh size in the underlying LibP2P network. However, the setting for parameter “gParams.Dhi” is missing in this function, and the following line should be added:

"gParams.Dhi=gossipSubDhi"

The reason and the bug are as follows:

"gParams.Dlo", "gParams.D", and "gParams.Dhi" are used to maintain the mesh size for libP2P network: when the mesh size is smaller than "gParams.Dlo" or larger than "gParams.Dhi", the mesh size will be adjusted to "gParams.D". In current libP2P, the default values for these three parameters are 5, 6, and 12, respectively (details can be found in the following link).

https://github.com/libp2p/go-libp2p-pubsub/blob/f71345c1ec0ee4b30cd702bb605927f788ff9f36/gossipsub.go#L45-L47

In the current version of prysm, if we want to enlarge the mesh size so that our nodes can collect data in time, and then try to modify "gossipSubD=16", "gossipSubDlo = 14" and "gossipSubDhi = 20" by modifying the following codes. https://github.com/prysmaticlabs/prysm/blob/cfbfccb203eba6c41d21276dc88ef348dcb993ab/beacon-chain/p2p/pubsub.go#L22-L24

Through the above function of "pubsubGossipParam()", the parameter "gParams.Dlo" in libP2P will be modified to "14", and the parameter in lib P2P "gParams.D" will be modified to "16". But due to the miss of "gParams.Dhi=gossipSubDhi" in the function of
"pubsubGossipParam()", the parameter "gParams.Dhi" is still 12. Because "gParams.Dlo=14" and "gParams.D=16" are larger than "gParams.Dhi", and the following problem will occur: mesh

Fix the bug:

Based on the above discussion, I think "gParams.Dhi=gossipSubDhi" should be added in the function of "pubsubGossipParam()".

Has this worked before in a previous version?

No response

🔬 Minimal Reproduction

No response

Error

No response

Platform(s)

No response

What version of Prysm are you running? (Which release)

No response

Anything else relevant (validator index / public key)?

No response

james-prysm commented 1 month ago

fixed with #14521