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

Fixed incorrect version numbers.

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.

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

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

Needs rebase.

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

Going to try and break this into smaller pieces.

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