Updated handling of config values reading bad configuration

[rolfyone]

The constants that got moved have caused issues in remote config, and now in local config.

RemoteConfig was loading the local config to work around the issue but it was in the wrong order, presets load first, then config.

LocalConfig could potentially do this with a CLI flag but shouldn't be default behaviour IMO, so i've left local config failing.

The error handling has been updated to report all fields not just the first field that fails validation. The error will look something like:

java.lang.IllegalArgumentException: The specified network configuration had missing or invalid values for constants GOSSIP_MAX_SIZE, ATTESTATION_SUBNET_PREFIX_BITS, RESP_TIMEOUT, ATTESTATION_SUBNET_EXTRA_BITS, MAXIMUM_GOSSIP_CLOCK_DISPARITY, MAX_CHUNK_SIZE, ATTESTATION_PROPAGATION_SLOT_RANGE, MESSAGE_DOMAIN_INVALID_SNAPPY, TTFB_TIMEOUT, EPOCHS_PER_SUBNET_SUBSCRIPTION, MESSAGE_DOMAIN_VALID_SNAPPY, ATTESTATION_SUBNET_COUNT, MIN_EPOCHS_FOR_BLOCK_REQUESTS, MAX_REQUEST_BLOCKS

fixes #7555

Documentation

• I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

• I thought about adding a changelog entry, and added one if I deemed necessary.

[tbenr]

Why attempting to load config doesn't solve the issue? Is because web3signer doesn't use SpecConfigLoader and only uses the Builder? I really would like to not hardcode values this way TBH...

[zilm13]

Maybe implement it in similar way to this? https://github.com/ConsenSys/teku/blob/6d4572ab8d6554f95035468aee97a4b8a221bb40/teku/src/main/java/tech/pegasys/teku/cli/subcommand/ValidatorClientCommand.java#L59
At least it will be in one place with proper comment (I assume we don't need it for subscommands like I spread noop kzg)

[rolfyone]

Why attempting to load config doesn't solve the issue? Is because web3signer doesn't use SpecConfigLoader and only uses the Builder? I really would like to not hardcode values this way TBH...

I'm not 100% sure on web3signer, but the old configuration being loaded didn't have the required values.

Basically the constants we moved had well defined values, so this is a potential solution. by initialising to the values from constants, if an implementation doesn't provide the (now configured) value, it can fall back to the constant, and by 'hard coding' here we can minimise breakages on custom config and interop between clients that are yet to implement these config elements.

Maybe implement it in similar way to this? https://github.com/ConsenSys/teku/blob/6d4572ab8d6554f95035468aee97a4b8a221bb40/teku/src/main/java/tech/pegasys/teku/cli/subcommand/ValidatorClientCommand.java#L59
At least it will be in one place with proper comment (I assume we don't need it for subscommands like I spread noop kzg)

I think the future answer is if we move a constant, it should also be listed in a preset, that'd be the ideal. The main thing i need is to be able to have any actual configured values override the defaults that get set so that networks work correctly.

One option would be to just add a note saying 'all these values must be set in configuration', but that doesnt help remote VC interactions with other clients (which the initial remote vc ticket was about)

[tbenr]

As per my comment here I think we should not do this. We could eventually consider the initial approach which was trying to load CONFIG_NAME config locally first, before applying the preset and load the additional config params. But I'm not for defaulting arbitrary values, since I think the root cause is elsewhere.
If we consider the spec repo as the source of truth, any config not respecting the spec should be treated as invalid.
We may try to make life easier for BN-VC upgrades, but we should not try to load configs that are formally invalid.

[zilm13]

I'm on one side with Enrico, and we should follow spec. But if we decide to add some handy UX for non-spec configs, we may introduce dedicated flag, something like --network=autoMergeMainnet, which will apply mainnet config and merge remote a top of it. So it will have zero chance to affect those who have not provided this flag.
Hardcoding some values looks like a risk, plus spec config will change again and again in the future.

[rolfyone]

As per my comment here I think we should not do this. We could eventually consider the initial approach which was trying to load CONFIG_NAME config locally first, before applying the preset and load the additional config params. But I'm not for defaulting arbitrary values, since I think the root cause is elsewhere.

The thing about that is it does solve 'forever', but it's a bit of a mess. if i go that way i'm going to need to make a lot larger change so that people whats magically populated for them i think, and i was hoping to avoid that, but i guess i'll hack something together.

This solution uses the actual constant values that was moved, so its far more targeted, and IMO more appropriate to the problem, but I can spend a day on it and see if there's a not awful, more generic, solution we can use.

[tbenr]

But what if we simply do nothing?

[rolfyone]

But what if we simply do nothing?

At a bare minimum we can't just tell people the first flag and expect them to sit through 20 failures, thats horrible.
That may be the better solution, fix the validate to error on everything thats missing, or maybe a cli arg to overlay our version of config_name in that instance

[tbenr]

But what if we simply do nothing?

At a bare minimum we can't just tell people the first flag and expect them to sit through 20 failures, thats horrible.

That may be the better solution, fix the validate to error on everything thats missing, or maybe a cli arg to overlay our version of config_name in that instance

Yes that would be super nice.

[rolfyone]

this should be in a state to merge @tbenr just need approval...