BIP 32 binary encoding functions are extracted from base58 by dr-orlovsky · Pull Request #470 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2020-09-10T00:40:13Z

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 inside Display and FromStr 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

stevenroose · 2020-10-10T17:23:31Z

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.

dr-orlovsky · 2020-10-11T02:57:36Z

@stevenroose done

rust-bitcoin/rust-bitcoin#470

stevenroose

I don't understand one of the From cases and I think one error can be expect'ed.

stevenroose · 2020-10-25T11:32:39Z

src/util/bip32.rs

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..

stevenroose · 2020-10-25T11:35:15Z

src/util/bip32.rs

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(),.)

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

src/util/bip32.rs

dr-orlovsky · 2020-11-06T16:15:19Z

Rebased onto fixed master (requires #512 to be merged first) and addressed @stevenroose comments

dr-orlovsky · 2020-11-06T19:53:42Z

Found another incorrect error conversion which fixed with 97e7d0d

apoelstra · 2020-11-07T23:52:06Z

src/util/bip32.rs

Did you mean to use the new UnknownVersion error variant?

apoelstra · 2020-11-07T23:54:17Z

src/util/bip32.rs

extra space added here

apoelstra · 2020-11-07T23:56:05Z

src/util/bip32.rs

This whole impl is weird and should be deleted.

dr-orlovsky · 2020-11-08T12:05:14Z

@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.

apoelstra · 2020-11-08T16:08:58Z

src/util/bip32.rs

-        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()[..]))


Can you use https://docs.rs/bitcoin/0.25.2/bitcoin/util/base58/fn.check_encode_slice_to_fmt.html to avoid the allocation

stevenroose

Looks good, could you perhaps do that one extra inline? Feel free to ignore the two nits otherwise.

stevenroose · 2020-11-09T19:52:36Z

src/util/bip32.rs

+            child_number: child_number,
+            chain_code: ChainCode::from(&data[13..45]),
+            public_key: PublicKey::from_slice(
+                &data[45..78])?,


nit: No reason for a newline here, but going to ack anyway

Don't know how it happened, probably occasionally hit Enter... Fixed

stevenroose · 2020-11-09T19:53:03Z

src/util/bip32.rs

+        }
+
+        let cn_int: u32 = endian::slice_to_u32_be(&data[9..13]);
+        let child_number: ChildNumber = ChildNumber::from(cn_int);


nit: Type doesn't need to be specified twice.

True, remnants on the older code

stevenroose · 2020-11-09T19:54:08Z

src/util/bip32.rs

+            },
+            depth: data[4],
+            parent_fingerprint: Fingerprint::from(&data[5..9]),
+            child_number: child_number,


In fact this one can be inlined just like the above one to endian::slice_to_u32_be(&data[9..13]).into()

dr-orlovsky · 2020-11-10T08:31:34Z

Updated according to @stevenroose review; all the changes are in fa4ecb4

stevenroose

LGTM

This was referenced Sep 10, 2020

PSBT proprietary key system matching BIP 174 #471

Merged

BIP 32/SLIP 132 y,z...pub/priv types with generics #469

Closed

[epic] PSBT and related BIP 32 refactoring tracking issue #473

Closed

dr-orlovsky mentioned this pull request Oct 11, 2020

New PSBT global keys #499

Merged

apoelstra added the API break This PR requires a version bump for the next release label Oct 14, 2020

dr-orlovsky added a commit to LNP-BP/rust-lnpbp that referenced this pull request Oct 21, 2020

[revert] when bitcoin merge BIP32 encodings

ee4aa5c

rust-bitcoin/rust-bitcoin#470

stevenroose requested changes Oct 25, 2020

View reviewed changes

dr-orlovsky requested review from elichai and stevenroose November 6, 2020 19:47

apoelstra reviewed Nov 7, 2020

View reviewed changes

src/util/bip32.rs Outdated

Copy link
Copy Markdown

Member

apoelstra Nov 7, 2020

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?

apoelstra reviewed Nov 7, 2020

View reviewed changes

src/util/bip32.rs Outdated

Copy link
Copy Markdown

Member

apoelstra Nov 7, 2020

Choose a reason for hiding this comment

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

extra space added here

apoelstra reviewed Nov 7, 2020

View reviewed changes

src/util/bip32.rs Outdated

Copy link
Copy Markdown

Member

apoelstra Nov 7, 2020

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.

stevenroose reacted with thumbs up emoji

dr-orlovsky added 2 commits November 8, 2020 13:03

BIP 32 binary encoding functions are extracted from base58

259259e

Improvements to extended keys encoding logic end errors

d802872

dr-orlovsky requested a review from apoelstra November 8, 2020 12:05

apoelstra reviewed Nov 8, 2020

View reviewed changes

Using base58::check_encode_slice_to_fmt for BIP32 encodings

a802ca8

dr-orlovsky requested a review from apoelstra November 8, 2020 17:31

apoelstra previously approved these changes Nov 9, 2020

View reviewed changes

stevenroose requested changes Nov 9, 2020

View reviewed changes

Making ExtendedPubKey decode aligned with ExtendedPrivKey variant

fa4ecb4

dr-orlovsky dismissed apoelstra’s stale review via fa4ecb4 November 10, 2020 08:30

dr-orlovsky requested review from apoelstra and stevenroose November 10, 2020 08:31

apoelstra approved these changes Nov 11, 2020

View reviewed changes

stevenroose approved these changes Nov 23, 2020

View reviewed changes

stevenroose merged commit ee192eb into rust-bitcoin:master Nov 23, 2020

Conversation

dr-orlovsky commented Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenroose commented Oct 10, 2020

Uh oh!

dr-orlovsky commented Oct 11, 2020

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dr-orlovsky commented Nov 6, 2020

Uh oh!

dr-orlovsky commented Nov 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Nov 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Nov 10, 2020

Uh oh!

stevenroose left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dr-orlovsky commented Sep 10, 2020 •

edited

Loading