Page MenuHomePhabricator
Differential D3950

Merge #12953: Deprecate accounts
AbandonedPublic
Actions

Authored by nakihito on Aug 26 2019, 20:51.

Details

Reviewers
deadalnix
Fabien
jasonbcox
markblundeberg
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

cead28b [docs] Add release notes for deprecated 'account' API (John Newbery)
72c9575 [wallet] [tests] Add tests for accounts/labels APIs (John Newbery)
109e05d [wallet] [rpc] Deprecate wallet 'account' API (John Newbery)
3576ab1 [wallet] [rpc] Deprecate account RPC methods (John Newbery)
3db1ba0 [tests] Set -deprecatedrpc=accounts in tests (John Newbery)
4e671f0 [tests] Rename rpc_listtransactions.py to wallet_listtransactions.py (John Newbery)
a28b907 [wallet] [rpc] Remove duplicate entries in rpcwallet.cpp's CRPCCommand table (John Newbery)

Pull request description:

Deprecate all accounts functionality and make it only accessible by using `-deprecatedrpc=accounts`.

Accounts specific RPCs, account arguments, and account related results all require the `-deprecatedrpc=accunts` startup option now in order to see account things.

Several wallet functional tests use the accounts system. Those tests are unchanged, except to start the nodes with `-deprecatedrpc=accounts`. We can slowly migrate those tests to use the 'label' API instead of the 'account' API before accounts are fully removed.

Tree-SHA512: 89f4ae2fe6de4a1422f1817b0997ae22d63ab5a1a558362ce923a3871f3e42963405d6573c69c27f1764679cdee5b51bf52202cc407f1361bfd8066d652f3f37

Backport of Core PR12953
https://github.com/bitcoin/bitcoin/pull/12953/

Depends on D3949 and D3997

Fixes duplicate help entry issues mentioned in discussion of D3308.

Test Plan
make check
test_runner.py

./bitcoind
./bitcoin-cli help

Verify that there are not duplicate entries for setlabel, getlabeladdress, getreceivedbylabel, and listreceivedbyaddress.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR12953
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7245
Build 12535: Bitcoin ABC Buildbot (legacy)
Build 12534: 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 reviewer: markblundeberg.Aug 26 2019, 20:53
nakihito mentioned this in D3308: Merge #9894: remove 'label' filter for rpc command help.
teamcity added a comment.Aug 26 2019, 20:59

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7245: Diff 10966.Aug 26 2019, 21:00
Fabien requested changes to this revision.Aug 27 2019, 10:08

Please fix the version numbers. Next major is 0.21 not 0.30, and there are some core numbers remaining.

This revision now requires changes to proceed.Aug 27 2019, 10:08
nakihito updated this revision to Diff 10971.Aug 27 2019, 19:41

Fixed incorrect version numbers.

teamcity added a comment.Aug 27 2019, 19:41

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 27 2019, 19:48

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7248: Diff 10971.Aug 27 2019, 19:48
nakihito updated this revision to Diff 11049.Aug 30 2019, 18:10

Rebased.

teamcity added a comment.Aug 30 2019, 18:10

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Aug 30 2019, 18:17

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7294: Diff 11049.Aug 30 2019, 18:18
jasonbcox requested changes to this revision.Sep 4 2019, 18:29
jasonbcox added inline comments.
src/wallet/rpcwallet.cpp
1040 ↗(On Diff #11049)

s/toavoid/to avoid/g

1074 ↗(On Diff #11049)

PR11050 has not been backported and the related changes here are missing.

1469 ↗(On Diff #11049)

The param size doesn't match. Is there a missing backport?

This revision now requires changes to proceed.Sep 4 2019, 18:29
nakihito planned changes to this revision.EditedSep 4 2019, 21:44

Nits fixed. Will rebase after D3997 is landed.

src/wallet/rpcwallet.cpp
1040 ↗(On Diff #11049)

Fixed.

1074 ↗(On Diff #11049)

Backport here: D3997

1469 ↗(On Diff #11049)

It is a missing backport, but one that I believe we purposely skipped: https://github.com/bitcoin/bitcoin/pull/10589/. Its from a line of changes that have to do with fee estimates.

nakihito edited the summary of this revision. (Show Details)Sep 5 2019, 22:48
nakihito added a parent revision: D3997: Merge #11050: Avoid treating null RPC arguments different from missing arguments.
nakihito updated this revision to Diff 13599.Oct 17 2019, 19:06

Rebased.

teamcity added a comment.Oct 17 2019, 19:07

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 17 2019, 19:18

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7802: Diff 13599.Oct 17 2019, 19:18
jasonbcox requested changes to this revision.Oct 18 2019, 18:26
jasonbcox added inline comments.
src/wallet/rpcwallet.cpp
2064 ↗(On Diff #13599)

brackets

2096 ↗(On Diff #13599)

brackets

2134 ↗(On Diff #13599)

brackets

5030 ↗(On Diff #13599)

missing |dummy

test/functional/rpc_deprecated.py
24 ↗(On Diff #13599)

This should be removed...

This revision now requires changes to proceed.Oct 18 2019, 18:26
nakihito updated this revision to Diff 13617.Oct 19 2019, 02:59

Rebase and fixed nits.

teamcity added a comment.Oct 19 2019, 02:59

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7814: Diff 13617.Oct 19 2019, 03:11
teamcity added a comment.Oct 19 2019, 03:11

Build Bitcoin-ABC / Diffs / Diff Testing passed.

jasonbcox accepted this revision.Oct 21 2019, 16:09
Fabien requested changes to this revision.Oct 25 2019, 06:51

Please rebase the stack on current master, to allow for running the tests.

src/wallet/rpcwallet.cpp
1100 ↗(On Diff #13617)

Part of PR missing.

1403 ↗(On Diff #13617)

There is no such thing in our codebase.

1406 ↗(On Diff #13617)

Dito.

1408 ↗(On Diff #13617)

Dito.

1418 ↗(On Diff #13617)

Of course the following code has to be adapted with the good arguments, including the examples.

test/functional/wallet_basic.py
482 ↗(On Diff #13617)

Revert spaces.

test/functional/wallet_labels.py
16 ↗(On Diff #13617)

0.21

test/functional/wallet_listsinceblock.py
14 ↗(On Diff #13617)

The layout can likely be improved.

test/functional/wallet_txn_clone.py
19 ↗(On Diff #13617)

Dito.

test/functional/wallet_txn_doublespend.py
21 ↗(On Diff #13617)

Dito.

This revision now requires changes to proceed.Oct 25 2019, 06:51
nakihito updated this revision to Diff 13712.EditedOct 25 2019, 20:35

Rebased, added missing part of PR, removed irrelevant help text, and fixed layout for tests.

nakihito planned changes to this revision.Oct 25 2019, 20:35
teamcity added a comment.Oct 25 2019, 20:35

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 25 2019, 20:46

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7875: Diff 13712.Oct 25 2019, 20:46
nakihito updated this revision to Diff 13729.Oct 28 2019, 17:14

Last bit of formatting.

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

Build Bitcoin-ABC / Diffs / Diff Testing started.

nakihito planned changes to this revision.Oct 28 2019, 17:15

Needs rebase.

teamcity added a comment.Oct 28 2019, 17:24

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7886: Diff 13729.Oct 28 2019, 17:24
nakihito updated this revision to Diff 13733.Oct 28 2019, 18:13

Rebased.

teamcity added a comment.Oct 28 2019, 18:13

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Oct 28 2019, 18:23

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7888: Diff 13733.Oct 28 2019, 18:23
Fabien requested changes to this revision.Oct 31 2019, 09:18

This will really benefit from splitting the diff, this is very long and difficult to review.

src/wallet/rpcwallet.cpp
1372 ↗(On Diff #13733)

replaceable conf_target \"estimate_mode\" are still here.

1428 ↗(On Diff #13733)

?

1438 ↗(On Diff #13733)

?

1446 ↗(On Diff #13733)

?
Is something wrong with your clang-format ?

1447 ↗(On Diff #13733)

Dito

1518 ↗(On Diff #13733)

Please make a complete pass on these unused argument and remove all related leftovers.

2176 ↗(On Diff #13733)

Dito

This revision now requires changes to proceed.Oct 31 2019, 09:18
nakihito planned changes to this revision.Oct 31 2019, 20:24

Going to try and break this into smaller pieces.

nakihito abandoned this revision.Nov 4 2019, 19:33

Broken into smaller, chunks for easier review. See D4361, D4368, D4369, and D4370.

Revision Contents
Changeset List

Diff 10966

View Options

doc/release-notes.md

Loading...
View Options

src/wallet/rpcwallet.cpp

Loading...
View Options

test/functional/rpc_deprecated.py

Loading...
View Options

test/functional/rpc_listtransactions.py

Loading...
View Options

test/functional/wallet_basic.py

Loading...
View Options

test/functional/wallet_import_rescan.py

Loading...
View Options

test/functional/wallet_importprunedfunds.py

Loading...
View Options

test/functional/wallet_keypool_topup.py

Loading...
View Options

test/functional/wallet_labels.py

Loading...
View Options

test/functional/wallet_listreceivedby.py

Loading...
View Options

test/functional/wallet_listsinceblock.py

Loading...
View Options

test/functional/wallet_listtransactions.py

Loading...
View Options

test/functional/wallet_txn_clone.py

Loading...
View Options

test/functional/wallet_txn_doublespend.py

Loading...