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

psbt: confusing encoding (LE uint32) of MasterKeyFingerprint #1796

Open
kallerosenbaum opened this issue Apr 15, 2020 · 3 comments
Open

psbt: confusing encoding (LE uint32) of MasterKeyFingerprint #1796

kallerosenbaum opened this issue Apr 15, 2020 · 3 comments

Comments

@kallerosenbaum
Copy link

The master key fingerprint in BIP174 records are byte arrays of size 4, and in descriptors they are represented by 8 hex characters. These are the first four bytes of HASH160(pubkey).

bip32.go uses little-endian when parsing the master key fingerprint into an uint32. This confuses the user (me :-P), because one would expect Bip32Derivation.MasterKeyFingerprint to be represented as a [4]byte or at least a big-endian uint32.

I suggest using [4]byte instead, because the fingerprint is a sequence of four bytes.

If the current encoding must remain because of external dependencies, Bip32Derivation.MasterKeyFingerprint should at least be documented as being little-endian.

@kallerosenbaum
Copy link
Author

PS. I'd be happy to make a PR for the [4]byte solution if wanted.

@onyb
Copy link
Collaborator

onyb commented May 5, 2020

I think the confusion comes from the concatenation of the master key fingerprint with uint32 little-endian integers, in BIP174. There is no mention of the byte-ordering of the parent key fingerprint in the BIP32 spec, so assuming little-endianness is not right.

On the contrary, there is an implementation of BIP32 in the codebase which uses big-endian when parsing the fingerprint into a uint32 integer.

It seems we should be able to come up with a failing test. I don't know if the [4]byte solution is better or the big-endian uint32 one; I suggest coming up with a PR and then discuss it during the review.

@Roasbeef Roasbeef transferred this issue from btcsuite/btcutil Jan 29, 2022
@Roasbeef
Copy link
Member

Transferred from the old btcutil repo.

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

No branches or pull requests

3 participants