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

Remove bolt-on accounting system #3816

Closed
jgarzik opened this issue Mar 7, 2014 · 80 comments
Closed

Remove bolt-on accounting system #3816

jgarzik opened this issue Mar 7, 2014 · 80 comments

Comments

@jgarzik
Copy link
Contributor

jgarzik commented Mar 7, 2014

The recent malleability issues exposed some cases not covered by the accounting system. Rather than fix it, the consensus seems to lean towards scrapping the accounting system entirely, and returning to a system where keys may have labels (or tags, as I prefer to call them).

The accounting system can easily result in negative or odd balances if misused or misunderstood, which has happened in the field. Use of raw tx API or direct spends exacerbates this. Users are used to seemingly-odd practices of transferring imaginary money from a dummy account, to eliminate a negative number in some cases.

Practically speaking, this seems likely to be a large, incompatible change to RPC. This implies

  • other incompatible RPC changes are possible
  • the following line in rpcserver.cpp should be updated along standard REST API versioning conventions, changing the standard URI path from "/" to "/v2/" or somesuch.
        if (strURI != "/") {
@gavinandresen
Copy link
Contributor

Actually... the "weird balances" issues should be fixed by the transaction malleability fixes. If you find one, please write a regression test and submit a bug.

There is still an issue with CoinJoin balances being reported incorrectly, but that is separate from the accounts system (e.g. listtransactions will give you wacky amounts for CoinJoin txns).

I do think we should deprecate and remove the accounts feature, I think it encourages lazy development of systems that hold Other People's Money, when it was designed to help keep track of your own money, and it doesn't mesh with fees in a satisfying way.

@ghost
Copy link

ghost commented Mar 7, 2014

I love the idea of versioning the RPC but what about backwards
compatibility with a predetermined deprecation policy for "/" version 1 RPC?
Changes like this are going to break stuff for some implementations but
eventually the code must all be gone. Would users not wishing to upgrade
need to continue using an oder version of the client or would they run side
by side for X amount of time? Google has spent a lot of time on these
policies maybe something can be gleaned from it http://googledevelopers.
blogspot.ca/2012/04/changes-to-deprecation-policies-and-api.html

On Fri, Mar 7, 2014 at 12:25 PM, Jeff Garzik notifications@github.comwrote:

The recent malleability issues exposed some cases not covered by the
accounting system. Rather than fix it, the consensus seems to lean towards
scrapping the accounting system entirely, and returning to a system where
keys may have labels (or tags, as I prefer to call them).

The accounting system can easily result in negative or odd balances if
misused or misunderstood, which has happened in the field. Use of raw tx
API or direct spends exacerbates this. Users are used to seemingly-odd
practices of transferring imaginary money from a dummy account, to
eliminate a negative number in some cases.

Practically speaking, this seems likely to be a large, incompatible change
to RPC. This implies

  • other incompatible RPC changes are possible

  • the following line in rpcserver.cpp should be updated along standard
    REST API versioning conventions, changing the standard URI path from "/" to
    "/v2/" or somesuch.

    if (strURI != "/") {
    

Reply to this email directly or view it on GitHubhttps://github.com//issues/3816
.

@laanwj
Copy link
Member

laanwj commented Mar 7, 2014

Yes, let's finally get rid of it.

I think we should keep labeling of addresses (like the GUI), but remove anything related to accounting or ledgers (such as the 'move' command, and faux balances).

I already made a proposal once #3664 (it can be worded stronger if this is going to be a fully new RPC version):

  • Rename
    • setaccount to setlabel
    • getaccount to getlabel
    • getaddressesbyaccount to getaddresseswithlabel
  • Rename optional account argument of listtransactions to label
  • Deprecate the move RPC command and any account ledger usage
  • Deprecate the use of the account argument in sendmany and sendfrom as well as getbalance
  • Deprecate the following:
    • getreceivedbyaccount
    • listreceivedbyaccount
    • Not sure about: getaccountaddress. Probably deprecate it. "label address" wouldn't really be a sensible thing.

@bananas2
Copy link

bananas2 commented Mar 7, 2014

I think the main reason of misunderstood is the Wiki "Customer creates an account on the website: web server either assigns them a unique customer id number or uses their email address or other unique identifier, calls getaccountaddress "userid" and tells the customer to send to that address to fund their account. ".

By the name "account" in the api commands i just supposed what it was designed to, then i went to the Wiki that made me sure that it was what i thought. Then everyone here says it should not be used fot hat purpose.

@bananas2
Copy link

bananas2 commented Mar 7, 2014

My opinion is that should be made a proper basic accounting system, account based third party services are very important to bitcoin itself. I understand that as a feature it may be out of focus, but it will help bitcoin itself.

@gmaxwell
Copy link
Contributor

gmaxwell commented Mar 7, 2014

It's impossible to make account data in the current Bitcoin software durable against hardware failure, anyone who is directly using it for accounting third party funds is behaving irresponsibly on this basis alone. I think the bitcoin daemon / reference wallet is the wrong layer of the stack to support multiuser accounting.

@ghost
Copy link

ghost commented Mar 7, 2014

@bananas2 I'm dealing with the exact same challenges right now and from the
looks of it this will need to be solved sooner than later in my
implementation to read for a newer version of the RPC API. Let me know want
to collaborate on something but as of right now my understanding is that
this is not something to be considered going forward as part of the
bitcoind/QT product.

On Fri, Mar 7, 2014 at 1:15 PM, bananas2 notifications@github.com wrote:

My opinion is that should be made a proper basic accounting system,
account based third party services are very important to bitcoin itself. I
understand it is not the focus, but some help does not hurt.

Reply to this email directly or view it on GitHubhttps://github.com//issues/3816#issuecomment-37051457
.

@bf0d7998-81ec-48d1-b236-076ed3c77581

Probably worthwhile to keep in mind that if the API is abruptly removed then people will just refuse to upgrade and expose themselves to any future CVEs. For a while at least it would be sensible to commit to backporting fixes to these versions if people need to back their software away from using them.

I agree that it needs to go, people seem to enjoy making web-wallet services using accounts then wondering why it blew up in their faces. Definitely not something the core client should be handling at any rate.

@luke-jr
Copy link
Member

luke-jr commented Mar 7, 2014

@bf0d7998-81ec-48d1-b236-076ed3c77581 There are already maintained backports for at least 0.7.x and 0.8.x, so no need to worry over that IMO.

@int03h
Copy link

int03h commented Mar 8, 2014

Much lurking lately since nothing to add, but I personally would love to decouple the base network from everything else. --disable-wallet was an important step.Building out bitcoind so that it offers better scalability is probably the next most important thing I believe we need.

This means that any accounting or additional functionality will have to be modular and separate anyway since its going to have to deal with potentially a new architecture that makes this possible.

IF you guys have the time to make that happen so be it. Most of the guys that are doing any of this stuff commercially SHOULD be building out their own transaction "management" mechanisms since its their money and they should have appropriate architecture, code and processes in place to mitigate losses for themselves and their customers. IF they don't have the skills to do it .. too bad, no money for Timmy.

Fundamentally: The bullshit we saw recently with our dear friend mark can not be repeated again - giving "people" all the OPEN code, the fruit of YOUR loins, so they can take it out there and make Bitcoin look bad is really not cool. IMHO YMMV ..etc..

EDIT : someone left their soapbox behind .. I will step off it now. There you go .. you can have it back .. ;)

@bf0d7998-81ec-48d1-b236-076ed3c77581

@int03h

The fundamental issue is that understanding the census and potential problems is something of a mystery to most programmers who are not contributors or the core team. Some of the biggest sites - coinbase and mtgox - have both tried the best to re-implement transactions and network behavior, but both seem to have had extraordinary trouble managing to keep up with smaller nuances. Coinbase seems to have some strange issues with their software keeping up with the blockchain and sending proper transactions, mtgox has had issues sending transactions (invalid DER encoding) and managing to automatically resend unconfirmed transactions without double spending the inputs.

If some incredibly well funded ventures have trouble scaling bitcoin transactions, what chance does the self-funded startup or small company have? bitcoind is inadequate for almost any situation larger than a couple of issues, which leaves users reaching for blockchain.info, who have been dispensing completely incorrect (missing transactions, bad spent/unspent information, invalid balancea) information for at least a week now.

If we want to avoid centralization the available solutions need to be improved, removing the awful accounts feature is hopefully a catalyst.

@laanwj
Copy link
Member

laanwj commented Mar 8, 2014

@bf0d7998-81ec-48d1-b236-076ed3c77581, @bananas2 I agree that it would be nice for scalable solutions to bitcoin accounting be available freely, however, as it is now no one is willing to create and/or maintain such system. Open source doesn't work by "this should exist" but by "scratch your own itch". The open source community is not a free resource to fix your company's issues.

The reality is that the few developers of Bitcoin Core are struggling to maintain what exists now. This means that the scope needs to be narrowed, not widened.

If the current broken solution is dropped from bitcoind that leaves the way open for independent, new systems that can be built on top of bitcoind. No need to implement transaction handling yourself, you can still use bitcoind for that.

@int03h
Copy link

int03h commented Mar 8, 2014

@bf0d7998-81ec-48d1-b236-076ed3c77581 Small point of clarification so that I don't leave the impression that I have the kind of application that requires this: When I said scalability .. I didn't just mean TPs. There are many elements that could fall into the scalability realm including High Availability and better p2p sync that I most certainly would put ahead of that kind of scalability ;).

@bf0d7998-81ec-48d1-b236-076ed3c77581

@laanwj

The reality is that the few developers of Bitcoin Core are struggling to maintain what exists now.

I certainly agree, I'm all for removing these features in the hope that somebody else picks up the slack.

Open source doesn't work by "this should exist" but by "scratch your own itch". The open source community is not a free resource to fix your company's issues.

Again, I agree. It's unfortunately something that not a lot of people want to get involved with. To contribute such a codebase anonymously would insight trust issues, to do it with a real name would make the person a target should a programming mistake result in the loss of Bitcoin for a third party. I don't think it's a risk anybody reasonably wants to put themselves at risk of, given how the community has treated actors like this in the past and today.

@laanwj
Copy link
Member

laanwj commented Mar 9, 2014

If we're going with multiple 'dialects' for JSON RPC selectable by the client, it may be useful to include the formatting/parsing of monetary amounts (#3759) in this as well. It's one of the things that would be better to let the client choose rather than making it a global setting.

@luke-jr
Copy link
Member

luke-jr commented Mar 9, 2014

Or just switch to a Number of satoshis in all cases with the new RPC version...

@int03h
Copy link

int03h commented Mar 10, 2014

@luke-jr Persistence is what counts. Will is survive the scrutiny of those that come after us ? If the RPC is fully fleshed out and solid then you can wipe your hands and walk away - if it isn't then don't even start. IMO.

@laanwj
Copy link
Member

laanwj commented Mar 10, 2014

@luke-jr Please keep that discussion in the issue I linked

@benjiqq
Copy link

benjiqq commented Nov 5, 2014

@jgarzik @laanwj What's the status of this? I have been working on this topic. It would be good if the accounts feature would be indeed flagged as deprecated (see laanjw's proposal to change to labels [1]). I know of plenty merchants, exchanges, etc. who (mis-) use the account system. It will be very helpful to have a much better documented RPC, so that people who implement account features can build together in the open, instead of closed source. Quite a few people use bitcoind only through the RPC without knowing internals. I'm implementing an account system which is layered over bitcoind, not using the internal account system. As I understand that's what most larger operations do.

[1] #3664 (comment)

@jgarzik
Copy link
Contributor Author

jgarzik commented Nov 5, 2014

@benjyz The status is reasonable summarized here. There appears to be consensus that it should be deprecated and removed.

However, no patches have been produced, and this would need staging & lots of communication with existing stakeholders, where breakage is quite likely. There are known users, some of them big.

@sipa
Copy link
Member

sipa commented Nov 5, 2014

I'm fine with marking them deprecated in 0.10, but not remove until later.

@jgarzik
Copy link
Contributor Author

jgarzik commented Nov 5, 2014

@sipa Yes, it cannot be removed without breaking several existing users. This one requires more deprecation planning, and perhaps coding an example alternative for contrib/

@PierreRochard
Copy link
Contributor

I agree with @sipa, we need time to have open source Bitcoin accounting platforms develop and mature. That will ease the transition for those currently dependent on this feature.

@luke-jr
Copy link
Member

luke-jr commented Nov 5, 2014

Note: merchants and exchanges should be using some kind of account system, but shouldn't be using Bitcoin Core's wallet implementation at all right now...

@sipa
Copy link
Member

sipa commented Nov 5, 2014

@luke-jr whether they should or not, I'm pretty sure that several do, and we don't want them to stop upgrading their software because we drop a feature.

ryanofsky pushed a commit to ryanofsky/bitcoin that referenced this issue Feb 12, 2018
Add label API to wallet RPC.

This is one step towards bitcoin#3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.

These initially mirror the account functions, with the following differences:

- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
  associated with addresses, instead of addresses associated with labels. (unlike
  with accounts.)
- Labels have no balance
- `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
  Being unable to delete them is a common annoyance (see bitcoin#1231).
  Currently only by reassigning all addresses using `setlabel`, but an explicit
  call `deletelabel` which assigns all address to the default label may make
  sense.
ryanofsky pushed a commit to ryanofsky/bitcoin that referenced this issue Mar 7, 2018
Add label API to wallet RPC.

This is one step towards bitcoin#3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.

These initially mirror the account functions, with the following differences:

- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
  associated with addresses, instead of addresses associated with labels. (unlike
  with accounts.)
- Labels have no balance
- `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
  Being unable to delete them is a common annoyance (see bitcoin#1231).
  Currently only by reassigning all addresses using `setlabel`, but an explicit
  call `deletelabel` which assigns all address to the default label may make
  sense.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue Mar 15, 2018
Add label API to wallet RPC.

This is one step towards bitcoin#3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.

These initially mirror the account functions, with the following differences:

- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
  associated with addresses, instead of addresses associated with labels. (unlike
  with accounts.)
- Labels have no balance
- `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
  Being unable to delete them is a common annoyance (see bitcoin#1231).
  Currently only by reassigning all addresses using `setlabel`, but an explicit
  call `deletelabel` which assigns all address to the default label may make
  sense.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this issue Mar 19, 2018
Add label API to wallet RPC.

This is one step towards bitcoin#3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.

These initially mirror the account functions, with the following differences:

- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
  associated with addresses, instead of addresses associated with labels. (unlike
  with accounts.)
- Labels have no balance
- `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
  Being unable to delete them is a common annoyance (see bitcoin#1231).
  Currently only by reassigning all addresses using `setlabel`, but an explicit
  call `deletelabel` which assigns all address to the default label may make
  sense.
ryanofsky pushed a commit to ryanofsky/bitcoin that referenced this issue Mar 23, 2018
Add label API to wallet RPC.

This is one step towards bitcoin#3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.

These initially mirror the account functions, with the following differences:

- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
  associated with addresses, instead of addresses associated with labels. (unlike
  with accounts.)
- Labels have no balance
  - No balances in `listlabels`
  - `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
  Being unable to delete them is a common annoyance (see bitcoin#1231).
  Currently only by reassigning all addresses using `setlabel`, but an explicit
  call `deletelabel` which assigns all address to the default label may make
  sense.
jnewbery pushed a commit to jnewbery/bitcoin that referenced this issue Apr 6, 2018
Add label API to wallet RPC.

This is one step towards bitcoin#3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.

These initially mirror the account functions, with the following differences:

- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
  associated with addresses, instead of addresses associated with labels. (unlike
  with accounts.)
- Labels have no balance
  - No balances in `listlabels`
  - `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
  Being unable to delete them is a common annoyance (see bitcoin#1231).
  Currently only by reassigning all addresses using `setlabel`, but an explicit
  call `deletelabel` which assigns all address to the default label may make
  sense.
jnewbery pushed a commit to jnewbery/bitcoin that referenced this issue Apr 10, 2018
Add label API to wallet RPC.

This is one step towards bitcoin#3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.

These initially mirror the account functions, with the following differences:

- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
  associated with addresses, instead of addresses associated with labels. (unlike
  with accounts.)
- Labels have no balance
  - No balances in `listlabels`
  - `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
  Being unable to delete them is a common annoyance (see bitcoin#1231).
  Currently only by reassigning all addresses using `setlabel`, but an explicit
  call `deletelabel` which assigns all address to the default label may make
  sense.

Thanks to Pierre Rochard for test fixes.
laanwj added a commit that referenced this issue Apr 11, 2018
41ba061 [docs] Add release notes for wallet 'label' API. (John Newbery)
189e0ef [wallet] [rpc] introduce 'label' API for wallet (Wladimir J. van der Laan)

Pull request description:

  Add label API to wallet RPC.

  This is one step towards #3816 ("Remove bolt-on account system") although it doesn't
  actually remove anything yet.

  These initially mirror the account functions, with the following differences:

  - These functions aren't DEPRECATED in the help
  - Help mentions 'label' instead of accounts. In the language used, labels are
    associated with addresses, instead of addresses associated with labels. (unlike
    with accounts.)
  - Labels have no balance
    - No balances in `listlabels`
    - `listlabels` has no minconf or watchonly argument
  - Like in the GUI, labels can be set on any address, not just receiving addreses
  - Unlike accounts, labels can be deleted.
    Being unable to delete them is a common annoyance (see #1231).
    Currently only by reassigning all addresses using `setlabel`, but an explicit
    call `deletelabel` which assigns all address to the default label may make
    sense.

Tree-SHA512: 45cc313c68ad529ce3a15c02181d2ab0083a7e14fe824e2cde34972713fecce512e3d4b9aa46db5355f2baa857c44b234d4fe9709225bc23c7ebbc0e03febbf5
@jnewbery
Copy link
Member

Suggest we close this since it's very old and unwieldly. This issue contains useful historic context, but concrete actions to remove the 'account' API can be tracked in #12952.

@int03h
Copy link

int03h commented Apr 11, 2018

Yeah. I think much water has flowed under this bridge. I see from #12952 it's still not deprecated, but at least the intent to deprecate is formally stated. New implementations will be at your own perl. Good enough for me.

stamhe pushed a commit to stamhe/bitcoin that referenced this issue Jun 27, 2018
Add label API to wallet RPC.

This is one step towards bitcoin#3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.

These initially mirror the account functions, with the following differences:

- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
  associated with addresses, instead of addresses associated with labels. (unlike
  with accounts.)
- Labels have no balance
  - No balances in `listlabels`
  - `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
  Being unable to delete them is a common annoyance (see bitcoin#1231).
  Currently only by reassigning all addresses using `setlabel`, but an explicit
  call `deletelabel` which assigns all address to the default label may make
  sense.

Thanks to Pierre Rochard for test fixes.
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this issue Sep 5, 2018
Add label API to wallet RPC.

This is one step towards bitcoin#3816 ("Remove bolt-on account system") although it doesn't
actually remove anything yet.

These initially mirror the account functions, with the following differences:

- These functions aren't DEPRECATED in the help
- Help mentions 'label' instead of accounts. In the language used, labels are
  associated with addresses, instead of addresses associated with labels. (unlike
  with accounts.)
- Labels have no balance
  - No balances in `listlabels`
  - `listlabels` has no minconf or watchonly argument
- Like in the GUI, labels can be set on any address, not just receiving addreses
- Unlike accounts, labels can be deleted.
  Being unable to delete them is a common annoyance (see bitcoin#1231).
  Currently only by reassigning all addresses using `setlabel`, but an explicit
  call `deletelabel` which assigns all address to the default label may make
  sense.

Thanks to Pierre Rochard for test fixes.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests