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

Encrypted Slatepack Metadata #428

Merged
merged 4 commits into from
Jun 10, 2020

Conversation

yeastplume
Copy link
Member

@yeastplume yeastplume commented Jun 8, 2020

Adds encrypted metadata to the slatepack definition, which works as follows:

  • Encrypted metadata isn't serialized separately, it's rather prefixed to the slate payload on encrypt, and split off on decrypt.
  • On encryption, the sender field is moved from the unencrypted header into the encrypted metadata field, and is moved back on decryption. This means that anyone using the struct only needs to worry about setting the sender field in one place.
  • Recipients can be added to the Slatepack, but if the payload is not encrypted they won't be included in serialization (as they can only exist in the payload of an encrypted slate)

Also includes

  • Slatepack function to add recipients
  • Tests ensuring that SlatepackEncMetadata struct will continue to parse correctly when additional fields are added (similar to how the optional header fields work now)

@yeastplume yeastplume changed the title [WIP] Encrypted Slatepack Metadata Encrypted Slatepack Metadata Jun 9, 2020
Copy link
Member

@quentinlesceller quentinlesceller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍 Small comments here and there.

libwallet/src/slatepack/types.rs Show resolved Hide resolved
libwallet/src/slatepack/address.rs Outdated Show resolved Hide resolved
libwallet/src/slatepack/types.rs Outdated Show resolved Hide resolved
libwallet/src/slatepack/types.rs Show resolved Hide resolved
libwallet/src/slatepack/types.rs Outdated Show resolved Hide resolved
libwallet/src/slatepack/types.rs Outdated Show resolved Hide resolved
libwallet/src/slatepack/types.rs Show resolved Hide resolved
libwallet/src/slatepack/types.rs Show resolved Hide resolved
libwallet/src/slatepack/types.rs Outdated Show resolved Hide resolved
libwallet/src/slatepack/types.rs Outdated Show resolved Hide resolved
Copy link
Member

@j01tz j01tz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to implement the encrypted metadata fields as discussed in the RFC. The specific details like the 4 byte length and additional optional content flags will require an update to the RFC to keep details accurate and in sync.

To echo Quentin's point about versioning, in the RFC I have the first release as 0.1 and was planning for 1.0 in the final HF once we have finalized all of the details and are happy with long term support for the standard. It doesn't make a big difference though and want to go with whatever is consistent with other work- I can adjust the RFC if we want the first release to be 1.0.

I think we will likely need some more refining and thought needed around recipients field and how it would actually be used but is not critical to have this completely ironed out before the first release.

Otherwise looks good- I haven't manually verified all of the bit plumbing going on, but will get to it as we get into testing. Nice work getting this in, looking forward to testing exchanging some encrypted slatepacks this week 👍

@yeastplume yeastplume merged commit fe28809 into mimblewimble:master Jun 10, 2020
@yeastplume yeastplume deleted the sp_enc_metadata branch July 13, 2020 10:20
antiochp pushed a commit to antiochp/grin-wallet that referenced this pull request Aug 7, 2020
* addition of slatepack metadata

* finish adding tests ensuring forwards compatibility

* fix to version check

* updates based on review feedback
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

Successfully merging this pull request may close these issues.

3 participants