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

Network message SSZ schema #871

Closed
jannikluhn opened this issue Apr 1, 2019 · 9 comments
Closed

Network message SSZ schema #871

jannikluhn opened this issue Apr 1, 2019 · 9 comments

Comments

@jannikluhn
Copy link
Contributor

(Continuing discussion from the PR)

Two nitpicks regarding the network message serialization format:

  1. If we use SSZ, we need a single base schema that embeds the payload message for network messages as otherwise receivers don't know how to decode it. However right now, the spec defines three message types:
Request: (
    id: uint64
    method_id: uint16
    body: Request
)

Reponse: (
    id: uint64
    response_code: uint16
    result: bytes
)

ErrorResponse: (
    id: uint64
    response_code: uint16
    result: bytes
)

Response and ErrorResponse are identical, but Request differs a little. It would also be nice to give them the same name and field names, because that's what implementations will use when they first decode a message (e.g. Message with message_id, request_id, and body). body would be a bytes that carries the SSZ serialized request/response/error. This adds an additional 4-byte prefix, but I think that's unavoidable.

  1. The spec says
The result member is OPTIONAL on errors, and MAY contain additional information about the error.

There aren't optional fields in SSZ right now, so it should just be a blank string if there's no information.

@mslipper

@mslipper
Copy link
Contributor

mslipper commented Apr 8, 2019

Thanks @jannikluhn! These are points well taken. During the networking workshop tomorrow I intend on working through these with the implementation teams; it's becoming clear that SSZ isn't a good choice for network data.

@jannikluhn
Copy link
Contributor Author

Personally I totally agree with this, but many people seem to like using the same format for everything. If we go with something else, I'd suggest RLP: It works quite well for Eth1, there are tons of implementations, it doesn't require a schema (at least not for the first decoding step), and messages can be extended.

Sadly, I'm not going to be able to participate in the workshop, but I'm looking forward to hear about the results.

@dankrad
Copy link
Contributor

dankrad commented Apr 10, 2019

So I just overheard the discussion from the workshop yesterday, and if the most important thing that is missing from SSZ right now is an Option type and a Null object, I think it may be possible to add this with 10ish lines of code. I think having the same serialization format in the core and networking would be nice. However I am wondering what the future implications with regards to upgrade (#861) are?

@arnetheduck
Copy link
Contributor

arnetheduck commented Apr 10, 2019

We have an option type (if you view option as a special-cased list of 0 or 1 elements) - the above messages could be defined as:

Request: (
    id: uint64
    method_id: uint16
    body: Request
)

Reponse: (
    id: uint64
    responseData: [Payload]
    error: [ErrorResponse]
)

@arnetheduck
Copy link
Contributor

it doesn't require a schema

this is a bit of a stretch - to do something meaningful with the data you need to interpret the values even in RLP - that's effectively what a schema does. it's just a matter of how many steps there are between raw bytes and useful data.

@dankrad
Copy link
Contributor

dankrad commented Apr 10, 2019

@

We have an option type (if you view option as a special-cased list of 0 or 1 elements) - the above messages could be defined as:

I see, this seems like a very hacky way of achieving this. Have a look at #893 for my suggestion?

@arnetheduck
Copy link
Contributor

my initial reaction would be that it doesn't carry its weight, considering the list trick. you can always decorate your code to treat the list as an option, but adding a special byte encoding and hashing instruction for it seems unnecessary.

@mslipper
Copy link
Contributor

Opened PR #974 with my suggested solution.

@hwwhww
Copy link
Contributor

hwwhww commented Aug 9, 2019

closing in favor of #1328

@hwwhww hwwhww closed this as completed Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants