Add Network Config Test to Check Against Upstream Values

[terencechain]

currently, we load beacon chain config from the spec repo to check they comply We don't load the network config for compliance check, and we should add that

[0xCMars]

Hi, I'd like to pick this up, and I think that prysm uses /tools/specs-checker and bazel command to run the check. And I'm a little confused about what network config we want to check. Which spec is the network config referred to? Should I modify and add the load & check func based on the specs-checker or should I add new tools?

[terencechain]

Hi @0xCMars, the network configs are separated in prysm config 1, but it's defined in the same place in the consensus spec yaml 2. We also skipped them in place holders 3 for load config test

[0xCMars]

Ok, I got it. And I found the TestLoadConfigFile check beacon config(which is defined in mainnet_config.go), and like u said we skipped the network config struct tag in the consensus spec by using a placeholder. And for now, I wonder if I should write a new t.Run in TestLoadConfigFile to check the network config or should I add this func in the exist t.Run? Which method is more in line with our code specifications？ For now, I will load the networkconfig from consensus and unmarshal it and write a func like assertEqualConfigs to compare the value.

[0xCMars]

Hi@terencechain . When I checked the network config, I found some inconsistencies between our network config and consensus spec. As shown below, MaxChunkSize and GossipMaxSize are defined in mainnetNetworkConfig, which values are respectively 1 << 20 and 1 << 20, but in network config, values are 10 1 << 20 and 10 1 << 20.

I looked into the code and think that this problem may caused by Ethereum upgrades and change specs, but we are still in the old version and use var Like GossipMaxSize and GossipMaxSizeBellatri to distinguish.

I don't know how to handle this situation. Hope u can give some advice.

[terencechain]

cc @nisdas for the networking spec quesitons

[0xCMars]

@terencechain Hi, Is there any new here? I have gone through part of the prysm code and want to participate in the development, but I found that issues are difficult to start, mainly because most of them are related to network synchronization. So could you give some suggestions and guidance? How to participate in the development of new features? (which I think it will help people better understand the code) Or whether there are part-time or intern like other communities. Thx for your time.

[nisdas]

@0xCMars The reason it is different is because the mainnet specs are not distinguishing between pre/post bellatrix gossip message and chunk sizes, while we are. This was removed in https://github.com/ethereum/consensus-specs/commit/5576d0e6850b0ddbffd1ec122841b35e31e66fee on the specification's end, so for our case we will also have to remove distingushing it and just use one variable name everywhere with the higher value.

[0xCMars]

Hi, While submitting my new PR, the DeepSource said

"func ReplaceHexStringWithYAMLFormat has a cyclomatic complexity of 28 with "very-high" risk"

the func is in here but I didn't modify any part of that func, Can anyone give me some advice to fix that problem?

[james-prysm]

@0xCMars it just means the switch statement is too long, it's probably ok until we try to break it out or something.

[0xCMars]

@james-prysm Yes, I have tried to fix it in my last PR commit. Thanks for your help, and I want to ask that my PR was waiting for the GitHub workflow to get some expected result, but it just stuck there, and no result was returned.