Skip to content
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 88: Templates for Hierarchical Deterministic derivation paths #1025

Merged
merged 8 commits into from
May 17, 2021

Conversation

dgpv
Copy link
Contributor

@dgpv dgpv commented Oct 26, 2020

Was first announced in https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2020-July/018024.html

Motivation and descriptions of usecases are given in the BIP draft, in the mailing-list post, and expanded in the replies to this mailing-list post and in Bitcoin Optech newsletter 105.

While this draft BIP did not receive extensive public discussions, it did not receive any rejections or criticism either. Within private or semi-private discussions, people said that they like the idea, noted the usefulness of this format and having it standardized. I attribute the lack of public discussions to the template format being boring but practical, and thus uncontroversial. I think that having this as a formal standard will be a benefit to the Bitcoin-related software and hardware ecosystem. I hope that submitting this as PR will bring more discussion and comments.

There are three implementations, including two reference implementations: in C and in Python (that is also compatible with micropython to be suitable for embedded applications). The implementations are thoroughly tested with the test data generated from the formal specification, and thus their behaviour follows the formal spec closely.

My thanks to @Enegnei for the help in fixing grammatical and punctuation errors in the early draft.

@dgpv dgpv force-pushed the bip-path-templates branch 2 times, most recently from f999930 to fce877f Compare October 27, 2020 03:56
@luke-jr
Copy link
Member

luke-jr commented Feb 3, 2021

Missing a backward compatibility section

@luke-jr luke-jr added the New BIP label Feb 3, 2021
@dgpv
Copy link
Contributor Author

dgpv commented Feb 4, 2021

IIUC, backward compatibility applies if there's a prior standard or software that implements some prior (maybe de-facto) standard. I am not aware of any prior standard or any software that implements BIP32 path templates beyond the software that is linked in the BIP text (and that linked software is compatible with the spec).

The https://walletsrecovery.org/ website uses their own format to describe derivation paths, but it seems to me the format they use is intended for illustration rather than being processed by software. The format https://walletsrecovery.org/ uses to illustrate paths is obviously not compatible, but it was not a "previous spec" to this spec in any way, and I doubt that this fact need to be explicitly stated in the BIP.

BIP2 says "Motivation and backward compatibility (when applicable) must be addressed". Since there's no prior specs or implementation, it seems to me that backward compatibility is not applicable here.

@dgpv
Copy link
Contributor Author

dgpv commented Feb 4, 2021

If needed, I can add a note after the link to walletrecovery.org saying that their format is not compatible. But as the website is not a static document like the BIP, they can change their format at any time, and this would mean the information in the BIP may become out of date.

@luke-jr
Copy link
Member

luke-jr commented Feb 12, 2021

Sounds like a plan. Maybe just reference their current thing?

@dgpv dgpv force-pushed the bip-path-templates branch 6 times, most recently from 8adb255 to bb217ea Compare February 15, 2021 09:41
@dgpv
Copy link
Contributor Author

dgpv commented Feb 15, 2021

Added the "Known solutions" section to "Motivation" part.

@dr-orlovsky
Copy link
Contributor

dr-orlovsky commented Feb 18, 2021

The proposed encoding for the index ranges will not work very well with current way how descriptors are structured in Bitcoin Core and rust-miniscript. In particular, square bracket notation [...] is already widely used in these implementation (and in many other software) for introducing public keys (XpubIndetifirer, fingerprints, extended private and public keys etc). So I proposet:

  1. Avoid square brackets for index paths and use simple ranges (like m/5-16'/.. etc).
  2. Allow embedding a public key information into derivation paths as already done by Bitcoin Core descriptors and miniscript using squared box notation. If a public key is given inside the path, use = separator (m/47'/5'=[xpubkey]/0-1/*)

With this such a BIP will be more interoperable with existing software and will allow keeping track of derivation paths which can be used without knowledge of extended private keys (when the last hardened index is provided with corresponding extended public key).

I have a reference Rust implementation for the format above here: https://github.com/LNP-BP/descriptor-wallet/tree/master/src/bip32 (path.rs, range.rs and xpubref.rs files). It is rust-miniscript compatible already used in the software:

@dgpv
Copy link
Contributor Author

dgpv commented Feb 19, 2021

Avoid square brackets for index paths and use simple ranges (like m/5-16'/.. etc).

Brackets were added for better visual identification of ranges, for readability and less ambiguity.

I think that m/[5-16]'/0'/[1,2]' or m/{5-16}'/0'/{1,2}' is easier to parse for human brain than m/5-16'/0'/1,2', and there's no ambiguity that the hardened marker ' applies to the range and not only to the index 16 or 2. Of course for software this might not matter, but for humans working with ranges IMO this will matter. Also there's no confusion of how you should write /1',2'/ or 1,2' - while for software 'hardened marker is at the end of the range' is unambiguous, for humans it will be more confusing. Allowing to write this both ways means that the formal spec and therefore correct implementations will be more complex.

Allow embedding a public key information into derivation paths as already done by Bitcoin Core descriptors and miniscript using squared box notation. If a public key is given inside the path, use = separator (m/47'/5'=[xpubkey]/0-1/*)

This will complicate the formal spec by requiring to either add xpub parsing or at least maintaining additional storage for arbitrary strings found between [ and ]. The goal of this BIP is to be complete and unambiguous, adding complexity means that the spec will grow and will be much harder to analyze (the number of states in FSM grows very fast when adding complexity)

AFAIK squared box notation is only used by Core in descriptors to denote master key fingerprints, not xpubs. The miniscript spec at http://bitcoin.sipa.be/miniscript/ also does not have any usage of square brackets to delimit keys.

@dr-orlovsky
Copy link
Contributor

dr-orlovsky commented Feb 19, 2021

Well, it's not about convenience, it is mostly about compatibility. Bitcoin Core currently use square brackets for xpub identifiers and fingerprints, so this renders standard already incompatible. Rust miniscript implementation uses them even more widely (not the site, but the rust code which is already part of Square Crypto BDK) – and I know about ongoing work of making that part of Bitcoin Core as well. So without this this standard will be just another standard contradicting existing "de facto" standards, not used by anybody other than author.

Without this compatibility I am concept NACK it

@dgpv
Copy link
Contributor Author

dgpv commented Feb 19, 2021

The square brackets can be replaced with curly brackets without problem, I think. In bitcoin/bitcoin#17190 curly brackets were used, for example.

The spec won't be any more complex without brackets at all. But I think that visually, bracket-less templates are more confusing. Having explicit grouping is better for UX when working with templates. This might be just my perception, though. Would be great to have more opinions on this.

Including 'correct xpubs' is out of the question, because this means that the formal spec will either be incomplete, or very complex. It might be useful and relatively simple spec-wise to allow arbitrary strings (with limited charset), and then xpubs or other interpretations of these strings might be defined in a separate BIP. Might be useful for labels/comments, for example: m/{44,49,84}'=@purpose/0'=xpubABCDEF/0'/{0-1}/{0-50000}=@should_be_enough. The parsing algorithm will then produce label=>position mapping like "@purpose": 0, "xpubABCDEF": 1, "@should_be_enough": 4. For the spec, these will be just arbitrary strings with limited charset starting from = and ending at /

@dgpv
Copy link
Contributor Author

dgpv commented Feb 19, 2021

Adding these arbitrary strings will increase the number of states to check when running TLC with the formal spec, and the amount of generated test data for comprehensive tests, so I'm not sure if it is worth it.

@dr-orlovsky
Copy link
Contributor

I am fine with curly (or any other) brackets unless they are squared, which breaks compatibility :) Let's hear other opinions.

As for the annotations, I doubt that we need arbitrary strings. We need to standardize already present [fingerprint] and [xpubid] in bitcoin core and it will be nice to extend it with a single (standard) encoding of full xpub data. Not sure why it will be hard to make formal: we just reference encoding from BIP-32.

@dgpv
Copy link
Contributor Author

dgpv commented Feb 19, 2021

why it will be hard to make formal:

formal in the sense of using a formal specification language, in this case TLA+ (the spec is referenced in the BIP text: https://github.com/dgpv/bip32_template_parse_tplaplus_spec)

@dgpv
Copy link
Contributor Author

dgpv commented Feb 19, 2021

Right now the prose spec (the BIP) corresponds to the TLA+ spec, and generated test data from the run of TLC on the TLA+ spec can be used to comprehensively test the reference implementations. If the notion of xpub is added and it is stated that it should be the correct xpub, the prose/formal spec correspondence will not be complete, because it is infeasible to implement a formal spec of xpub format for this purpose. The formal spec will either need to ignore the xpub part (and the comprehensiveness of the tests for reference implementations will be diminished), or have a simplified version - that is, the 'arbitrary string of a limited charset'.

@prusnak
Copy link
Contributor

prusnak commented Feb 19, 2021

Thank you @dgpv for bringing this BIP effort up again. We already use your proposal in Trezor for internal purposes: https://github.com/trezor/trezor-firmware/blob/master/core/src/apps/common/paths.py

We use almost exactly the same syntax with one extension:

    - m/* can be followed by any number of _unhardened_ path components
    - m/*' can be followed by any number of _hardened_ path components
    - m/** can be followed by any number of _any_ path components

@dgpv
Copy link
Contributor Author

dgpv commented Feb 19, 2021

I think that

any number of path components

is dangerous when using it for path restrictions (as 'can we allow a change output to go to this path') because it creates a possibility of some arbitrary paths to pass the 'filter' of the template, and create a situation where change amount can be held ransom, for example. Because of this, I made decision to use * to denote only a range of indexes at one level, rather than 'any path with matching prefix'.

What is the usecase for allowing an arbitrary-length path tail ? Is it justified vs the dangers it can enable ? Can it be substituted with just using different match functions (full_match vs prefix_match) ?

@dgpv
Copy link
Contributor Author

dgpv commented Feb 19, 2021

edited the above comment: 'can we allow to sign for this path' -> 'can we allow a change output to go to this path' because that's what I meant actually.

@dgpv
Copy link
Contributor Author

dgpv commented Feb 21, 2021

@prusnak what do you think about switching from square brackets to curly brackets to denote index ranges ? (on the reason that square brackets are used for other purposes in descriptors)

@prusnak
Copy link
Contributor

prusnak commented Feb 21, 2021

@prusnak what do you think about switching from square brackets to curly brackets to denote index ranges ? (on the reason that square brackets are used for other purposes in descriptors)

I don't mind this

@luke-jr
Copy link
Member

luke-jr commented Apr 25, 2021

@dgpv Were you going to add a bit on compatibility?

@luke-jr luke-jr changed the title Add BIP draft for BIP32 path templates BIP 88: Templates for Hierarchical Deterministic derivation paths May 14, 2021
@luke-jr
Copy link
Member

luke-jr commented May 14, 2021

Assigned BIP number 88. Please rename & update README

@dgpv dgpv force-pushed the bip-path-templates branch 2 times, most recently from 2852cbe to 8f92f60 Compare May 15, 2021 06:17
@dgpv
Copy link
Contributor Author

dgpv commented May 15, 2021

Done. @luke-jr

@dgpv
Copy link
Contributor Author

dgpv commented May 15, 2021

I dropped the word "derivation" from the title to fit into 44 max chars limit.

@Rspigler
Copy link
Contributor

Strong Concept ACK

@Rspigler
Copy link
Contributor

@dgpv Trying to go through this, have some questions. Are you on IRC?

@dgpv
Copy link
Contributor Author

dgpv commented May 15, 2021

I think you can just ask here, in a comment at relevant line of the file.

I've changed the status from "Draft" to "Proposed", because it seemed that the only disagreement that were emerged was about {} vs [], and it is settled now. But if you had thoughts that may warrant further discussion, then maybe it should stay at the "Draft" status a bit longer...

bip-0088.mediawiki Outdated Show resolved Hide resolved
@Rspigler
Copy link
Contributor

I think it is my misunderstanding (and I don't want to crowd the PR), but I will comment here, thanks

bip-0088.mediawiki Outdated Show resolved Hide resolved
bip-0088.mediawiki Outdated Show resolved Hide resolved
@luke-jr luke-jr merged commit 868c4f8 into bitcoin:master May 17, 2021
@luke-jr
Copy link
Member

luke-jr commented May 17, 2021

Further changes can be added in additional PRs

@Rspigler
Copy link
Contributor

ACK commit abc113e

Happy with all the changes!

@dgpv
Copy link
Contributor Author

dgpv commented May 18, 2021

Thanks!

I didn't add the Acknowledgements section in time before the BIP88 was merged, doing it in a separate PR: #1123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants