Add a `-permitbarepubkey` option #29309

vostrnad · 2024-01-25T04:19:17Z

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 · 2024-01-25T04:19:20Z

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:

#30594 (docs: doc update for mempoolfullrbf default + log deprecation by glozow)
#30592 (Remove mempoolfullrbf by instagibbs)
#30239 (Ephemeral Dust by instagibbs)
#30232 (refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options by luke-jr)
#29942 (Remove redundant -datacarrier option by vostrnad)
#29520 (add -limitdummyscriptdatasize option by Retropex)

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 · 2024-01-25T08:51:13Z

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 · 2024-01-25T11:17:02Z

Concept ACK.

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

There is, it gives node runners choice.

chrisguida · 2024-01-25T14:30:45Z

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

Retropex

tACK vostrnad@8c1114a

Steps to test:

Build vostrnad branch
Start a regtest with -chain=regtest
Create a wallet with bitcoin-cli createwallet
Get address with bitcoin-cli getnewaddress
Generate blocks to get bitcoins generatetoaddress 500 <address obtained previously>
Create a P2PK tx, this repository can be useful to you.
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:~~

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.
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.
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 · 2024-01-26T13:25:05Z

src/init.cpp

@@ -635,6 +635,8 @@ void SetupServerArgs(ArgsManager& argsman)
                             MAX_OP_RETURN_RELAY),
                   ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
    argsman.AddArg("-mempoolfullrbf", strprintf("Accept transaction replace-by-fee without requiring replaceability signaling (default: %u)", DEFAULT_MEMPOOL_FULL_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
+    argsman.AddArg("-permitbarepubkey", strprintf("Relay legacy pubkey outputs (default: %u)", DEFAULT_PERMIT_BAREPUBKEY), ArgsManager::ALLOW_ANY,


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 · 2024-01-26T20:09:40Z

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

P2TR also directly exposes a public key in its output, and all other output types expose it at spend time.
P2PK outputs actually have a smaller footprint than P2PKH over their lifecycle because there's no pubkey hash overhead.
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 · 2024-01-27T06:41:34Z

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

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.

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.

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 · 2024-01-27T07:40:34Z

@shaavan

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).
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.
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 · 2024-01-27T09:15:00Z

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.

src/policy/policy.h

+/** Default for -permitbarepubkey */
+static constexpr bool DEFAULT_PERMIT_BAREPUBKEY{true};


luke-jr · 2024-02-20T19:58:47Z

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

Github-Pull: bitcoin#29309 Rebased-From: 8c1114a

DrahtBot · 2024-08-07T15:31:56Z

🐙 This pull request conflicts with the target branch and needs rebase.

Retropex · 2024-08-07T18:42:06Z

Why closing this? @vostrnad

vostrnad · 2024-08-07T18:50:03Z

@Retropex I've rebased this enough times already for something I don't really think should be merged. Also I suspect anyone interested in this option has already switched to Knots anyway (which has it implemented).

chrisguida · 2024-08-08T17:18:12Z

Hmm, odd choice to leave out even the ability to filter out potentially abusive txs.

But sure, I guess we've started a process where Knots is the client that gives users a choice over what to relay, whereas Core just forces them to relay certain things even if they don't want to.

Seems like a confusing path for Core to want to head down, but maybe mempool policy should be unbundled from the rest of the consensus code anyway.

Github-Pull: bitcoin#29309 Rebased-From: 8c1114a

Retropex approved these changes Jan 25, 2024

View reviewed changes

shaavan reviewed Jan 26, 2024

View reviewed changes

1440000bytes reviewed Jan 27, 2024

View reviewed changes

src/policy/policy.h

Comment on lines +38 to +39

/** Default for -permitbarepubkey */

static constexpr bool DEFAULT_PERMIT_BAREPUBKEY{true};

This comment was marked as abuse.

Sign in to view

willcl-ark added the TX fees and policy label Jan 30, 2024

DrahtBot mentioned this pull request Mar 1, 2024

add -limitdummyscriptdatasize option #29520

Closed

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Mar 10, 2024

Add a -permitbarepubkey option

7cdc0b4

Github-Pull: bitcoin#29309 Rebased-From: 8c1114a

DrahtBot mentioned this pull request Mar 29, 2024

test: Add test case for spending bare multisig #29120

Merged

DrahtBot mentioned this pull request Apr 24, 2024

Remove redundant -datacarrier option #29942

Closed

DrahtBot added the Needs rebase label May 1, 2024

vostrnad force-pushed the permitbarepubkey branch from 8c1114a to ffc6c0b Compare May 1, 2024 20:16

DrahtBot removed the Needs rebase label May 1, 2024

DrahtBot mentioned this pull request May 7, 2024

refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition #29086

Merged

DrahtBot added the Needs rebase label May 15, 2024

Add a -permitbarepubkey option

1dfe27e

vostrnad force-pushed the permitbarepubkey branch from ffc6c0b to 1dfe27e Compare May 15, 2024 14:06

DrahtBot removed the Needs rebase label May 15, 2024

This was referenced Jun 6, 2024

refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options #30232

Closed

Ephemeral Dust #30239

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024

Add a -permitbarepubkey option

62533f5

Github-Pull: bitcoin#29309 Rebased-From: 8c1114a

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 19, 2024

Add a -permitbarepubkey option

da23cc3

Github-Pull: bitcoin#29309 Rebased-From: 8c1114a

DrahtBot mentioned this pull request Aug 5, 2024

Remove mempoolfullrbf #30592

Merged

DrahtBot mentioned this pull request Aug 7, 2024

docs: doc update for mempoolfullrbf default + log deprecation #30594

Merged

DrahtBot added the Needs rebase label Aug 7, 2024

vostrnad closed this Aug 7, 2024

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Feb 22, 2025

Add a -permitbarepubkey option

df9da3a

Github-Pull: bitcoin#29309 Rebased-From: 8c1114a

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 5, 2025

Add a -permitbarepubkey option

0a7566b

Github-Pull: bitcoin#29309 Rebased-From: 8c1114a

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jul 17, 2025

Add a -permitbarepubkey option

c56b5cb

Github-Pull: bitcoin#29309 Rebased-From: 8c1114a

bitcoin locked and limited conversation to collaborators Aug 8, 2025

	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,

		/** Default for -permitbarepubkey */
		static constexpr bool DEFAULT_PERMIT_BAREPUBKEY{true};

Add a -permitbarepubkey option #29309

Add a -permitbarepubkey option #29309

Uh oh!

Conversation

vostrnad commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Retropex commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristapsk commented Jan 25, 2024

Uh oh!

chrisguida commented Jan 25, 2024

Uh oh!

Retropex left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaavan Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

vostrnad commented Jan 26, 2024

Uh oh!

shaavan commented Jan 27, 2024

Uh oh!

vostrnad commented Jan 27, 2024

Uh oh!

shaavan commented Jan 27, 2024

Uh oh!

This comment was marked as abuse.

Uh oh!

luke-jr commented Feb 20, 2024

Uh oh!

DrahtBot commented Aug 7, 2024

Uh oh!

Retropex commented Aug 7, 2024

Uh oh!

vostrnad commented Aug 7, 2024

Uh oh!

chrisguida commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Add a `-permitbarepubkey` option #29309

Add a `-permitbarepubkey` option #29309

vostrnad commented Jan 25, 2024 •

edited

Loading

DrahtBot commented Jan 25, 2024 •

edited

Loading

Retropex commented Jan 25, 2024 •

edited

Loading

shaavan left a comment •

edited

Loading

chrisguida commented Aug 8, 2024 •

edited

Loading