Page MenuHomePhabricator

Merge #12953: Deprecate accounts
AbandonedPublic

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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 7888
Build 13790: Bitcoin ABC Buildbot
Build 13789: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.

nakihito updated this revision to Diff 11049.Aug 30 2019, 18:10

Rebased.

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 updated this revision to Diff 13599.Thu, Oct 17, 19:06

Rebased.

jasonbcox requested changes to this revision.Fri, Oct 18, 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.Fri, Oct 18, 18:26
nakihito updated this revision to Diff 13617.Sat, Oct 19, 02:59

Rebase and fixed nits.

jasonbcox accepted this revision.Mon, Oct 21, 16:09
Fabien requested changes to this revision.Fri, Oct 25, 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.Fri, Oct 25, 06:51
nakihito updated this revision to Diff 13712.EditedFri, Oct 25, 20:35

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

nakihito planned changes to this revision.Fri, Oct 25, 20:35
nakihito updated this revision to Diff 13729.Mon, Oct 28, 17:14

Last bit of formatting.

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

Needs rebase.

nakihito updated this revision to Diff 13733.Mon, Oct 28, 18:13

Rebased.

Fabien requested changes to this revision.Thu, Oct 31, 09:18

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

src/wallet/rpcwallet.cpp
1372

replaceable conf_target \"estimate_mode\" are still here.

1428

?

1438

?

1446

?
Is something wrong with your clang-format ?

1447

Dito

1518

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

2176

Dito

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

Going to try and break this into smaller pieces.

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

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