policy: treat P2TR outputs with invalid x-only pubkey as non-standard

theStack commented at 3:41 am on January 20, 2022: contributor

P2TR outputs with an invalid x-only pubkey (i.e. one that is not on the secp256k1 curve) can obviously never be spent: https://suredbits.com/taproot-funds-burned-on-the-bitcoin-blockchain/ (https://twitter.com/Suredbits/status/1483804177507790863?s=20) Treating transactions with such unspendable ouputs as non-standard is a measurement to prevent users from burning funds and bloating the UTXO set. The x-only pubkey for the introduced unit tests are taken from the link above (invalid one) and the first ever taproot transaction in mainnet (valid one): https://twitter.com/Bitcoin/status/1459911733166829568

Further possible related improvements:

treat bech32m (segwit v1) addresses as invalid if the encoded x-only pubkey is not on the curve (most importantly affects wallet sending RPCs, but also verifyaddress)
mark these outputs as unspendable and remove them from the UTXO set (idea expressed by ajtowns on IRC)

fanquake added the label TX fees and policy on Jan 20, 2022

fanquake commented at 7:33 am on January 20, 2022: member

Restarted the ARM CI, which failed due to an intermittent download issue. Failure in the fuzzer ci:

0INFO: Seed: 4040717080
1INFO: Loaded 1 modules   (722258 inline 8-bit counters): 722258 [0x55ae9f2bf690, 0x55ae9f36fbe2), 
2INFO: Loaded 1 PC tables (722258 PCs): 722258 [0x55ae9f36fbe8,0x55ae9fe75108), 
3INFO:     4124 files found in /tmp/cirrus-ci-build/ci/scratch/qa-assets/fuzz_seed_corpus/script
4INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
5INFO: seed corpus: files: 4124 min: 1b max: 3146072b total: 220237367b rss: 162Mb
6fuzz: test/fuzz/script.cpp:62: void script_fuzz_target(FuzzBufferType): Assertion `which_type == TxoutType::NONSTANDARD || which_type == TxoutType::NULL_DATA || which_type == TxoutType::MULTISIG' failed.
7==19805== ERROR: libFuzzer: deadly signal

policy: treat P2TR outputs with invalid x-only pubkey as non-standard

P2TR outputs with an invalid x-only pubkey (i.e. a pubkey
that is not on the secp256k1 curve) can never be spent.
Treating them as non-standard is a measurement to prevent
users from burning funds and bloating the UTXO set.

0216b044ce

test: add unit-test for P2TR output non-standard policy b6a45a834c

theStack force-pushed on Jan 20, 2022

theStack commented at 8:51 am on January 20, 2022: contributor

Restarted the ARM CI, which failed due to an intermittent download issue. Failure in the fuzzer ci:

0INFO: Seed: 4040717080
1INFO: Loaded 1 modules   (722258 inline 8-bit counters): 722258 [0x55ae9f2bf690, 0x55ae9f36fbe2), 
2INFO: Loaded 1 PC tables (722258 PCs): 722258 [0x55ae9f36fbe8,0x55ae9fe75108), 
3INFO:     4124 files found in /tmp/cirrus-ci-build/ci/scratch/qa-assets/fuzz_seed_corpus/script
4INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
5INFO: seed corpus: files: 4124 min: 1b max: 3146072b total: 220237367b rss: 162Mb
6fuzz: test/fuzz/script.cpp:62: void script_fuzz_target(FuzzBufferType): Assertion `which_type == TxoutType::NONSTANDARD || which_type == TxoutType::NULL_DATA || which_type == TxoutType::MULTISIG' failed.
7==19805== ERROR: libFuzzer: deadly signal

Thanks, the fuzzing issue should be fixed with the latest forced-push.

w0xlt approved

w0xlt commented at 11:50 am on January 20, 2022: contributor

crACK b6a45a8

sipa commented at 1:20 pm on January 20, 2022: member

Concept NACK as-is, I’m afraid.

This new policy can be used to hurt batch withdrawals. Imagine a user withdrawing funds from an exchange, specifying an bech32m address for a point not on the curve. With this policy, the entire transaction will be rejected, including any other addresses the other outputs of the transactions were trying to pay.

If we really want to push for this, I think far more is necessary:

Discuss a change like this on the mailinglist and with industry.
Start by rejecting such outputs in our address parser.
Get other address parsers (including open source libraries, and potential closed-source ones) to do the same.
Finally, decide to introduce a policy to actually mark these nonstandard, perhaps with a delay so other software can start doing this consistently.

Anything else, I’m afraid, will hurt bech32m adoption even more, as reports of stuck withdrawals may cause organizations to choose to not adopt it at all.

sdaftuar commented at 1:22 pm on January 20, 2022: member

This type of protection is something we’ve discussed many times in the past in various forms, and we’ve generally avoided adding footgun protections like this, with the reasoning being that the set of things that a transaction creator might do to burn funds is simply too great for this approach to make sense.

Moreover, there’s an additional cost to these protections, which is that users participating in batch payment protocols (perhaps a coinjoin, or maybe an exchange withdrawal) can cause operational problems for others if transactions which would have been relayed in the past would no longer be relayed – eg if one user supplies an address to a batched-output transaction process, and we then make such an address policy-invalid, then that might cause headaches for everyone else, as the resulting transaction relays less well and/or fails to be mined. To avoid it, the transaction creator/process would need to be made aware of the new restriction, too.

I think the principle we’ve settled on historically is that we’d like to standardize the set of checks that transaction creators might run on a given address to precisely the things that are defined in the various BIPs that define what a valid P2PKH, P2SH, or bech32(m) address is, so that there is certainty on the network around what addresses people can be using, and leave it to the users creating addresses to be creating valid ones.

maflcko commented at 1:42 pm on January 20, 2022: member

For reference, #15846 was the previous discussion, which also includes a link to the IRC discussion.

Maybe it could make sense to add a line or two as documentation?

Edit: And a fuzz test:

 0diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script.cpp
 1index eb170aab76..8524822f96 100644
 2--- a/src/test/fuzz/script.cpp
 3+++ b/src/test/fuzz/script.cpp
 4@@ -55,12 +55,19 @@ FUZZ_TARGET_INIT(script, initialize_script)
 5     }
 6 
 7     TxoutType which_type;
 8-    bool is_standard_ret = IsStandard(script, which_type);
 9+    const bool is_standard_ret{IsStandard(script, which_type)};
10     if (!is_standard_ret) {
11         assert(which_type == TxoutType::NONSTANDARD ||
12                which_type == TxoutType::NULL_DATA ||
13                which_type == TxoutType::MULTISIG);
14     }
15+    {
16+        int v;
17+        std::vector<uint8_t> p;
18+        if (script.IsWitnessProgram(v, p)) {
19+            assert(is_standard_ret);
20+        }
21+    }
22     if (which_type == TxoutType::NONSTANDARD) {
23         assert(!is_standard_ret);
24     }

kallerosenbaum commented at 3:37 pm on January 20, 2022: none

If such a policy was implemented, would the CPU time required to verify all segwit v1 pubkeys be negligible? What ballpark are we talking about?

sipa commented at 3:44 pm on January 20, 2022: member

@kallerosenbaum A benchmark would be useful, but my ballpark estimate would be in the order of 3+ microseconds on modern desktop x86_64 CPUs. The current implementation of XOnlyPubkey actually fully decompresses the public key, which isn’t necessary. A more optimized implementation (using code already available in libsecp, but not exposed through its API) could probably speed it up by a factor 2x-3x still.

That said, I still think this is a bad idea to introduce as a policy. It may make sense in address decoding though (just as a warning), but there performance isn’t that critical.

kallerosenbaum commented at 3:57 pm on January 20, 2022: none

@sipa thanks. 100,000 segwit v1 outputs per hour would cost at least in the order of 0.1s CPU per hour, then, given your mentioned optimizations. (Note, I’m not suggesting this policy is a good idea)

theStack commented at 12:35 pm on January 21, 2022: contributor

@sipa @sdaftuar: Good points, I agree that this policy change could lead to problems in batching transactions and similar scenarios that I haven’t thought about, potentially hindering adoption. My initial assumption would be that exchanges are professional enough to know what they are doing and are able to fix such a potential problem quite quickly if it comes up (rather than going a step backwards and removing bech32m support completely), but probably that is too optimistic.

What I think can be easily done though is to at least warn the user in RPCs like verifyaddress etc. if the address output contains an invalid x-only pubkey. This is the first address type that can be verifiably unspendable (previously it was just hashes encoded), so the concept is new, but we could introduce an unspendable field in the result or similar.

maflcko commented at 1:36 pm on January 21, 2022: member

My initial assumption would be that exchanges are professional enough to know what they are doing

Exchanges can’t even decode valid addresses (https://bitcoin.stackexchange.com/a/111441/3576), so expecting them to implement an ever-changing list of heuristics to detect valid addresses that hold an “invalid” payload seems overly optimistic.

This is the first address type that can be verifiably unspendable

No, the same is possible with v0 bech32 addresses: https://github.com/bitcoin/bitcoin/blob/e3ce019667fba2ec50a59814a26566fb67fa9125/src/script/interpreter.cpp#L1914 . Potentially others as well if you know what was wrapped into a script-hash, see #23486.

verifyaddress

Adding a field to validateaddress/decodescript seems ok, understanding that this can only ever be a heuristic and not something to rely on.

theStack commented at 3:32 pm on January 21, 2022: contributor

Exchanges can’t even decode valid addresses (https://bitcoin.stackexchange.com/a/111441/3576), so expecting them to implement an ever-changing list of heuristics to detect valid addresses that hold an “invalid” payload seems overly optimistic.

I agree that my assumption w.r.t. to exchange competence is overly optimistic, though I neither think the list would be “ever-changing” nor that it even needs a heuristic (see last paragraph below). Addresses so far only encode either hashes or pub-keys; hashes are neutral, for pub-keys the only way to be verifiably unspendable is if they are not on the curve.

This is the first address type that can be verifiably unspendable

No, the same is possible with v0 bech32 addresses:

https://github.com/bitcoin/bitcoin/blob/e3ce019667fba2ec50a59814a26566fb67fa9125/src/script/interpreter.cpp#L1914

Fair enough, though v0 bech32 addresses with witness program sizes other than 20 or 32 bytes don’t match a specific output type and thus were never treated as standard in the first place. Let me rephrase: “This is the first address-encoded output script type that can be verifiably unspendable.” (I think previously this was only possible for P2PK and bare multisig, but those didn’t have an address format).

Potentially others as well if you know what was wrapped into a script-hash, see #23486.

Yes, but that was not the point of this PR / discussion. I’m assuming that we don’t know anything other than the output script.

Adding a field to validateaddress/decodescript seems ok, understanding that this can only ever be a heuristic and not something to rely on.

Not sure why you would call this heuristic, as in this specific case there are no “smart guesses” or probabilities involved. If the x-only pubkey is not on the curve, then there is verifiably no chance this output can ever be spent. I.e. the unspendable result flag would only be set if we are sure about it, without any sophisticated heuristics involved. (This does of course not mean that all outputs where the unspendable flag is not set, are spendable; there is no way to guarantee that).

maflcko commented at 4:23 pm on January 21, 2022: member

Sorry, I wrongly assumed that v0 witness programs of sizes without attached meaning are standard (WITNESS_UNKNOWN). In fact they are mapped to NONSTANDARD, and thus not relayed as you point out. However, all other witness programs, including v1 programs of sizes different than the taproot output size are treated standard (WITNESS_UNKNOWN). They should also be relayed, see #15846.

For those, it is impossible to run any checks because the meaning of the program payloads isn’t known yet. However, as transactions with those outputs are standard today and are relayed, you may end up populating your address book with (let’s say v2) addresses. This is by design, so that users of an exchange can start using a new witness version before the exchange updates its backend nodes to understand the new witness version. Once the exchange updates the backend to understand the new rules, they might find that some addresses or past payouts are unspendable. With “heuristic” I meant that even an exchange that always runs the latest software can not protect against this, as a user today might go ahead and request a payout to a witness program that they assume to be unspendable in the future, based on a draft of the next soft fork. Or to quote your words: “(This does of course not mean that all outputs where the unspendable flag is not set, are spendable; there is no way to guarantee that).”

sipa commented at 4:27 pm on January 21, 2022: member

@MarcoFalke Yeah, thought V0 witness outputs with payloads of sizes other than 20/32 are a special case, as those are even consensus unspendable (and thus BIP173 says to treat them as invalid). In retrospect, I think that was a mistake (the fact that they’re consensus unspendable).

theStack commented at 7:35 pm on January 21, 2022: contributor

@MarcoFalke: Thanks for clarifying, that makes sense. IIUC, the conclusion of this is, all output scripts that are treated as standard by now, will/should be treated as standard forever (particularly, all v1+ segwit outputs for the future), and we only ever extend the set, but likely never reduce it?

theStack commented at 2:07 pm on January 28, 2022: contributor

Closing this PR, since this approach would introduce many potential problems (as pointed out in #24106 (comment), #24106 (comment), #24106 (comment)).

theStack closed this on Jan 28, 2022

theStack deleted the branch on Dec 15, 2022

bitcoin locked this on Dec 15, 2023

policy: treat P2TR outputs with invalid x-only pubkey as non-standard #24106