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

zkvm: refactor encoding/decoding #424

Closed
oleganza opened this issue Apr 22, 2020 · 5 comments
Closed

zkvm: refactor encoding/decoding #424

oleganza opened this issue Apr 22, 2020 · 5 comments

Comments

@oleganza
Copy link
Contributor

oleganza commented Apr 22, 2020

We currently have multiple uses for encoding throughout zkvm, blockchain and p2p crates:

  1. Writing various objects (TxHeader, TxLog, Tx, BlockTx) into merlin::Transcript.
  2. Encoding and decoding contracts within VM from byte strings.
  3. Reading and writing BlockTx to the network sockets.

Downsides of the current approach:

  1. Writing to the Transcript is usually done with an intermediate encoding of the entire object into a buffer (see zkvm::Contract::id() and blockchain::BlockTx::witness_hash()).
  2. When we write to the wire, we want to use Tokio Codecs with their buffers (bytes::BytesMut/BufMut), but keep the rest of the system independent from Tokio or any I/O framework.

Requirements

We need a pair of traits Reader/Writer akin to Buf/BufMut, but with several modifications:

  1. Never panics.
  2. Reads and writes return Result, with associated Error type, defined by the impl.
  3. Read fn checks the remaining length.
  4. Write fn accepts a label of type &'static str that can be used with the Transcript (or JSON ;-)) Binary buffers will simply ignore the label.
  5. We only need u8/u16/u32/u64 (LE) and &[u8] types supported for now. (Keep _le explicit in the naming, though.)

Organization

  1. Separate readerwriter crate.
  2. Optional dependency on bytes (for interop with Bytes/Buf), with corresponding module bytes_impls.rs enabled on #[cfg(feature="bytes")].
  3. Optional dependency on merlin (for interop with Transcript), with corresponding module merlin_impls.rs enabled on #[cfg(feature="merlin")].
  4. Crate zkvm depends on readerwriter, SliceReader is removed.
  5. In crates zkvm and blockchain: encoding/decoding methods are changed to use Reader/Writer API. Hashing is redefined in the spec to write data to transcript field-by-field.
  6. Crate blockchain depends on readerwriter, but not on p2p.
  7. p2p uses same readerwriter API for its encoding needs.
@oleganza
Copy link
Contributor Author

Reader/Writer traits: #427, #429

@p0lunin
Copy link
Contributor

p0lunin commented Apr 29, 2020

  1. Crate blockchain depends on readerwriter, but not on p2p.

But CustomMessage trait placed in p2p, so p2p will be in blockchain dependencies. Moving this trait into another place seems illogical to me.

@oleganza
Copy link
Contributor Author

But CustomMessage trait placed in p2p, so p2p will be in blockchain dependencies. Moving this trait into another place seems illogical to me.

How about this:

  1. blockchain implements encoding via Reader/Writer traits.
  2. The node crate (e.g. demo) that provides concrete storage and I/O, implements p2p::CustomMessage for blockchain::protocol::Message (via a newtype wrapper) and wires all things together.

@p0lunin
Copy link
Contributor

p0lunin commented May 2, 2020

Final results:

  1. Add traits Encodable/Decodable in readerwriter crate. Implement trait Codable for T where T both implement Encodable + Decodable.
  2. Use in p2p trait Codable instead of CustomMessage.
  3. Implement Encodable/Decodable to blockchain::Message.

@oleganza
Copy link
Contributor Author

oleganza commented May 5, 2020

#430 seems to be the final nail in this coffin

@oleganza oleganza closed this as completed May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants