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

Sprout keys and addresses #317

Merged
merged 39 commits into from
Mar 28, 2020
Merged

Sprout keys and addresses #317

merged 39 commits into from
Mar 28, 2020

Conversation

dconnolly
Copy link
Contributor

@dconnolly dconnolly commented Mar 24, 2020

Supports deriving the key structure.

Currently a draft until the PR to the hashes repo is merged in. 🤞 Done!

@dconnolly dconnolly marked this pull request as ready for review March 25, 2020 00:01
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

Untested ACK, modulo comments.

Copy link
Contributor

@hdevalence hdevalence left a comment

Choose a reason for hiding this comment

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

This looks great. I think it would be good to have the address types implement matching FromStr and Display so that we can stick addresses as strings and then .parse() them.

One potential source of test vectors is the Founders' Reward addresses, which we'll have to encode into the source code anyways.

For the Sprout key derivation code, it would be nice to have some test vectors -- not sure if these could be extracted from librustzcash (i.e., if librustzcash has any sprout support). Otherwise stuff like the IncomingViewingKey we could just drop and leave for later (only focus on parsing and validating Sprout transactions).

let mut state = [0u32; 8];
let mut block = [0u8; 64]; // Thus, t = 0

block[0..32].copy_from_slice(&spending_key.0[..]);
Copy link
Contributor

@daira daira Mar 27, 2020

Choose a reason for hiding this comment

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

The type of SpendingKey doesn't enforce (or document) that the 4 high-order bits of the first byte must be zeros. Therefore, if a SpendingKey is constructed incorrectly, this could end up copying 1 into the bits of weight 2^4 or 2^5 of block[0], which would violate domain separation.

Copy link
Contributor

@daira daira Mar 27, 2020

Choose a reason for hiding this comment

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

Also document that this and the t=1 case below are implementing PRF^addr (or factor it into a separate function named to make that obvious).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed some updates to how we can construct these that should be better?

Copy link
Contributor

@daira daira Mar 27, 2020

Choose a reason for hiding this comment

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

This looks great, thanks. Also I suggest adding a comment here or an assertion that the top 4 bits are zero.

let mut bytes = [0u8; 32];
OsRng.fill_bytes(&mut bytes);

let spending_key = SpendingKey(bytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

This test passes even when the high 4 bits of bytes[0] are nonzero, which it should not.

Currently fills/receives 32 random bytes and forces the top 4 bits to
zero, ala clamping. If there is a nicer way to csprng'ly fill 252 bits
without clamping, that would be nicer, less bias.
@dconnolly dconnolly merged commit 2a7838d into main Mar 28, 2020
@dconnolly dconnolly deleted the keys branch March 28, 2020 06:42
skyl added a commit to skyl/zebra that referenced this pull request Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants