Page MenuHomePhabricator
Differential D4190

Rename master key to seed
ClosedPublic
Actions

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
Branch
PR12924-script
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7764
Build 13567: Bitcoin ABC Buildbot (legacy)
Build 13566: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
nakihito added a parent revision: D4171: Merge #12560: [wallet] Upgrade path for non-HD wallets to HD.Oct 1 2019, 16:39
nakihito added a child revision: D4175: Merge #12924: Fix hdmaster-key / seed-key confusion.
teamcity added a comment.Oct 1 2019, 16:48

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7648: Diff 13294.Oct 1 2019, 16:48
nakihito requested review of this revision.Oct 1 2019, 17:10

Separated from D4175.

nakihito edited the test plan for this revision. (Show Details)Oct 1 2019, 17:11
nakihito mentioned this in D4175: Merge #12924: Fix hdmaster-key / seed-key confusion.Oct 1 2019, 17:14
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 updated this revision to Diff 13305.Oct 1 2019, 19:19

Added release notes.

nakihito planned changes to this revision.Oct 1 2019, 19:19
teamcity added a comment.Oct 1 2019, 19:19

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 1 2019, 19:21

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 1 2019, 19:22

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7654: Diff 13305.Oct 1 2019, 19:41
teamcity added a comment.Oct 1 2019, 19:41

Build Bitcoin-ABC / Diffs / Diff Testing passed.

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 edited the summary of this revision. (Show Details)Oct 6 2019, 18:30
nakihito added a parent revision: D4207: Bump wallet version for pre split keypool.
nakihito removed a parent revision: D4171: Merge #12560: [wallet] Upgrade path for non-HD wallets to HD.
nakihito edited the test plan for this revision. (Show Details)Oct 7 2019, 23:35
nakihito updated this revision to Diff 13400.Oct 8 2019, 00:19

Rebased.

nakihito planned changes to this revision.Oct 8 2019, 00:19
teamcity added a comment.Oct 8 2019, 00:20

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 8 2019, 00:21

Build Bitcoin-ABC / Diffs / Diff Testing failed.

Harbormaster failed remote builds in B7715: Diff 13400!Oct 8 2019, 00:21
nakihito updated this revision to Diff 13401.Oct 8 2019, 00:30

Rebased.

nakihito planned changes to this revision.Oct 8 2019, 00:30
teamcity added a comment.Oct 8 2019, 00:31

Build Bitcoin-ABC / Diffs / Diff Testing started.

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).

teamcity added a comment.Oct 8 2019, 00:45

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7716: Diff 13401.Oct 8 2019, 00:45
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
nakihito planned changes to this revision.Oct 9 2019, 01:15
nakihito added a comment.Oct 9 2019, 01:25

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

nakihito updated this revision to Diff 13420.Oct 9 2019, 18:13

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

teamcity added a comment.Oct 9 2019, 18:14

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 9 2019, 18:15

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 9 2019, 18:29

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7727: Diff 13420.Oct 9 2019, 18:29
nakihito planned changes to this revision.Oct 9 2019, 22:33
nakihito updated this revision to Diff 13480.Oct 9 2019, 23:00

Rebased and fixed some formatting.

nakihito planned changes to this revision.Oct 9 2019, 23:00
teamcity added a comment.Oct 9 2019, 23:00

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 9 2019, 23:15

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7731: Diff 13480.Oct 9 2019, 23:15
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 accepted this revision.Oct 14 2019, 03:43
jasonbcox added inline comments.
test/functional/wallet_keypool.py
34 ↗(On Diff #13480)

nit: it's unnecessary to include these format changes

nakihito updated this revision to Diff 13540.Oct 14 2019, 20:09

Formatting changes to wallet_keypool.py undone.

teamcity added a comment.Oct 14 2019, 20:14

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 14 2019, 20:28

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7764: Diff 13540.Oct 14 2019, 20:29
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
nakihito edited the summary of this revision. (Show Details)Oct 15 2019, 17:28
nakihito edited the test plan for this revision. (Show Details)Oct 15 2019, 17:30
nakihito requested review of this revision.Oct 16 2019, 02:06
In D4190#102420, @deadalnix wrote:

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
nakihito updated this revision to Diff 13637.Oct 22 2019, 00:18

Rebased.

nakihito planned changes to this revision.Oct 22 2019, 00:18
teamcity added a comment.Oct 22 2019, 00:19

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7829: Diff 13637.Oct 22 2019, 00:32
teamcity added a comment.Oct 22 2019, 00:32

Build Bitcoin-ABC / Diffs / Diff Testing passed.

nakihito requested review of this revision.Oct 25 2019, 20:47
deadalnix accepted this revision.Oct 29 2019, 22:48
This revision is now accepted and ready to land.Oct 29 2019, 22:48
Closed by commit rABC4d0e21a6bdea: Rename master key to seed (authored by Jonas Schnelli <dev@jonasschnelli.ch>, committed by nakihito). · Explain WhyOct 29 2019, 23:39
This revision was automatically updated to reflect the committed changes.
Nico Guiton <nico@bitframe.org> added a commit: rABC4d0e21a6bdea: Rename master key to seed.
nakihito added a commit: rSTAGING4d0e21a6bdea: Rename master key to seed.Apr 7 2020, 21:31

Revision Contents
Changeset List

PathSize
src/
2 lines
2 lines
test/
2 lines
wallet/
10 lines
16 lines
6 lines
30 lines
12 lines

Diff 13540

View Options

src/key.h

Loading...
View Options

src/key.cpp

Loading...
View Options

src/test/bip32_tests.cpp

Loading...
View Options

src/wallet/rpcdump.cpp

Loading...
View Options

src/wallet/rpcwallet.cpp

Loading...
View Options

src/wallet/wallet.h

Loading...
View Options

src/wallet/wallet.cpp

Loading...
View Options

src/wallet/walletdb.h

Loading...