Moved configuration into network preset instead of constants. #3394
Conversation
Now that `MAX_CHUNK_SIZE` and `GOSSIP_MAX_SIZE` are in configuration, we no longer need separate constants to represent them in the spec when they change in Bellatrix. I've changed the usage, and put the values into the presets, but I'm not sure if I've updated the descriptions in the best way... This is following on from the work in ethereum#3375 where a number of constants got moved into configuration, so we no longer need these constants to be separately represented, they can simply be updated in presets.
There was a problem hiding this comment.
Thank you @rolfyone for catching it!
However, it seems odd to have them concurrently in both the preset and the config. An alternative solution could be to follow the pattern of other preset updates like INACTIVITY_PENALTY_QUOTIENT_BELLATRIX. We could keep the suffix and relocate them to either the preset or the config.
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
It looked like the items listed in configuration about the max size and chunk size were no longer needed since we're updating preset values now and the preset changes seem to only be listed in the changes at the top.
|
I agree, I hadn't really taken much time to look at the formats of the other files. Updated that, and it looks like we don't need to call out changes in config now that they're preset so removed the updated descriptions. |
|
It's not clear to me that you can get rid of the individual configuration values at the time of forks, otherwise you might downscore a node that has already upgraded pre-fork (or vice versa) |
I'm not sure what you mean by this specifically... We haven't got rid of the value, just that it's now represented in the presets... When making the changes in teku I realised that we have this bellatrix constant that should really just be part of the spec presets that we're reading, otherwise bellatrix will forever have to have special code on it's gossip size and chunk size... This should be a logical equivalence to the 2 constants, just that now the value being altered is represented in the presets... Because it's preset, and the description of the usage of those is identical to phase 0, it seems that there's no point repeating it unless we're calling out that it's now 10MiB, and we didnt seem to do that for other values that I'd looked at in the presets on the beacon-chain doc... I'm happy to put the comments back about the values increasing from 1MiB to 10Mib if that's clearer? |
|
Ah, you're right. I wasn't paying attention to the distinction that presets are per fork. sorry! |
|
But yeah, leaving a call-out about the increase and/or how/when to do the change would probably be good |
|
Do clients use 10 MiB as the maximum size for gossiping the phase0 objects, if any? Separately, my proposal of relocating them to preset would be like this: 822b744 Rationales:
|
|
Oh! Ok I've never done presets, maybe 3rd time lucky, I think I had the same problem in code that you're describing, where i can't just update the value, which I had assumed we could do :/ Thanks for your patience... I'll get to this this morning. |
…sets completely as suggested.
potentially yes we could just update that setting... We were just using 10MB as soon as Bellatrix was defined.... |
Nimbus also simply always uses 10MB message size limits now, so would prefer to update the phase0 setting too. |
…... I think this should be close but want to see what's outstanding.
|
simplified based on feedback. ready to look at again. |
In line with ethereum/consensus-specs#3394 moved the fields into mainnet.yaml and minimal.yaml and updated the phase0 values to be the old bellatrix values, since that's basically in line with implementation anyway (bellatrix enabled meant use bellatrix values) Signed-off-by: Paul Harris <paul.harris@consensys.net>
…um#3394) * Moved configuration into network preset instead of constants. Now that `MAX_CHUNK_SIZE` and `GOSSIP_MAX_SIZE` are in configuration, we no longer need separate constants to represent them in the spec when they change in Bellatrix. I've changed the usage, and put the values into the presets, but I'm not sure if I've updated the descriptions in the best way... This is following on from the work in ethereum#3375 where a number of constants got moved into configuration, so we no longer need these constants to be separately represented, they can simply be updated in presets. * Update presets/minimal/bellatrix.yaml Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com> * Update presets/mainnet/bellatrix.yaml Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com> * Moved preset items into the correct section and updated TOC. It looked like the items listed in configuration about the max size and chunk size were no longer needed since we're updating preset values now and the preset changes seem to only be listed in the changes at the top. * review feedback * hopefully correct this time! Moved the 2 fields from configs into presets completely as suggested. * WIP - changing back to being in config and updating the phase 0 value... I think this should be close but want to see what's outstanding. * fix intellij's formatting of table. * more fixes --------- Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
…um#3394) * Moved configuration into network preset instead of constants. Now that `MAX_CHUNK_SIZE` and `GOSSIP_MAX_SIZE` are in configuration, we no longer need separate constants to represent them in the spec when they change in Bellatrix. I've changed the usage, and put the values into the presets, but I'm not sure if I've updated the descriptions in the best way... This is following on from the work in ethereum#3375 where a number of constants got moved into configuration, so we no longer need these constants to be separately represented, they can simply be updated in presets. * Update presets/minimal/bellatrix.yaml Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com> * Update presets/mainnet/bellatrix.yaml Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com> * Moved preset items into the correct section and updated TOC. It looked like the items listed in configuration about the max size and chunk size were no longer needed since we're updating preset values now and the preset changes seem to only be listed in the changes at the top. * review feedback * hopefully correct this time! Moved the 2 fields from configs into presets completely as suggested. * WIP - changing back to being in config and updating the phase 0 value... I think this should be close but want to see what's outstanding. * fix intellij's formatting of table. * more fixes --------- Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
In line with ethereum/consensus-specs#3394 moved the fields into mainnet.yaml and minimal.yaml and updated the phase0 values to be the old bellatrix values, since that's basically in line with implementation anyway (bellatrix enabled meant use bellatrix values) Signed-off-by: Paul Harris <paul.harris@consensys.net>
Now that
MAX_CHUNK_SIZEandGOSSIP_MAX_SIZEare in configuration, we no longer need separate constants to represent them in the spec when they change in Bellatrix.I've changed the usage, and put the values into the presets, but I'm not sure if I've updated the descriptions in the best way...
This is following on from the work in #3375 where a number of constants got moved into configuration, so we no longer need these constants to be separately represented, they can simply be updated in presets.