search

bitcoinjs / bitcoinjs-lib

A javascript Bitcoin library for node.js and browsers.
MIT License
5.66k stars 2.09k forks source link

processTx considered harmful #260

Closed dcousens closed 9 years ago

dcousens commented 10 years ago

Copied from https://github.com/bitcoinjs/bitcoinjs-lib/pull/258 for discussion.

[Is] processTx actually over complicating things more than it should? Why do we need anything other than the unspents? The Wallet should probably just contain all unspents, and as unspents usually have an accompanying confirmations field, we could just feed that in and put in a parameterized minimum confirmations for the following functions:

  • getBalance
  • getUnspentOutputs
  • createTransaction

There is never going to be a 'catch-all' Wallet design, so maybe we should just make this one as simple as possible? In what scenario is processTransaction actually useful unless we start including support for non-addressable scriptPubKeys anyway (arguably could still avoid it then)? This is basically an exact contradiction to [some of the changes I made in #258]... but I've made the issue more transparently 2-sided with [#258]. Anything more complicated than this would be context specific anyway.

My [other] gripe with processTransaction right now is the fact that if you don't process transactions in exact chronological order, then there is the real possibility for missed spends and invalid unspent outputs to occur inside the Wallet. The only way to work around this is to ensure that all transactions are correctly chained and processed in accordance to their graph-like structure. Thankfully its acyclic at least. [Still], it would require a lot more support code which we probably don't need to provide with bitcoinjs-lib. Therefore I vote to veto and remove processTransaction completely in 2.0.0, and completely contradict my self from #258, by saying unspents should be the only way to deal with the Wallet in future.

This kind of sucks, but I think we'll see 2.0.0 sooner than we'd like, simply because this was part of the library I just didn't have time to get around to until now (in terms of my involvement).

dcousens commented 10 years ago

Ping @kyledrake, @jprichardson, @weilu and any others to chime in their input.

dcousens commented 10 years ago

tl;dr we're not a bitcoin node.

Wallet.__processTx attempts to give us some of that functionality, but fails without a way to chain transaction history properly; which is something entirely on the user to feed us properly.

We could fake it using various approaches, but [I feel] we should only handle unspents and do that well.

If the user wants to be fancy with their own processTx, they can go ahead and process the unspents themselves.

sidazhang commented 10 years ago

If this proposal was developed and merged when do you expect the next release to be? I would really like to use it!

dcousens commented 10 years ago

A 1.x.y compatible release is in the works, but its up to @weilu and others as to when/if this gets merged and tagged.

sidazhang commented 10 years ago

When is the expected release to be?

dcousens commented 10 years ago

I think we could get a 1.0.3 release soon, within the next 24 hrs if @weilu is around for review. But otherwise a clean 2.0.0 release may be a while yet. No ETA.

weilu commented 10 years ago

@dcousens my primary use case for processPendingTx currently is when a user creates & sends a transaction, I can reflect that immediately on balance, unspent & history, instead of waiting for a blockchain API to pick it up. Moving forward, to update wallet balance, unspent and history, I expect relevant transactions will be pushed from blockchain API to client where only processPendingTx and processConfirmedTx will be used for handling them. In other words, setUnspent is meant for one time use upon wallet initialization, subsequently processPendingTx and processConfirmedTx should be used.

And yes, transactions are expected to be fed in exact the same order they appear on blockchain. I don't think that should come in surprise to any dev? Missing tx is a concern but imho that's a server/client communication/sync issue that's out of the scope and control for bitcoinjs-lib.

All that being said, I'm open to solutions that address the common use cases and simplifies the wallet logic.

dcousens commented 10 years ago

@weilu I think the basics are obvious, but the assumption you've made is that the blockchain API is giving you transactions/unspents in exact chronological order. This is most certainly not the case, and can't be relied on either.

dcousens commented 10 years ago

I also feel that it'd be nice to move unspent management to an abstraction other than Wallet.

Right now, its responsibilities are:

At most, I feel the top two are relevant, where as the last should be done by a different abstraction.
The 'out of place' nature of createTransaction in Wallet is indicative enough of this, since, at least in colloquial terms, since when does a Wallet "create" a transaction.

dcousens commented 9 years ago

Wallet has been removed in 2.0.0 in favour of https://www.npmjs.org/package/bip32-wallet or https://www.npmjs.org/package/cb-wallet. Both approximately mimic the original Wallet API, with the former using the original unspents style of UTXO management and the latter using transaction chains.

@weilu correct me if I'm wrong.

edit: We should probably update the README to point to these (and other) implementations.