Add a `-permitbarepubkey` option

vostrnad commented at 4:19 am on January 25, 2024: none

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 commented at 4:19 am on January 25, 2024: contributor

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 commented at 8:51 am on January 25, 2024: none

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 commented at 11:17 am on January 25, 2024: contributor

Concept ACK.

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

There is, it gives node runners choice.

chrisguida commented at 2:30 pm on January 25, 2024: none

Concept ACK, it’s great to give users the option of filtering out potentially abusive txs :+1:

Retropex approved

Retropex commented at 6:10 pm on January 25, 2024: none

tACK https://github.com/vostrnad/bitcoin/commit/8c1114aa61c46e58efb44368b5428d3ccb65f9d0

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:

0[
1  {
2    "txid": "03c1140befa500e1809f5fa6950a45516f651a2e88fe252ac6c9fec393c68f11",
3    "wtxid": "03c1140befa500e1809f5fa6950a45516f651a2e88fe252ac6c9fec393c68f11",
4    "allowed": false,
5    "reject-reason": "bare-pubkey"
6  }
7]

in src/init.cpp:638 in 8c1114aa61 outdated

634@@ -635,6 +635,8 @@ void SetupServerArgs(ArgsManager& argsman)
635                              MAX_OP_RETURN_RELAY),
636                    ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
637     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);
638+    argsman.AddArg("-permitbarepubkey", strprintf("Relay legacy pubkey outputs (default: %u)", DEFAULT_PERMIT_BAREPUBKEY), ArgsManager::ALLOW_ANY,

shaavan commented at 1:25 pm on January 26, 2024:

nit: Should we mention P2PK here?

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

shaavan commented at 1:25 pm on January 26, 2024: contributor

Concept ACK

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

vostrnad commented at 8:09 pm on January 26, 2024: none

@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 commented at 6:41 am on January 27, 2024: contributor

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 commented at 7:40 am on January 27, 2024: none

@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 commented at 9:15 am on January 27, 2024: contributor

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.

in src/policy/policy.h:39 in 8c1114aa61 outdated

34@@ -35,6 +35,8 @@ static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/
35 static constexpr unsigned int DEFAULT_INCREMENTAL_RELAY_FEE{1000};
36 /** Default for -bytespersigop */
37 static constexpr unsigned int DEFAULT_BYTES_PER_SIGOP{20};
38+/** Default for -permitbarepubkey */
39+static constexpr bool DEFAULT_PERMIT_BAREPUBKEY{true};

1440000bytes commented at 3:18 pm on January 27, 2024:

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.

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

luke-jr commented at 7:58 pm on February 20, 2024: member

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

luke-jr referenced this in commit 7cdc0b4b41 on Mar 10, 2024

DrahtBot added the label Needs rebase on May 1, 2024

vostrnad force-pushed on May 1, 2024

DrahtBot removed the label Needs rebase on May 1, 2024

DrahtBot added the label Needs rebase on May 15, 2024

Add a `-permitbarepubkey` option 1dfe27e49a

vostrnad force-pushed on May 15, 2024

DrahtBot removed the label Needs rebase on May 15, 2024

luke-jr referenced this in commit 62533f5887 on Jun 13, 2024

luke-jr referenced this in commit da23cc3f53 on Jun 19, 2024

DrahtBot added the label Needs rebase on Aug 7, 2024

DrahtBot commented at 3:31 pm on August 7, 2024: contributor

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

vostrnad closed this on Aug 7, 2024

Retropex commented at 6:42 pm on August 7, 2024: none

Why closing this? @vostrnad

vostrnad commented at 6:50 pm on August 7, 2024: none

@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 commented at 5:18 pm on August 8, 2024: none

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.

Add a -permitbarepubkey option #29309

Code Coverage

Reviews

Conflicts

Add a `-permitbarepubkey` option #29309