Skip to content

Fix Invalid Private Key Generation - #12

Merged
miguelmota merged 1 commit into
miguelmota:masterfrom
witoff:master
Mar 14, 2021
Merged

miguelmota merged 1 commit into
miguelmota:masterfrom
witoff:master

Conversation

@witoff

@witoff witoff commented Mar 2, 2021 •

Copy link
Copy Markdown
Contributor

Fixes #10 by upgrading btcutil dependencies.

This issue was tracked in the upstream btcsuite/btcutil dependency at btcsuite/btcutil#172

When a child key has less than 32 bytes, it is incorrectly padded leading to incorrect key derivation. This effects 1/256 hardened key derivations and twice is many (1/128) accounts with two hardened component. Upgrading dependencies replaces key.Child with key.Derive which correctly pads addresses.

Note that merging this will fix derivation, but break existing implementations for ~1/128 keys. btcsuite addressed this by adding a IsAffectedByIssue172() bool function and changing Child() to Derive() to break the default interface and force implementers to address as needed and I suggest taking a similar approach.

@miguelmota

Copy link
Copy Markdown
Owner

Hi @witoff! Thank you for the detailed explanation and PR. Looks good and happy to merge it. I like your suggestion of adding a DeriveNonStandard method to keep backward compatibility.

@witoff

witoff commented Mar 5, 2021

Copy link
Copy Markdown
Contributor Author

@miguelmota updated so that current functionality doesn't break. Since this is used by many upstream libraries, users will want a way to enable correct behavior without requiring everyone else to update so i also added envar support w/ tests.

// This works as it did before.
account, err := wallet.Derive(path, false)

// This derives correctly
os.Setenv(issue179FixEnvar, "1")
account, err = wallet.Derive(path, false)

// This also derives correctly
wallet.SetFixIssue172(true)
account, err = wallet.Derive(path, false)

Can you merge or advise on any other changes you think are needed?

@miguelmota

Copy link
Copy Markdown
Owner

@witoff This is perfect! Awesome work. Merging.

@miguelmota
miguelmota merged commit f39b32a into miguelmota:master Mar 14, 2021
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

Successfully merging this pull request may close these issues.

Invalid private key generation

2 participants