Improve the schema for skip_randao_verification flag type

[nazarhussain]

There are few issues the way current spec defined the skip_randao_verification parameter. As of my understanding that's the only boolean type path parameter defined in the spec, but we should follow the approach in this fix for any future case as well.

Problem

The current spec says if you want to set this flag, don't set value. Which is not semantically possible as it's a query parameter, so eventually any value which is not set is considered as empty string from most HTTP request parser.

Flags implies it's a boolean value, so eventually the spec says if you want to set this flag to true set it to am empty string value. Which is logically considered false on different dynamic programming languages, or in dynamic execution of a static languages. e.g.

JS

"" ? true : false; // false 

Python

 True if "" else False # False

This causes compatibility issues among different clients.

ChainSafe/lodestar#6637
Was compatibility issue with Lighthouse as well.

Solution

The solution is to set a proper type and make sure the value for this parameter is sent properly. This will be a breaking change onward, but to keep old Validator Clients running, we should accept empty string as true boolean value.

This backward compatibility check should be removed in upcoming hard fork, where appropriate.

Verification

The behavior for the change in schema can be verified on the following link:
https://runkit.com/nazarhussain/661d0724d9a8fd0008526ae7

[mcdee]

Because this is a flag you wouldn't check the value, but the existence of the key. That should work fine in all languages, no?

If this change were made to the spec would it break existing interactions? I'm quite wary of making this sort of change.

[nazarhussain]

Because this is a flag you wouldn't check the value, but the existence of the key. That should work fine in all languages, no?

Checking values for all other parameters and checking only key for one causes enough of a confusion for implementers. So it's better to have explicit values rather implicit understanding of the implementer and platform.

[mcdee]

The spec seems clear enough as it is. Different doesn't have to mean worse, and bools are a bit of a law unto themselves (what if the value is 1? What if it is True?.

I that that we need to see the results of skip_randao_verification=false against all beacon nodes to confirm that this wouldn't be a breaking change.

[nazarhussain]

The spec seems clear enough as it is. Different doesn't have to mean worse

If it was clear enough we would not have issue with 3 client implementations. So probably something needs to fixed to resolve any future understanding issues for any flag/boolean type query parameters.

And general rule of clarity is to have explicit values rather implicit understanding. Having boolean type is better than someone understanding empty sting is a true value.

Mentioned in the description as well, this will be a breaking change but the proposed changes will allow old validator clients to keep working with new beacon node implementations.

[nflaig]

The spec seems clear enough as it is. Different doesn't have to mean worse, and bools are a bit of a law unto themselves (what if the value is 1? What if it is True?.

The problem with this query param is that it does not have a schema which is not ideal.

Openapi is pretty specific on how booleans should look like

That's the whole point, having a standard over the wire which is not specific to any platform / language

[nflaig]

I that that we need to see the results of skip_randao_verification=false against all beacon nodes to confirm that this wouldn't be a breaking change.

agreed, this seems problematic. Let's not introduce schemaless values in the future though, block v4 will likely be needed at some point, and can be fixed there

[mcdee]

...the proposed changes will allow old validator clients to keep working with new beacon node implementations.

But will they allow new validators clients to work with old beacon node implementations?

I get that we want to standardize where possible, but not at the risk of breaking existing systems.

[michaelsproul]

But will they allow new validators clients to work with old beacon node implementations?

No, it won't. I think @nflaig's idea of specifying the value to be "" in the OpenAPI schema is better than specifying it to be bool or "". Maybe we should have used bool from the start, but IMO that ship has sailed so we're better off leaving the parameter as it's defined in the textual part of the spec.

If we go the empty string route, Lodestar implementing the flag as a bool was just a bug – one that has already been fixed. Hence, no more changes required to clients, and a clearer spec. Win, win.

[michaelsproul]

I that that we need to see the results of skip_randao_verification=false against all beacon nodes to confirm that this wouldn't be a breaking change.

This doesn't work in Lighthouse. Nor does true. We check that the value is "".

[michaelsproul]

Looking back at the old PR, there are also reasons not to permit skip_randao_verification=false. Some clients do not check the randao at all and completely ignore the skip_randao_verification parameter: #222 (comment)

[nazarhussain]

Maybe remove the boolean type and only specify type: string with the min/max length

@michaelsproul @nflaig

1. Changing to type: string mentioned in comment as above will fix the semantic of schema without any change. And obviously that will not be a breaking change.

2. My PR was intending to properly use boolean type for such flags and that will be a breaking change. I feel this would be right approach.

So what do you suggest from above two options?

[michaelsproul]

I have a strong preference for (1) because the clean-up for (2) is IMO not worth coordinating a breaking change around. If we were to make a new version of the endpoint I would be fine with whatever.

[michaelsproul]

Did you see my comment about skip_randao_verification=false being incompatible with Teku (and Prysm)?

[nazarhussain]

Did you see my comment about skip_randao_verification=false being incompatible with Teku (and Prysm)?

Yes I checked the comment. With the recent commit, there is no change that can impact Teku or other client. It's just setting explicitly empty string schema rather mentioning in description.

[nflaig]

This is really only relevant for block v3 as other two APIs are deprecated already, and will be removed in next hard fork.

Although we can still update the description for all APIs I'd prefer not to have this separate from the API description, it's less friendly (and visible) for people reading the yaml files directly, and not the rendered explorer.

[nazarhussain]

It's feel better to define all params here, there is no rule how much repetition to remove. While reading through the YAML files, if one is using some good IDE, it's usually one click effort to navigate.

As when I started looking into params, I opened this file as first item, but found some params are not defined here. So better to make it consistent.

[michaelsproul]

LGTM!

[rolfyone]

LGTM, i'll raise a ticket on our end anyway just to make sure we conform with what's here, and maybe add tests.

[nflaig]

I am fine with this PR as well, although moving skip_randao_verification into params/index.yaml seems like a really arbitrary refactoring.

What about the other parameters in this API, like slot, randao_reveal, graffiti? I feel like it would be better to consistenly either put them in params file or have them repeated on the API definition.

Since @nazarhussain feels strongly about moving this in params, maybe can do a follow up PR for other values as well?

[nflaig]

Are we fine with removing the explicit note?

This query parameter is a flag and does not take a value.

The schema now enforces flag-like behavior as it won't allow you to set a value anymore but since openapi does not natively support a flag schema (OAI/OpenAPI-Specification#1782), being explicit aboout might be good.