search

opendexnetwork / opendex.network

Website đź‘‹
https://opendex.network
GNU Affero General Public License v3.0
19 stars 10 forks source link

BOLD: Move remaining JSON to protobuf #34

Open kilrau opened 3 years ago

kilrau commented 3 years ago

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.

LePremierHomme commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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