Fix: derive_pub() is now infalible#3036
Conversation
Pull Request Test Coverage Report for Build 10483091871Details
💛 - Coveralls |
|
OT I removed the mentions from the PR description because when used in the description box they cause github to spam the person with notifications. cc @storopoli and @Kixunil (because this is not my PR and I edited the description). |
Kixunil
left a comment
There was a problem hiding this comment.
Looks much better but there are still some blocking issues.
Kixunil
left a comment
There was a problem hiding this comment.
Damn, my review didn't get sent again. I'm starting to suspect network issues.
|
So, to get the things done to merge, ill consider the marjority of the "stable"(we have an idea to follow), and implement it on the examples and the rest of the code(saw some things while running 'just check'). After that, squash. |
|
To get a proper final review you need
|
|
Also regarding squashing, not all commits deserve to be squashed but I think you'll have to squash these. Or edit the git history to e.g. create the newtypes first. |
33f400e to
d9f7f8d
Compare
storopoli
left a comment
There was a problem hiding this comment.
Bip32 Remake Part 2 (Directors Cut) is not a good commit message.
What about:
bip32: infallible derive_pub
?
I know 🤣. |
47f52af to
f2ea8c7
Compare
|
🚨 API BREAKING CHANGE DETECTED |
storopoli
left a comment
There was a problem hiding this comment.
A couple more.
Please note that there are still old comments/suggestion that were not addressed by the new commit.
GitHub might have hidden it away in the "view more" style of buttons.
And please do not make dozens of commits. The bot is very "trigger-happy".
Thanks for working on this!
|
🚨 API BREAKING CHANGE DETECTED |
cbb1f47 to
c3c3c35
Compare
|
🚨 API BREAKING CHANGE DETECTED |
storopoli
left a comment
There was a problem hiding this comment.
ACK c3c3c3553f4dc2cb02849379faabcdc04d1cf017
bitcoin/examples/taproot-psbt.rs
Outdated
There was a problem hiding this comment.
This example is silly, rather than parsing it should just construct the path directly. But not needed in this PR.
bitcoin/examples/taproot-psbt.rs
Outdated
There was a problem hiding this comment.
This conversion shouldn't be needed. If it is, we have a bug in the API.
There was a problem hiding this comment.
indeed
I added I: Into<ChildNumber>, and worked fine without the method
bitcoin/examples/taproot-psbt.rs
Outdated
There was a problem hiding this comment.
In this case i dont see a good fix... KeySource is a tupple spread all of the code... trying any abstraction or change on it will run out of scope for this pr.
There was a problem hiding this comment.
I'll hack deeper for that
There was a problem hiding this comment.
/// Full information on the used extended public key: fingerprint of the
/// master extended public key and a derivation path from it.
pub type KeySource = (Fingerprint, DerivationPath);We could make KeySource a newtype, but in another pr... i dont see how to discard the conversion without changing the KeySource itself.
Ill gladly work on it after this one. by now, i would leave the example as it is.
There was a problem hiding this comment.
Oh, I think I understand now. OK, fine to look at it later. It may be that we can't actually change it because it might break the spec but we'd have to dig much deeper.
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
This should be forwarded using fmt::Display::fmt(&self.to_raw_number, f) so that all formatting works as with integers.
bitcoin/src/bip32.rs
Outdated
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
This should be pub, the allow is wrong.
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
I don't love the name but I can't think of any better. But why not just have fn push_child(&mut self, cn: ChildNumber)?
There was a problem hiding this comment.
i did not understand the point of this comment, do you want a rename ?
There was a problem hiding this comment.
I suggest changing both the interface and the name.
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
What's the point of this if there's already extend?
There was a problem hiding this comment.
abstraction before calling extend. redundant but i would prefer to maintain this function rather than deleting it and further rewriting it back because i dont want thousand impls on the code
There was a problem hiding this comment.
Why can't you just replace extend_from calls with extend?
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
I don't see the point of creating a new one, especially since you're not optimizing allocations. Further I think the method should be called join like on Path.
For sure i didnt pasted literally your commit message, but the message is literally a bullet point of yours. |
|
If I spent all that time writing a thoughtful message I'd prefer it to be there. Why wouldn't you just copy-paste it? |
It's big but okay |
c3c3c35 to
19c175f
Compare
|
🚨 API BREAKING CHANGE DETECTED |
tcharding
left a comment
There was a problem hiding this comment.
I have not been following this work closely so please forgive me if I suggest something already discussed. Also I note that the bip32 module is still a mess but this PR is a definite improvement. Thanks for contributing.
bitcoin/examples/ecdsa-psbt.rs
Outdated
There was a problem hiding this comment.
This change shouldn't be in here, its unrelated.
There was a problem hiding this comment.
Maybe was something that i called like fmt or something like that. Ill put the blank line back i guess.
bitcoin/examples/ecdsa-psbt.rs
Outdated
There was a problem hiding this comment.
nit: Perhaps just use expect here like done in other places.
There was a problem hiding this comment.
Showing the proper error handling to users might be better.
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
| /// Sub-module for [`HardenedChildNumber`] type | |
| /// This module exists to protect the invariants on the inner field of [`HardenedChildNumber`]. |
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
| /// A child number for a derived key | |
| /// Indexes `2_147_483_648` to `4_294_967_295` (the second half of all possible children) | |
| /// A child number for a derived key. | |
| /// | |
| /// Guarantees that the inner field is a valid hardened value (indexes `2_147_483_648` to | |
| /// `4_294_967_295`, the second half of all possible children). |
There was a problem hiding this comment.
Won't these auto doc wrapping formatting be done in the weekly rustfmt PR job?
There was a problem hiding this comment.
rustfmt doesn't change rustdocs line lengths.
There was a problem hiding this comment.
Oh I swear that this was set to true
Line 20 in c061d93
There was a problem hiding this comment.
I think its correct to be false, its kind of nice to control manually docs IMO.
There was a problem hiding this comment.
Yes you are right. Sometimes that can be really nasty...
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
All the public re-exports would fit in with the style we use better if they were put below the 'use crate::foo' statements and written like this:
#[rustfmt::skip] // Keep public re-exports separate.
#[doc(inline)]
pub use self::{
hardened::HardenedChildNumber,
normal::NormalChildNumber,
derivation_path::{
DerivationPath, IntoIter as DerivationPathIntoIter, Iter as DerivationPathIter,
},
normal_derivation_path::{
NormalDerivationPath, IntoIter as NormalDerivationPathIntoIter, Iter as NormalDerivationPathIter,
},
};Feel free not to use my custom formating, I grouped similar things instead of using alphabetic order, also one line is longer that the formatter allows.
There was a problem hiding this comment.
EDIT: Note below I question the non-uniformity of the NormalDerivationPath and DerivationPath types, same question applies to the module names.
There was a problem hiding this comment.
EDIT: Note below I question the non-uniformity of the NormalDerivationPath and DerivationPath types, same question applies to the module names.
You say... the names ?
Feel free not to use my custom formating, I grouped similar things instead of using alphabetic order, also one line is longer that the formatter allows.
I dont spend to much time organizing this normally because i know that, usually on many other projects, the fmt will just mess it up or standardize it.
But, nice! its better !
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
Perhaps this should be called from_idx to match the other two constructors? (And put above from_normal_idx.)
But really, this idx stuff is non-uniform, there are other functions called from_index - we should pick one. Can be done as a follow up though.
There was a problem hiding this comment.
from_raw_index rather.
I prefer index rather than idx.
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
All this could be written as
pub fn from_idx(index: u32) -> Self {
match NormalChildNumber::from_index(index) {
Ok(normal) => ChildNumber::Normal(normal),
Err(_) => ChildNumber::Hardened(HardenedChildNumber::from_index(index).expect("valid since not normal")),
}
}
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
Perhaps this should be HardenedDerivationPath to match the NormalDerivationPath?
There was a problem hiding this comment.
No, it's not hardened. It can be anything.
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
nit: Should probably be P to match the other one in derive_priv
bitcoin/src/bip32.rs
Outdated
There was a problem hiding this comment.
Same suggestions as for hardened module.
19c175f to
f3cf8dd
Compare
|
Adressed the requested changes by @tcharding |
|
🚨 API BREAKING CHANGE DETECTED |
The BIP32 API had several problems that we wanted to fix for some time. It didn't protect the invariants of child numbers, had some methods fallible even if the errors were cryptographically impossible, it lacked infallible APIs for deriving xpubs when absence of hardened children was statically known, and had annoying ChildNumber enum with named field for no good reason. In this commit we do a bunch of refactoring to fix these problems. We add newtypes for the child numbers and a newtype for derivation path that is guaranteed to only contain normal child numbers with appropriate parsing and conversion methods. We refactor the ChildNumber enum to use tuple variants containing the newtypes and fix the infallible APIs to not return Result. Dealing with hardened child numbers can still be annoying or non-performant in some scenarios so we still provide a fallible derivation method. Further we put the newtypes into separate submodules to make accidental breaking of invariatns less likely. Signed-off-by: jaoleal <jgleal@protonmail.com> Signed-off-by: jaoleal <=>
f3cf8dd to
c3e0813
Compare
|
🚨 API BREAKING CHANGE DETECTED |
|
I'm not sure what to do with this PR. Its 700 lines of changes, its very hard to review just looking at the diff but when I look at the My primary concern is that at the moment it is not obvious what an "index" is and what a "number" is. In order to fully understand the problem I had a play, would you be open to incorporating some additional ideas? You could do any of the following (if you are willing)
/// A child key number.
///
/// Please note, "index" and "number" have specific defined meanings in this module.
///
/// - A "number" is always less that 2^31 and is used in derivation paths eg "0'/0/1".
/// - An "index" is the encoded form, where one bit is used to represent normal vs hardened.
///
/// > Each extended key has 231 normal child keys, and 231 hardened child keys. Each of these
/// > child keys has an index. The normal child keys use indices 0 through 231-1. The hardened
/// > child keys use indices 231 through 232-1. To ease notation for hardened key indices, a
/// > number iH represents i+231.
///
/// So, when serializing a child number as an index BIP-32 uses the top bit of the index (greater
/// than or equal to 2^31) to indicate hardened child keys.
#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
pub enum ChildNumber {
/// A normal child number e.g., non-hardened.
Normal(NormalChildNumber),
/// A hardened child number.
Hardened(HardenedChildNumber),
}
/// This module exists to protect the invariants on the inner field of [`HardenedChildNumber`].
mod hardened {
use super::*;
/// A hardened child number for a derived key.
#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
pub struct HardenedChildNumber(u32);
impl HardenedChildNumber {
/// The first hardened child (number 0).
pub const ZERO: Self = HardenedChildNumber(0);
/// The second hardened child (number 1).
pub const ONE: Self = HardenedChildNumber(1);
/// Creates a [`HardenedChildNumber`] from a `number` (less than 2^31).
///
/// # Errors
///
/// If the top bit of `number` is set.
pub const fn from_number(number: u32) -> Result<HardenedChildNumber, NumberError> {
if index & (1 << 31) == 0 {
Ok(HardenedChildNumber(index))
} else {
Err(NumberError(index))
}
}
/// Creates a hardened child number from an `index` (greater than 2^31).
///
/// # Errors
///
/// If the top bit of `index` is not set.
pub const fn from_index(index: u32) -> Result<HardenedChildNumber, HardenedIndexError> {
if index & (1 << 31) != 0 {
Ok(HardenedChildNumber(index))
} else {
Err(HardenedIndexError(index))
}
}
/// Creates a hardened child number from a [`ChildNumber`].
pub const fn from_child_number(number: ChildNumber) -> Result<HardenedChildNumber, HardenedChildNumberError> {
match number {
ChildNumber::Hardened(num) => Ok(num),
ChildNumber::Normal(num) => Err(HardenedChildNumberError(number)),
}
}
/// Converts this hardened child number to a number (less than 2^31).
pub const fn to_number(self) -> u32 { self.0 }
/// Converts this hardened child number to an index (greater than 2^31).
pub const fn to_index(self) -> u32 { self.0 & (1 << 31) }
/// Converts this hardened child number to a [`ChildNumber`].
pub const fn to_child_number(self) -> ChildNumber { ChildNumber::Hardened(self) }
}
}
/// This module exists to protect the invariants on the inner field of [`NormalChildNumber`].
mod normal {
use super::*;
/// A normal child number for a derived key.
///
/// Guaranteed to be less than 2^31.
#[derive(Copy, Clone, PartialEq, Eq, Debug, PartialOrd, Ord, Hash)]
pub struct NormalChildNumber(u32);
impl NormalChildNumber {
/// The first normal child (number 0).
pub const ZERO: Self = NormalChildNumber(0);
/// The second normal child (number 1).
pub const ONE: Self = NormalChildNumber(1);
/// Creates a normal child number from a number (less than 2^31).
///
/// # Errors
///
/// If the top bit of `number` is set.
pub const fn from_number(index: u32) -> Result<NormalChildNumber, NumberError> {
if index & (1 << 31) == 0 {
Ok(NormalChildNumber(index))
} else {
Err(NumberError(index))
}
}
/// Creates a normal child number from an index (less than 2^31).
///
/// # Errors
///
/// If the top bit of `index` is set.
pub const fn from_index(index: u32) -> Result<NormalChildNumber, NormalIndexError> {
if index & (1 << 31) == 0 {
Ok(NormalChildNumber(index))
} else {
Err(NormalIndexError(index))
}
}
/// Creates a normal child number from a [`ChildNumber`].
pub const fn from_child_number(number: ChildNumber) -> Result<NormalChildNumber, NormalChildNumberError> {
match number {
ChildNumber::Normal(num) => Ok(num),
ChildNumber::Hardened(_) => Err(NormalChildNumberError(number)),
}
}
/// Converts this normal child number to a number (less than 2^31).
pub const fn to_number(self) -> u32 { self.0 }
/// Converts this normal child number to an index (less than 2^31).
pub const fn to_index(self) -> u32 { self.to_number() }
/// Converts this normal child number to a [`ChildNumber`].
pub const fn to_child_number(self) -> ChildNumber { ChildNumber::Normal(self) }
}
}
impl fmt::Display for HardenedChildNumber {
/// Displays the number without the ' or h marker.
///
/// The displayed number is always less than 2^31.
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
fmt::Display::fmt(&self.to_number(), f)
}
}
impl fmt::Display for NormalChildNumber {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
fmt::Display::fmt(&self.to_number(), f)
}
}
impl FromStr for HardenedChildNumber {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Error> {
let number = s.parse::<u32>()?;
HardenedChildNumber::from_number(index)
}
}
impl FromStr for NormalChildNumber {
type Err = Error;
fn from_str(s: &str) -> Result<Self, Error> {
let number = s.parse::<u32>()?;
NormalChildNumber::from_number(number)
}
}
impl From<NormalChildNumber> for ChildNumber {
fn from(num: NormalChildNumber) -> Self { ChildNumber::Normal(num) }
}
impl From<HardenedChildNumber> for ChildNumber {
fn from(num: HardenedChildNumber) -> Self { ChildNumber::Hardened(num) }
}
impl TryFrom<ChildNumber> for NormalChildNumber {
type Err = NormalChildNumberError;
fn from(num: NormalChildNumber) -> Self { Self::from_child_number(number) }
}
impl TryFrom<ChildNumber> for HardenedChildNumber {
type Err = HardenedChildNumberError;
fn from(num: HardenedChildNumber) -> Self { Self::from_child_number(number) }
}Still TODO
This is a non-trivial amount of work, I appreciate you taking it on, if you choose to do so. If you don't I can get to it but it is waaay down my list of priorities. |
Yes, of course... I just wonder if is this being caused by my lack of knowledge in rust/bitcoin or because i am not used to the design of Working on anything here is also working on any other thing outside but related to bitcoin programming in rust. I'm eagerly to help and will stick to apoelstra advice that he gave on some podcast talking about this specific issue. I will review every line of this PR but this may took another couple of days to be done(i need to focus a little bit on my internship and getting a real job for myself). Thank you for the time that yall spent giving good advices and hope to help more after this one. |
|
For my part I've only been skimming this PR and it's got 400 comments and (as Tobin says) 700 lines of changes in one commit. So I'm skeptical that we'll be able to get this in as-is. @jaoleal I would suggest that you try to find individual parts (say, renaming stuff to use "index" and "number" consistently) and PR those separately. This will be tough to do because of how tightly-coupled everything is. Another strategy might be to make a new And I understand that you're busy and have higher-priority things to do (focusing on your career certainly sounds more important than banging your head against a volunteer contribution to this project!) so possibly one of us will need to do this. Or maybe out of respect for your time we'll need to merge the PR in an incomplete state and one of us should try to make a followup. Unsure. |
|
@tcharding to prove that I personally think the main reasons for the complexity of the PR are:
I think separating number change from path change should resolve most of the complexity. @jaoleal if you feel like it you can try but if you don't, I'll take a look at some point soon enough to hit the next release because I happen to need this change. :) |
Ha! Nice. I rest my case :) |
|
Guys, quick update: I've been working on this and staring at the PR like a dog staring at a chicken rotisserie, trying to figure out a better way to implement it. I even found a few other things that could be adjusted to fit the original idea we discussed. I'll close this PR because it's gone beyond the initial scope. All the changes from this PR will be included in a new one. I understand this isn't ideal, and someone with more experience could probably refactor what I did, but I ask for a bit of patience. I'm not far from pushing the new changes.
I agree that splitting things up would be better, and I’ll handle that in separate commits, making sure everything is well documented and explained. That said, the components in this module are REALLY tightly coupled, and splitting them into separate PRs might make things worse, in my opinion. |
Rest of #2911.
Agreeing with storopoli 's comment and tried to follow Kixunil advice.
The changes hopefully will confirm that
derive_pub()never fails.The
NormalChildIteratorensures that everyChildNumberpassed tointernal_derive_pub()is aChildNumber::Normal, in other case the iterator will end when find aChildNumber::Hardened. This can introduce to a new behavior where we try to insert aDerivationPaththat contain aChildNumber::Hardenedandderive_pub()will not fail but instead, yielding a different public key.The focus was not changing too much the code and make something that serves as a safe filter for
derive_pub(), because of this,NormalChildIteratoris meant to only be used insidederive_pub()Sorry for taking so long to become with a solution.