Change skip_randao_verification param to string

[gfukushima]

PR Description

This PR changes the skip_randao_verification to comply with the changes in ethereum/beacon-APIs#444 to a string flag like. The impact for us here is pretty much minimal since we ignore that param.

Fixed Issue(s)

Fixes #8223

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]

Since we are not using it, and we never added this param to the previous API versions, I'd be tempted to solve this by removing the definition completely.

We added in V3 by just following the new API spec without really thinking about previous version.

The only side-effect is us printing a swagger which is not 100% compliant but I don't think it's a big deal in this case.

WDYT?
cc @rolfyone

maybe we could add a note in the descriptions saying that Teku doesn't verify the field

[tbenr]

intelliJ suggesting to move to a dedicated method this snippet:

final String param = parameterMetadata.getName();
if (queryParams.containsKey(param)
|| queryParamsAllowEmpty.containsKey(param)
|| requiredQueryParams.containsKey(param)
|| queryListParams.containsKey(param)) {
throw new IllegalStateException("Query parameters already contains " + param);
}

makes sense to me

[rolfyone]

oh i thought this was used somewhere - technically a list could be expressed as
validator_id=[1,2,3,4] or validator_id=1&validator_id=2&validator_id=3&validator_id=4 if you really want to make your life difficult...
eg. https://medium.com/@AADota/spring-passing-list-and-array-of-values-as-url-parameters-1ed9bbdf0cb2

anyway - TL/DR, lets not do this.

[gfukushima]

tbh, I'm about not adding unnecessary code but since this is pretty much done and the changes required weren't too big I think there might be some value in keeping it in case any other param go through the same change.

[rolfyone]

We added in V3 by just following the new API spec without really thinking about previous version.

The only side-effect is us printing a swagger which is not 100% compliant but I don't think it's a big deal in this case.

really depends on the beacon-api spec.
If we define in beacon-api it's needed i'd be happy with listing it in the type but noting that we don't pay attention to the field...

[rolfyone]

there's a must here - do we care?
i'm wondering if we check that at all, im guessing not. i'd just truncate before 'if this flag is set'
Skip verification of the randao_reveal value. Ignored in the Teku implementation.

[gfukushima]

I think we don't care based on this comment: ethereum/beacon-APIs#222 (comment). Cehcked the other impl blockV2 and blinded_block didn't care about this param.
Changed the description as you suggested.

[rolfyone]

LGTM i think... im not sure where discussions landed but i think we could live with this in its current form.

[tbenr]

I don't oppose to merge it.

Just for the records: the reason why I'm not convinced is because in my opinion our swagger should reflect our functionalities. I think there is no point being compliant with swagger-doc and ignoring the underlying behaviour.
Advertising the param in swagger and saying "hey bare in mind we ignore it", is more confusing than not showing the param at all (which obviously means we don't support the param and we ignore it)

But this is just an opinion and it isn't super strong, so go for it!

[gfukushima]

Thanks everyone for the input!
To your point @tbenr, I'm slightly inclined towards having in swagger and making it clear to users that we ignore that parameter rather than not displaying it and having users confused and asking why we don't have that param specified in our API.