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

ZIP 32 APIs #29

Merged
merged 2 commits into from Aug 31, 2018
Merged

ZIP 32 APIs #29

merged 2 commits into from Aug 31, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Aug 3, 2018

No description provided.

@str4d str4d force-pushed the str4d:zip32 branch from bc58122 to a58f9f7 Aug 20, 2018
@str4d str4d force-pushed the str4d:zip32 branch from a58f9f7 to 006f6d2 Aug 27, 2018
str4d pushed a commit to str4d/librustzcash that referenced this pull request Aug 28, 2018
63 chunks per pedersen hash segment
@daira
daira approved these changes Aug 29, 2018
Copy link
Contributor

daira left a comment

utACK; however, this PR has no tests.

source = "registry+https://github.com/rust-lang/crates.io-index"
dependencies = [
"aes-soft 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)",
"aesni 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)",

This comment has been minimized.

Copy link
@daira

daira Aug 29, 2018

Contributor

I haven't reviewed these; I'm just assuming they correctly implement AES.

@@ -270,6 +270,35 @@ extern "C" {
uint64_t vpub_old,
uint64_t vpub_new
);

/// Derive the master ExtendedSpendingKey from a seed.
void librustzcash_zip32_xsk_master(

This comment has been minimized.

Copy link
@daira

daira Aug 29, 2018

Contributor

Use extsk, extfvk, etc.

This comment has been minimized.

Copy link
@str4d

str4d Aug 29, 2018

Author Contributor

Huh? I thought we decided on xsk and xfvk for the ZIP 32 constructs (as used in ZIP 32 itself), and expsk / fvk for the corresponding Sapling key structs?

This comment has been minimized.

Copy link
@daira

daira Oct 11, 2018

Contributor

ZIP 32 uses Ext*.

pub extern "system" fn librustzcash_zip32_xsk_master(
seed: *const c_uchar,
seedlen: size_t,
xsk_m: *mut [c_uchar; 169],

This comment has been minimized.

Copy link
@daira

daira Aug 29, 2018

Contributor

s/_m/_master/


#[no_mangle]
pub extern "system" fn librustzcash_zip32_xsk_derive(
xsk_p: *const [c_uchar; 169],

This comment has been minimized.

Copy link
@daira

daira Aug 29, 2018

Contributor

s/_p/_parent/


let xfvk = match xfvk_p.derive_child(i) {
Ok(xfvk) => xfvk,
Err(_) => return false,

This comment has been minimized.

Copy link
@daira

daira Aug 29, 2018

Contributor

This doesn't (say) zero the output array in case of error. libsodium does that and I think it's a good idea. Does not block.

This comment has been minimized.

Copy link
@str4d

str4d Aug 29, 2018

Author Contributor

If we want to do that, we should do so consistently in all of the FFI APIs. Opened #34.

@str4d str4d force-pushed the str4d:zip32 branch from 006f6d2 to e045230 Aug 29, 2018
@str4d

This comment has been minimized.

Copy link
Contributor Author

str4d commented Aug 29, 2018

Rebased on master to fix merge conflicts, and address @daira's comments.

@str4d str4d force-pushed the str4d:zip32 branch from e045230 to 77ee1d6 Aug 29, 2018
@ebfull ebfull changed the base branch from master to zcash-2.0.1 Aug 30, 2018
.expect("valid ExtendedFullViewingKey");
let i = zip32::ChildIndex::from_index(i);

let xfvk = match xfvk_parent.derive_child(i) {

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Aug 30, 2018

Add somewhere descriptive error message for hardened child index?
The one in derive_child just returns an error without a message.

This comment has been minimized.

Copy link
@str4d

str4d Aug 30, 2018

Author Contributor

That should happen in the librustzcash repo now, where we should have a better overall error story instead of arbitrary strings. As for here, we don't get to easily send error states across the FFI :(

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Aug 30, 2018

Where should that happen exactly? Isn't this librustzcash?

This comment has been minimized.

Copy link
@str4d

str4d Aug 30, 2018

Author Contributor

Sorry, I meant in the refactor work happening in the master branch. This PR is merging into a 2.0.1-specific branch, which will later be merged into master.

unsigned char *xfvk_i
);

/// Derive a PaymentAddress from an ExtendedFullViewingKey.

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Aug 30, 2018

I'm wondering how this will be designed to work without repeating addresses. E.g. if j_ret=j+1 we should store j_ret somewhere to know we should start from j+2 for the next address.

This comment has been minimized.

Copy link
@str4d

str4d Aug 30, 2018

Author Contributor

Correct; you are given back the "index" of the address being returned, and should store it so that you know to try index + 1 the next time you want an address.

This comment has been minimized.

Copy link
@arielgabizon

arielgabizon Aug 30, 2018

Wondering if the following interface is better: You give as input the index j of a valid diversifier - the method return an error if j is not a valid index; and if it is, returns the next valid diversifier

This comment has been minimized.

Copy link
@str4d

str4d Aug 30, 2018

Author Contributor

It might be; we can explore that in a follow-up PR when we have more users of the API.

@str4d str4d merged commit f5e5cb2 into zcash:zcash-2.0.1 Aug 31, 2018
@str4d str4d deleted the str4d:zip32 branch Aug 31, 2018
zkbot added a commit to zcash/zcash that referenced this pull request Aug 31, 2018
Use ZIP 32 for all Sapling spending keys

The wallet now only stores Sapling extended spending keys, and thus can
only be used with keys generated from an HDSeed via ZIP 32. This means
that all Sapling keys and addresses generated by users can be recovered
as long as they have a backup that includes the seed.

Depends on zcash/librustzcash#29

Closes #3380.
zkbot added a commit to zcash/zcash that referenced this pull request Aug 31, 2018
Use ZIP 32 for all Sapling spending keys

The wallet now only stores Sapling extended spending keys, and thus can
only be used with keys generated from an HDSeed via ZIP 32. This means
that all Sapling keys and addresses generated by users can be recovered
as long as they have a backup that includes the seed.

Depends on zcash/librustzcash#29

Closes #3380.
zkbot added a commit to zcash/zcash that referenced this pull request Aug 31, 2018
Use ZIP 32 for all Sapling spending keys

The wallet now only stores Sapling extended spending keys, and thus can
only be used with keys generated from an HDSeed via ZIP 32. This means
that all Sapling keys and addresses generated by users can be recovered
as long as they have a backup that includes the seed.

Depends on zcash/librustzcash#29

Closes #3380.
zkbot added a commit to zcash/zcash that referenced this pull request Sep 3, 2018
Use ZIP 32 for all Sapling spending keys

The wallet now only stores Sapling extended spending keys, and thus can
only be used with keys generated from an HDSeed via ZIP 32. This means
that all Sapling keys and addresses generated by users can be recovered
as long as they have a backup that includes the seed.

Depends on zcash/librustzcash#29

Closes #3380.
zkbot added a commit to zcash/zcash that referenced this pull request Sep 11, 2018
Use ZIP 32 for all Sapling spending keys

The wallet now only stores Sapling extended spending keys, and thus can
only be used with keys generated from an HDSeed via ZIP 32. This means
that all Sapling keys and addresses generated by users can be recovered
as long as they have a backup that includes the seed.

Depends on zcash/librustzcash#29

Closes #3380.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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