Finally add support for watch-only addresses

tuttleorbuttle commented at 6:25 pm on April 10, 2014: none

Rebased and improved version of #3383

Based on a patch by Eric Lombrozo, further work by Pieter Wuille and JaSK.

unspent outputs have a spendable=true/false property to mark watchonly outputs
Some compatibility work for interaction with coin control was added by Pieter
the GUI now displays watchonly balances separately. the layout could be improved, but it works
getbalance, listaccounts, listtransactions and listsinceblock by default ignore watchonly addresses
transaction details returned by ListTransactions in RPC replies have an involvesWatchonly=true property if they involve watchonly addresses.

I haven’t done much testing with this yet but i don’t think it needs any major changes. Most of the commits are optional. Please tell me if anything needs to be changed.

dgenr8 commented at 7:13 pm on April 10, 2014: contributor

Should CWallet::ReacceptWalletTransactions() add unconfirmed watch-only transactions to the mempool?

#3883 (dyslexia alert, not 3383) adds an IsMine check to prevent this for tracked doublespends. It looks that should be IsMine==MINE_SPENDABLE if watches are not to be broadcast.

sipa commented at 7:19 pm on April 10, 2014: member

IMHO, watches should be broadcast.

I like the idea that the receiver of a transaction is responsible for broadcasting it, and for watched wallets, the spending key is unlikely to be online.

There are potentially privacy concerns with broadcasting transactions in general, but I think that’s an independent issue.

laanwj added this to the milestone 0.10.0 on Apr 16, 2014

laanwj commented at 7:19 am on April 16, 2014: member

Added milestone 0.10.0. It’s too risky for 0.9.2 but should definitely be in the next major release.

luke-jr commented at 9:11 am on April 16, 2014: member

This seems to still have the bug where transactions unrelated to the address are added to the wallet.

sipa commented at 10:46 am on April 16, 2014: member

@luke-jr That’s not a bug but a feature.

This patch allows manually constructing a wallet, and observing its wallet balance, and the transactions that affect it. Just like importprivkey/importwallet allow manual construction of a wallet.

It’s a low-level, micro-management feature, and yes, it allows observing transactions that spend coins assigned to one particular address. That’s not what I want to encourage, but it’s far from the only way of using this.

This is just the equivalent of importprivkey, without needing to reveal the key to the software, if you know you’re not going to need it.

Now please stop calling that a bug. Maybe call it a possibly unintentional misuse, or something.

luke-jr commented at 11:11 am on April 16, 2014: member

@sipa Then it should be “importscriptpubkey” instead of “importaddress” (and take a script as an argument), since it’s working with scriptpubkeys, not addresses… This makes it more flexible too.

sipa commented at 11:16 am on April 16, 2014: member

@luke-jr All that matters to a pubkey is how it can be used to receive coins. As we’re not caring about how they need to be spent, it doesn’t matter.

This is more flexible, as it also means support for P2SH and multisig.

luke-jr commented at 11:18 am on April 16, 2014: member

@sipa Using a scriptpubkey would do everything an address does here (including P2SH and multisig), but it ALSO allows me to add a custom scriptpubkey that I might want to monitor (such as a BC Script puzzle).

sipa commented at 11:30 am on April 16, 2014: member

@luke-jr You can. Add its p2sh hash, and it will watch for raw scriptpubkeys that match it.

luke-jr commented at 11:38 am on April 16, 2014: member

That’s ugly… :/

sipa commented at 11:52 am on April 16, 2014: member

I think you just convinced me that this indeed confuses the concepts of script hash and address.

Here’s what I suggest (it should be a small change, I’ll try to do that this evening):

CKeyStore still maintains a list of ScriptID’s (=160-bit hashes of scripts to watch for), but to match, hash(scriptPubKey) must match that ScriptID exactly (instead of trying to “guess” the destination and use P2SH script directly for lookups, and its hash otherwise).
A new importscriptpubkey takes the hash of its hex-encoded argument, and adds it to the list of watched ScriptIDs.
importaddress is a wrapper around importscriptpubkey which expands the address into a scriptpubkey, and calls importscriptpubkey.

This means importaddress still behaves as before, except:

A normal pay-to-pubkeyhash address will no longer watch payments to just the corresponding pubkey directly (the fact that the normal wallet does this is a bit of an ugly artifact of confusing addresses with key ids before).
A P2SH address will no longer watch payments to the corresponding raw multisig script.

gavinandresen commented at 2:16 pm on April 16, 2014: contributor

@sipa : I think we want this to continue to work:

 01) Mine a block, payment to <pubkey> CHECKSIG
 12) Somebody else sends to <pubkey> CHECKSIG
 2  EXPECT: coins show up in listtransactions/getbalance/etc.
 3
 4For backwards compatibility I think we need:
 53)  Somebody else sends via p2pubkeyhash(pubkey)
 6  ??EXPECT?? : coins show up in listtransactions/getbalance/etc.  (current behavior, I think)
 7
 8We COULD, but in my opinion shouldn't, support:
 94) Somebody sends to any of:
10   p2sh(<pubkey> OP_CHECKSIG)
11   p2sh(1 <pubkey> 1 OP_CHECKMULTISIG)
12   p2sh(p2pubkeyhash(pubkey))
13  EXPECT: wallet is blissfully unaware
14
155) addmultisigaddress each of the above:
16  EXPECT: transactions detected and added to wallet.

sipa commented at 2:30 pm on April 16, 2014: member

@gavinandresen What I suggest is that if you want to see transactions paying to a raw pubkey, you can always use importpubkeyscript [pay to pubkey script]. Similarly, if you want to see payments to some raw multisig script, add that multisig script directly.

Reasoning: when receiving a payment, you expect it to go to exactly the script you told someone it should go it. An address is a shorthand for one particular script.

The fact that we consider payments to raw public key script (“pubkey OP_CHECKSIG”) identical to payments to their corresponding pay-to-pubkeyhash script (“OP_DUP OP_HASH160 pubKeyHash OP_EQUALVERIFY OP_CHECKSIG”) is a historic mistake, IMHO. This is confusing the notion of an address (a payment destination script shorthand) with a key identifier.

Similarly, the fact that we in some cases consider payments to raw multisig scripts that we know about as payments to the list of pay-to-pubkeyhash addresses equivalent to the the individual pubkeys is confusing.

Note that I’m just talking about watch-only cases here.

As to your points:

Irrelevant, mining happens to newly generated keys, which are never watch-only.
If you have the actual pubkey in your wallet, yes - if you did importaddress [hashofthatpubkey] no, as it’s not a payment to that address.
Of course.
Fully agree. If you want to accept payments to alternate script forms, you need to explicitly tell others to pay to that, and explicitly tell your software to watch for it. In general, I dislike being “flexible in what you accept” - this just leads to confusing expectations.
This currently only works in case all keys are present. With importaddress (with or without addmultisigaddress) you could make the wallet see payments to the P2SH address, without the ability to spend.

gavinandresen commented at 2:39 pm on April 16, 2014: contributor

RE: talking about watch-only: ok. I was just worried (maybe unnecessarily) that changes to support watch-only could have unintended consequences for the ‘key is in my wallet’ cases. I think regression tests to make sure old behavior doesn’t change are needed here.

tuttleorbuttle commented at 8:56 pm on April 18, 2014: none

If you watch a multisig address of which you have all keys it doesn’t change anything i think. The coins will only show up as Pending like they always did (no duplication in the Watchonly column). I’m gonna do some testing with this next week to see if I can find any bugs.

The only minor glitch i noticed is that validateaddress will always show watchonly: false for multisig addresses added with addmultisigaddress because IsMine returns MINE_SPENDABLE for those.

laanwj commented at 11:17 am on April 29, 2014: member

GUI changes look good. I like the extra column, although it appears that the alignment of the header is slightly wrong:

Apart from this it appears to work fine in my (limited) testing. Great work!

in src/qt/transactiondesc.cpp: in 9497d4f493 outdated

 97@@ -98,10 +98,11 @@ QString TransactionDesc::toHTML(CWallet *wallet, CWalletTx &wtx, int vout, int u
 98                                 strHTML += "<b>" + tr("From") + ":</b> " + tr("unknown") + "<br>";
 99                                 strHTML += "<b>" + tr("To") + ":</b> ";
100                                 strHTML += GUIUtil::HtmlEscape(CBitcoinAddress(address).ToString());
101+                                std::string addressOwned = wallet->IsMine(txout) == MINE_SPENDABLE ? "own address" : "watch-only";
102                                 if (!wallet->mapAddressBook[address].name.empty())
103-                                    strHTML += " (" + tr("own address") + ", " + tr("label") + ": " + GUIUtil::HtmlEscape(wallet->mapAddressBook[address].name) + ")";
104+                                    strHTML += " (" + tr(addressOwned.c_str()) + ", " + tr("label") + ": " + GUIUtil::HtmlEscape(wallet->mapAddressBook[address].name) + ")";

laanwj commented at 11:21 am on April 29, 2014:

There is no use in passing a dynamically-generated string to tr. Qt’s translation tools scan the source code looking for tr("…string…"). If you want a dynamic label, put the tr(…) above, so

0QString addressOwned = wallet->IsMine(txout) == MINE_SPENDABLE ? tr("own address") : tr("watch-only");
1...
2strHTML += ... + addressOwned + ...

tuttleorbuttle commented at 3:18 pm on April 29, 2014:

okay, will change the string thing.

about the GUI: yeah i know it’s not perfect. if that’s a dealbreaker I’ll change it sometime but I’ll be glad if someone else fixes it instead. and yes, it’s like bitcoin-qt was made to have an extra column there :D

in src/rpcwallet.cpp: in 9497d4f493 outdated

1256-
1257+    isminefilter filter = MINE_SPENDABLE;
1258+    if (params.size() > 0)
1259+    {
1260+        strAccount = params[0].get_str();
1261+        if (params.size() > 1)

laanwj commented at 11:26 am on April 29, 2014:

Nit: for readability I’d prefer this to be linear, instead of a nested ‘pyramid’, ie

0if (params.size() > 0)
1{
2    strAccount = params[0].get_str();
3}
4if (params.size() > 1)
5{
6    nCount = params[1].get_int();
7}
8...

After all, params.size()>2 implies params.size()>1 … and so on.

tuttleorbuttle commented at 3:19 pm on April 29, 2014:

hm you are right, it looks better in linear form.

in src/walletdb.h: in 9497d4f493 outdated

 6@@ -7,6 +7,7 @@
 7 
 8 #include "db.h"
 9 #include "key.h"
10+#include "keystore.h"

laanwj commented at 11:33 am on April 29, 2014:

Do you need to create this include-dependency on keystore.h? (if you need this for CTxDestination, I’d suggest doing a forward declaration)

Edit: Hm that may not work as CTxDestination is defined using a template.

tuttleorbuttle commented at 3:18 pm on April 29, 2014:

that one is not from me but I can have a look at it.

tuttleorbuttle commented at 5:21 pm on April 29, 2014:

forward declaring it as class CTxDestination didn’t work. there are probably other ways but I’m not so well versed in template techniques.

laanwj commented at 6:15 pm on April 29, 2014:

OK just leave it be, then.

in src/wallet.cpp: in 9497d4f493 outdated

772@@ -758,9 +773,9 @@ void CWalletTx::GetAmounts(list<pair<CTxDestination, int64_t> >& listReceived,
773             // Don't report 'change' txouts
774             if (pwallet->IsChange(txout))
775                 continue;
776-            fIsMine = pwallet->IsMine(txout);
777+            fIsMine = (pwallet->IsMine(txout) & filter);

laanwj commented at 11:50 am on April 29, 2014:

You could move the

0fIsMine = (pwallet->IsMine(txout) & filter);

to before and outside the if statement (for example to the instantiation of the variable on line 767). This avoids repeating it twice here, and removes the need for the hard-to-read ‘assignment in if() clause’ below.

tuttleorbuttle commented at 3:41 pm on April 29, 2014:

heh, silly me. you are right of course. it’s a leftover from some experiment.

in src/wallet.h: in 9497d4f493 outdated

281@@ -274,17 +282,17 @@ class CWallet : public CCryptoKeyStore, public CWalletInterface
282 
283     std::set<CTxDestination> GetAccountAddresses(std::string strAccount) const;
284 
285-    bool IsMine(const CTxIn& txin) const;
286-    int64_t GetDebit(const CTxIn& txin) const;
287-    bool IsMine(const CTxOut& txout) const
288+    isminetype IsMine(const CTxIn& txin) const;
289+    int64_t GetDebit(const CTxIn& txin, const isminefilter& filter=(MINE_SPENDABLE|MINE_WATCH_ONLY)) const;

laanwj commented at 11:56 am on April 29, 2014:

In many other places (at least in the rpcwallet) you default the filter to only MINE_SPENDABLE. Do you have a specific reason for doing (MINE_SPENDABLE|MINE_WATCH_ONLY) here? [may be a potential source of bugs]

tuttleorbuttle commented at 3:39 pm on April 29, 2014:

iirc the reason for this is that i started with the GUI related code and did the rpcwallet code at the very end.

In most places I found it useful to include watchonly addresses in Debit/Credit calculations. e.g to find out if a transaction is relevant to you: bool IsFromMe(const CTransaction& tx) const { return (GetDebit(tx) > 0); }

In the rpcwallet i would actually prefer to also default to (MINE_SPENDABLE|MINE_WATCH_ONLY) and include watchonly balance/transactions in the RPC calls by default if everyone can agree with that.

However you are correct that this might be a source of future bugs or visual glitches so if you prefer I’ll default the filters to MINE_SPENDABLE everywhere and change the affected function calls.

laanwj commented at 3:53 pm on April 29, 2014:

Thanks for the explanation. Another option would be not to include a default argument at all. Let all caller sites explicitly specify what they want.

BTW: how about a MINE_ALL constant that is defined as (MINE_SPENDABLE|MINE_WATCH_ONLY)? Beats repeating it everywhere :)

tuttleorbuttle commented at 4:02 pm on April 29, 2014:

MINE_ALL sounds like a good idea. I don’t care if we include a default argument or not, will do whatever you think is best :p

laanwj commented at 4:07 pm on April 29, 2014:

I prefer explicit. That’s easier for reviewing, as we can check each caller site and see if the right argument was passed. And it makes it harder to introduce bugs by thinking that the default is something else than what it is.

unknown commented at 4:16 pm on April 29, 2014:

I’m not sure I like the use of “mine” here in the API and constants. IsOwner, ADDRESS_SPENDABLE, ADDRESS_WATCH_ONLY (or KEY_*) and so on.

tuttleorbuttle commented at 12:04 pm on May 16, 2014:

okay, so i will

add MINE_ALL
remove default values from getdebit, getcredit,..
rename MINE_ to ADDRESS_

in src/qt/transactiondesc.cpp: in 9497d4f493 outdated

209@@ -192,8 +210,8 @@ QString TransactionDesc::toHTML(CWallet *wallet, CWalletTx &wtx, int vout, int u
210                     // Payment to self
211                     int64_t nChange = wtx.GetChange();
212                     int64_t nValue = nCredit - nChange;
213-                    strHTML += "<b>" + tr("Debit") + ":</b> " + BitcoinUnits::formatWithUnit(unit, -nValue) + "<br>";
214-                    strHTML += "<b>" + tr("Credit") + ":</b> " + BitcoinUnits::formatWithUnit(unit, nValue) + "<br>";
215+                    strHTML += "<b>" + tr("Total debit") + ":</b> " + BitcoinUnits::formatWithUnit(unit, -nValue) + "<br>";
216+                    strHTML += "<b>" + tr("Total credit") + ":</b> " + BitcoinUnits::formatWithUnit(unit, nValue) + "<br>";

tuttleorbuttle commented at 3:55 pm on April 29, 2014:

I changed this to “Total credit” because i found it irritating, it was somewhat like: Debit: X Credit: Y Credit: Z It’s not really related to watchonly addresses though so i can revert it if someone is unhappy with this.

tuttleorbuttle commented at 8:02 pm on April 29, 2014: none

woops. how to proceed from here? should i rebase my commits onto the latest bitcoin commit and create a new pull request or just merge bitcoin master into my watchonly branch, fix the conflicts and push that?

wtogami commented at 8:20 pm on April 29, 2014: contributor

Rebase all of your commits on top of master and git push –force to your topic branch. Doing so will automatically update the commits here with a cleaner commit history.

tuttleorbuttle commented at 5:35 am on April 30, 2014: none

that sanity test is wrong, it fast-forward merges perfectly fine :/

leofidus commented at 11:46 am on April 30, 2014: none

Looking at the test log, it merged and built, but a test failed.

laanwj commented at 3:00 pm on April 30, 2014: member

Don’t worry. It appears that there’s something wrong with the pulltester at the moment.

laanwj commented at 8:38 am on May 8, 2014: member

If you do a rebase+repush, that will trigger the pulltester. It should be fixed now.

tuttleorbuttle commented at 1:32 pm on May 15, 2014: none

hm it failed again.

masterbitcoin commented at 5:02 am on May 16, 2014: none

this change is based on bitcoin master? why not base on bitcoin 0.9.2?

masterbitcoin commented at 5:10 am on May 16, 2014: none

@tuttleorbuttle @laanwj with this patch, ./bitcoind listtransactions doesn’t list watchonly address’s received and sent transactions, however the bitcoin-qt gui does show all received and sent transactions of the watchonly address, why?

ghost commented at 8:10 am on May 16, 2014: none

@masterbitcoin Actually it does work you need to use the includeWatchonly flag, For example

listtransactions "" 100 0 true

ghost commented at 8:14 am on May 16, 2014: none

@tuttleorbuttle should listreceivedbyaddress also include a watch only filter flag at the moment? It currently includes watchonly by default and there is no way to turn that off. I dont have a problem with that, but just wondering that was intended.

Otherwise ACK from me. I tested things and it works like a dream.

wtogami commented at 8:36 am on May 16, 2014: contributor

this change is based on bitcoin master? why not base on bitcoin 0.9.2?

This change is too invasive for 0.9.2. It is aimed for 0.10

ghost commented at 8:38 am on May 16, 2014: none

Also the merge strategy used here is to merge everything into master and cherry-pick out bug fixes for maintenance branch rather than commit to lowest branch and merge up.

tuttleorbuttle commented at 11:33 am on May 16, 2014: none

@masterbitcoin i added the includeWatchonly flag for api functions in commit c6d8337 just in case someone might prefer it that way and i guess i simply forgot to add it for listreceivedbyaddress. listunspent also includes watchonly transactions by default so there’s some inconsistency here.

Since watchonly outputs/transactions are marked in the API responses as ‘spendable=false’ or ‘involvesWatchonly=true’ we could just get rid of c6d8337 or default includeWatchonly to true if that’s fine for everyone?

I noticed some glitches that I will look at over the next time, I think in one instance the entire amount was erroneously shown as fee instead of credit or debit in the GUI…

laanwj commented at 12:37 pm on May 16, 2014: member

@tuttleorbuttle Having an includewatchonly flag is important for the api calls where it isn’t possible to distinguish them in the result. For example in getbalance. For listtransactions and such it’s less important.

masterbitcoin commented at 3:42 pm on May 16, 2014: none

@drak @wtogami great, thanks a lot, i will test these apis and see if everything goes smooth. one more question, does master-bitcoin only mean 0.10? what does it mean after we reach/beyond 0.10 version?

beside, when i complied a 0.9.2 bitcoin-qt with this patch, i got a warning “warning: Clock skew detected. Your build may be incomplete.”, the bitcoin-qt was still built and looks like working well, should i worry about it? however there was no such warning showed up if i only complied the bitcoind without the bitcoin-qt

leofidus commented at 11:09 am on May 17, 2014: none

@masterbitcoin master always contains always all approved new code. Some of it will be cherry-picked for the next bugfix release (e.g. 0.9.2), and as a general rule all of it will be included in the next minor release (e.g. 0.10).

masterbitcoin commented at 4:13 pm on May 17, 2014: none

@leofidus great answer, thx, any idea about the “warning: Clock skew detected” problem?

tuttleorbuttle commented at 11:34 am on May 18, 2014: none

@laanwj e035837 and a31d01b add the includeWatchonly flag for getbalance and listaccounts, c6d8337 does it for listunspent. @masterbitcoin maybe this helps: https://stackoverflow.com/questions/13745645/makefile-clock-skew-detected

nickkhan commented at 6:22 pm on May 23, 2014: none

when should we expect this to make in to master ? how much time is needed?

laanwj commented at 12:36 pm on May 25, 2014: member

@nickkhan have you tested it? Does it do what you expect it to? Any glitches?

For a large wallet change like this we want to see a larger number of reviews and test reports than for a simple UI change. There is a lot of value at risk.

nickkhan commented at 1:50 pm on May 25, 2014: none

OK I be happy to test it.

On May 25, 2014 8:36 AM, “Wladimir J. van der Laan” notifications@github.com wrote: @nickkhan have you tested it? Does it do what you expect it to? Any glitches?

For a large wallet change like this we want to see a larger number of reviews and test reports than for a simple UI change. There is a lot of value at risk.

Reply to this email directly or view it on GitHub: #4045 (comment)

laanwj commented at 10:57 am on May 28, 2014: member

@tuttleorbuttle Are there still things that you need to fix here, or do you consider this ready to be merged?

sipa commented at 11:49 am on May 28, 2014: member

Can you wait a few days? I’d like to have a look, and suggest some changes (to make the RPC interface not confuse scripts with addresses).

laanwj commented at 1:06 pm on May 28, 2014: member

There is not really a hurry, more testing is always good, I just wanted to know what his own assessment of the status of this pull was.

nickkhan commented at 2:09 pm on May 28, 2014: none

Hi laanwj, I have a silly question. I have got the pull request checked out and I am running the client now but i dont see any watchonly wallet gui? Is there a test harness available to test against? or maybe you walk me through on how to test it ?

laanwj commented at 2:43 pm on May 28, 2014: member

@nickkhan To test, go to the debug window, type importaddress <address> to import an address. If you want to import multiple addresses pass ‘false’ as second argument to avoid automatic rescanning to save time. You should really be asking the developer of the patch, not me, though.

tuttleorbuttle commented at 2:44 pm on May 31, 2014: none

@laanwj I need to fix a few more things first, there are still some glitches i think.

tuttleorbuttle commented at 10:49 am on June 1, 2014: none

I think all serious issues are solved so whoever wants to test it, please go ahead. I’m going to test it over the next few days.

What’s left to code:

remove default argument values from getdebit(), getcredit(),.. functions
add MINE_ALL = MINE_SPENDABLE | MINE_WATCHONLY
rename MINE_ prefix to ADDRESS_ or similar because it makes more sense

nickkhan commented at 3:24 am on June 8, 2014: none

@tuttleorbuttle i dont see a debug window in bitcoin core wallet

Also I noticed there is no watchonly section.

leofidus commented at 4:19 pm on June 8, 2014: none

@nickkhan For the debug window, move your mouse to the upper screen border. That makes the menus File, Settings and Help appear. Within the Help menu, there is an item Debug Window

nickkhan commented at 4:53 pm on June 8, 2014: none

oh ok i see it now! I created a receive request from the client and send myself some btc to myself. then ran importaddress mywatchonlyaddres. the command ran ok. But I still dont see watchnoly gui section.

ghost commented at 5:16 pm on June 8, 2014: none

@nickkhan did you wait for it to rescan? It takes time to scan the database to get the balance of that address.

nickkhan commented at 5:21 pm on June 8, 2014: none

Yes it rescanned. Took sometime to rescan.is the build version correct ?

nickkhan commented at 8:23 pm on June 8, 2014: none

@tuttleorbuttle I tried sending some more test coins, and i cant get the watchonly section to show.

sipa commented at 7:19 pm on June 9, 2014: member

I have a suggested patch at sipa/bitcoin@watchonly. See the commit message for more information.

Link: sipa/bitcoin@19e72bc6db5947d7db5714cfe2fa21eed89bdfb9

nickkhan commented at 0:42 am on June 10, 2014: none

@sipa does your patch relate to my comment? just wanted to be sure!

laanwj commented at 7:54 am on June 10, 2014: member

@nickkhan Not really – it just proposes a more flexible way of doing watchonly internally, based on output scripts instead of (derived) destinations. This allows watching even scripts that bitcoind doesn’t know itself.

sipa commented at 8:31 am on June 10, 2014: member

@nickkhan @laanwj Maybe it is. I noticed that the current PR’s code does not consider watch-only addresses as change. That may interact badly with some wallet code for detecting payments.

nickkhan commented at 5:54 pm on June 11, 2014: none

ok I am not sure why i don’t see the watchonly gui. importaddress worked though and it performed a rescan as well. any ideas?

tuttleorbuttle commented at 8:19 am on June 13, 2014: none

@nickhan sorry, i don’t know why this would be. there might still be some glitches in the transaction history and transaction detail display but the balance and watchonly section should get displayed correctly.

did you use two different clients to send yourself the coins? doesn’t look like it on the screenshot. it doesn’t work if you import addresses to which you have the private key or something. This is for the same reason that multisig addresses show ismine=true and watchonly=false in ‘validateaddress’. A fix would probably just be a few lines but I’m not sure. use bitcoin-qt -datadir=[dir] to run multiple clients or download “testnet in a box”. @sipa i think viewing watchonly addresses as change would mean some outputs don’t get shown in the history list and i didn’t like that. Nice commit btw!

Oh and I think we should also provide a command to remove imported addresses. Not sure how much effort that would be.

nickkhan commented at 12:50 pm on June 13, 2014: none

@tuttleorbuttle OK I can try two different clients. Although I think the watch only GUI should show regardless.

tuttleorbuttle commented at 5:32 pm on June 13, 2014: none

@nickkhan but then you would count the same transaction twice, once towards spendable and once towards watchonly balance :s edit: or do you mean show the watchonly gui even if balance is zero?

laanwj commented at 7:47 am on June 18, 2014: member

@tuttleorbuttle Right. A transaction/balance is either watchonly or normal. It should not be possible to have it in both. That would complicate things enormously with no gain.

nickkhan commented at 7:59 am on June 18, 2014: none

@tuttleorbuttle my point is to show the watch only GUI section with a zero balance if the transaction is normal and of course like laanwj mentioned the balance is either watch only or normal. The watch only GUI would only account for watch only addresses. Yes I meant to show the GUI even if the balance is zero. Also the normal addressee are spendable where as watch only are receive only so I’m not sure how the transaction would be counted twice.

laanwj commented at 8:05 am on June 18, 2014: member

The watch-only GUI is intentionally hidden when there is no watch-only balance. This is common sense GUI development where you don’t want to burden the user with details about functionality they aren’t using.

nickkhan commented at 8:16 am on June 18, 2014: none

I think it would add more readability if the watch only sectioned showed for zero balance because after getting this patch I could not tell if watch only addresses were there/implemented even though the command worked. If the section shows with 0 balance then that tells the user the watch only section is there and will get updated when coins are received. Its like when you login your bank account you have a chequing and saving account. If you have zero balance in your chequing you still see the account its not hidden from the user. That’s all.

nickkhan commented at 8:19 am on June 18, 2014: none

Or check if the user has atleast one watch only address and then show the watch only GUI.

tuttleorbuttle commented at 1:54 pm on June 18, 2014: none

i see what you mean. I’m currently not sure how failure is handled in the importaddress rpc function but generally if after entering a command you don’t get an error returned it means it worked. you can also call ‘validateaddress’ and it will show ‘iswatchonly=true" or something, as long as it’s not a multisig address created with addmultisigaddress and you don’t own all keys i think.

implying that importaddress returns an error when something goes wrong I think we don’t need to display the watchonly gui when there is no balance. i wouldn’t mind if it was the case, but I don’t think it’s necessary.

sipa commented at 2:08 pm on June 18, 2014: member

Do you mind including the patch I linked to earlier? I feel the API and implementation are far cleaner that way.

tuttleorbuttle commented at 5:35 pm on June 18, 2014: none

yes, sorry, I’ll do that.

nickkhan commented at 6:10 pm on June 18, 2014: none

well is it easy to at least show an imported watchonly address in the gui once it’s imported? If the importaddress returns no error, it would make life for the user easier to see what address they imported in the gui.

tuttleorbuttle commented at 0:41 am on June 19, 2014: none

@nickhan i might look into this sometime but not yet for this pull request. maybe someone else will do it?

tuttleorbuttle commented at 1:00 am on June 19, 2014: none

eh, just noticed the amounts in TransactionDesc aren’t always right. nNet is wrong or something.

edit: I’ll do the outstanding tasks now and see where the error is.

tuttleorbuttle commented at 2:03 pm on June 20, 2014: none

oops. well, it should work now.

tuttleorbuttle commented at 11:05 pm on June 22, 2014: none

@sipa not sure whose fault it is but I just noticed that ‘validateaddress’ now returns ismine=false and isWatchonly=true for multisig, which is great, but it does not return redeemScript and the addresses it is made from anymore. any idea what might be causing this?

sipa commented at 11:11 pm on June 22, 2014: member

@tuttleorbuttle For a multisig address where you own all the keys (which would make it ismine=true even before this patch), or not?

tuttleorbuttle commented at 11:26 pm on June 22, 2014: none

okay, I can rename it again. the problem is that when you don’t have all the keys then validateaddress only returns { “isvalid” : true, “address” : “2NBXfVTLJTfM3JN5hArWDTg9PdqJLiAjf2H”, “ismine” : false, “iswatchonly” : true “account” : “multitest” } whereas it always used to also return redeemScript and addresses[] whether you had all the keys or not.

sipa commented at 11:51 pm on June 22, 2014: member

Well the block for adding the addresses in computing the details is now guarded by a == ADDRESS_SPENDABLE test. That won’t do much for non-spendable ones :)

tuttleorbuttle commented at 8:57 am on June 23, 2014: none

Ah found it, thanks!

Someone must have fixed the issue where multisig addresses are considered _SPENDABLE and now all those details don’t get returned anymore. Will fix that in a minute.

What prefix would you prefer instead of ADDRESS_ btw? Revert it to MINE_ or maybe SCRIPT_?

laanwj commented at 7:32 am on June 29, 2014: member

What about ISMINE_* instead of MINE_*? It removes the association with mining.

ghost commented at 7:50 am on June 29, 2014: none

Or use the word OWN

laanwj commented at 8:01 am on June 29, 2014: member

Could be but well it’s an IsMineType and the function is called IsMine, I don’t think OWN fits into there very well :)

Add support for watch-only addresses

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.

Conflicts:
	src/qt/walletmodel.cpp
	src/rpcserver.cpp
	src/wallet.cpp

c8988460a2

qt: Hide unspendable outputs in coin control 2935b21103

Watchonly balances are shown separately in gui. ffd40da361

Watchonly transactions are marked in transaction history d2692f6116

Added argument to getbalance to include watchonly addresses and fixed errors in balance calculation. d4640d7d8c

Added argument to listaccounts to include watchonly addresses 83f3543f20

Showing 'involvesWatchonly' property for transactions returned by 'listtransactions' and 'listsinceblock'.

It is only appended when the transaction involves a watchonly address.

952877e01c

Added argument to listtransactions and listsinceblock to include watchonly addresses d7d5d23b77

fixed tiny glitch and improved readability like laanwj suggested a5c6c5d6df

added includeWatchonly argument to 'gettransaction' because it affects balance calculation f87ba3df64

added includedWatchonly argument to listreceivedbyaddress/...account 0fa2f8899a

Use script matching rather than destination matching for watch-only.

This changes the keystore data format, wallet format and IsMine logic
to detect watch-only outputs based on direct script matching rather
than first trying to convert outputs to destinations (addresses).

The reason is that we don't know how the software that has the spending
keys works. It may support the same types of scripts as us, but that is
not guaranteed. Furthermore, it removes the ambiguity between addresses
used as identifiers for output scripts or identifiers for public keys.

One practical implication is that adding a normal pay-to-pubkey-hash
address via importaddress will not cause payments to the corresponding
full public key to be detected as IsMine. If that is wanted, add those
scripts directly (importaddress now also accepts any hex-encoded script).

Conflicts:
	src/wallet.cpp

d5087d1ba0

removed default argument values for ismine filter 80dda36a07

Fixed some stuff in TransactionDesc 23b0506c91

Added MINE_ALL = (spendable|watchonly) 519dd1c89a

fixed bug in ListReceived() f28707a845

fixed bug where validateaddress doesn't display information 53a2148f0c

replaced MINE_ with ISMINE_ a3e192a327

tuttleorbuttle commented at 1:57 pm on July 2, 2014: none

i think everything is fine now.

BitcoinPullTester commented at 2:04 pm on July 2, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4045_a3e192a3274817517671f624d5744297905e20d2/ 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.

laanwj commented at 12:29 pm on July 7, 2014: member

Ok. I’m going to merge this.

ghost commented at 1:03 pm on July 7, 2014: none

ACK again.

jgarzik commented at 1:48 pm on July 7, 2014: contributor

general ACK

laanwj merged this on Jul 7, 2014

laanwj closed this on Jul 7, 2014

laanwj referenced this in commit f748ff730b on Jul 7, 2014

nickkhan commented at 2:12 pm on July 7, 2014: none

Yay!

ghost commented at 2:14 pm on July 7, 2014: none

@tuttleorbuttle Thank you for your diligence with this PR and thanks for everyone who participated and tested. I think this is an awesome feature and will be really appreciated by users.

ocZio commented at 5:07 pm on July 7, 2014: none

Can’t wait!

xor-freenet commented at 5:59 pm on July 7, 2014: none

Thanks for this. There are two features which I think must also be implemented to make this useful:

Dump all public keys (= your own Bitcoin addresses), in a wallet, including ones without coins, to JSON.
Batch-import such a JSON as watchonly addresses.

Reasons:

This is useful if you have several cold storage wallets whose addresses you want to watch with an online wallet.
As address re-use is dis-encouraged, having batch-operations for this seems desirable because one can easily lose overview of his own Bitcoin addresses over the years. Would be easier to just be able to dump all of them at once.
It shall be noted that 1 is a rather basic feature IMHO and yet still is not possible with bitcoind: It seems that its only possible to dump addresses which hold coins, not empty ones. This is bad because the empty ones might be published somewhere and receive coins in the future. Also, offline wallets usually have an outdated blockchain anyway so addresses can seem to be empty for them even if they are not.

Can you please implement this as well? I would be very glad :) A bitcoind function would be preferable as first implementation, less work for you and cold storage machines can be primitive and thus without a GUI.

laanwj commented at 7:36 am on July 8, 2014: member

This is already possible, with dumpwallet. It doesn’t output to JSON, but the wallet interchange format contains the address and is easy to parse and convert. It does contain the private key too, though. Maybe a function that gives only public keys would be useful.
This is already possible with JSON-RPC batch processing, you can submit an array of importaddress requests with rescan=false

xor-freenet commented at 8:07 am on July 8, 2014: none

@laanwj

This is already possible, with dumpwallet. It doesn’t output to JSON, but the wallet interchange format contains the address and is easy to parse and convert. It does contain the private key too, though. Maybe a function that gives only public keys would be useful.

For the paranoid it is a must have to be absolutely sure that the data does not contain any private keys. Therefore I am reluctant to apply manual postprocessing to it and would be very happy if you implemented a way to dump only the public keys :) This applies especially given that you said that its a non-standard format, so its easy to mess things up. It could be a beautifully straightforward option publickeysonly=true which can be supplied to dumpwallet.

This is already possible with JSON-RPC batch processing, you can submit an array of importaddress requests with rescan=false

This would require me to write a script which can read the dumpwallet output and call importaddress for each public key, right? While I am willing to do some scripting, the fact that dumpwallet output is a custom format still is an issue there. Also, it doesn’t make sense to have a dumpwallet but not importwallet. Such functions usually should have a symmetric counterpart.

Anyway, let’s be honest here: I am trying to urge you to implement this as I’ve been waiting for it for months :) Maybe I should just be polite and offer you a symbolic donation for doing it. How about 20 dollars in Bitcoin at value of date when the solution to both is merged?

laanwj commented at 10:06 am on July 8, 2014: member

There is an importwallet command, actually. And WIF is not a non-standard format, the idea is that different wallets use the same format. The format could be extended with address-only entries (or maybe that already exists) which should then also be imported/exported.

Adding a parameter to dumpwallet to just export addresses could make sense, although if it is just a boolean it makes it less safe - what if someone accidentally provides that argument, they’ll make a backup without private keys… Maybe a mode string in [“all”, “addressonly”] instead of a boolean, that’s very difficult to accidentally confuse.

Then importing an addressonly wallet will result in the entire balance appearing under watchonly.

I can’t promise that I’ll get around to it.

tuttleorbuttle commented at 1:32 pm on July 9, 2014: none

thank you for merging it and thanks to sipa and laanwj for doing the complicated work.

I just noticed that ‘getbalance’ seems to be somewhat broken now, if I try to exclude watchonly transactions I get a higher balance than I should have. I’ll have a look at d4640d7 later today or tomorrow and create a PR as soon as I have a solution.

tuttleorbuttle commented at 10:37 am on July 14, 2014: none

Solution is in #4525. That was a ridiculously small mistake. Sorry for not pushing the fix sooner, was having trouble with my local testnet.

jessepeterson commented at 5:16 pm on August 8, 2014: none

I’ve got some questions on scalability of watch-only addresses.

I’m a little hazy on the difference between accounts and address groupings and how they relate to watch-only addresses. But I noticed here that it says:

The accounts code does not scale up to thousands of accounts with tens of thousands of transactions, because by-account (and by-account-by-time) indices are not implemented. So many operations (like computing an account balance) require accessing every wallet transaction.

Is this related to the scalability of the number of watch-only addresses or transactions related to watch-only addresses? I noted back in February @erth64net said he had imported 10,000 addresses and seemingly working fine. But could it go higher? A few orders higher? If I understand it accounts are implemented in the wallet in a Berkeley database and that’s the limiting factor?

Thanks for this feature! Will be quite useful!

jgarzik commented at 5:35 pm on August 8, 2014: contributor

The limiting factor is really the wallet scalability, and that has little to do with watch-only addresses.

10,000 addresses is a tiny amount, but you run into memory use and other issues once you are managing million+ keys.

laanwj commented at 6:06 am on August 9, 2014: member

@jessepeterson Watch-only would be something that is trivial to scale. Just run multiple servers w/bitcoind, set a maximum of addresses per server (depending on the memory available on the server - you’d have to profile what is a sane value), and once a server is full, add a next server.

Don’t confuse scalablity with efficiency. Scalability just means that it’s possible to add more hardware to do more processing, and scale roughly linearly.

(another question may be: do you really need to watch all those addresses, or could you set up an expiration scheme)

wtogami referenced this in commit 793d650fcc on Sep 9, 2014

wtogami referenced this in commit 033b4819e5 on Sep 9, 2014

wtogami referenced this in commit 17e33b7b70 on Sep 9, 2014

wtogami referenced this in commit b772fa02b2 on Sep 9, 2014

wtogami referenced this in commit e740bb77b9 on Sep 9, 2014

wtogami referenced this in commit 34dabd92d0 on Sep 9, 2014

wtogami referenced this in commit 88343bf010 on Sep 9, 2014

wtogami referenced this in commit 11707803ef on Sep 9, 2014

wtogami referenced this in commit d05afab6dc on Sep 9, 2014

wtogami referenced this in commit 3af3590ecc on Sep 9, 2014

wtogami referenced this in commit eedd487399 on Sep 9, 2014

wtogami referenced this in commit bb65b36b9b on Sep 9, 2014

wtogami referenced this in commit 0c94fe4550 on Sep 9, 2014

wtogami referenced this in commit beed538a12 on Sep 9, 2014

wtogami referenced this in commit a304622c8c on Sep 9, 2014

wtogami referenced this in commit 83d735b8cb on Sep 9, 2014

wtogami referenced this in commit dbc7bfb1a8 on Sep 9, 2014

wtogami referenced this in commit 108135f700 on Sep 9, 2014

wtogami referenced this in commit be1f4b67de on Sep 10, 2014

wtogami referenced this in commit 6e4dbe240d on Sep 10, 2014

wtogami referenced this in commit 3faaa1a743 on Sep 10, 2014

wtogami referenced this in commit 3770909278 on Sep 10, 2014

wtogami referenced this in commit 272046bb85 on Sep 10, 2014

wtogami referenced this in commit 14dfe8c385 on Sep 10, 2014

wtogami referenced this in commit cc86f4743a on Sep 10, 2014

wtogami referenced this in commit 67a472288c on Sep 10, 2014

wtogami referenced this in commit 3dd9d4b863 on Sep 10, 2014

wtogami referenced this in commit 6292e99652 on Sep 10, 2014

wtogami referenced this in commit 2f015fbdea on Sep 10, 2014

wtogami referenced this in commit df1a2c381c on Sep 10, 2014

wtogami referenced this in commit d8a15f5957 on Sep 10, 2014

wtogami referenced this in commit 82b6696e3e on Sep 10, 2014

wtogami referenced this in commit 19f6ca6558 on Sep 10, 2014

wtogami referenced this in commit 54081502b2 on Sep 10, 2014

wtogami referenced this in commit 50bc5112e3 on Sep 10, 2014

wtogami referenced this in commit be89383c34 on Sep 10, 2014

wtogami referenced this in commit 2672c04fc9 on Sep 19, 2014

wtogami referenced this in commit 47ff604092 on Sep 19, 2014

wtogami referenced this in commit df8225daa7 on Sep 19, 2014

wtogami referenced this in commit 7334b609de on Sep 19, 2014

wtogami referenced this in commit 72aa20ba4e on Sep 19, 2014

wtogami referenced this in commit d24c725979 on Sep 19, 2014

wtogami referenced this in commit 2d0a8118b2 on Sep 19, 2014

wtogami referenced this in commit 36d132b711 on Sep 19, 2014

wtogami referenced this in commit 15110457ef on Sep 19, 2014

wtogami referenced this in commit 2bb9ebb0a2 on Sep 19, 2014

wtogami referenced this in commit 47ea12d873 on Sep 19, 2014

wtogami referenced this in commit a3f6305d91 on Sep 19, 2014

wtogami referenced this in commit 14538d6d1e on Sep 19, 2014

wtogami referenced this in commit 0dcb3bbc1d on Sep 19, 2014

wtogami referenced this in commit 43491b2c35 on Sep 19, 2014

wtogami referenced this in commit 366783d20f on Sep 19, 2014

wtogami referenced this in commit c31be42b33 on Sep 19, 2014

wtogami referenced this in commit feb60c6e06 on Sep 19, 2014

wtogami referenced this in commit 437278dbe1 on Oct 2, 2014

wtogami referenced this in commit 5e1399b96f on Oct 2, 2014

wtogami referenced this in commit 93436313ec on Oct 2, 2014

wtogami referenced this in commit 8ca3a82440 on Oct 2, 2014

wtogami referenced this in commit c9ec2da214 on Oct 2, 2014

wtogami referenced this in commit 5c76409229 on Oct 2, 2014

wtogami referenced this in commit e5787b57f3 on Oct 2, 2014

wtogami referenced this in commit 4ee4b2657c on Oct 2, 2014

wtogami referenced this in commit 33ab951e74 on Oct 2, 2014

wtogami referenced this in commit 714029e8d6 on Oct 2, 2014

wtogami referenced this in commit e269c5b694 on Oct 2, 2014

wtogami referenced this in commit f15841e3c5 on Oct 2, 2014

wtogami referenced this in commit 6ee632fa09 on Oct 2, 2014

wtogami referenced this in commit 4790ffd756 on Oct 2, 2014

wtogami referenced this in commit 9e2b259c0d on Oct 2, 2014

wtogami referenced this in commit c240936cf4 on Oct 2, 2014

wtogami referenced this in commit 559a2f7f55 on Oct 2, 2014

wtogami referenced this in commit ce9d2f6882 on Oct 2, 2014

wtogami referenced this in commit f044b9c544 on Nov 14, 2014

wtogami referenced this in commit 03ff7569b9 on Nov 14, 2014

wtogami referenced this in commit 5f9d516339 on Nov 14, 2014

wtogami referenced this in commit 60ee438a3d on Nov 14, 2014

wtogami referenced this in commit 61d4f24b93 on Nov 14, 2014

wtogami referenced this in commit 5215c72f7d on Nov 14, 2014

wtogami referenced this in commit 144e090dae on Nov 14, 2014

wtogami referenced this in commit 294c6ccb50 on Nov 14, 2014

wtogami referenced this in commit cbce8de199 on Nov 14, 2014

wtogami referenced this in commit a51a46cfff on Nov 14, 2014

wtogami referenced this in commit e448659c88 on Nov 14, 2014

wtogami referenced this in commit b893c5be05 on Nov 14, 2014

wtogami referenced this in commit f9c8727395 on Nov 14, 2014

wtogami referenced this in commit 8d1cb78406 on Nov 14, 2014

wtogami referenced this in commit 599856ced4 on Nov 14, 2014

wtogami referenced this in commit d225a5a2c9 on Nov 14, 2014

wtogami referenced this in commit 2086abe904 on Nov 14, 2014

wtogami referenced this in commit c690b8a194 on Nov 14, 2014

wtogami referenced this in commit fa960268f1 on Dec 23, 2014

wtogami referenced this in commit 37b6cfab82 on Dec 23, 2014

wtogami referenced this in commit 34cbe909df on Dec 23, 2014

wtogami referenced this in commit 808b46377c on Dec 23, 2014

wtogami referenced this in commit 3456df55d6 on Dec 23, 2014

wtogami referenced this in commit 27beee8fad on Dec 23, 2014

wtogami referenced this in commit 2925922198 on Dec 23, 2014

wtogami referenced this in commit c07f488bd9 on Dec 23, 2014

wtogami referenced this in commit c3f005f0cd on Dec 23, 2014

wtogami referenced this in commit b02a0a7054 on Dec 23, 2014

wtogami referenced this in commit 2da425b866 on Dec 23, 2014

wtogami referenced this in commit 2fde5c64b2 on Dec 23, 2014

wtogami referenced this in commit 8cf8424de7 on Dec 23, 2014

wtogami referenced this in commit a84cf46e68 on Dec 23, 2014

wtogami referenced this in commit 7a52fcc877 on Dec 23, 2014

wtogami referenced this in commit 7a43c5b9c6 on Dec 23, 2014

wtogami referenced this in commit e42acc8e50 on Dec 23, 2014

wtogami referenced this in commit 0ac057b77c on Dec 23, 2014

DrahtBot locked this on Sep 8, 2021

Finally add support for watch-only addresses #4045