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

Use SSZ union-types in RPC request/response #974

Merged
merged 2 commits into from
May 17, 2019

Conversation

mslipper
Copy link
Contributor

@mslipper mslipper commented Apr 21, 2019

This PR addresses #871 by using SSZ union types in request body

@JustinDrake
Copy link
Collaborator

JustinDrake commented Apr 21, 2019

Since the body and result fields depend on the value of method_id and response_code, a schema for the above structure cannot be known beforehand.

Do SSZ unions (over the possible method_id and response_code) allow for this use case? It would be nice to use SSZ everywhere.

+ id (uint64) +
| |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
| response_code (uint16) | result_len (uint32) |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we keep these aligned a bit? ie life gets easier when types are aligned to their size, which in this case can be had by changing response_code to uint32.. alternatively, stick another uint16 in there for future expansion set to 0 for now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing.

@djrtwo
Copy link
Contributor

djrtwo commented Apr 22, 2019

If we use specific stream for each RPC, we avoid having to do this correct?
Is there any downside to this approach?

@arnetheduck
Copy link
Contributor

arnetheduck commented Apr 22, 2019

SSZ unions can be done with:

{
  some_a: [AType]
  some_b: [BType]
}

if you know all the type possibilities up-front and don't mind that lists in theory can have more than one item. in practice, this tends not to be a big deal (see for example the protobuf encoding where the last value with matching key is used for single-item (optional) fields, and lists simply collect them instead - the only difference is a cosmetic one, in the schema.

in terms of forward/backward compatibility, it's a "heavy" solution - every change, even purely additive, needs a new "protocol" or "request".

thus, at minimum, we need a new stream every time we add some field or request, even if technically we can have multiple message types in a single stream.

@mslipper
Copy link
Contributor Author

mslipper commented Apr 22, 2019

@djrtwo using separate streams allows us to avoid some of this. You still won't be able to parse responses from multiple RPC requests at once without some identifier linking a response to a request. It does become much simpler, however - we'd only need to include an ID.

@jannikluhn
Copy link
Contributor

and simple byte sequences for everything else

We could also call everything else message header and encode it in SSZ. This wouldn't change anything on the wire (as long as we only use fixed size values in the header), but maybe it's a little bit simpler conceptually.

@djrtwo
Copy link
Contributor

djrtwo commented May 1, 2019

Would a union type (#893) preclude us from having to define this external, non-ssz wrapper?

@djrtwo
Copy link
Contributor

djrtwo commented May 6, 2019

@mslipper See above question

@mslipper
Copy link
Contributor Author

mslipper commented May 6, 2019

My bad, missed that notification - a union type technically solves this, but introduces other problems. The union type would need to be over a list of all possible RPC request/responses, which IMO rapidly reduces maintainability.

Beyond the desire for consistency, is there another reason for why we're leaning towards SSZ everywhere?

@JustinDrake
Copy link
Collaborator

The union type would need to be over a list of all possible RPC request/responses

The list of all possible RPC request/responses should be readily available, right? For example, every request type needs to have a corresponding request handler. As such, maintenance of the union can be abstracted away programmatically.

@mslipper
Copy link
Contributor Author

mslipper commented May 6, 2019

@JustinDrake doesn't this seem needlessly complex to you? We're talking about creating tooling to work around SSZ's limitations in a specification that hasn't been implemented yet. Is there a specific reason for requiring SSZ everywhere? It seems to me that decoding a sequence of bytes on the wire then decoding the SSZ payload is far simpler than maintaining a union type programmatically.

@JustinDrake
Copy link
Collaborator

JustinDrake commented May 6, 2019

We're talking about creating tooling to work around SSZ's limitations

I'm suggesting reusing the existing query/response infrastructure that needs to exist anyway. (In other words, no extra tooling needs to be created.)

Is there a specific reason for requiring SSZ everywhere?

The reason is the long tail of benefits we get from standardisation :)

As a side note, judging from internal conversations, I think those who expressed an opinion (Danny, Vitalik, myself) are inclined to stick with SSZ. Having said that, I don't particularly care too much either way 😂

@dankrad
Copy link
Contributor

dankrad commented May 7, 2019

doesn't this seem needlessly complex to you? We're talking about creating tooling to work around SSZ's limitations in a specification that hasn't been implemented yet.

Could you describe the tooling that would be needed? I also still don't get what the problem is here, as long as there is a finite enum of possible request types that's going to be used, it should be fine? It is even possible to maintain forwards compatibility by just adding new types to the union and make old clients ignore requests with unkown type numbers.

@djrtwo djrtwo changed the title Don't use SSZ in RPC request/response wrappers Use SSZ union-types in RPC request/response May 17, 2019
@djrtwo djrtwo merged commit e0b4dd1 into ethereum:dev May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants