Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Arbitrary labels for extended keys (u32, H256 built-in) #4438

Merged
merged 7 commits into from
Feb 9, 2017

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Feb 5, 2017

Can be basically any byte sequence (but caller must be sure what is he doing since key is compacted into a 512-bit hash after all, no matter of original data length). This slightly increase complexity of the signatures, but probably worth it.

@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Feb 5, 2017
@NikVolf NikVolf assigned gavofyork and unassigned gavofyork Feb 5, 2017
@NikVolf NikVolf requested a review from gavofyork February 5, 2017 15:26
@NikVolf NikVolf mentioned this pull request Feb 5, 2017
@@ -21,6 +21,46 @@ use Public;
use bigint::hash::{H256, FixedHash};
pub use self::derivation::Error as DerivationError;

pub trait Label {
Copy link
Contributor

Choose a reason for hiding this comment

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

document!

Copy link
Contributor

Choose a reason for hiding this comment

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

particularly the implied invariant on store that target.len() must be > T::len(), and that breaking this invariant will lead to panic.

@@ -228,24 +266,24 @@ mod derivation {
// curve point (compressed public key) -- index
// 0.33 -- 33..37
data[0..33].copy_from_slice(&public_serialized);
BigEndian::write_u32(&mut data[33..37], index);
index.store(&mut data[33..37]);
Copy link
Contributor

Choose a reason for hiding this comment

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

should use T::len()

@rphmeier rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 5, 2017
@NikVolf NikVolf added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Feb 5, 2017
@gavofyork
Copy link
Contributor

looks like this only supports hard derivation for > 31 bit indexes, no? it's important to support soft derivation for 256-bit indexes...

@NikVolf
Copy link
Contributor Author

NikVolf commented Feb 5, 2017

@gavofyork Nope, both soft and hard derivation can be made for any H256
There is a test for it h256_soft_match

It's just the original bitcoin spec that splits 0..2^32 field to two fields of 0..2^(31-1) and 2^31..2^32 artifically
actually, you can have any sequence of bytes to be used both in hard and soft derivation

@gavofyork
Copy link
Contributor

cool

fn len() -> usize { 32 }

fn store(&self, target: &mut [u8]) {
self.copy_to(&mut target[0..4]);
Copy link
Contributor

Choose a reason for hiding this comment

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

what's happening here? only copying the first 4 bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah right
i added it more like an example here, didn't payed much attention, thanx

@gavofyork gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 9, 2017
@gavofyork gavofyork merged commit fea76c0 into master Feb 9, 2017
@gavofyork gavofyork deleted the hd-wallet-ext branch February 9, 2017 15:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants