Rebased version of #2861
This is not mergeable in the current state, but people seem to be using this.
Based on a patch by Eric Lombrozo, further work by Pieter Wuille.
Some compatibility work for interaction with coin control was added by me. This needs to be tested carefully.
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.
@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 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:
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:
0$ bitcoind -datadir=2 importaddress mhou3jmi6Snxw4Y7nszn7gFTbUzHdiBt9y 'test' false
Send to that address from the mining process:
0$ bitcoind -datadir=1 sendtoaddress mhou3jmi6Snxw4Y7nszn7gFTbUzHdiBt9y 1
Confirm the unspent has been sent:
0$ bitcoind -datadir=2 listunspent 0
1[
2 {
3 "txid" : "36016dffdfb5ca0d571f30349fa772123ac92be814c7945d6155869785e1f7d3",
4 "vout" : 1,
5 "address" : "mhou3jmi6Snxw4Y7nszn7gFTbUzHdiBt9y",
6 "account" : "test",
7 "scriptPubKey" : "76a91419249cd3bf74403fd1e918cc4cc07545b7b8cc2c88ac",
8 "amount" : 1.00000000,
9 "confirmations" : 0,
10 "spendable" : false
11 }
12]
Kill the second bitcoind, delete the wallet file, restart:
0$ kill `cat 2/testnet3/bitcoind.pid`
1$ rm 2/testnet3/wallet.dat
2$ bitcoind -datadir=2 -daemon
Reimport the address:
0$ bitcoind -datadir=2 importaddress mhou3jmi6Snxw4Y7nszn7gFTbUzHdiBt9y 'test' true
Look for unspents, and they no longer appear:
0$ bitcoind -datadir=2 listunspent 0
1[
2]
Any ideas/thoughts on this? I might have to disable wallet imports if I can’t figure this one out soon.
I noticed there’s a bunch of instances of bool “isMine” in several parts of the code after this commit.
0<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
1<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:
Changes:
* Add Add/Have WatchOnly methods to CKeyStore, and implementations
in CBasicKeyStore.
* Add similar methods to CWallet, and support entries for it in
CWalletDB.
* Make IsMine in script/wallet return a new enum 'isminetype',
rather than a boolean. This allows distinguishing between
spendable and unspendable coins.
* Add a field fSpendable to COutput (GetAvailableCoins' return type).
* Mark watchonly coins in listunspent as 'watchonly': true.
* Add 'watchonly' to validateaddress, suppressing script/pubkey/...
in this case.
Based on a patch by Eric Lombrozo.
@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 19007cf5529bc35a3baf53b14c6559bda3b2b206
easiest way to apply it is wget -O - #3383.diff | patch -p1
Yesterday, I decided to fork bitcoin/bitcoin and applied #3383 using: curl #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.
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.
Do you seriously consider “encrypt your wallet file and throw away the key, then import it elsewhere” a user-friendly suggestion? Also, what do you do when new keys were generated in the mean time and you need to sync? Shut down and do the copying all over? Please…
This just gives a way to import addresses instead of their private keys. Yes, it requires key-level management and with BIP32 it could be improved. But it is useful, and it’s currently the only decent way possible that I know.
@drak If you want this merged please solve the outstanding issues mentioned in the OP (as it was already ACKed once apart from those nits). Further discussion will not aid here:
Interested in closing this issue out and it’s not clear how the UI will show the non-spendable/watch only balance. I believe we also need to show the confirmed/unconfirmed separately somehow. Moreover, the RPC’s representation of these values should maybe be in another RPC method?
Suggestions?
@laanwj @erth64net @ryancdotorg i am in $ cd /bitcoin-0.8.2-linux/src/src now yum install patch done
how do i use #3383.diff to get a watchonly bitcoind? thank you
@wtogami said: watch-only entirely separate from real balances in both RPC and qt right? separate unconfirmed and confirmed watchonly? @laanwj said: yes, basically all the balances would be duplicated available watchonly, pending watchonly, immature watchonly, total watchonly as to how to display this, could be done in two ways, most straightforward would be adding another table if the user uses watch-only @wtogami said: hide it entirely if zero watchonly? @laanwj said: yes like we do for immature now if there is no immature as for RPC, easiest would be to add the watch-only balances to the ‘getwalletinfo’ RPC @wtogami said: and hide it from the other balances @laanwj said: yes, certainly to be clear: the default balance is spendable coins only if it’s shown in ‘getbalance’ it can be spent here and now, we’ve always used this convention and this PR should keep it also it would be nice to have a flag in ’listtransactions’ output to mark transactions that are watch-only (there is already a flag in ’listunspent’ output)
summary would be: completely isolate the watchonly balances from the “readwrite” balances, mark watchonly transactions
“as to how to display this, could be done in two ways” -> the other way would be having two columns in the “Wallet” table, one for the “readwrite” balance and one for the “watchonly” balance, hiding the watchonly column when the user doesn’t use watchonly.
“and hide it from the other balances” -> to be clear, this means that the two (set of) balances are disjunct, “hide” sounds sneakier than intended
FWIW, i think it would be very useful to have a ‘watch-only’ address capability.
Sidestepping all the UI issues herein discussed, (I think having separate balances, if any, is a fine idea), I will also note that it should be possible to author a transaction using the inaccessible inputs, for export and subsequent signing offline. That would be one of the major use cases, IMO - the ability to easily author transactions for later signing by your offline keys.
@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?’.
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?
@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.
@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.
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:
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.
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.