Skip to content
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.

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

[wallet] Kill accounts #13825

Closed
wants to merge 18 commits into from
Closed

[wallet] Kill accounts #13825

wants to merge 18 commits into from

Conversation

@jnewbery
Copy link
Member

@jnewbery jnewbery commented Jul 31, 2018

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

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

@DrahtBot DrahtBot commented Jul 31, 2018

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

@laanwj laanwj commented Aug 2, 2018

Bon débarras!

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

@jnewbery
Copy link
Member Author

@jnewbery 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
Member

@meshcollider 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
Member Author

@jnewbery jnewbery commented Aug 14, 2018

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

@jnewbery
Copy link
Member Author

@jnewbery jnewbery commented Aug 21, 2018

rebased

@ajtowns
Copy link
Contributor

@ajtowns 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
Member Author

@jnewbery 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 laanwj commented Aug 30, 2018

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

pull bot pushed a commit to jaschadub/bitcoin that referenced this issue Aug 30, 2018
c9c32e6 [wallet] Kill accounts (John Newbery)

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

@MarcoFalke MarcoFalke commented Aug 30, 2018

This was squashed to c9c32e6 and merged

@laanwj
Copy link
Member

@laanwj laanwj commented Aug 30, 2018

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

@cryppty
Copy link

@cryppty 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 issue Mar 6, 2020
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 issue Mar 6, 2020
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 issue Mar 6, 2020
…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 issue Mar 6, 2020
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 issue Mar 6, 2020
…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 issue Mar 6, 2020
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 issue Mar 6, 2020
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 issue Mar 6, 2020
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 issue Mar 6, 2020
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 issue Mar 6, 2020
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 issue Mar 6, 2020
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 issue Mar 6, 2020
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 inzider commented May 7, 2020

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 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 inzider commented May 7, 2020

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 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.
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet