Trying to refactor so that ExtractDestination don't return addresses is a can of worm... I will instead limit this change to RPC return only. It is enough to prevent people from copying this bad design, while making sure nothing else break.

This is ready to review.

Note that I keep addresses as empty array instead of removing the array completely. This will minimize chances to break clients.

Does it also remove addresses from getreceivedbylabel? #16655 (comment)

It doesn't break the interface but probably will break clients as they are expecting one address. Probably better is to remove the field and add a deprecation option?

It doesn't break the interface but probably will break clients as they are expecting one address. Probably better is to remove the field and add a deprecation option?

Though call. I am unsure about the deprecation though, program using addresses and expecting one address are necessarily broken already anyway.

Does it also remove addresses from getreceivedbylabel?

It does not. I tried to change the logic of ExtractDestination so that this change is more general, but it turned out to be a can of worms that would require some deeper refactoring, which I don't think really justify the end goal.

That said getreceivedbylabel should not show P2PK as addresses, I will comment directly on this PR.

Concept ACK

The convention for outputs without an address is to remove the array, rather than make it empty. See e.g. this OP_RETURN example: 8bae12b5f4c088d940733dcd1455efc6a3a69cf9340e17a981286d3778615684. If we follow that pattern imo there's no need to deprecate anything.

@Sjors good point, will do.

Ok I improved the PR as @Sjors suggested. It is actually less line of code changed and clearer.

Concept ACK. This is less risky than changing ExtractDestinations, of which it'd be much harder to oversee the consequences. On the other hand it's possible that addresses for P2PK still show up in other places.

Edit: so it might be better to rename this PR to more specifically don't show addresses or P2PK in decoderawtransaction.

Changed commit and PR title. That's fine if it still show at other place. I am trying at least to change the most obvious to prevent the mistake to spread into the mind of new devs.

Code review ACK 6d80349.

In the future we could add the inferred descriptor (though not always useful, without looking at the transaction that spent it to see the full script).

Code review ACK, and Concept ACK on moving away from showing P2PK as addresses. However, Does this need an RPC deprecation flag? I think it's unlikely, but someone may be relying on this odd behavior.

@Sjors Inferring descriptors isn't expensive; we should indeed just do that.

Also, I think we should get rid entirely of the 'addresses' field in decode* RPCs; it's very confusing that UTXOs are shown as having a list of addresses; a UTXO has at most one address.

It seems that I'm too late...

@NicolasDorier

because bitcoin address was not existing at the time.

Source: https://web.archive.org/web/20090303195936im_/http://bitcoin.org/screen3.png

significant amount of time explaining

IMHO it's probably better to put something like a reminder/note/extra option etc, instead of making such aggressive change.

Besides, this "ambiguous" "confusion" logic exists not only in the decoderawtransaction RPC call (oh it has been removed in this pr), but also in the GUI.

I didn't do a full rescan, but it's already enough to illustrate (taken from the freshly released Bitcoin Core 0.20.0):

I don't have the time and skills to do "commit archaeology", but however, I don't think it's good to treat this problem in such an aggressive way.

I think this should be fixed at the Bitcoin QT level as well, addresses for P2PK should not show up there.

I don't think it's good to treat this problem in such an aggressive way.

I had to deal with situations where one address receive a P2PK payment and wallet just crash unable to sign it. Then people see that difference "balances" show up on different block explorers, as some of them consider P2PK have an address and other not.

P2PK is dead. When somebody share an address he does not expect to be paid in P2PK (neither was expecting it at satoshi time!). This was a long time bug that caused too much bug, and misunderstanding.

P2PK is dead

Of course P2PK is ancient thing which is no longer used at all nowadays. However, those ancient P2PK UTXOs still consist of significant fraction of all existing bitcoins. There are also many historical posts mentioning them as 1-starting Base58Check-encoded addresses. Therefore I think it's still necessary to treat them properly. I don't think simply refusing to display them is a good way to treat them.

I had to deal with situations where one address receive a P2PK payment and wallet just crash unable to sign it.

Is P2PK still spendable? If the answer is still "yes", then probably it's the fault of that crashing wallet itself - to my understanding the wallet should fix the bug either by supporting signing P2PK properply or simply to ignore P2PK outputs at all. To me the former seems to be better, because the user would be probably confused that some funds would seem to be "missing".

Then people see that difference "balances" show up on different block explorers, as some of them consider P2PK have an address and other not.

Yeah it's bad. However I think providing a proper, unambiguous presentation for P2PK should be the correct way to fix this problem.

Therefore I think it's still necessary to treat them properly.

We are treating them properly. The proper way to treat P2PK output is to not show a P2PKH address for them. To show a P2PKH address for a P2PK script is literally incorrect and improper.

There are also many historical posts mentioning them as 1-starting Base58Check-encoded addresses.

And these should be corrected. Just because this is something that people do and have done does not mean that it is something we should continue to do. It is incorrect and misleading to show a P2PKH address for a P2PK script. This is a misconception that needs correcting, not appeasing.

However I think providing a proper, unambiguous presentation for P2PK should be the correct way to fix this problem.

Then P2PKH addresses are not that. They are literally unambiguously not P2PK. If you want to represent them as a P2PKH address, then those addresses become ambiguous which is not helpful either.

We already have an unambiguous, though verbose, representation of P2PK scripts: pk() descriptors. We could display a descriptor for P2PK (and maybe bare multisigs too) which are unambiguous representations of those scripts. However they can be very long and unwieldy so the user experience won't be that great.

Is P2PK still spendable? If the answer is still "yes", then probably it's the fault of that crashing wallet itself

The problem with this argument is that the number of spendable scripts by a public key is unlimited.
What about OP_NOP <pubkey> OP_CHECKSIG ? What about 1 <pubkey> 1 OP_MULTISIG ? Why those should not be spendable as well by the wallet and be treated as address?

The problem with this argument is that the number of spendable scripts by a public key is unlimited.

Of course.

Then why is Bitcoin Core still scanning out both P2PK and P2PKH transactions & counting them into the balance, in the case that the corresponding pubkey or privkey is imported? Is this a bug which needs to be fixed as well, by considering P2PK "nonstandard" so that they should be simply ignored/excluded from scanning result?

To show a P2PKH address for a P2PK script is literally incorrect and improper.

What's "correct" seems to be subjective here. What is P2PKH? It is in fact Base58Check-encoded HASH160 of a public key (together with a version number), then why can't it be used to represent a public key?

What about 1 1 OP_MULTISIG ? Why those should not be spendable as well by the wallet and be treated as address?

Aug.31th 2020: Note that BTC.COM has changed its behavoir (I don't know when they did this) that P2MS is no longer represented with (multiple) 1-starting P2PKH address(es), it shows an error message instead.

BTC.COM already did this for bare multisigs: https://btc.com/581d30e2a73a2db683ac2f15d53590bd0cd72de52555c2722d9d6a78e9fea510

Obviously this doesn't look very elegant & normative. However this is quite clear to me, in other words, it provides better usability than other block explorers which don't do this.

Just because this is something that people do and have done does not mean that it is something we should continue to do.

I never said we should continue to confuse P2PK and P2PKH. I completely agree with the point that the 1-starting P2PKH address itself should not match P2PK outputs. I just think the history should also be respected, just like the bare multisig transaction above, that those pubkeys were reused in other transactions by multiple times - I agree that it's not good behavoir & it should be discouraged. However it's just "fait accompli" which cannot be changed anymore.

And these should be corrected.

I'm afraid that there would always be some posts which can never be corrected. Simply refusing to display corresponding P2PKH address seems to contradict the history, thus making much more confusion & harming usability.

contradict the history, thus making much more confusion & harming usability.

The addresses that you were seeing in the satoshi's version were P2PKH, not P2PK by the way.

Then why is Bitcoin Core still scanning out both P2PK and P2PKH transactions & counting them into the balance, in the case that the corresponding pubkey or privkey is imported?

Backward compatibility, new wallet should not care.

The addresses that you were seeing in the satoshi's version were P2PKH, not P2PK by the way.

Yeah, to my knowledge P2PK only appears in ancient coinbase/generating transactions and pay-to-IP transactions (which was a deprecated & then removed feature). However it's still unclear how Satoshi himself would consider this problem.

But, anyway, just like what I have said multiple times, the community has been considering things like "the genesis address" "early miner address" from years ago up till now.

As we all know a hash is not reversible, so that the currently existing pk(KEY) and combo(KEY) descriptor is not an option if the user only knows the P2PKH address, which is essentially a hash. However it's totally feasible to match P2PK outputs with merely a P2PKH address. I never meant the P2PKH itself should match P2PK outputs. I just meant there should be an option to allow the user to do this, without making the ambiguity/confusion problem worse, probably with extended pk()/combo() output descriptor that they can accept either KEY or (P2PKH) ADDR.

There is always the descriptor for raw bytes that can be used for any scriptPubKey

There is always the descriptor for raw bytes that can be used for any scriptPubKey

The problem is that the pubkey itself is not yet known, what's known is only its HASH160.

Not sure what you mean. The raw descriptor works fine for me locally:

getdescriptorinfo "raw(4104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac)"

{
  "descriptor": "raw(4104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac)#6zf7atrr",
  "checksum": "6zf7atrr",
  "isrange": false,
  "issolvable": false,
  "hasprivatekeys": false
}

generatetodescriptor 1 "raw(4104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac)"

getblock(getbestblockhash(),2)

...

      "vin": [
        {
          "coinbase": "029b0e0101",
          "txinwitness": [
            "0000000000000000000000000000000000000000000000000000000000000000"
          ],
          "sequence": 4294967295
        }
      ],
      "vout": [
        {
          "value": 0.00001194,
          "n": 0,
          "scriptPubKey": {
            "asm": "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f OP_CHECKSIG",
            "hex": "4104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac",
            "type": "pubkey"
          }
        },
...

The raw descriptor works fine for me

Of course, but only if you know the pubkey itself. If you only know corresponding P2PKH address, which is the hash of the pubkey, I can't see how the current defined output descriptors can match it.

@andronoob Descriptors (apart from addr() and raw()) are designed to encapsulate all information necessary to spend an output (apart from secret keys). As such, a descriptor for a P2PK output without knowing the full pubkey makes no sense - you'll need the pubkey to spend it.

And if you don't have the pubkey, why do you care about the output in the first place?

don't have the pubkey

Obviously it can be known if any corresponding P2PK output does exist. It will be known once the corresponding P2PK output is matched.

why do you care about the output

Because P2PK and P2PKH share exactly the same ownership.

To be short, pk(1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa) distinguishes from addr(1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa). That's it.

I tried decodescript <compressed_p2pk_output_script> and then I found that the response data contained SegWit addresses converted from the pubkey,

decodescript 21037be71bdada01429453828831bdedc9a8b773ca9b7cb806a5c7eb9b1520504cbfac

{
  "asm": "037be71bdada01429453828831bdedc9a8b773ca9b7cb806a5c7eb9b1520504cbf OP_CHECKSIG",
  "type": "pubkey",
  "p2sh": "3E74dX1LvF6JcCyiCPK3XSaXjk2NFP11Z4",
  "segwit": {
    "asm": "0 d96e69a24dd5e55a927e8855a4d2c7a57853d870",
    "hex": "0014d96e69a24dd5e55a927e8855a4d2c7a57853d870",
    "reqSigs": 1,
    "type": "witness_v0_keyhash",
    "addresses": [
      "bc1qm9hxngjd6hj44yn73p26f5k854u98krsm0fn5n"
    ],
    "p2sh-segwit": "3BLEhRQy2YpqXjmGnD5s1MxG1M5eiMMQcu"
  }
}

while decodescript <uncompressed_p2pk_output_script> won't do such conversion.

decodescript 4104678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5fac

{
  "asm": "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f OP_CHECKSIG",
  "type": "pubkey",
  "p2sh": "3DjjKyU38gSfuVxajV43MUy4vHkg1JVL7T"
}

Such behavoir seems correct to me. However, if showing SegWit addresses is for convenience, why shouldn't that convenience include a P2PKH address as well?

I also found that decodescript would convert a P2PKH output script into SegWit by default. I think that may cause footguns, so I have submitted a new issue: #19383

Related: #19236

As a coder of blockchain explorers, I would like to chime in: would it be possible to decide on a standard way to represent these outputs ?

If no standard is defined, then this will just breed confusion with every wallet or explorer picking their own representations.

IMHO the RPC API is a good place to establish such a standard, as it is common ground (while QT UI f.i., is not)
Thanks for your consideration!

The only reasonable and unambiguous way to represent those is as a raw hex script, in my opinion.

Raw hex could work, but while these are not addresses, the public key can be the same as the one behind an address (and which becomes "public" when an address output is spent), so maintaining some relationship could be useful in terms of user comprehension.

Address reuse is problematic but it it is widespread (with miners, exchanges or PoS in alts), so it is common for the pubkey behind an address to be... public.

Would a prefixing or postfixing of the address associated to the pubkey work ?

Instead of "mpXwg4jMtRhuSpVq4xS3HFHmCmWp9NyGKt" it could be reported as "p2pk_mpXwg4jMtRhuSpVq4xS3HFHmCmWp9NyGKt" or something similar ?

In my opinion, P2PK should be represent as output descriptor like pk(PUBKEY), because that's what it technically is.

I think it's also good to allow the user to match P2PK output with its corresponding P2PKH address like (this is not yet supported) pk(ADDR). However pk(ADDR) is not so "transparent" like pk(PUBKEY).

pk(1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa) distinguishes from addr(1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa) or 1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa. I think that's good enough, although it still requires some explaination.

wait so if i import a pubkey into the client and it says "solvable" true does this mean the wallet client can spend the funds? if so why does it contradict and not show up in the balance but on watch only?

@etherx-dev Please don't ask questions on random, unrelated, old, closed, pull requests. You'll probably find better information on https://bitcoin.stackexchange.com.