Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom network configs break unless GOSSIP_MAX_SIZE and others are defined #7555

Closed
siladu opened this issue Sep 28, 2023 · 4 comments · Fixed by #7566
Closed

Custom network configs break unless GOSSIP_MAX_SIZE and others are defined #7555

siladu opened this issue Sep 28, 2023 · 4 comments · Fixed by #7566
Assignees
Labels
bug 🐞 Something isn't working

Comments

@siladu
Copy link
Contributor

siladu commented Sep 28, 2023

Following #7179 (spec change: ethereum/consensus-specs#3375) it is expected that networking config values are not inherited via the preset-base in the network config?

For example, when I use a custom network config, I am now required to define GOSSIP_MAX_SIZE despite PRESET_BASE: 'minimal', see siladu/beku-timestamp@de1f285

This also impacts web3signer due to the inherited dependency on the teku network config such that any custom networks in devnets or test infrastructure now requires the networking config.

@siladu siladu changed the title Custom network configs break unless GOSSIP_MAX_SIZE and others are definied Custom network configs break unless GOSSIP_MAX_SIZE and others are defined Sep 28, 2023
@rolfyone rolfyone self-assigned this Oct 2, 2023
@rolfyone
Copy link
Contributor

rolfyone commented Oct 2, 2023

I think this got broken by trying to cope with some remote values missing from constants that moved...

I'll work up a solution, the problem is specifically the ordering in remote config loader from #7510, it looks like we fixed it in the incorrect way...

@rolfyone rolfyone added the bug 🐞 Something isn't working label Oct 2, 2023
@rolfyone
Copy link
Contributor

rolfyone commented Oct 2, 2023

Actually, I may have slightly assessed this incorrectly... but I think the fix will be the same.

Basically in consensus-specs, a bunch of 'constants' got moved to 'config'. GOSSIP_MAX_SIZE is one, and it pretty much does mean that all custom configs need to now define those values, because they're no longer constants (3375 you mentioned above).

There's a 'similar' problem with remote config where we now load some defaults in a bad order, but that's different to the root cause of this issue, which is just not having those moved values set in config thats loaded from a local resource.

I think your initial assessment of it was correct in that its a flow on effect of the consensus-specs change, but this has also caused issues loading remote configs where 3375 hadn't been done on external BNs we're loading configuration from.

@rolfyone
Copy link
Contributor

rolfyone commented Oct 2, 2023

One potential issue of note is the MIN_EPOCHS_FOR_BLOCK_REQUESTS which defaults differently on minimal compared to mainnet, so if that value is of interest you should definitely set it to the value you require.

rolfyone added a commit to rolfyone/teku that referenced this issue Oct 2, 2023
The constants that got moved have caused issues in remote config, and now in local config.

This change defaults the values, which means if they're not supplied it'll silently succeed assuming the values from old constant values.

I've reverted the initial remote config change, as it didn't really address the local loading issue mentioned in Consensys#7555, and it was potentially also applying in the incorrect order.

`MIN_EPOCHS_FOR_BLOCK_REQUESTS` is different on mainnet and minimal, so that will be using the mainnet value by default.

fixes Consensys#7555

Signed-off-by: Paul Harris <[email protected]>
@tbenr
Copy link
Contributor

tbenr commented Oct 4, 2023

My position on this is that it isn't a Teku issue, it is a spec issue.

GOSSIP_MAX_SIZE is only defined in configs now, and is not present in any preset. This determine that this parameter (together with the other Networking params) is now mandatory to be set in every config. A config without it is effectively an invalid config.

Maybe the solution to this is to move this parameters on preset (but I don't have the reasoning why it was chosen to move them in config). Happy to continue the discussion on a spec issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants