Conversation
nisdas
left a comment
nisdas
left a comment
There was a problem hiding this comment.
Looks good but had a few questions
|
|
||
| // MaxGossipSize allowed for gossip messages. | ||
| var MaxGossipSize = params.BeaconNetworkConfig().GossipMaxSize // 1 Mib | ||
| var MaxGossipSize = params.BeaconNetworkConfig().GossipMaxSize // 1 Mib. |
There was a problem hiding this comment.
Can we use a proper setter method here to modify this ? Rather than setting a global var that we have. It is cleaner to understand that way and we do not expose a global variable from this package.
beacon-chain/p2p/pubsub_filter.go
Outdated
| return false | ||
| } | ||
| if parts[2] != fmt.Sprintf("%x", fd) && parts[2] != fmt.Sprintf("%x", digest) { | ||
| mergeForkDigest, err := forks.ForkDigestFromEpoch(params.BeaconConfig().BellatrixForkEpoch, s.genesisValidatorsRoot) |
There was a problem hiding this comment.
| mergeForkDigest, err := forks.ForkDigestFromEpoch(params.BeaconConfig().BellatrixForkEpoch, s.genesisValidatorsRoot) | |
| bellatrixForkDigest, err := forks.ForkDigestFromEpoch(params.BeaconConfig().BellatrixForkEpoch, s.genesisValidatorsRoot) |
| } | ||
|
|
||
| // from Bellatrix Epoch, the MaxGossipSize and the MaxChunkSize is changed to 10MB. | ||
| if currEpoch == params.BeaconConfig().BellatrixForkEpoch { |
There was a problem hiding this comment.
While it is fine to change this via the forkwatcher, how does this work when prysm starts up after the bellatrix fork epoch ? Prysm will revert to the old gossip max size and chunk size values and will be incompatible with the networking spec again.
|
|
||
| // MaxGossipSize allowed for gossip messages. | ||
| var MaxGossipSize = params.BeaconNetworkConfig().GossipMaxSize // 1 Mib | ||
| var MaxGossipSize = params.BeaconNetworkConfig().GossipMaxSize // 1 Mib. |
There was a problem hiding this comment.
Lets unexport these vars and keep their scope in this package.
There was a problem hiding this comment.
as discussed in discord, its hard to do this since we use the global var in test cases (with a different package)
beacon-chain/p2p/pubsub_filter.go
Outdated
| log.WithError(err).Error("Could not determine merge fork digest") | ||
| return false | ||
| } | ||
| if parts[2] != fmt.Sprintf("%x", phase0ForkDigest) && |
There was a problem hiding this comment.
maybe we can do a a switch and case here ?
switch parts[2] {
case fmt.Sprintf("%x", phase0ForkDigest) :
// do nothing
case fmt.Sprintf("%x", altairForkDigest) :
// do nothing
case fmt.Sprintf("%x", mergeForkDigest) :
// do nothing
default:
return false
}
3 participants
Bringing down Bellatrix p2p changes from
kintsugitodevelopbranch. Credit to original author: @jmozah