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.

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.

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

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

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

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

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

@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).

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

That's ugly... :/

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.

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

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

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.

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.

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!

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?

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.

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

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