Add a -permitbarepubkey option

[vostrnad]

Partially fixes #29285 (adds the option but leaves the default policy unchanged)

No tests so far, if there's consensus to move forward with this I'll look into adding them.

Disclaimer: I don't care if this gets merged, personally I don't see any benefit of having this option (or -permitbaremultisig) and I would object to changing the default policy in this regard (similar to my objections to #28217). However, as I think someone would inevitably open a PR for this, I thought this might be a good opportunity to dip my toes into Bitcoin Core development.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	kristapsk, chrisguida, shaavan
Stale ACK	Retropex

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29942 (Remove redundant -datacarrier option by vostrnad)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[Retropex]

Concept NACK, there's no point of adding this if default policy is with P2PK enabled so it certainly doesn't fix #29285.

Concept ACK.

For me it's a good idea to already have code to do it, once merged we can discuss a default activation.

[kristapsk]

Concept ACK.

there's no point of adding this if default policy is with P2PK enabled

There is, it gives node runners choice.

[chrisguida]

Concept ACK, it's great to give users the option of filtering out potentially abusive txs 👍

[Retropex]

tACK vostrnad@8c1114a

Steps to test:

1. Build vostrnad branch
2. Start a regtest with -chain=regtest
3. Create a wallet with bitcoin-cli createwallet
4. Get address with bitcoin-cli getnewaddress
5. Generate blocks to get bitcoins generatetoaddress 500 <address obtained previously>
6. Create a P2PK tx, this repository can be useful to you.
7. Test if the tx is accepted with bitcoin-cli testmempoolaccept '["<RAW tx>"]'

Result obtained in my case with -permitbarepubkey=0:

[
  {
    "txid": "03c1140befa500e1809f5fa6950a45516f651a2e88fe252ac6c9fec393c68f11",
    "wtxid": "03c1140befa500e1809f5fa6950a45516f651a2e88fe252ac6c9fec393c68f11",
    "allowed": false,
    "reject-reason": "bare-pubkey"
  }
]

[shaavan]

Concept ACK

Problems of P2PK:

1. Security Risk: Directly exposing the public key poses a risk if cryptographic standards change or are compromised. Pubkey is invariably exposed at the spent time for all the transaction types. This is not a problem limited to P2PK.
2. Larger Transaction Size: P2PK transactions are larger due to the inclusion of full public keys, leading to higher transaction fees. The difference becomes insignificant after the usage of compressed Pubkey as the standard.
3. Privacy Concerns: Exposing public keys can compromise user privacy by allowing easier linkage of transactions.

Considering the flaws of P2PK and the fact that it has almost been phased out from usage (https://transactionfee.info/charts/inputs-and-outputs-p2pk/?start=2019-01-01) I think it's a good idea to default to not relaying P2PK transaction. And I believe this PR is a good first step in doing that.

Code-wise, the PR looks great, and I think it’s a good idea to go forth with test implementation for it.

Edit: Removed the Problems with the P2PK section in light of the following discussion. 1, 2, 3

[shaavan]

nit: Should we mention P2PK here?

Suggested change

	argsman.AddArg("-permitbarepubkey", strprintf("Relay legacy pubkey outputs (default: %u)", DEFAULT_PERMIT_BAREPUBKEY), ArgsManager::ALLOW_ANY,
	argsman.AddArg("-permitbarepubkey", strprintf("Relay legacy pubkey (P2PK) outputs (default: %u)", DEFAULT_PERMIT_BAREPUBKEY), ArgsManager::ALLOW_ANY,

[vostrnad]

@shaavan The section "Problems of P2PK" seems completely wrong to me:

1. P2TR also directly exposes a public key in its output, and all other output types expose it at spend time.
2. P2PK outputs actually have a smaller footprint than P2PKH over their lifecycle because there's no pubkey hash overhead.
3. Exposing public keys has nothing to do with linkage of transactions, and again, all output types do it.

There are problems with P2PK, just not these ones. Did you perhaps use ChatGPT to generate this section?

[shaavan]

Thanks, @vostrnad, for pointing out the potential flaws in my review.

Yes, I did use ChatGPT to structure my thoughts on the “problems with P2PK” in a coherent fashion, but I don’t believe the points are wrong.

Taking references from, Programming Bitcoin, Jimmy Song

https://github.com/jimmysong/programmingbitcoin/blob/master/ch06.asciidoc#problems-with-p2pk

1. Problem of Security: P2PK, exposes the bare public key to the world, and if ever ECDSA gets broken, we might lose our funds

Reference:

because we’re storing the public keys in the ScriptPubKey field, they’re known
to everyone. That means should ECDSA someday be broken, these outputs could be
stolen.

2. The length of pubkey (especially uncompressed pubkey) was one of the major reasons that P2PKH was introduced in the first place

Reference:

First, the public keys are long. … early Bitcoin transactions didn’t use the compressed ver‐
sions, so the hexadecimal addresses were 130 characters each! This is not fun or easy
for people to transcribe, much less communicate by voice.

Second, the length of the public keys causes a subtler problem: because they have to
be kept around and indexed to see if the outputs are spendable, the UTXO set
becomes bigger. This requires more resources on the part of full nodes.

3. The third point of transaction linkage again links back to the first point. If one had used the same private key to derive multiple pubkey over the ECDSA curve, and if it ever gets compromised, one can easily link the transaction to the same source.

I would love to hear your thoughts on it. Any points of correction on my understanding of the problem are very appreciated.
Thank you.

[vostrnad]

@shaavan

1. Putting bare public keys into the output doesn't make things materially worse if ECDSA/ECDLP gets broken. This was agreed on when designing Taproot, which is why P2TR outputs also use bare public keys (see BIP341).
2. Yes, P2PKH has a shorter output script, but overall uses more blockspace than P2PK. It does take less space in the UTXO set, but P2PK only takes one more byte than e.g. P2TR or P2WSH. Note that P2PKH was introduced at a time when compressed keys were not known.
3. A private key can only generate one public key. And again, all output types expose the public key, just some at spending time. This linkage theory doesn't make any sense, and since you aren't sourcing it, I'm assuming it was generated entirely by ChatGPT. (Out of curiosity, I tried asking ChatGPT for the weaknesses in P2PK, and it did indeed include "enabling easy linkage of transactions".)

If you have any other questions, please post them to Bitcoin Stack Exchange (where I'll gladly respond). GitHub comments are meant for discussing the code change at hand, not for asking general Bitcoin questions.

Lastly, I would like to ask you not to use ChatGPT for writing pull request reviews. If I want to know what it'll hallucinate when asked for a review, I can ask it myself.

[shaavan]

Thanks, @vostrnad, for helping me bring clarity to my understanding.

I understand that my points were made based on my limited knowledge surrounding the discussions around P2TR. And also, I made an incorrect point in my explanation.

I have struck the “Problems of P2PK” section to avoid confusion for future reviewers.

Lastly, though I might sometimes use ChatGPT to get better clarity on concepts or coherently express my thoughts, I will make sure to properly fact-check it before using any information when writing a review to ensure its quality.

Thank you again.

[1440000bytes]

Concept ACK because default policy is unchanged.

I don't care if this gets merged, personally I don't see any benefit of having this option (or -permitbaremultisig) and I would object to changing the default policy in this regard (similar to my objections to #28217).

It might be useful for testing and a few other use cases.

Concept ACK, it's great to give users the option of filtering out potentially abusive txs

The only potential abuse I foresee in the future is trying to make it default in another pull request without rationale, agreement from reviewers, etc.

[luke-jr]

@shaavan Also note ChatGPT output is not necessary clear of copyright issues, so should never be included directly.