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

Memo type, existing EncryptedCiphertext types for Sprout and Sapling #249

Merged
merged 12 commits into from Feb 13, 2020

Conversation

@dconnolly
Copy link
Member

dconnolly commented Feb 12, 2020

This is a nice isolated chunk that can get merged in first.

@dconnolly dconnolly requested a review from hdevalence Feb 12, 2020
@dconnolly dconnolly added this to In progress in 🦓 via automation Feb 12, 2020
@dconnolly dconnolly added this to the Validate transactions. milestone Feb 12, 2020
dconnolly and others added 2 commits Feb 12, 2020
Co-Authored-By: Henry de Valence <hdevalence@hdevalence.ca>
…t note will be encrypted
@hdevalence

This comment has been minimized.

Copy link
Member

hdevalence commented Feb 13, 2020

This is great! A few comments:

  • I think it would be cleaner in the long run to move the Memo impl into note_encryption/memo.rs and pub use memo::Memo; inside of note_encryption.rs, so that we separate the module tree organization from the implementations, and potentially also to split out the tests into separate files.
  • 500-byte arrays are big enough that we might want to use Box<[u8; 512]>, so that moving ownership of Memo, EncCiphertext, etc doesn't result in a bunch of stack memcpys.
  • Using TryFrom rather than From seems like less of a sharp edge.

Non-blocking for this PR, looking at the part of the protocol spec you linked, there's a section about memo encodings at the top of page 73 that has four cases:

Screenshot from 2020-02-12 15-58-30

I wonder whether it makes sense to represent this in the Memo structure itself with something like

enum Memo {
    // encoded as 0xf6 0x00 ... 0x00
    NoMemo,
    // valid UTF-8, encoded with trailing 0x00 padding
    Utf8(String),
    // encoded as 0xf5 <data>
    Bytes(Box<[u8; 511]>),
    // everything else we can't parse + reserved
    Other(Box<[u8; 512]>),
}

and handle the parsing in the serialization implementations. This has the upside that users of the Memo field don't have to do any parsing to figure out what's in the memo, but has the downside that we don't have a way to bound the length of the string, and we need the string's length to be bounded so that ZcashSerialize is infallible. So I'm not sure if this quite works, and it definitely shouldn't block this PR, but it might be good to do something like that later?

@hdevalence

This comment has been minimized.

Copy link
Member

hdevalence commented Feb 13, 2020

Hmm, actually.... I wonder whether the ZcashSerialize/ZcashDeserialize impls for Memo will end up being used, since the plaintext isn't committed to the chain, so perhaps it would work to only have the ciphertexts be ZcashSerialize and for the Memo to not be. Then the encryption method or something would have to be fallible (e.g., failing on long messages), but the Memo itself could have a structured internal representation, or something?

@hdevalence

This comment has been minimized.

Copy link
Member

hdevalence commented Feb 13, 2020

@dconnolly dconnolly requested a review from hdevalence Feb 13, 2020
@dconnolly dconnolly moved this from In progress to Review in progress in 🦓 Feb 13, 2020
@dconnolly dconnolly merged commit 99e3b16 into main Feb 13, 2020
3 checks passed
3 checks passed
Google Cloud Build
Details
Code Coverage & Clippy Code Coverage & Clippy
Details
clippy clippy
Details
🦓 automation moved this from Review in progress to Done Feb 13, 2020
@dconnolly dconnolly deleted the note-encryption branch Feb 13, 2020
@dconnolly dconnolly restored the note-encryption branch Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
🦓
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.