Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finally add support for watch-only addresses #4045

Merged
merged 18 commits into from Jul 7, 2014

Conversation

Projects
None yet
@tuttleorbuttle
Copy link

commented Apr 10, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2014

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

This comment has been minimized.

Copy link
Member

commented Apr 10, 2014

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 laanwj added this to the 0.10.0 milestone Apr 16, 2014

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

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

@luke-jr

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

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

@sipa

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

@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

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

@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

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

@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

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

@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

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

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

@luke-jr

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

That's ugly... :/

@sipa

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2014

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

1) Mine a block, payment to <pubkey> CHECKSIG
2) Somebody else sends to <pubkey> CHECKSIG
  EXPECT: coins show up in listtransactions/getbalance/etc.

For backwards compatibility I think we need:
3)  Somebody else sends via p2pubkeyhash(pubkey)
  ??EXPECT?? : coins show up in listtransactions/getbalance/etc.  (current behavior, I think)

We COULD, but in my opinion shouldn't, support:
4) Somebody sends to any of:
   p2sh(<pubkey> OP_CHECKSIG)
   p2sh(1 <pubkey> 1 OP_CHECKMULTISIG)
   p2sh(p2pubkeyhash(pubkey))
  EXPECT: wallet is blissfully unaware

5) addmultisigaddress each of the above:
  EXPECT: transactions detected and added to wallet.
@sipa

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

@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:

  1. Irrelevant, mining happens to newly generated keys, which are never watch-only.
  2. 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.
  3. Of course.
  4. 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.
  5. 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

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2014

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

This comment has been minimized.

Copy link
Author

commented Apr 18, 2014

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

This comment has been minimized.

Copy link
Member

commented Apr 29, 2014

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

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

@laanwj

View changes

src/qt/transactiondesc.cpp Outdated
if (!wallet->mapAddressBook[address].name.empty())
strHTML += " (" + tr("own address") + ", " + tr("label") + ": " + GUIUtil::HtmlEscape(wallet->mapAddressBook[address].name) + ")";
strHTML += " (" + tr(addressOwned.c_str()) + ", " + tr("label") + ": " + GUIUtil::HtmlEscape(wallet->mapAddressBook[address].name) + ")";

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 29, 2014

Member

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

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

This comment has been minimized.

Copy link
@tuttleorbuttle

tuttleorbuttle Apr 29, 2014

Author

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

@laanwj

View changes

src/rpcwallet.cpp Outdated
if (params.size() > 0)
{
strAccount = params[0].get_str();
if (params.size() > 1)

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 29, 2014

Member

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

if (params.size() > 0)
{
    strAccount = params[0].get_str();
}
if (params.size() > 1)
{
    nCount = params[1].get_int();
}
...

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

This comment has been minimized.

Copy link
@tuttleorbuttle

tuttleorbuttle Apr 29, 2014

Author

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

@@ -7,6 +7,7 @@

#include "db.h"
#include "key.h"
#include "keystore.h"

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 29, 2014

Member

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.

This comment has been minimized.

Copy link
@tuttleorbuttle

tuttleorbuttle Apr 29, 2014

Author

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

This comment has been minimized.

Copy link
@tuttleorbuttle

tuttleorbuttle Apr 29, 2014

Author

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

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 29, 2014

Member

OK just leave it be, then.

@laanwj

View changes

src/wallet.cpp Outdated
@@ -758,9 +773,9 @@ void CWalletTx::GetAmounts(list<pair<CTxDestination, int64_t> >& listReceived,
// Don't report 'change' txouts
if (pwallet->IsChange(txout))
continue;
fIsMine = pwallet->IsMine(txout);
fIsMine = (pwallet->IsMine(txout) & filter);

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 29, 2014

Member

You could move the

fIsMine = (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.

This comment has been minimized.

Copy link
@tuttleorbuttle

tuttleorbuttle Apr 29, 2014

Author

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

@laanwj

View changes

src/wallet.h Outdated
int64_t GetDebit(const CTxIn& txin) const;
bool IsMine(const CTxOut& txout) const
isminetype IsMine(const CTxIn& txin) const;
int64_t GetDebit(const CTxIn& txin, const isminefilter& filter=(MINE_SPENDABLE|MINE_WATCH_ONLY)) const;

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 29, 2014

Member

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]

This comment has been minimized.

Copy link
@tuttleorbuttle

tuttleorbuttle Apr 29, 2014

Author

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 29, 2014

Member

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 :)

This comment has been minimized.

Copy link
@tuttleorbuttle

tuttleorbuttle Apr 29, 2014

Author

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

This comment has been minimized.

Copy link
@laanwj

laanwj Apr 29, 2014

Member

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.

This comment has been minimized.

Copy link
@ghost

ghost Apr 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.

This comment has been minimized.

Copy link
@tuttleorbuttle

tuttleorbuttle May 16, 2014

Author

okay, so i will

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

View changes

src/qt/transactiondesc.cpp Outdated
strHTML += "<b>" + tr("Debit") + ":</b> " + BitcoinUnits::formatWithUnit(unit, -nValue) + "<br>";
strHTML += "<b>" + tr("Credit") + ":</b> " + BitcoinUnits::formatWithUnit(unit, nValue) + "<br>";
strHTML += "<b>" + tr("Total debit") + ":</b> " + BitcoinUnits::formatWithUnit(unit, -nValue) + "<br>";
strHTML += "<b>" + tr("Total credit") + ":</b> " + BitcoinUnits::formatWithUnit(unit, nValue) + "<br>";

This comment has been minimized.

Copy link
@tuttleorbuttle

tuttleorbuttle Apr 29, 2014

Author

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

This comment has been minimized.

Copy link
Author

commented Apr 29, 2014

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

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2014

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

This comment has been minimized.

Copy link
Author

commented Apr 30, 2014

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

@leofidus

This comment has been minimized.

Copy link

commented Apr 30, 2014

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

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Nov 14, 2014

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

Rebased-from: d5087d1 bitcoin#4045

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Nov 14, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Nov 14, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Nov 14, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Nov 14, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Nov 14, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Nov 14, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

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

Conflicts:
	src/script.h

Rebased-from: c898846 bitcoin#4045

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

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

Conflicts:
	src/wallet.h

Rebased-from: d4640d7 bitcoin#4045

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

Showing 'involvesWatchonly' property for transactions returned by 'li…
…sttransactions' and 'listsinceblock'.

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

Rebased-from: 952877e bitcoin#4045

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

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

Rebased-from: d5087d1 bitcoin#4045

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

wtogami added a commit to litecoin-project/litecoin that referenced this pull request Dec 23, 2014

@AnotherDroog AnotherDroog referenced this pull request Oct 30, 2018

Closed

Add on-chain invoicing #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.