Skip to content

[Wallet] keep HD seed during salvagewallet#8324

Merged
laanwj merged 1 commit intobitcoin:masterfrom
jonasschnelli:2016/07/hd_salvage
Jul 14, 2016
Merged

[Wallet] keep HD seed during salvagewallet#8324
laanwj merged 1 commit intobitcoin:masterfrom
jonasschnelli:2016/07/hd_salvage

Conversation

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Jul 9, 2016

Calling -salvagewallet (CWalletDB::Recover in keys only mode) will result in dropping the information which private key is used for hd key derivation (and also loses the child key counter).

This simple PR would try to keep the hdchain record if it was available during the time when -salvagewallet was called.

@maflcko
Copy link
Member

maflcko commented Jul 9, 2016

Thanks! I can confirm that b993671 fixes the travis failure in #8319.

@maflcko
Copy link
Member

maflcko commented Jul 11, 2016

This feels like a bug fix, which should be release as part of 0.13.0... Am I wrong?

@laanwj laanwj added this to the 0.13.0 milestone Jul 11, 2016
@laanwj
Copy link
Member

laanwj commented Jul 11, 2016

I agree this is a bug fix. I don't think -salvagewallet dropping the master key is in any way expected behavior. Would be nice to have a test for this.

@maflcko
Copy link
Member

maflcko commented Jul 11, 2016

Would be nice to have a test for this.

You can find the test in #8319

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Jul 11, 2016

[...] I don't think -salvagewallet dropping the master key is in any way expected behavior. [...]

At the moment, the private key itself will not be dropped, only the information which of the private keys is used as HD seed. Without that information, current masters wallet will create another private key and uses this one as the HD seed. This will result in two different HD chains once you call -salvagewallet.

Yes. It's a bugfix and It probably should go into 0.13, though I think #8323 is more important.

@maflcko maflcko mentioned this pull request Jul 12, 2016
Closed
@maflcko
Copy link
Member

maflcko commented Jul 13, 2016

utACK b993671

@laanwj laanwj merged commit b993671 into bitcoin:master Jul 14, 2016
laanwj added a commit that referenced this pull request Jul 14, 2016
@laanwj
Merge #8324: [Wallet] keep HD seed during salvagewallet
bc94b87 
b993671 [Wallet] keep HD seed during salvagewallet (Jonas Schnelli)
@laanwj
Copy link
Member

laanwj commented Jul 14, 2016

code review ACK b993671

@TheBlueMatt TheBlueMatt mentioned this pull request Jul 19, 2016
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

0.13.0

Development

Successfully merging this pull request may close these issues.

3 participants

@jonasschnelli @maflcko @laanwj