🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

Pull Request Test Coverage Report for Build 11126557509

Details

• 221 of 255 (86.67%) changed or added relevant lines in 4 files are covered.
• 3 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.01%) to 82.589%

Totals
Change from base Build 11118652309:	0.01%
Covered Lines:	19112
Relevant Lines:	23141

💛 - Coveralls

Hey @jaoleal, thanks for tackling this. I don't know how to review this PR for a few reasons:

1. The current bip32 module is really bad (public API and internal code)
2. This PR is really big (over 500 lines of green)
3. I don't know the final state you are trying to get to (this PR is labelled number 1)

Also, I don't know exactly how is the best way to approach revamping this module.

I definitely appreciate you working on this, the module sorely needs some love, but if I'm to have any hope of reviewing perhaps I can give you a few pointers:

• Do a series of smaller PRs, you can put them all up at once, just have the later ones include the patches from the earlier ones and mark them as draft.
• Also, be sure to separate out things that are really meaningful and hard to review (like how the top bit of hardened index is handled) from things that are easy to review (like renaming ChildNumber to ChildKeyIndex)

This is not easy work, and anything that involves bit twidling and invariants is non-trivial to review.

I understand your concern about the large diff size and appreciate your explanation.

Let me refactor my idea:

The extensive changes primarily stem from necessary renaming across the codebase and comprehensive documentation updates. While the diff appears substantial, the core functional changes are minimal, as evidenced by the small modifications to the examples demonstrating module usage.

I acknowledge that separating these changes into multiple commits might be preferable. However, the interdependencies between the refactoring steps make it REALLY HARD to create isolated, functional commits. The primary goal here is to enhance the module's robustness by transitioning from "enums with fields" to more secure "NewType structs with getters, setters, and constructors for each case."

This refactoring is crucial for improving the safety of key handling in Bip32, addressing potential vulnerabilities that could arise over time. The new design adheres to the principle of making it difficult for consumers to inadvertently misuse the API.

While this PR is part of a larger improvement effort (as discussed in #3036), it stands alone as a significant enhancement to the codebase's security. I've updated the PR title to better reflect its scope.

Looking ahead, I anticipate one more substantial PR implementing the NormalDerivationPath, which will build upon the security improvements introduced here.

From BIP32:

Each extended key has 2^31 normal child keys, and 2^31 hardened child keys. Each of these child keys has an index. The normal child keys use indices 0 through 2^(31-1). The hardened child keys use indices 2^31 through 2^(32-1). To ease notation for hardened key indices, a number i_H represents i+2^31.

So my read is that an "index" is a integer between 0 and 2^32. A number, which may be normal or hardened, is an integer between 0 and 2^31, and which index it corresponds to depends on whether it is normal or hardened.

So the existing type name ChildNumber sounds like it's correct. The new terms "hardened index" and "normal index" you are introducing do not appear anywhere in BIP32.

I understand your point, but I believe this is a somewhat superficial topic.

I’ll explain my reasoning behind the decision to replace all mentions of "number" and simplify everything to "index."

"Number" ends up being a generic term that appears a lot throughout the codebase, often referring to entities that have no relation to any "number" that could have an index and be used for deriving keys. Based on this, I began removing abstractions that seemed to only complicate understanding. I turned "ChildNumber" to "ChildKeyIndex" based on this thought process:

"I have an extended key and I have the index of its child key, which contains a value that can be either Hardened or Normal."

Two other minor points were:

• "Number" is rarely mentioned in this BIP, and even then, it’s not always referring to the same entity we’re discussing here. In other instances, it’s used to describe account numbers in BIP 32’s abstraction.

• The term "index" is used throughout the BIP and, unlike "number," is never used to describe anything else. Even the letter "i" is used as a variable in the pseudocode.

It just felt more natural, but as I said, I think this is a somewhat superficial point.

Naming in Bitcoin is a whole field of study in itself. I see a lot of issues with naming conventions in Bitcoin and all the alternatives always seem far from the concept.

Anyway, that’s the reasoning. I just wanted to clarify so it doesn’t seem like a change without thought behind it, but you’re the maintainer @apoelstra , so it’s your call!

Anyway, that’s the reasoning. I just wanted to clarify so it doesn’t seem like a change without thought behind it,

For sure. I understand that you thought through the reasoning of this and are trying to simplify the concepts in the API. But I still want to stick with child number", since it's widely used, while "child index" less so, and the BIP seems to use these terms in slightly different ways (which correspond to the existing names).

Weirdly, if I search for "bip32 child number" online I get many instances of the phrase "index number" which I have never heard of.

The term "index" is used throughout the BIP and, unlike "number," is never used to describe anything else. Even the letter "i" is used as a variable in the pseudocode

The term "child number" is the explicit name given to a field of an xkey. The term "child index" does not appear, only "index" by itself. And "index" is nearly as generic a term as "number", and in the context of a software library, is likely to appear more often in non-bip32 contexts.