Add bip32::parse_derivation_path by stevenroose · Pull Request #185 · rust-bitcoin/rust-bitcoin

stevenroose · 2018-11-01T23:34:28Z

Utility method to parse a m/0'/0'/0-like path.

Harden characters ' and h are supported.

clarkmoody · 2018-11-05T21:46:33Z

Concept ACK.

Fuzz testing is failing in the CI build. I'm not sure what to do about that.

stevenroose · 2018-11-06T12:36:39Z

Hmm, it fails to build a fuzzing dep. I have no idea about the fuzzing setup in rust-bitcoin, bth.

src/util/bip32.rs

stevenroose · 2018-11-15T22:45:00Z

You're suggesting that should be supported? Or add tests to catch that they are not supported?

…

On Thu, Nov 15, 2018, 21:17 Gregory Sanders ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/util/bip32.rs <#185 (comment)> : > use super::{ChildNumber, ExtendedPrivKey, ExtendedPubKey}; use super::ChildNumber::{Hardened, Normal}; use super::Error; + #[test] + fn test_parse_derivation_path() { + assert_eq!(parse_derivation_path("42"), Err(Error::InvalidDerivationPathFormat)); + assert_eq!(parse_derivation_path("n/0'/0"), Err(Error::InvalidDerivationPathFormat)); + assert_eq!(parse_derivation_path("m/0h/0x"), Err(Error::InvalidChildNumberFormat)); + suggestion: add test for something like m//3/0' with double-slashes as well has capital H for harden — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#185 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0F3G5BRYwjBqKKDJVwH6LznskTuT3Xks5uvdoGgaJpZM4YKKuR> .

instagibbs · 2018-11-15T22:47:54Z

Tests.

…

On Thu, Nov 15, 2018, 5:45 PM Steven Roose ***@***.*** wrote: You're suggesting that should be supported? Or add tests to catch that they are not supported? On Thu, Nov 15, 2018, 21:17 Gregory Sanders ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/util/bip32.rs > < #185 (comment) > > : > > > use super::{ChildNumber, ExtendedPrivKey, ExtendedPubKey}; > use super::ChildNumber::{Hardened, Normal}; > use super::Error; > > + #[test] > + fn test_parse_derivation_path() { > + assert_eq!(parse_derivation_path("42"), Err(Error::InvalidDerivationPathFormat)); > + assert_eq!(parse_derivation_path("n/0'/0"), Err(Error::InvalidDerivationPathFormat)); > + assert_eq!(parse_derivation_path("m/0h/0x"), Err(Error::InvalidChildNumberFormat)); > + > > suggestion: add test for something like m//3/0' with double-slashes as > well has capital H for harden > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < #185 (review) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AA0F3G5BRYwjBqKKDJVwH6LznskTuT3Xks5uvdoGgaJpZM4YKKuR > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#185 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgC0xmKPu5nWu-fcIGmuFEpV1o4nQ6Jks5uve5vgaJpZM4YKKuR> .

src/util/bip32.rs

This public method parses a formatted derivation path into a `Vec<ChildNumber>`.

stevenroose · 2018-12-05T21:28:00Z

Fuzz found something. Should I keep those: https://travis-ci.org/rust-bitcoin/rust-bitcoin/jobs/463440802

sgeisler · 2018-12-06T08:19:05Z

Fuzz found something. Should I keep those: https://travis-ci.org/rust-bitcoin/rust-bitcoin/jobs/463440802

I think that's just a bug in a serde implementation we use because of 1.14 :/ it happens from time to time (iirc it's because of a stack overflow that can be triggered, this seems consistent with the large output that was produced and the deep stack trace). I'd just run the test again, but maybe @apoelstra can confirm or correct this (I know I read this somewhere, but I don't remember where).

apoelstra · 2018-12-08T21:54:39Z

I don't think it's serde, I think it's just Yet Another Decimal Overflow Bug. This one probably should be fixed because it's actually a stack overflow rather than panic, but in general the type is not designed properly and needs to be completely rewritten.

In any case, it's unrelated to this PR.

src/util/bip32.rs

stevenroose · 2019-01-19T21:41:27Z

Ready to be merged!

dongcarl

LGTM, in the future we should make this more generic by parsing to an Iterator over Result<ChildNumber, Error>.

stevenroose · 2019-01-20T01:16:41Z

I'd be more in favor of having a new type `struct DerivationPath(Vec<ChildNumber>)` in this case, to be honest.

…

On Sat, Jan 19, 2019, 23:12 Carl Dong ***@***.*** wrote: ***@***.**** approved this pull request. LGTM, in the future we should make this more generic by parsing to an Iterator over Result<ChildNumber, Error>. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#185 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0F3MQeNTXcqx06DgqT5R0WQEdXVgAZks5vE6ZZgaJpZM4YKKuR> .

stevenroose · 2019-01-24T13:11:35Z

Would there be more interest in a DerivationPath type that has a Display and FromStr implementation instead of this standalone method?

dongcarl · 2019-01-25T02:43:38Z

Would there be more interest in a DerivationPath type that has a Display and FromStr implementation instead of this standalone method?

Yes, let's do that! I remember needing it for PSBT at some point.

dongcarl · 2019-02-04T15:41:40Z

@stevenroose Any progress on this?

stevenroose · 2019-02-04T19:00:58Z

Ah didn't see your previous comment. I can do this whenever I'm back from PTO. Don't have decent internet here but can try :) I want to get this in too, I have a copy of that code in hal and reserves.

…

On Mon, Feb 4, 2019, 16:41 Carl Dong ***@***.*** wrote: @stevenroose <https://github.com/stevenroose> Any progress on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#185 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0F3BZ8212dJHqXK_PFsJo-mrm8UdIlks5vKFS1gaJpZM4YKKuR> .

stevenroose · 2019-02-07T21:14:56Z

@dongcarl Because your suggestion made the code change entirely, I created an alternative PR: #233

This one can be ignored if the other one is preferred.

dongcarl · 2019-02-18T00:47:47Z

Superseded by #233, which is now merged.

…ions 8806879 Migrate CI to github actions (Tibo-lg) Pull request description: Closes rust-bitcoin#185 I'm not a github action expert so there might be more concise ways of expressing things, but at least it works. ACKs for top commit: notmandatory: ACK 8806879 Tree-SHA512: 0c347867a66d1a19fad182ebb3961819a34190573dfec4c1425c1f295bc1a32a7f0adb8da31a9e0b004144b8f17efea36c0d81244e09b743af36df6705039f3a

Test Display/Debug for SortedMultiVec

…itcoin#185) PR rust-bitcoin#180 used `MultiPeerNetworkManager`, this was renamed in rust-bitcoin#183 which was merged before rust-bitcoin#180 but after its CI run.

…-bitcoin#197) * feat(spv): flush headers on shutdown * move fn lower in the impl * refactor: `MultiPeerNetworkManager` -> `PeerNetworkManager` (rust-bitcoin#184) * refactor: `MultiPeerNetworkManager` -> `PeerNetworkManager` * Fix formatting and apply review * feat: Update ffi headers (rust-bitcoin#183) * feat(spv): broadcast transaction (rust-bitcoin#180) * fix: Fix `PeerNetworkManager` cast in `broadcast_transaction` (rust-bitcoin#185) PR rust-bitcoin#180 used `MultiPeerNetworkManager`, this was renamed in rust-bitcoin#183 which was merged before rust-bitcoin#180 but after its CI run. * fix: Use non-blocking `TcpStream` in `dash-spv::network::TcpConnection` (rust-bitcoin#188) * refactor: Improve SPV shutdown handling with `CancellationToken` (rust-bitcoin#187) * refactor: `TcpConnection` -> `Peer` and `ConnectionPool` -> `PeerPool` (rust-bitcoin#190) * fix: Locking issue after rust-bitcoin#190 (rust-bitcoin#191) rust-bitcoin#190 removed the read timeouts of the `Peer::receive_message` which currently leads to a lockup of the peer because the write lock is held while waiting for the message. Needs some more refactoring but this works for now. * fix: More follow-up to rust-bitcoin#190 (rust-bitcoin#193) The sleep timeout branch introduced in rust-bitcoin#191 returns an `Err(NetworkError::Timeout)` which leads to a misbehavior update below in the `msg_result` match and eventually in a peer ban. This shouldn't happen because the `sleep` timing out only means that there is no data available right now. Instead, it now returns `Ok(None)` which will just keep things going. * flush after mn list sync too * move flush to after header sync instead of mnlist * fix --------- Co-authored-by: Kevin Rombach <35775977+xdustinface@users.noreply.github.com>

stevenroose commented Nov 6, 2018

View reviewed changes

src/util/bip32.rs Outdated Show resolved Hide resolved

stevenroose force-pushed the parse-bip32 branch from 97f62bd to b9700de Compare November 14, 2018 11:38

instagibbs reviewed Nov 15, 2018

View reviewed changes

src/util/bip32.rs Outdated Show resolved Hide resolved

stevenroose force-pushed the parse-bip32 branch from b9700de to 9cae500 Compare November 16, 2018 18:05

apoelstra reviewed Dec 3, 2018

View reviewed changes

src/util/bip32.rs Outdated Show resolved Hide resolved

stevenroose force-pushed the parse-bip32 branch from 9cae500 to 4b13153 Compare December 3, 2018 13:02

dongcarl suggested changes Dec 4, 2018

View reviewed changes

src/util/bip32.rs Outdated Show resolved Hide resolved

src/util/bip32.rs Outdated Show resolved Hide resolved

stevenroose force-pushed the parse-bip32 branch 2 times, most recently from 06e3c83 to 5b14f57 Compare December 4, 2018 14:30

dongcarl suggested changes Dec 4, 2018

View reviewed changes

src/util/bip32.rs Outdated Show resolved Hide resolved

src/util/bip32.rs Outdated Show resolved Hide resolved

src/util/bip32.rs Outdated Show resolved Hide resolved

stevenroose force-pushed the parse-bip32 branch from 5b14f57 to 02c06bb Compare December 4, 2018 16:53

Add bip32::parse_derivation_path

b70b046

This public method parses a formatted derivation path into a `Vec<ChildNumber>`.

stevenroose force-pushed the parse-bip32 branch from 02c06bb to 4981e08 Compare December 5, 2018 21:31

Rewrite bip32 vector tests to use parser

4981e08

apoelstra reviewed Dec 9, 2018

View reviewed changes

src/util/bip32.rs Outdated Show resolved Hide resolved

dongcarl reviewed Dec 27, 2018

View reviewed changes

src/util/bip32.rs Show resolved Hide resolved

stevenroose force-pushed the parse-bip32 branch from fce8421 to 73baeca Compare January 19, 2019 21:41

Prevent double iteration over childnumbers

73baeca

dongcarl approved these changes Jan 19, 2019

View reviewed changes

stevenroose mentioned this pull request Feb 7, 2019

bip32: Add DerivationPath type #233

Merged

dongcarl closed this Feb 18, 2019

yancyribbens pushed a commit to yancyribbens/rust-bitcoin that referenced this pull request Mar 23, 2024

Merge pull request rust-bitcoin#185 from justinmoon/test

b2896e7

Test Display/Debug for SortedMultiVec

Conversation

stevenroose commented Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clarkmoody commented Nov 5, 2018

Uh oh!

stevenroose commented Nov 6, 2018

Uh oh!

Uh oh!

Uh oh!

stevenroose commented Nov 15, 2018 via email

Uh oh!

instagibbs commented Nov 15, 2018 via email

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenroose commented Dec 5, 2018

Uh oh!

sgeisler commented Dec 6, 2018

Uh oh!

apoelstra commented Dec 8, 2018

Uh oh!

Uh oh!

Uh oh!

stevenroose commented Jan 19, 2019

Uh oh!

dongcarl left a comment

Choose a reason for hiding this comment

Uh oh!

stevenroose commented Jan 20, 2019 via email

Uh oh!

stevenroose commented Jan 24, 2019

Uh oh!

dongcarl commented Jan 25, 2019

Uh oh!

dongcarl commented Feb 4, 2019

Uh oh!

stevenroose commented Feb 4, 2019 via email

Uh oh!

stevenroose commented Feb 7, 2019

Uh oh!

dongcarl commented Feb 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stevenroose commented Nov 1, 2018 •

edited

Loading