[wallet] Kill accounts #13825

pull jnewbery wants to merge 18 commits into bitcoin:master from jnewbery:kill_accounts changing 12 files +45 −447

jnewbery commented at 9:53 PM on July 31, 2018: member

To make space for new words, it's time to eliminate a word that has fallen into disuse: accounts. We make it fade into the night of time.

RIP accounts.

Completes #12952
jnewbery force-pushed on Jul 31, 2018
MarcoFalke added this to the milestone 0.18.0 on Jul 31, 2018
jnewbery force-pushed on Jul 31, 2018
jnewbery force-pushed on Jul 31, 2018
fanquake added the label Wallet on Jul 31, 2018
DrahtBot commented at 11:30 PM on July 31, 2018: member
Note to reviewers: This pull request conflicts with the following ones:
- #14021 (Import key origin data through importmulti by achow101)
- #13462 (Simplify common case of CHashWriter and drop SerializeHash by Empact)
- #11652 (Add missing locks: validation.cpp + related by practicalswift)
- #11634 (wallet: Add missing cs_wallet/cs_KeyStore locks to wallet by practicalswift)
- #10973 (Refactor: separate wallet from node by ryanofsky)
- #10785 (Serialization improvements by sipa)
- #10102 ([experimental] Multiprocess bitcoin by ryanofsky)
- #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
laanwj commented at 2:55 PM on August 2, 2018: member

Bon débarras!

Concept ACK, looks good in a first skim of commits.
DrahtBot added the label Needs rebase on Aug 8, 2018
jnewbery force-pushed on Aug 14, 2018
jnewbery commented at 11:11 AM on August 14, 2018: member

Rebased.

V0.17 is branched. This is now ready for review/merge.
jnewbery renamed this:
~~[wallet] [Do not merge until v0.18] Kill accounts~~
[wallet] Kill accounts
on Aug 14, 2018
DrahtBot removed the label Needs rebase on Aug 14, 2018
meshcollider commented at 1:29 PM on August 14, 2018: contributor

I think the general agreement towards this is clear but Concept ACK regardless. Will try and find time to review this soon
jnewbery commented at 2:30 PM on August 14, 2018: member

Perec redacted to remove any potential copyright material from git logs 😢
DrahtBot added the label Needs rebase on Aug 21, 2018
jnewbery force-pushed on Aug 21, 2018
jnewbery commented at 6:54 PM on August 21, 2018: member

rebased
DrahtBot removed the label Needs rebase on Aug 21, 2018
DrahtBot added the label Needs rebase on Aug 22, 2018
jnewbery commented at 1:03 PM on August 22, 2018: member

Thanks @DrahtBot . Rather than continually rebase this large PR, I've split the first three commits into #14023.

Reviewers, please review that first.
jnewbery renamed this:
~~[wallet] Kill accounts~~
[WIP] [wallet] Kill accounts
on Aug 22, 2018
laanwj referenced this in commit f180e81d57 on Aug 27, 2018

[wallet] Remove 'account' argument from GetLegacyBalance()

GetLegacyBalance() is never called with an account argument.
Remove the argument and helper functions.

0629de4919

[wallet] Remove CWallet::ListAccountCreditDebit()
Function no longer used.
e6fcc5d396
[wallet] Remove AccountMove()
Function no longer used.
ad63e01724
[wallet] Remove AddAccountingEntry()
Function no longer used.
cf15ebd80c
[wallet] Remove GetAccountCreditDebit()
Function no longer used.
7a0d8f49b3

[wallet] Don't rewrite accounting entries when...

reordering wallet transactions.

Accounting entries are deprecated. Don't rewrite them to the wallet
database when re-ordering transactions.

3f248a873f

[wallet] Remove WriteAccountingEntry()
Function no longer used.
f992f7fa3e
[wallet] Don't read acentry key-values from wallet on load. a544ab2857
[wallet] Remove ListAccountCreditDebit()
Function no longer used.
b7fd01a553
[wallet] Remove CAccountingEntry class
No long used
7c6f1a1f95
[wallet] Remove GetLabelDestination
Function no longer used.
53038b47ec

[wallet] Delete unused account functions

Deletes:
- ReadAccount
- WriteAccount
- EraseAccount
- DeleteLabel

f85988ca74

[wallet] Remove fromAccount argument from CommitTransaction() 0bb1b4e229
[wallet] Remove strFromAccount.
No longer used.
1031fa0c53
[wallet] Remove strSentAccount from GetAmounts().
No longer used.
87e8123539
[wallet] Update zapwallettxes comment to remove accounts. a92bdd1749
[wallet] Remove CAccount
No longer used
3053884696
jnewbery force-pushed on Aug 27, 2018
jnewbery renamed this:
~~[WIP] [wallet] Kill accounts~~
[wallet] Kill accounts
on Aug 27, 2018
jnewbery commented at 4:07 PM on August 27, 2018: member

The 'account' RPC has been removed in #14023. This PR removes all dead code left behind by that PR.
DrahtBot removed the label Needs rebase on Aug 27, 2018
promag commented at 8:54 PM on August 28, 2018: member

Care to fix #14023 (review)?
laanwj commented at 1:34 PM on August 29, 2018: member

utACK 30538846964e8c7490f94d0b57580e65aaf9c5a9 agree fixing @promag's nit would be nice
[docs] fix typo in release notes for PR 14023 85ec8d645f
jnewbery commented at 6:59 PM on August 29, 2018: member

Thanks @laanwj & @promag . Typo fixed in https://github.com/bitcoin/bitcoin/pull/13825/commits/85ec8d645f9aace793514ab36d6cc7a5382f3e7c
laanwj commented at 7:34 AM on August 30, 2018: member

re-utACK 85ec8d6
jonasschnelli commented at 7:35 AM on August 30, 2018: contributor

utACK 85ec8d645f9aace793514ab36d6cc7a5382f3e7c
gmaxwell commented at 7:46 AM on August 30, 2018: contributor

utACK .. but mostly I just like the PR text.
ajtowns commented at 8:50 AM on August 30, 2018: member

utACK 85ec8d645f9aace793514ab36d6cc7a5382f3e7c

Some of the changes for the "GetLabelDestination" commit (53038b47) are actually in the following "Delete unused account functions" commit, causing "make all" to fail. Otherwise looks good.
jnewbery commented at 12:08 PM on August 30, 2018: member

4 utACKs. Rather than fix the build break in the intermediate commit, I think it makes sense to just squash everything down to one commit (I only split it up so granularly to aid reviews). @laanwj - if you agree, I'm happy for you to squash these down when you merge, or I can do it. Let me know.
laanwj commented at 1:57 PM on August 30, 2018: member

@jnewbery ok, I'll do that, no problem
pull[bot] referenced this in commit 07033a8f91 on Aug 30, 2018
MarcoFalke closed this on Aug 30, 2018
MarcoFalke commented at 2:21 PM on August 30, 2018: member

This was squashed to c9c32e6b844fc79467b7e24c6c916142a0d08484 and merged
laanwj commented at 2:24 PM on August 30, 2018: member

yep I should probably have left the [doc] commit separate, nah
jnewbery deleted the branch on Aug 30, 2018
cryppty commented at 3:43 PM on November 11, 2019: none

Hi, this is a real pity that you have removed the "accounts" functionality. Many platforms have been designed around this as to "move" and "sendfrom" between accounts Is there a discussion somewhere where one can see the reasoning behind removing this functionality?
deadalnix referenced this in commit 6c13b2f1e9 on Mar 6, 2020
deadalnix referenced this in commit 65cbc5e408 on Mar 6, 2020
deadalnix referenced this in commit ff8832ec36 on Mar 6, 2020
deadalnix referenced this in commit 557e4143f8 on Mar 6, 2020
deadalnix referenced this in commit ff6fdf66f7 on Mar 6, 2020
deadalnix referenced this in commit ae17dc97a0 on Mar 6, 2020
deadalnix referenced this in commit 5ec97bc2a8 on Mar 6, 2020
deadalnix referenced this in commit af67f67026 on Mar 6, 2020
deadalnix referenced this in commit 224a4cd817 on Mar 6, 2020
deadalnix referenced this in commit 43dc709045 on Mar 6, 2020
deadalnix referenced this in commit 23fe7e7d52 on Mar 6, 2020
deadalnix referenced this in commit 23ee283f51 on Mar 6, 2020
inzider commented at 1:07 AM on May 7, 2020: none

Hi, this is a real pity that you have removed the "accounts" functionality. Many platforms have been designed around this as to "move" and "sendfrom" between accounts Is there a discussion somewhere where one can see the reasoning behind removing this functionality?

I fully agree with you. This is close minded mentality, not looking at all side of the story, only at green users and miners side.

I need this CORE functionality!!!

What should I do now? Downgrade my version ? How odd. -deprecated=accounts options seems without effect.

What if I want one wallet with multiple addresses and move funds in between addresses? You guys just erased a whole functionality without replacement?
sipa commented at 1:13 AM on May 7, 2020: member

@inzider The accounts functionality we had almost certainly did not what you would expect it to do. It never supported moving coins between addresses, for example. You may be interested in the more modern multiwallet feature that supports having multiple truly independent wallets simultaneously.
inzider commented at 2:22 AM on May 7, 2020: none

I appreciate the suggestion and this is exactly what I DON'T want. I'm looking at one wallet with multiple addresses as in the core code.

I'm using a code that is based on bitcoin (multichain) and sendfrom does exactly that! Moving fund from one address to the other in the same wallet.

This is useful is MANY situation - I could write a novel on it.

Refer to : https://stackoverflow.com/questions/21450384/how-can-i-transfer-bitcoins-from-my-address-to-other-bitcoin-addresses

As the core command document say: https://en.bitcoin.it/wiki/Original_Bitcoin_client/API_calls_list

Along the way it almost certainly seems some people did not understand the purpose of it, and just removed it...

Oh well, I just downgraded my version and it works now,... This strongly is inappropriate at my opinion.
jnewbery locked this on May 7, 2020
ryanofsky commented at 2:51 AM on May 7, 2020: member

I don't understand use-case described #13825 (comment), since sendfrom didn't control coin selection and had no influence on what addresses (so to speak) would be sent from. It only affected how account balances were computed.

If there's an actual use-case made more difficult by removing accounts, it'd be good to open a feature request to see how it could be addressed. I did describe some steps for replicating account functionality outside of the wallet in #12952 (comment)