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

BOLD: Move remaining JSON to protobuf #34

Open
kilrau opened this issue Nov 3, 2020 · 3 comments
Open

BOLD: Move remaining JSON to protobuf #34

kilrau opened this issue Nov 3, 2020 · 3 comments
Assignees
Labels
enhancement New feature or request hardening All things to reduce edge cases and make the protocol less error prone P2 mid priority
Milestone

Comments

@kilrau
Copy link
Contributor

kilrau commented Nov 3, 2020

The OpenDEX protocol still has JSON in various places, which is bad because JSON can't be guaranteed to always be UTF-8 conform and requires implementations to use e.g. https://www.npmjs.com/package/json-stable-stringify in exactly the same way.

This issue is about moving all remaining JSON parts to protobuf to mitigate this issue. We should also evaluate alternatives like XDR.

@kilrau kilrau added enhancement New feature or request hardening All things to reduce edge cases and make the protocol less error prone labels Nov 3, 2020
@kilrau kilrau added this to the 1.1.0 milestone Nov 3, 2020
@kilrau kilrau changed the title Move remaining JSON to protobuf Move remaining JSON to protobuf (or something else entirely?) Nov 4, 2020
@LePremierHomme
Copy link

LePremierHomme commented Jan 6, 2021

Protobuf serialization scheme isn’t deterministic. Many binary representations are possible for a given object, because fields can be encoded in any order. This malleability is acceptable for most use-cases, but not for signing and hashing in cryptographic attestations and consensus code.

OpenDEX’s serialization use-cases are the following:

  1. All p2p messages are serialized to be place on the wire as the payload. Current serialization is using protobuf. I don’t see a problem with keeping this.
  2. All p2p messages are serialized to obtain sha256 checksum, to be added to the wire message header. Current serialization is using JSON. I don’t see a problem with changing this to protobuf.
  3. The SessionInit message fields are serialized to get sha256-hashed before getting secp256k1-signed for the handshake authentication. Current serialization is using JSON. I don’t see a problem with changing this to protobuf.

In case OpenDEX will be evolved to support gossip of orders, the order message will need to be signed, and the order id will likely to be a hash. Using protobuf here means that a malicious node which forward messages will be able to change the order id (without breaking the signature), which will lead, if instantiated, to a swap failure (because the message originator, the maker, won’t find that order id).
In addition, all wire messages id is currently a plain UUID, because it is only required to be unique per socket. If gossip is applied, this will likely to be a hash, and so malicious nodes could again cause some troubles.

So in the overall, it seems to me that protobuf is suitable for OpenDEX, except in a gossip scenario. Since it’s not clear whether it will be supported, I would recommend to switch the 2 & 3 JSON use-cases to protobuf, at least for now.

@kilrau
Copy link
Contributor Author

kilrau commented Jan 8, 2021

Thank you for the neat and understandable summary of the status quo @LePremierHomme . Until a real gossip protocol is on the table, I would say let's do what you suggested and move everything to protobuf.

@BitcoinOG
Copy link
Contributor

PR is in testing 👍🏾 Are you also taking care of the changes in the BOLD docs? @LePremierHomme

@BitcoinOG BitcoinOG added P2 mid priority and removed P1 top priority labels Mar 10, 2021
@BitcoinOG BitcoinOG changed the title Move remaining JSON to protobuf (or something else entirely?) BOLD: Move remaining JSON to protobuf Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hardening All things to reduce edge cases and make the protocol less error prone P2 mid priority
Projects
None yet
Development

No branches or pull requests

3 participants