👍

re-ACK

The previous pull request went around and around in an ultimately circular discussion. I disagree that the multi-balance situation is a blocker for this PR. Multi-balance has existed since the beginning of bitcoin, in the form of confirmed/unconfirmed balance. Add on top of that multi-sig. On top of that, locked outputs. Adding public-key-only to the wallet is elegant and does not overly complicate the existing multi-balance situation.

The alternatives only truly make sense if multi-wallet support is deployed and fleshed out, making it trivial to divide up your coins across wallets without restarts etc. Otherwise, your choice is either running N copies of bitcoind -- in parallel if real-time wallet access is required, or this pull req.

That is the fundamental design choice. We are not close to multi-wallet, and this PR has people actively using it. This and coin control are two features that seem to continue to be requested, and IMO, need to be pushed based on user feedback.

OK from me, but I don't have time to test.

@jgarzik But all the current balances can be seen by the user, it's easy to see what part of the balance is confirmed, unconfirmed, etc. This patch as it is now provides no way to see what part of the balance is 'watch-only'.

This could be dangerous: there is no way to distinguish a wallet that only contains addresses from one that contains actual private keys for the same without trying to spend. Well -- apart from getrawtransaction which can see spendable/unspendable status. But there is no way to see this at a glance.

I think we are pretty close to multi-wallet actually... just some relatively small glue needed to expose it to the GUI or daemon, right?

I think having non-spendable balances visible is a good thing. Adding a boolean for that to getbalance/getunconfirmedbalance sounds fine to me (it should also deal with locked outputs).

For the GUI, something like:

• Balance: X (+ Y unspendable)
• Unconfirmed balance: Z (+ W unspendable)
  (where the bracketed part is only shown when Y/W are nonzero)
  would be fine to me.

@sipa agree
@laanwj agree -- and this is easy to fix, by simply exposing the information.

For RPC, showing multiple balances and providing information about locked/multisig/readonly should be visible.

Hi guys,

I'm investigating a problem where importaddress fails to find unspents sent to the imported address, even when rescan is set to true. I'm currently using https://github.com/sipa/bitcoin/tree/watchonly, so I haven't tested with this latest rebase yet, but I wanted to bring this up ASAP incase this is an issue that has not yet been discovered.

Reproducing is pretty simple with a private testnet-in-a-box:

Import an address:

$ bitcoind -datadir=2 importaddress mhou3jmi6Snxw4Y7nszn7gFTbUzHdiBt9y 'test' false

Send to that address from the mining process:

$ bitcoind -datadir=1 sendtoaddress mhou3jmi6Snxw4Y7nszn7gFTbUzHdiBt9y 1

Confirm the unspent has been sent:

$ bitcoind -datadir=2 listunspent 0
[
    {
        "txid" : "36016dffdfb5ca0d571f30349fa772123ac92be814c7945d6155869785e1f7d3",
        "vout" : 1,
        "address" : "mhou3jmi6Snxw4Y7nszn7gFTbUzHdiBt9y",
        "account" : "test",
        "scriptPubKey" : "76a91419249cd3bf74403fd1e918cc4cc07545b7b8cc2c88ac",
        "amount" : 1.00000000,
        "confirmations" : 0,
        "spendable" : false
    }
]

Kill the second bitcoind, delete the wallet file, restart:

$ kill `cat 2/testnet3/bitcoind.pid`
$ rm 2/testnet3/wallet.dat
$ bitcoind -datadir=2 -daemon

Reimport the address:

$ bitcoind -datadir=2 importaddress mhou3jmi6Snxw4Y7nszn7gFTbUzHdiBt9y 'test' true

Look for unspents, and they no longer appear:

$ bitcoind -datadir=2 listunspent 0
[
]

Any ideas/thoughts on this? I might have to disable wallet imports if I can't figure this one out soon.

Could that issue be related to #3502 ?

@wtogami I'm not sure, but it's similar for sure.

importaddress <address> [label] [rescan=true]

It does seem to rescan when rescan=true because the CPU spikes, but it doesn't find the unspent transactions for the address.

@kyledrake mhou3jmi6Snxw4Y7nszn7gFTbUzHdiBt9y doesn't seem to import any transactions into the wallet with master + #3383 while other addresses do.

That's crazy. Is it possible there's something wrong with that address?

I noticed there's a bunch of instances of bool "isMine" in several parts of the code after this commit.

<sipa> warren: the return value of IsMine(CTransaction) is supposed to be a bool - it should be called "IsRelevantToMe", as it could just as easily be a mix
<sipa> for NotifyAddressBookChanged it could be made into isminetype, if the GUI uses/needs that information

@sipa clarification?

So, there are two instances where something "IsMine"-like occurs in non-GUI code that returns a bool, and not an isminetype:

• In CWallet::IsMine(CTransaction), we return a bool because it's not about a particular output being spendable or not, just about whether the transaction is relevant to our wallet. So, this is correct. It would be more clear if the method were renamed to CWallet::IsRelevant or something.
• The NotifyAddressBoolChanged signal passes a bool along to indicate whether a destination is ours or not. If we wanted to distinguish in the GUI whether listed addresses of ours are considered spendable-from, then it may make sense to upgrade that bool to an isminetype, but that will depend on the GUI using it for something.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c1e048fbe4b59ffc539eb2e7b11d3dd5ae4abff6 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

I just patched bitcoin/bitcoin:master with this (which now has #3502 applied) and the rescan now picks up the transactions that were previously missed.

@ryancdotorg where can I find your patch for latest?

Your patch is possibly my last hope for getting the transaction malleability fixes into this.

@kyledrake not sure what you're asking about. This patch applied cleanly against bitcoin/bitcoin:master at 19007cf

easiest way to apply it is wget -O - https://github.com/bitcoin/bitcoin/pull/3383.diff | patch -p1

Yesterday, I decided to fork bitcoin/bitcoin and applied #3383 using:
curl https://github.com/bitcoin/bitcoin/pull/3383.diff | patch -p1

Patch applied without issue, and we're now running a watch-only wallet. Over 10,000 addresses have been imported, and a mere ~15 minutes after starting rescan we were ready to rock. Wallet balance is accurate - so it looks like rescan picked up everything without issue.

Thank you for putting this patch out there and considering(?) it for 0.9.0. Prior to this point, we were heavily dependent upon blockchain.info's rawaddr/total_received API function. With hot-button topics such as transaction malleability, MITM attacks, and further decentralization of the bitcoin network. It would be immensely useful to leverage watch-only (e.g. public keys only) wallets when integrating a web app with bitcoind.

awesome! if this patch makes it into 0.9.0 it would be a great help for Open Transactions.

@Drak It remains inappropriate to merge due to lack of a reasonable use case.

@luke-jr Um, what?

@sipa It does not make sense to monitor individual addresses,in a wallet. Addresses only receive, so the balance would just grow unbounded. The only apparent "use case" for this AFAIK is trying to emulate watch-only wallets.

It will require some manual synchronization with a live wallet, to feed it new addresses, but apart from that, there is not difference. You can monitor incoming payment, and remaining wallet balance.

Yes, it would be nicer if we had BIP32 watch-only wallets that could generate these addresses automatically, but one does not prevent the other.

@sipa You can't see a remaining wallet balance, since sends are not related to mere addresses. If this tracks when outputs received by the address are spent, I'd consider that a bug since those are unrelated to the address.

@luke-jr Please... how is that different from any other wallet?

@wangbus Any luck finishing this? If you're no longer working on this please let us know too, so someone else could pick this up.

Will pick this up early next week. Posting again if that doesn't come through.

Apologies, swamped this week. Will be picking it up after Friday.

I am playing around with it a bit, added some simple changes to make it more clear which is watch-only and which is your own. maybe you can use this, I'll upload my fork soon.

here it is: 6824f5a44a776ee6124dcccf6f92c27b2fdb850d
it adds another row below Immature balance and some details to transaction description.

@tuttleorbuttle it's on the right track! however, see @wtogami's post above and my reply to it; all the balances (available, immature, pending, total) would be replicated for watch-only.

Putting 'watch only' in the same category as those balances is confusing because it raises questions like 'is this only the confirmed part, or everything?'.

I'd like to better understand @luke-jr's argument that this shouldn't be merged.

My interpretation of @luke-jr's argument is that this allows building a wallet from arbitrary addresses, allowing tracking of transaction to individual address, and their spendability, essentially providing "balance of an address" - management at the address level rather than at the wallet level.

I agree that is unfortunate, and shouldn't be encouraged, but it is no different than allowing importing individual private keys in the first place, which also allows that level of micromanagement.

A higher-level solution is using watch-only wallets, using public BIP32 chains, rather than needing to build them yourself from individual addresses. That isn't available right now, however, and will not work for all legacy systems anyway.

@luke-jr Correct me if I'm wrong, of course.

I get that the watchonly patch is not popular with the core team. I would like to propose an alternative idea here and get your thoughts on it.

My alternative idea is to have bitcoind compile an index of all the existing utxos in the blockchain, with the address as the key.

This is not related to the wallet. Instead, it's exactly the same idea as the txindex. You could have a flag to bitcoind when it starts as such:

utxoindex=1

Signaling that you would like this. Then, there could be an api call to retreive the current list of utxos for addresses:

getutxos <address> [confirmations]

Or something similar. It wouldn't be a wallet or related to the wallet code - it would just be an index of utxos, simple as that.

Does this satiate the concerns core has with the watchonly patch?

The current way to do this outside of bitcoind is to manually scan the entire blockchain from bitcoind using the RPC and write code to track all of this elsewhere, which is 1) difficult to do safely (and correctly the first time), 2) requires a lot of code duplication that bitcoind already does, and 3) takes a very long time. It's also lead to stability issues when for example blockchain.info had a database issue and had to manually rescan their entire database. It's a really byzantine solution to the problem, one that is best handled by bitcoind IMHO and really could have nothing to do with the wallet code.

@kyledrake What you're suggesting is inherently unscalable. The watch-only patch isn't liked by Luke, but other than some concerns about how the UI works for unspendable coins I think it's fine with everyone else.

I don't think we'll consider including an addr index until we have pruning deployed so that the full marginal cost (right now about 20Gbytes) of turning on such an index is visible to those who use it.

@gmaxwell I think @kyledrake is only suggesting indexing the UTXOs, not the full blockchain, which is far more scalable (and likely to happen with committed UTXOs anyway, if the address-indexed version of that gets deployed).

@sipa Yeah exactly! Just an index of the utxos. Same idea as the txindex. And you need to explicitly enable it in the config, so that it doesn't build for people that don't want it.

I actually would prefer that approach vs watchonly, simply because it increases reliability: Instead of telling bitcoind which addresses I need to listen to, I can run a pool of them and just query addresses anytime I want. It also increases privacy because an attacker cannot get a full list of which addresses the server is managing from bitcoind.

@sipa hm. indeed. utxo address indexing is an interesting thought. I don't think it replaces watching wallets but I can certantly see uses for that.

@kyledrake But you can't implement a full wallet using it (no history, only currently available).

@sipa I believe I can. The wallets I'm designing don't have the notion of sending from other wallets, so payments can only be sent out from the wallet itself, which can record incoming utxos as transactions. And then when I send them out, I record that as a sent transaction. Coinpunk actually works this way as-is - I keep a transaction ledger with the wallet.

The watchonly patch is fine (see the amount of ACKs). As I've said about 10 times (browse back please) there are just a nit left that need to be fixed:

• The watch-only balances must be separate from the main balance (both in RPC and UI)

If someone picked this up and fixed the outstanding issues it could have been merged 10 times by now.

Create a new issue for the UTXO index, this is not the place to discuss that, it just adds to the already cluttered thread.

Agree with @laanwj . On the other hand, although it's not directly related to this PR kyle's idea actually render's watch-only wallets issue "moot". I'm quite sure most, if not all, use case of watching addresses.

@kyledrake would love to know if you're pursuing this.

@anton000 I will file an issue for this shortly, and reference it here.

here's a commit that entirely separates watchonly and regular balance in the GUI: [deprecated]

here's one that marks watchonly transactions in transaction history with a "w": [deprecated]

I'll also add a commit to fix the RPC commands if no one else is working on that right now.

I added a true/false flag to the affected RPC commands so you can select if you want watchonly transactions shown too.
The GUI layout needs a little bit of work but everything works.
https://github.com/tuttleorbuttle/bitcoin/commits/watchonly

Not sure yet how to mark watchonly transactions in listtransactions but I'll add something for that too.

Here's a patch to mark watchonly transactions in listtransactions: 9497d4f493310bb645911031fb1e8a6c799baea2 (does not require 9ad838c3b65ab5d4aa92db6e47813530a6c66f33)
I rebased my watchonly branch on bitcoin master. Can someone have a look at it?

@tuttleorbuttle thanks a lot for your work on this!
As for getting people to look at it, I suggest filing it as a new pull request (I'll close this one), that's easier for review and discussion and such.

Alright, i created a new pull request: #4045

Closing this one, thanks for picking this up @tuttleorbuttle