-
Notifications
You must be signed in to change notification settings - Fork 668
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
BIP 32 binary encoding functions are extracted from base58 #470
Conversation
Could this perhaps be rebased just on master? I think this can be merged faster than the ypub stuff as it's way less opinionated on. |
@stevenroose done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand one of the From cases and I think one error can be expect
'ed.
src/util/bip32.rs
Outdated
|
||
impl From<Error> for base58::Error { | ||
fn from(err: Error) -> Self { | ||
base58::Error::Other(err.to_string()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, why exactly is this needed? This would create a bip32::Error::Base58(base58::Error::Other(bip32::Error::xxx))
which seems unnecessarily recursive, and should just be the inner bip32::Error::xxx
instead..
src/util/bip32.rs
Outdated
network: network, | ||
depth: data[4], | ||
parent_fingerprint: Fingerprint::from(&data[5..9]), | ||
child_number: child_number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This here could be inlined to child_number: ChildNumber::from(endian::slice_to_u32_be(&data[9..13])),
.
(You seem to not like .into()
, because it could also be child_number: endian::slice_to_u32_be(&data[9..13]).into(),
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just didn't wanted to touch original code (apart from moving it to a standalone function) to simplify the review. Done with special commit.
And I have nothing against .into()
when it is obvious from the context into which type the conversion happens
Rebased onto fixed master (requires #512 to be merged first) and addressed @stevenroose comments |
Found another incorrect error conversion which fixed with 97e7d0d |
src/util/bip32.rs
Outdated
} else if data[0..4] == [0x04u8, 0x35, 0x83, 0x94] { | ||
Network::Testnet | ||
} else { | ||
return Err(base58::Error::InvalidVersion((&data[0..4]).to_vec()).into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to use the new UnknownVersion
error variant?
src/util/bip32.rs
Outdated
@@ -531,16 +531,14 @@ impl ExtendedPrivKey { | |||
network: network, | |||
depth: data[4], | |||
parent_fingerprint: Fingerprint::from(&data[5..9]), | |||
child_number: child_number, | |||
child_number: endian::slice_to_u32_be(&data[9..13]).into(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra space added here
src/util/bip32.rs
Outdated
match err { | ||
Error::Base58(err) => err, | ||
other => base58::Error::Other(other.to_string()) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole impl is weird and should be deleted.
@apoelstra your comments are addressed. Rebased onto master with the merged fix and kept two commits in the history: one for moving logic from old functions to a new once w/o code modification inside those functions; and the other on top with all improvements and addressing review suggestions. |
src/util/bip32.rs
Outdated
ret[45] = 0; | ||
ret[46..78].copy_from_slice(&self.private_key[..]); | ||
fmt.write_str(&base58::check_encode_slice(&ret[..])) | ||
fmt.write_str(&base58::check_encode_slice(&self.encode()[..])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use https://docs.rs/bitcoin/0.25.2/bitcoin/util/base58/fn.check_encode_slice_to_fmt.html to avoid the allocation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, could you perhaps do that one extra inline? Feel free to ignore the two nits otherwise.
src/util/bip32.rs
Outdated
child_number: child_number, | ||
chain_code: ChainCode::from(&data[13..45]), | ||
public_key: PublicKey::from_slice( | ||
&data[45..78])?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: No reason for a newline here, but going to ack anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know how it happened, probably occasionally hit Enter... Fixed
src/util/bip32.rs
Outdated
} | ||
|
||
let cn_int: u32 = endian::slice_to_u32_be(&data[9..13]); | ||
let child_number: ChildNumber = ChildNumber::from(cn_int); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Type doesn't need to be specified twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, remnants on the older code
src/util/bip32.rs
Outdated
}, | ||
depth: data[4], | ||
parent_fingerprint: Fingerprint::from(&data[5..9]), | ||
child_number: child_number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact this one can be inlined just like the above one to endian::slice_to_u32_be(&data[9..13]).into()
Updated according to @stevenroose review; all the changes are in fa4ecb4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
… more conditions d206891 bump version to 0.23.4 (Andrew Poelstra) b01337c context: unconditionally disable auto-rerandomization on wasm (Andrew Poelstra) 7482846 apply `global-context-not-secure` logic to Secp256k1::new (Andrew Poelstra) Pull request description: Fixes rust-bitcoin#470 ACKs for top commit: Kixunil: ACK d206891 tcharding: ACK d206891 sanket1729: ACK d206891 Tree-SHA512: 2a7db5b75f55a007aa780b6317804c819c0366e207623220f72a06c2af09087accf1bc834f05899897afcc2035f5e9a5480d8a7ffff83536327c695602ba138d
Binary (non-base58) encoding for public and private keys are required for storing PSBT
PSBT_GLOBAL_XPUB
according to BIP 174. This PR extracts binary encoding from insideDisplay
andFromStr
functions and put this as a standalone methods.This PR is dependent on #469; to see only changes specific to the current PR pls check commit 94c2dda