A rebased and modified version of #2121. Changes:
Note: untested for now.
THANK YOU SO MUCH! @sipa you are my hero (and probably a bunch of other people's too)!
I will pull this and test it tomorrow, and try to do a more extensive test this weekend. I'll report back if I find any issues.
Again, you are awesome, and I think this is hugely important for bitcoin. Thank you, thank you, thank you.
@sipa is there any chance this could make it into master for the next release? I'm not sure if it's out of scope for the next release or not, just curious on timing so I can plan accordingly. I would love to see in the next release of course, but I don't want to derail all the other important stuff you guys are working on.
Thanks for giving this pull request some love, sipa!
I think having both spendable and watch-only accounts in the same wallet is problematic. For instance, how do we deal with getbalance, account labels, etc...?
I had delayed attempting this merge until multiwallet capability had been merged, where we could simply have a wallet be entirely watch-only or entirely spendable. I'm glad some people are finding it useful as it is, though.
@CodeShark The approach that this pullreq takes is that for almost everything, watch-only is considered equal to normal keys. You'll see it in getbalance, in listtransactions, in listunspent, ...
However, these are not considered when creating a transaction. Presumably we'll need a getspendablebalance too, and some GUI special-casing (perhaps only shown when watch-only addresses are present at all).
I have tested this patch by importing an address withimportaddress, and getbalance, listtransactions, getreceivedbyaddress, listreceivedbyaddress work as expected (and they agree with blockchain.info).
sendtoaddress from a wallet that contains only a single watch-only address fails gracefully with:
error: {"code":-4,"message":"Signing transaction failed"}
One interesting thing to test would be having a mixed wallet, and checking that you can spend the amount in normal keys, but not more.
Summarizing IRC review comments:
ACK once issues are fixed
I just tested sending transactions on testnet. Sending amounts higher than the balance available for spending (from non-watch-only addresses) results in a "Signing transaction failed" error.
mq7se9wy2egettFxPbmn99cK8v5AFq55Lx is the watch-only address and mv7VTUiBNmNSNrWaXrySbZcEzQ9C6ZNTNu I have the private key for:
rune@rune-desktop:~/Programming/bitcoin-sipa/src$ ./bitcoind listunspent
[
{
"txid" : "98278b9eb6cde5d3124ffc6c0ff75fa1d813b41383c35414315ce47f85fc45cc",
"vout" : 1,
"address" : "mv7VTUiBNmNSNrWaXrySbZcEzQ9C6ZNTNu",
"scriptPubKey" : "76a914a019eab435cd509dce95353febe6b48e7b15075388ac",
"amount" : 0.39800000,
"confirmations" : 5
},
{
"txid" : "bd9a149d58d9d2eff00e9bb1c0f463a2ce213f99affa8261bdbe8e85d60f5158",
"vout" : 1,
"address" : "mq7se9wy2egettFxPbmn99cK8v5AFq55Lx",
"account" : "",
"scriptPubKey" : "76a9146953ce65058e5e68125a9163d74b277d6a7f4a9e88ac",
"amount" : 23.49990000,
"confirmations" : 101
}
]
rune@rune-desktop:~/Programming/bitcoin-sipa/src$ ./bitcoind sendtoaddress mq7se9wy2egettFxPbmn99cK8v5AFq55Lx 0.4
error: {"code":-4,"message":"Signing transaction failed"}
rune@rune-desktop:~/Programming/bitcoin-sipa/src$ ./bitcoind sendtoaddress mq7se9wy2egettFxPbmn99cK8v5AFq55Lx 0.398
error: {"code":-4,"message":"Signing transaction failed"}
rune@rune-desktop:~/Programming/bitcoin-sipa/src$ ./bitcoind sendtoaddress mq7se9wy2egettFxPbmn99cK8v5AFq55Lx 0.397
14ce96d4809b18a7a7bc7f6501801572d3e673f6eb2ea645ad731a056e953221
rune@rune-desktop:~/Programming/bitcoin-sipa/src$ ./bitcoind getbalance
33.69660000
Significant changes, re-testing would be welcome :)
Instead of fRequireSpendable, the IsMine family of functions now return MINE_NO, MINE_WATCH_ONLY or MINE_SPENDABLE (instead of a boolean). listunspend should show whether a coin is spendable.
Overall ACK of updated code. Did not test, just code review. Minor taste-based nits:
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/02e10bf92bcb693288740ff7af0422d4fa0572d8 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.
Some changes:
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/80f264464ff2f424cc65e6c910411b7ba059f893 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 tested the new changes and now re-scanning works both on testnet and mainnet for me. I can send and receive on testnet, and I get a
error: {"code":-4,"message":"Insufficient funds"}
error message when I try to send more than I have in spendable funds (according to listunspent) but less than I have in unspendable funds.
On mainnet, the listransactions output agrees with blockchain.info for the tested address.
@gavinandresen You've voiced some objections to this on IRC, and prefer watch-only wallets over individual watch-only addresses IIRC, care to mention them here?
I don't feel like working on this further if it is controversial, but I think it's extremely useful and pretty much necessary to enable managing P2SH addresses without having all keys available. To get that same functionality with watch-only wallets, this becomes dependent on multiwallet, which doesn't seem to be making any progress.
IMO there is a strong user-based case for merging. There is clear user interest. People are building websites based on this branch, among other things.
I'd like to add that it increases decentralization, thus increasing the resilience of the Bitcoin ecosystem. Instead of having to rely on blockchain.info to get information about addresses not owned by myself, I can monitor these addresses using my own node. The same goes for other Bitcoin services. I suspect a lot of people depend on the blockchain.info JSON API simply because they have no other option right now. I know at least one service whose developer I spoke with using the blockchain.info API because they're the only ones offering a way to pull data about addresses for which one doesn't own the private key.
Every time blockchain.info has problems, or gets DDOSed, my service is unable to continue because I have no way of retrieving this information in a decentralized manner. It becomes a single point of failure for many of the smaller Bitcoin-related services.
I agree that separate, watch-only wallets would be preferable, but at the same time I don't see why it has to be an either-or issue.
It might be useful to have 'getinfo' or another RPC return a boolean indication of watch-only addresses in the wallet.
We'd love to have this feature. It is nonsense that we have to rely on blockexplorer.com and blockchain.info while having the entire blockchain stored on our own node.
Currently bitcoind doesn't have a friendly API for merchants and payment services providers. It is crucial to be able to handle tens of thousands of addresses for which we don't want to share the private keys with a certain copy of bitcoind.
It would be really cool to have this merged ASAP without creating dependencies on other not yet developed features.
To show how people overcomplicate this: The best would be to simply have an RPC call which queries the database for a certain address, as a one-off query. No need to add it to any wallets, manage it, etc. Just query the database for it. Make getreceivedbyaddress work with any address. That's all we need.
I really appreciate the work put into this branch, in fact we live off it for quite some time now. Would love to see this merged.
Thanks for listening!
@sipa is the importaddress code expected to update the account balance when a transaction goes out? I just dropped a generated raw transaction onto blockchain.info's tx send with one of the unspent transactions, and it removed the unspent from listunspent, but it didn't change the balance for the account that the address is associated with.
I didn't send it through my bitcoind instance, so that may be why that's happening, I need to do another test and send through my local bitcoind. Just wanted to mention incase it was a bug:
~ $ bitcoind listunspent
[
{
"txid" : "d266240586fc70f4e3927f3b0a70351179a5f2dbd189aecd31907f8596f0ffc3",
"vout" : 1,
"address" : "134GKGyWFftj2m4ZFKsBuCbm3GgXfDmuxX",
"account" : "RErMnwLZqmdGXIiWeJ83frHs+FG/CVCQhPXZ14USk9I=",
"scriptPubKey" : "76a914168e48aa5551a3ce7339dd55048b976edea3687288ac",
"amount" : 0.06000000,
"confirmations" : 2668,
"spendable" : false
}
]
~ $ bitcoind getbalance
0.06000000
~ $ bitcoind getbalance RErMnwLZqmdGXIiWeJ83frHs+FG/CVCQhPXZ14USk9I=
0.12000000
~ $ bitcoind listaccounts
{
"" : -0.06000000,
"RErMnwLZqmdGXIiWeJ83frHs+FG/CVCQhPXZ14USk9I=" : 0.12000000
}
And the transaction that was sent: https://blockchain.info/tx/87eb6a89ad7c53bc597a9c8431a22072e83d5e4b4ecbeb071b651aa5b5e964bd?show_adv=true
@kyledrake Accounts are not addresses, and do not have anything to do with the coins available to an address. The abstraction provided by the reference client has at no point any concept of a "balance of an address", only "balance of a wallet".
Addresses are associated with accounts, and coins received by those address will credit the account. For debits, you need to explicitly use "sendfrom".
Accounts are just virtual counters, and they can even go negative. I don't see a useful way to combine them with watch-only addresses. I think in general that their use is very limited, and in most cases what people really want is multiple individual wallets.
I'll be happy with watch-only addresses in the wallet if their meaning is clear and that meaning doesn't break people's existing code.
If I understand, as implemented their meaning is "Exactly like any other address in your wallet (shown as part of total balance, shown in listtransactions, ??shown in listunspent??), except you cannot spend them without constructing a raw transaction yourself".
That breaks two assumptions people may have:
getbalace shows the total number of bitcoins available to spend. I think that is an important assumption not to break.
If all your receive transactions are confirmed, sum(listtransactions) == getbalance '*' == sum(listunspent) .
Gavin Andresen
They are included in listtransactions, listreceivedbyaddress, and balances. They are also included in listunspent, but marked as watch-only there. Adding a flag to balance, or a separate RPC to query only spendable / watch-only addresses is probably useful.
More thoughts:
I agree that there is consensus that SOME solution is needed.
I'll (grudgingly) go along with a watch-only address breaking the "getbalance == coins you can spend" assumption, as long as that is well documented.
RE: flag to balance or separate RPC: "meh" -- if people want to keep track of watch-only balances separately, they should give them all a "watchonly" label. In fact, I'd vote that the default label for importaddress be "watchonly" (or "imported" -- don't care about the name) instead of "".
RE: documentation: How about: importaddress help says something like:
Adds an address that can be watched as if it were in your wallet, but cannot be used to spend. Transactions to or from watch-only addresses will appear in listtransactions and listunspent and will be counted in getbalance, but are not spendable without the private key. Watch-only addresses may be made spendable by importing the corresponding private key using importprivkey.
One more random thought:
Will the typical use case be a merchant importing a bunch of addresses? If yes, maybe this should be: importaddresses [address1,address2,...] <label> <rescan>
"six of one, half-dozen of the other" -- JSON-RPC 2.0 batches work just as well, for multiple importaddress.
@jgarzik Do they? (considering imports rescan the blockchain..)
Reminder that the original author of this pull request has abandoned it in favour of watch-only wallets. IMO, it makes a lot more sense to approach this after HD wallets are merged, as then there's a clear use-case that doesn't violate the no-address-reuse principle.
@luke-jr importaddress doesn't do a rescan if you pass "false" as the third (rescan) parameter. So the approach for batch imports of watch-only addresses would be to add all your addresses except the last one with rescan=false, and add the last one with rescan=true.
Thinking about my use cases, I don't think I would have any use case for a mixed watch-only/regular wallet. Ie. I wouldn't have a need for a wallet containing private keys that can spend money, and a wallet containing watch-only addresses. To me, the whole point of watch-only addresses is not letting bitcoind manage private keys for added security. Either for use with a web service (which can be a blockchain.info-style site, or a web wallet), or a local Armory-style wallet, where an external program handles the wallet, and only uses bitcoind to maintain the blockchain and query it about relevant transactions.
As Armory functions now, it uses bitcoind to maintain the blockchain, but not to query information from it. Armory scans the entire blockchain (loads the raw "blkxxxxx.dat" files) on startup, and to deduce the balance for the wallets maintained by Armory. If these watch-only extensions were to become available, Armory could add the relevant addresses using the importaddress function, and let bitcoind gather information on these addresses. To me this would be the first step in separating the wallet functionality from bitcoind.
I agree that mixing these two address types can be confusing - even though it has to be done manually - so I would much rather have the ability to have either a watch-only wallet or a wallet with private keys than nothing at all, although I'm not sure how much work that would be to implement.
Another solution would be to add watch-only versions of the relevant affected RPC calls. Ie. using wogetbalance to get the balance of watch-only addresses, wolistunspent to list unspent transactions for watch-only addresses etc. Another option would be to include an optional [includewatchonly=false] parameter to the relevant RPC calls, as another solution to make sure we don't break existing installations. I agree with Gavin that this is an important concern.
FYI I have worked around the getbalance issue by calling listunspent for the addresses I am watching and looping to calculate the balance.
The wiki page on raw transactions doesn't yet document this, but there's an optional array of addresses you can add to the end:
listunspent [minconf=1] [maxconf=9999999] ["address",...]
I believe this solves the balance calculation for me!
Another issue I'm running into is that listtransactions ACCOUNT doesn't show sent transactions anymore when I send via sendrawtransaction. I can use listtransactions to see them, but I can't filter it on bitcoind itself using an array of addresses, so I would have scaling issues after a certain point if I used it for a site running a lot of transactions.
Ideally it would be nice to see a list of transactions filtered by an array of addresses, similar to how listunspent can be used.
I'm not saying this is a problem for this commit (and you mentioned this issue above), I'm just adding this info incase anyone was interested. I will work around this by scanning the transactions manually for now, and try to come up with a longer term solution later on.
Update: Actually I won't be able to do a listransactions dump either, because there isn't really a way to see the source of the sends in the output except via the transaction id. I'll probably have to keep track of transactions/txids on my end.
The only way to have a transaction debit an account, is by using sendfrom. I'm not sure how you'd expect sendrawtransaction to interact with this. Accounts are not about address balances, and any application that tries to do so will likely have a hard time working around the abstraction that the wallet provides:
Just wanted to chime in to say I'd love watch-only functionality. IMO, it's the best way to go for server security, and I'd love to be able to do this without using a 3rd party service like blockchain.info. For me, it doesn't matter whether it's a separate watch-only wallet or integrated into the regular wallet, although I'd agree with runeksvendsen that I can't think of any cases I'd want to use them together. It would also probably keep things simpler to have them separate, but I'm not at all against the idea of combining them, either.
I'm also coming in with a +1 on this. Echoing earlier comments, for merchants this feature is an absolute must. Having the ability to listunspent for a watch only address is IMO, essential for merchants and developers of BTC applications. Currently the only way to get unspent is with blockchain.info /unspent api ~ having this feature locally would obviously be a massive improvement.
I have been using this in testing for Coinpunk and haven't run into any problems yet. Would love to see it get merged to master!
@kyledrake You know, the P2SH support @sipa just added could work well with Coinpunk: store funds in a 2-of-2 multisig P2SH addresses (including change) and have both the server and the user's client-side wallet sign transactions. Because both parties need to be involved you'd be able to implement useful features like daily spending limits, 2-factor-authentication to keep coins safe even if the user's computer is completely compromised, and safe instant transfers between users, while still preventing coins from being stolen if the server itself is hacked.
1429 | - return false; 1430 | - return IsMine(keystore, subscript); 1431 | + if (keystore.GetCScript(scriptID, subscript)) { 1432 | + isminetype ret = IsMine(keystore, subscript); 1433 | + if (ret) 1434 | + return ret;
This seems inconsistent with the way DescribeAddressVisitor is written: it allows for the case where the wallet has a redeemScript in it that can't be spent, yet the visitor will only show information on the redeemScript if it can be spent. I think it'd make more sense for it to be possible to have a redeemScript in your wallet that you can't spend, and for DescribeAddressVisitor to still show all the information known about the address in that case.
109 | @@ -110,6 +110,16 @@ bool CWallet::AddCScript(const CScript& redeemScript) 110 | return CWalletDB(strWalletFile).WriteCScript(Hash160(redeemScript), redeemScript); 111 | } 112 | 113 | +bool CWallet::AddWatchOnly(const CTxDestination &dest) 114 | +{ 115 | + if (!CCryptoKeyStore::AddWatchOnly(dest)) 116 | + return false; 117 | + nTimeFirstKey = 1; // No birthday information for watch-only keys.
Why not? In some cases you will in fact know when the private keys were generated, even though you don't actually have them. For instance that's true for keys generated by the client-side Javascript in Coinpunk.
Absolutely, but that's orthogonal. I believe importprivkey and importaddress should gain some standard way of specifying birth time (e.g., allow an @unixtimestamp suffix). I'd rather keep that in a separate pull request.
Sounds reasonable to me.
111 | @@ -112,6 +112,51 @@ Value importprivkey(const Array& params, bool fHelp) 112 | return Value::null; 113 | } 114 | 115 | +Value importaddress(const Array& params, bool fHelp) 116 | +{ 117 | + if (fHelp || params.size() < 1 || params.size() > 3) 118 | + throw runtime_error( 119 | + "importaddress <address> [label] [rescan=true]\n" 120 | + "Adds an address that can be watched as if it were in your wallet but cannot be used to spend.");
I'm in favor of keeping importaddress as it is, but if we do allow the wallet to contain redeemScripts that you don't have the keys to spend it'd make sense to also add my "addredeemscript" patch, but more importantly make sure addmultisigaddress automatically creates a watch-only address if you add a multi-sig address to your wallet for which you don't have all the private keys.
The reason for not automatically causing ownership when a multisig address is created, is because the software still deals very badly with double spending, so you don't want multiple instances to be able to spend the same coins simultaneously. With watch-only, that concern doesn't exist, and I think the best solution is consider anything related to the imported address 'mine'.
Ah, you're saying that when adding a multisig script for which you don't have all keys, it should automatically become watch-only. That seems reasonable, but maybe the semantics will be too confusing in that case.
Ok, scrap that idea then.
I see how it could cause confusion, especially for someone debugging a RPC-using thing who is assuming they can spend every addr.
What's the safest (least amount of changes compared to a stable version) version containing this patch? Are there any binaries available? I tried getting them from jenkins from last auto sanity check but it returns 404.
very quiet here.. How far is this from going into the 0.9 release ?
Would also like to see this merged in for an upcoming release! Let me know if I can help.
I have been using this functionality extensively with Coinpunk. I have refactored the service so that it now depends on nothing but the watch only address functionality, and have figured out how to deal with the transaction management without using the bitcoind wallet.
It would be great to not require users to compile an older custom version of bitcoind to use Coinpunk. I have been testing the watch address functionality extensively over the last month and have not run into a single issue with it.
I'll try merging this into master and doing some testing.
Edit: though on second thought I'd prefer a solution in which an entire wallet is made watch-only, instead of adding watch-only keys to an existing wallet and creating two classes of IsMine. This feels error prone somehow.
A recent suggestion by gmaxwell is to make coins received through watch-only function as if they were automatically (and irreverisbly) marked locked. Not much would change in practice, though.
@sipa I'm not sure what the benefits are of using this over the current implementation (which appears to work very well under testing), but I'll defer to the experts.
How long do you think it will take to make that change? Is it a candidate for making it into 0.9?
Not trying to pushy, just need to provide information to people using Coinpunk. Thanks for the feedback!
@kyledrake It's not about implementation - I expect that little would change. The reason to suggest it, is because the main controversy seems to be what the semantics of unspendable coins would be. If they behave identically to an already existing concept (lockunspent), the semantics are clear.
And I'm working on other things first.
Just to add to the testing reports: I haven't found a single error using importaddress and listunspent using this code so far, and I've used it quite a bit. It's been working great in testnet and production for me.
302 | + char fYes; 303 | + ssValue >> fYes; 304 | + if (fYes == '1') 305 | + pwallet->LoadWatchOnly(CBitcoinAddress(strAddress).Get()); 306 | + 307 | + // Watch-only addresses have no birthday information for now,
@petertodd Note the 'for now' here.
@keo It seems I never replied to your comment, but what may seem as overcomplication is rather the opposite.
Being able to always query information about any address's history, requires a rather large index to be maintained (multiple gigabytes). Using that for a simple wallet service means you become dependent on such an index, and your system will for example not be compatible with block chain pruning (not storing old parts of the block chain on disk). And most of all, it isn't needed for normal operation. Contrary to how many people see the system, internally it doesn't work with addresses at all - those are just part of an authentication layer on top of transactions (implemented through scripts).
For anyone just needing information about some wallet, where potentially not all keys are known, this is a much more efficient and scalable solution.
It doesn't really make sense for address-watching to add outputs to the wallet in the first place, since the outputs aren't really related beyond the receive notification.
1005 | @@ -997,9 +1006,10 @@ void CWallet::AvailableCoins(vector<COutput>& vCoins, bool fOnlyConfirmed) const 1006 | continue; 1007 | 1008 | for (unsigned int i = 0; i < pcoin->vout.size(); i++) { 1009 | - if (!(pcoin->IsSpent(i)) && IsMine(pcoin->vout[i]) && 1010 | + isminetype mine = IsMine(pcoin->vout[i]); 1011 | + if (!(pcoin->IsSpent(i)) && mine && 1012 | !IsLockedCoin((*it).first, i) && pcoin->vout[i].nValue > 0) 1013 | - vCoins.push_back(COutput(pcoin, i, pcoin->GetDepthInMainChain())); 1014 | + vCoins.push_back(COutput(pcoin, i, pcoin->GetDepthInMainChain(), mine == MINE_SPENDABLE));
Does it really make sense to include a spendability boolean in each COutput? Your code doesn't seem to actually use it anywhere. The last parameter of COutput doesn't seem to be useful in its current form because it is ascertained too late to be useful. What plans did you have for it? Perhaps it would be more useful as the tri-state enum instead of boolean?
https://github.com/litecoin-project/bitcoinomg/commit/8ebd9b64a8974d2fbc47fffdef5dbd72d3860a37 The simplest way I could think of to combine watchonly with coincontrol was to add a fOnlySpendable boolean to AvailableCoins(). This allowed me to filter the watchonly outputs from the coincontrol selection dialog. Can you think of a better way?
My testing indicates that Coin Control #2343 and Watch Only operate fine independently, but the combination has issues.
https://github.com/litecoin-project/bitcoinomg/commits/bitcoin-omg-0.8 Here is my branch of Bitcoin 0.8.5 where I initially tried to add watchonly to coin control by filtering out the watchonly outputs in AvailableCoins(). That succeeded in excluding the unspendable coins from the selection dialog, Coin Control appears to calculate the tx size and fee correctly, but then things go wrong after you Send.
One transaction I attempted had 67 inputs and was just under 10KB qualifying for free with a sufficiently high priority. After send, the client ended up failing with the "Transaction too large" error, meaning the client thought the transaction was in excess of 100KB. Another attempt with a 2KB transaction failed with an insufficient fee as the client thought the actual size was much larger than what Coin Control believed it to be.
The way Coin Control is currently structured it is uncertain if it can use watchonly's fSpendable as intended. I also wonder if it would be useful to anything for it to be tri-state like IsMIne() instead of a boolean.
I forgot to mention, Coin Control + Watch Only exhibits this strange erroneously large transaction behavior even if you have zero watchonly addresses.
@kyledrake Would Coinpunk be usable with a fully watch-only wallet? It would be a lot safer and less confusing to allow watch-only addresses only if a wallet does not mix with privkey addresses. I have to admit while testing this earlier that the user experience with the current watchonly implementation is very confusing when you have a mixed wallet. The "fake-encrypted" wallet approach would allow a watch-only wallet to happen safely and easily.
@kyledrake Given the current conflict Watch Only will not improve until Coin Control #2343 is first merged. CC is pretty much done except people need to step through the Test Plan in that ticket. Could you please direct people to participate in that?
I fully agree that having a mixed wallet, with both spendable keys and other unspendable addresses, leads to a more complex and potentially confusing interface. However:
lockunspent
@sipa I agree. IMO the complication of the current solution would be less if it was more visible to the user through the interface; for example if getinfo and such were to return two balances: a spendable and a watch-only balance.
Internally this could be implemented by making GetBalance take an IsMineType. This would also fit better into the overview page in the GUI.
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.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/a13f1e8f61f2c4f32c72c72409ea591228557434 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.
This is working awesome for me. Thank you @sipa and all other people here too. I want to ask, how can we delete a watched only address from being "watched". Because I sync my cache from "listtransactions *" from now and then I don't want bitcoind to keep being aware of some address. Can I remove them?
Anyone able to share a built windows exe? Trying to build bitcoind in windows without much Make experience is like chewing sand. :(
I'd love to see this feature to be merged.
@np : WHY would you love to see it merged-- can you please describe exactly how you are using, or are planning on using, this feature? Could you just run with an always-encrypted wallet instead, and if not, why not?
Watch Only Wallet now?
@gavinandresen I would like to see it merged because if I want to watch an address right now I have to use a block explorer.. ie, either blockchain.info, or install abe, which adds big mysql db, php, apache, running abe itself, keeping it up to date... all these additional overheads wouldn't be nessesary if I could just add the nessesary watch only addresses to a watch only wallet as such
@wtogami Agreed, that's a good way of creating a watch-only wallet right now.
The only feature missing then is to watch addresses that you have no private key for. This could be due to logistic reasons, ex. a "shop" server that wants to monitor that payments have come in, but cannot spend them. Or multisig addresses that the user has only one of the keys to, but knows the P2SH address for.
I think the "importaddress" idea is solid. But my irk about this implementation is that it combines spendable and non-spendable balances. This is only acceptable for read-only wallets.
My apologies if this comes off as dramatic, but we're having a really hard time explaining how important this is, and I'm kindof at a loss for how to explain how important this pull request is, having tried so hard previously and failing to get attention.
I want everybody reading this to take a moment of silence to ponder the tens, perhaps hundreds of millions of dollars USD that have been stolen because of storing private keys on the server. Bitcoin security is in really bad shape. There's so much theft of Bitcoins these days, in such high amounts, that it makes bank heists look like shoplifting. This is not something the police are going to solve, we have to solve it. We have to get more serious about security, and providing safer mechanisms for dealing with Bitcoin. That's what this pull request does.
What this pull request does is turn bitcoind into a listener node you don't need to trust anymore, and as such, this is the most important addition to bitcoind ever. It is critically important for hybrid wallets that we don't disclose the private key to the server, ever. Coinpunk completely and utterly depends on it, and if this gets rejected, it endangers a new Bitcoin wallet. In that amount of time, several new hosted wallets will go online that use server-side key management because no alternative exists for them, which will create more honeypots for hackers, which will lead to more major wallet thefts.
There are at least a dozen Bitcoin startups using it in production (not just for Coinpunk, for many other things), and I don't want to see them forced to depend on an old unstable version of bitcoind that could lead to serious problems and possibly a blockchain fork involving a huge swath of Bitcoin users, which could cause a crash in the price of Bitcoin, or worse.
It's been tested heavily and not a single person has reported a problem yet.
I will not use a replacement for this that requires the private keys, because it's dangerous and I won't do it. If there is a way to not disclose private keys and have this same functionality that's through a different mechanism, document it in this thread and I will gladly switch to it. But anything involving private keys is a total deal breaker.
All the work is done. We're one merge away, and Bitcoin immediately becomes a more secure, better ecosystem. That merge will give developers safer tools to work with Bitcoin, and will probably save millions of dollars from future theft.
Anyone interested in using this, please +1. And please merge this pull request. Thank you.
@kyledrake You know, this is a great example where practicality beats architectural purity.
Sure, maybe watch-only probably shouldn't have been setup to mix watch and non-watch addresses, but if you're not using the functionality, so what if it's there? Better to get this out the door given the high real-world demand for it than let the code rot and become unmergable again as things get changed around it.
@petertodd The problem is that once you introduce certain functionality, people start relying on the interface to be in that way, and it is very difficult to change it again.
One example of this is the "account" system. It is extremely unintuitive in various ways, does strange things in interaction with labels (and the GUI). But we're kind of stuck there. Everyone wants accounts to work in their preferred way, but doing anything to it breaks backwards compatibility in possibly dangerous ways.
So -- we need to get this (the RPC interface, not so much the implementation) right the first time.
IMHO the only solution that can satisfy everyone is to get rid of wallet functionality in bitcoind (split it off, not delete it), and provide an interface for wallet software to connect to. @kyledrake and others can then use their own custom wallet without depending on us. In principle the wallet functionality is a distraction from the purpose of bitcoind which is to run a validating node for the P2P network.
@laanwj Yes, and we agree on that. When the wallet functionality is split off interfaces can be gradually moved to whatever they should be. But given people are using this in production and semi-production, that's saying it's got direct real-world uses and maybe the interface is actually working. Once people's usage strategies settle down a bit, maybe our ideas of what was a perfect RPC interface will turn out to be wrong anyway - I myself can think of times where mixing funds that have full keys and funds that need additional signatures from elsewhere to be spent would be useful. (my one-time-password protected wallet idea)
@kyledrake Aside from the problems here, the current watch-only code is broken when merged with Coin Control.
The above Watch Only Wallet now? procedure is not ideal, but it is a way to have a watch-only wallet TODAY. If you create such wallets offline, private keys are effectively never on the server. This is the best, proven option you have for now.
The only possible implementation of watch-only that has a chance of merging in 0.9 might be a fully watch-only mode where privkeys are disabled, there is no spending and you can importaddress. I know some people who would be willing to write that for a little bounty, but there is no guarantee that it would be accepted into mainline.
@laanwj People are already relying on this interface, because they desperately needed it. Rejecting this pull request doesn't prevent that, it causes more problems. Sometimes we can't control how open source software is used. The street finds its own use for things, and the street is using watch-only addresses. We should recognize that this is happening and help people solve the most important problem with Bitcoin right now: security.
Getting the RPC interface right (I'm using it right now, and I have no changes, it's perfect for what I'm doing anyway), and splitting off the RPC are different goals, and I really don't think we should reject this pull request because of an ongoing purity debate regarding the wallet code in bitcoind. There's always warts in APIs, but I actually think the RPC interface in Bitcoin is pretty good. I've worked with HTTP APIs for years, and I would say it's one of the best interfaces I've ever worked with (The Facebook API was a nightmare in comparison). @gavinandresen's improvements to raw transaction were well done and very helpful.
I agree with you that splitting off is a good thing and I support that, but how does holding back on this pull request right now prevent you from splitting it off later? Let's not let bitcoind fall into disrepair while having this discussion. Let's make it better right now, and split off the code later. This merge is so simple it could be put in the next release of Bitcoind, but the code splitting probably needs another major version, so it's a good partition to do it this way.
Tomorrow I'm going to be at the Subway in Allentown, PA that accepts Bitcoin, and I'm going to demo the first-ever pure HTML QR-based POS transaction ever done, and it's going to be based on Coinpunk, which is based on this pull request. This is significant, because it's a mobile Bitcoin wallet that supports QR that Apple can't ban, which is another major problem we've been having. If that's not a good use case of this pull request, I don't know what is. Please help me solve these problems. I can write the code on my side, but I can't merge this code in. Throw me a bone here, I'll make it worth your time. Thank you.
@kyledrake I think we realize how important it is. Just 1) the code is broken right now 2) it needs a fair bit more work to be acceptable in mainline.
@wtogami I appreciate the workaround and it's interesting, but if I understand it, it can't be dynamically changed efficiently without exposing private keys and restarting servers. A hacker with a packet sniffer would be able to read all of the incoming keys unless it was being transferred remotely. For my use case, the private key must never touch the server.
I can't speak for everyone, but a "fully watch-only mode where privkeys are disabled" is a perfectly fine solution for my needs and I could use that.
No disrespect intended for the great work on Coin Control, but IMHO, this pull request is more important than Coin Control. Lack of anonymity hasn't caused nearly as many problems as private keys on servers. The list of thefts from Bitcoin servers is long and terrifying to read: https://bitcointalk.org/index.php?topic=83794.0
I wonder -- would it be possible to simulate "importaddress" in an always-encrypted wallet that is guaranteed to never by decrypted? (ie, by providing fake private key data)
@laanwj Can't speak for everyone of course, but that would also work great for me! I'm totally indifferent as to the mechanism (as long as it doesn't involve a real private key). Just adding an address, and being able to optionally scan the full blockchain (so I can skip if the key is new) solves my use case. Being able to put in a fake private key with a real address wouldn't be an issue for me at all.
Uh, didn't mean to do that, wrong button
So essentially:
importaddress command will only work in this mode.(The reason I wonder whether it's possible to do this minimally invasive using the current "encrypted wallet" logic is that it may require the full pubkey, not just an address, and it'd not work for pay-to-P2SH or cases in which the pubkey is not known)
Making watch-only addresses in a wallet turn the wallet watch-only entirely removes the use case of implementing true multisig through it (you need to be able to watch payments to the multisig address, even though you have only keys for some). Also, wallets can already be mixed spendable and non-spendable (through lockunspent).
@sipa Okay, good point, so in that case we need separate spendable and non-spendable balances per wallet either way.
What about my previous idea of having GetBalance (as well as other API entry points) take a IsMineType parameter? This would make it possible to show to the user that they have both spendable and unspendable coins in their wallet, and how much.
Do we really need wallets that contain a mix of both multisig and non-multisig coins? Seems to me that use case would be best served by having separate wallets too?
@luke-jr I think the best user experience for multisig coins would be indeed via a separate wallet create-multsig-wallet -> enter in extended pubkeys or an extended redeemscript (embeds the pubkeys) -> use wallet. But that perhaps doesn't preclude having mutt wallets that mix types.
@gavinandresen for all the reasons mentioned above. In particular avoiding the use of blockexplorer.com and trying out coinpunk.
Sounds like a -readonlywallet mode is what is wanted, with just public keys and no private keys. Is that correct? What wallet operations is CoinPunk doing? bitcoind really isn't designed to be a generic "Tell me the confirmed/unconfirmed balance of THIS set of public keys, please" service-- it seems like that is the functionality you really want (and isn't CoinPunk switching to libbitcoin anyway???).
The problem with this pull request is the mixing keys-that-can-be-spent with keys-that-cannot-be-spent.
I like the idea of a readonly wallet. This is a mode where you don't need to spend, don't need multisig, etc., because you're running it on a server as a validating node that can be queried instead of blockexplorer/blockchain. I wouldn't mix this with the GUI and personal use cases.
"Tell me the confirmed/unconfirmed balance of THIS set of public keys, please" is the exact thing bitcoind needs right now to be able to be used in production at wallets, merchants, PSPs, etc.
libbitcoin is buggy, and so are the others. When you want to verify a transaction and want to make sure, you need your own bitcoind node which doesn't contain any private keys.
This is is THE most important functionality that holds back hundreds of not thousands of businesses from being able to safely verify transactions. Having a safe and stable workaround for this problem is a huge cost right now.
"Tell me the confirmed/unconfirmed balance of THIS set of public keys, please" is the exact thing bitcoind needs right now to be able to be used in production at wallets, merchants, PSPs, etc.
^ I agree 100%. Having to setup and run your own block explorer to find unspent outputs for addresses that are not in your wallet is very bad.
By being able to query confirmed/unconfirmed balance and find unspent outputs..etc of addresses that you do not directly own... you can then build more secure apps around bitcoin handling spending and such in the browser.. only using bitcoind to push the transactions.
I also prefer the idea of a read-only wallet.
@gavinandresen importaddress, listunspent, gettransaction, sendrawtransaction. That's it.
I have never said I will be using libbitcoin, and haven't even considered the possibility until you mentioned. Upon quick inspection, it appears to be also not designed for my use (I'm not writing an anarchist-themed C++ application), but trying to implement it via FFI might be an interesting experiment. If I did consider doing that, I would still like to design Coinpunk to be usable by both clients. There's other issues with using an alternative implementation, such as stability and blockchain fork concerns and the like.
@kyledrake It is not necessary for you use libbitcoin directly, you could use sx (https://github.com/spesmilo/sx) which is built on top of libbitcoin.
However I think these projects are not yet mature enough and Coinpunk should aim at using both in the future.
@np I conveniently happen to be in the same room as the author of libbitcoin right now, and apparently there is a 0MQ interface for working with it I will investigate.
RE this pull request, it would be great to be able to continue supporting the importaddress approach as well. I realize that bitcoind isn't perfect, but nothing will be for a long time, and it's a known quantity that I'm comfortable using until a better solution comes out in the future.
I am working on a project that requires watch only addresses, I previously got this code working perfectly using the importaddress rpc command a few months ago, but seems to fail every way I try now. I documented my steps which was really just "git checkout -b remotes/origin/importaddress" before compiling. No matter what I try now, I get method not found.
Desperately need this command to build a POS system for restaurants which are waiting on me. Cannot have private key on the terminal. Help?
EDIT: Nevermind, I think I got it. Now see importaddress in the src files. Just want to say thanks for this feature. There will soon be several restaurants accepting bitcoin without any worry that the private key can be stolen by employees/customers. To anyone needing this amazing feature, use: "git clone -b watchonly https://github.com/sipa/bitcoin.git"
In case anyone is still looking for this functionality, here is our project that creates a REST/Websocket API for any address: https://github.com/bitpay/insight | insight Homepage The project also includes a blockexplorer, but that is 100% on client-side JavaScript. The index is stored on leveldb, and the data is pulled from bitcoind's .dat files or RPC getrawtransaction calls, and updated via p2p.
Still under heavy development, but it is working. Patches are welcomed.