Page MenuHomePhabricator

Rename master key to seed
ClosedPublic

Authored by nakihito on Oct 1 2019, 16:30.

Details

Reviewers
deadalnix
Fabien
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rSTAGING4d0e21a6bdea: Rename master key to seed
rABC4d0e21a6bdea: Rename master key to seed
Summary

131d4450b scripted-diff: Rename master key to seed (John Newbery)
This only includes non-behavioral changes.

Commit Description:

-BEGIN VERIFY SCRIPT-

ren() { git grep -l "\<$1\>" 'src/*.cpp' 'src/*.h' test | xargs sed -i "s:\<$1\>:$2:g"; }
ren GenerateNewHDMasterKey  GenerateNewSeed
ren DeriveNewMasterHDKey    DeriveNewSeed
ren SetHDMasterKey          SetHDSeed
ren hdMasterKeyID           hd_seed_id
ren masterKeyID             seed_id
ren SetMaster               SetSeed
ren hdmaster                hdseed

-END VERIFY SCRIPT-

Partial backport of Core PR12924
https://github.com/bitcoin/bitcoin/pull/12924/commits/131d4450b913fa4f00dc8b176dc996d39f786c19

Depends on D4207

Test Plan
make check
test_runner.py

grep for the above master(key) variations. Only inactivemasterkey should show up in rpcdump.cpp and wallet_dump.py, and hdmasterkey in rpcdump.cpp and wallet_dump.py.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nakihito requested review of this revision.Oct 1 2019, 17:10

Separated from D4175.

deadalnix requested changes to this revision.Oct 1 2019, 18:49
deadalnix added inline comments.
src/wallet/rpcwallet.cpp
4355 ↗(On Diff #13294)

Reease notes

This revision now requires changes to proceed.Oct 1 2019, 18:49
nakihito requested review of this revision.Oct 2 2019, 01:38
deadalnix requested changes to this revision.Oct 6 2019, 18:05
deadalnix added inline comments.
doc/release-notes.md
8 ↗(On Diff #13305)

grepping through the doc history, the V0.21 nomenclature was never used.

Second there doesn't seem to be a test for hdmasterkeyid anywhere, so how do we know it works at all? (glancing at the diff, I strongly suspect it doesn't).

This revision now requires changes to proceed.Oct 6 2019, 18:05
nakihito requested review of this revision.Oct 8 2019, 00:42
nakihito added inline comments.
doc/release-notes.md
8 ↗(On Diff #13305)

I don't understand what you what you mean by the first part of your comment.

As for the second part of you comment, this is purely the scripted commit as you requested I split before (https://reviews.bitcoinabc.org/D4175#100125). The tests are included in the other half of this backport (see D4175).

deadalnix requested changes to this revision.Oct 8 2019, 17:09
deadalnix added inline comments.
doc/release-notes.md
8 ↗(On Diff #13305)

So this doesn't do what is written int he release notes, actually breaks backward compatibility. The mass rename can only occurs after dual naming is introduced. You should have realized this when putting release notes in there that do not match what the patch is doing, if not when doing the patch itself.

This revision now requires changes to proceed.Oct 8 2019, 17:09

The behavioral changes made by this diff will be moved to D4175.

Removed behavioral changes and relevant release notes (these were moved to child diff D4175).

Rebased and fixed some formatting.

nakihito requested review of this revision.Oct 9 2019, 23:17
nakihito edited the summary of this revision. (Show Details)
nakihito edited the test plan for this revision. (Show Details)
jasonbcox added inline comments.
test/functional/wallet_keypool.py
34 ↗(On Diff #13480)

nit: it's unnecessary to include these format changes

Formatting changes to wallet_keypool.py undone.

deadalnix requested changes to this revision.Oct 15 2019, 13:46

The diff doesn't match the description anymore. It's also unclear to me why the double outputs wasn't stacked first.

This revision now requires changes to proceed.Oct 15 2019, 13:46

The diff doesn't match the description anymore. It's also unclear to me why the double outputs wasn't stacked first.

Summary fixed. One coming before the other does not matter. I stacked it this way because its the order that the commits were authored in.

deadalnix requested changes to this revision.Oct 17 2019, 01:36

Where are the other rename done, then ? I don't see a patch in the stack doing that. I don't understand why the double write doesn't come first, then the scripted diff? The way this is done is exceedingly difficult to review.

This revision now requires changes to proceed.Oct 17 2019, 01:36
This revision is now accepted and ready to land.Oct 29 2019, 22:48