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

[Wallet] keep HD seed during salvagewallet #8324

Merged
merged 1 commit into from Jul 14, 2016

Conversation

Projects
None yet
3 participants
@jonasschnelli
Member

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.

@jonasschnelli jonasschnelli added the Wallet label Jul 9, 2016

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 9, 2016

Member

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

Member

MarcoFalke commented Jul 9, 2016

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

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 11, 2016

Member

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

Member

MarcoFalke 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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 11, 2016

Member

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.

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.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 11, 2016

Member

Would be nice to have a test for this.

You can find the test in #8319

Member

MarcoFalke commented Jul 11, 2016

Would be nice to have a test for this.

You can find the test in #8319

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 11, 2016

Member

[...] 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.

Member

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.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke
Member

MarcoFalke commented Jul 13, 2016

utACK b993671

@laanwj laanwj merged commit b993671 into bitcoin:master Jul 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 14, 2016

Merge #8324: [Wallet] keep HD seed during salvagewallet
b993671 [Wallet] keep HD seed during salvagewallet (Jonas Schnelli)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 14, 2016

Member

code review ACK b993671

Member

laanwj commented Jul 14, 2016

code review ACK b993671

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment