search

ethereum / beacon-APIs

Collection of RESTful APIs provided by Ethereum Beacon nodes
https://ethereum.github.io/beacon-APIs/
Creative Commons Zero v1.0 Universal
335 stars 169 forks source link

Improve the schema for skip_randao_verification flag type #444

Closed nazarhussain closed 4 months ago

nazarhussain commented 6 months ago

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.

https://github.com/ChainSafe/lodestar/issues/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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

image

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

nflaig commented 6 months ago

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 commented 6 months ago

...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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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: https://github.com/ethereum/beacon-APIs/pull/222#issuecomment-1188455636

nazarhussain commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

nazarhussain commented 6 months ago

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.