Skip to content

[wallet] Kill accounts #13825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
jnewbery wants to merge 18 commits into from
Closed

[wallet] Kill accounts #13825

jnewbery wants to merge 18 commits into from

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Jul 31, 2018 •
edited
Loading

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

@maflcko maflcko added this to the 0.18.0 milestone Jul 31, 2018
@jnewbery jnewbery mentioned this pull request Jul 31, 2018
Closed
6 tasks
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 31, 2018 •
edited
Loading

Note to reviewers: This pull request conflicts with the following ones:

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.

This was referenced Jul 31, 2018
Closed
Merged
Merged
Merged
Closed
@laanwj
Copy link
Member

laanwj commented Aug 2, 2018

Bon débarras!

Concept ACK, looks good in a first skim of commits.

This was referenced Aug 2, 2018
Closed
Merged
@jnewbery jnewbery mentioned this pull request Aug 10, 2018
Closed
@jnewbery
Copy link
Contributor Author

jnewbery commented Aug 14, 2018

Rebased.

V0.17 is branched. This is now ready for review/merge.

@jnewbery jnewbery changed the title [wallet] [Do not merge until v0.18] Kill accounts [wallet] Kill accounts Aug 14, 2018
@meshcollider
Copy link
Contributor

meshcollider commented Aug 14, 2018

I think the general agreement towards this is clear but Concept ACK regardless. Will try and find time to review this soon

@jnewbery
Copy link
Contributor Author

jnewbery commented Aug 14, 2018

Perec redacted to remove any potential copyright material from git logs 😢

This was referenced Aug 14, 2018
Closed
Merged
Merged
Merged
@jnewbery
Copy link
Contributor Author

jnewbery commented Aug 21, 2018

rebased

@ajtowns
Copy link
Contributor

ajtowns commented Aug 30, 2018

utACK 85ec8d6

Some of the changes for the "GetLabelDestination" commit (53038b4) are actually in the following "Delete unused account functions" commit, causing "make all" to fail. Otherwise looks good.

@jnewbery
Copy link
Contributor Author

jnewbery commented Aug 30, 2018

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
Copy link
Member

laanwj commented Aug 30, 2018

@jnewbery ok, I'll do that, no problem

pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Aug 30, 2018
@laanwj
Merge bitcoin#13825: [wallet] Kill accounts
07033a8 
c9c32e6 [wallet] Kill accounts (John Newbery)

Tree-SHA512: 783272e7df9042fb0a01826fa37a02b97218496459015d7457e56223da8690bdad930c223dd4a903a1d4df57f3f2f4a097d392d272a72419ea9a882b11e599f7
@maflcko maflcko closed this Aug 30, 2018
@maflcko
Copy link
Member

maflcko commented Aug 30, 2018

This was squashed to c9c32e6 and merged

@laanwj
Copy link
Member

laanwj commented Aug 30, 2018

yep
I should probably have left the [doc] commit separate, nah

@jnewbery jnewbery deleted the kill_accounts branch August 30, 2018 14:38
@practicalswift practicalswift mentioned this pull request Aug 31, 2018
Closed
@Bushstar Bushstar mentioned this pull request Sep 4, 2018
Merged
@ryanofsky ryanofsky mentioned this pull request Sep 5, 2018
Merged
@ryanofsky ryanofsky mentioned this pull request Nov 12, 2018
Merged
@cryppty
Copy link

cryppty commented Nov 11, 2019

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 pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Remove 'account' argument from GetLegacyBalance()
6c13b2f 
Summary:
GetLegacyBalance() is never called with an account argument.
Remove the argument and helper functions.

Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@0629de4

Depends on D5316

Test Plan:
  ninja check
  ninja check-functional

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5410
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Remove AccountMove()
65cbc5e 
Summary:
Function no longer used.

Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@ad63e01

Depends on D5316

Test Plan:
  ninja check

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D5411
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Remove CWallet::ListAccountCreditDebit() and GetAccountCredi…
ff8832e 
…tDebit()

Summary:
Functions no longer used.

Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@e6fcc5d
bitcoin/bitcoin@7a0d8f4

Depends on D5316

Test Plan:
  ninja check

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D5412
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Remove AddAccountingEntry()
557e414 
Summary:
Function no longer used.

Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@cf15ebd

Depends on D5411

Test Plan:
  ninja check

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D5413
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Don't rewrite accounting entries when reordering wallet tran…
ff6fdf6 
…sactions and remove WriteAccountingEntry()

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

Also remove unused function.

Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@3f248a8
bitcoin/bitcoin@f992f7f

Depends on D5413

Test Plan:
  ninja check
  test_runner.py wallet_*

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5414
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Don't read acentry key-values from wallet on load.
ae17dc9 
Summary:
Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@a544ab2

Depends on D5316

Test Plan:
  ninja check
  test_runner.py wallet_*

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5415
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Remove ListAccountCreditDebit()
5ec97bc 
Summary:
Function no longer used.

Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@b7fd01a

Depends on D5412, D5414, D5415

Test Plan:
  ninja check
  test_runner.py wallet_*

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5416
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Remove CAccountingEntry class
af67f67 
Summary:
No longer used

Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@7c6f1a1

Depends on D5411, D5412, D5413, D5414, D5415, D5416

Test Plan:
  ninja check
  test_runner.py wallet_*

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5418
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Delete unused account functions
224a4cd 
Summary:
Deletes:
- ReadAccount
- WriteAccount
- EraseAccount
- DeleteLabel
- GetLabelDestination

Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@f85988c
bitcoin/bitcoin@53038b4

Depends on D5316

Test Plan:
  ninja check
  test_runner.py wallet_*

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5419
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Remove fromAccount argument from CommitTransaction()
43dc709 
Summary:
Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@0bb1b4e

Depends on D5316

Test Plan:
  ninja check
  test_runner.py wallet_*

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5420
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Remove strFromAccount and strSentAccount
23fe7e7 
Summary:
No longer used.

Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@1031fa0
bitcoin/bitcoin@87e8123

Depends on D5316

Test Plan:
  ninja check
  test_runner.py wallet_*

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5421
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Mar 6, 2020
[wallet] Remove CAccount and Update zapwallettxes comment
23ee283 
Summary:
No longer used

Partial backport of Core [[bitcoin/bitcoin#13825 | PR13825]]
bitcoin/bitcoin@a92bdd1
bitcoin/bitcoin@3053884

Depends on D5419

Test Plan:
  ninja check
  test_runner.py wallet_*

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D5422
@inzider
Copy link

inzider commented May 7, 2020 •
edited
Loading

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
Copy link
Member

sipa commented May 7, 2020

@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
Copy link

inzider commented May 7, 2020 •
edited
Loading

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.

@bitcoin bitcoin locked as too heated and limited conversation to collaborators May 7, 2020
@ryanofsky
Copy link
Contributor

ryanofsky commented May 7, 2020

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)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

0.18.0

Development

Successfully merging this pull request may close these issues.