p2p: stop special-casing witness-stripped error for unconfirmed transactions

darosior commented at 2:23 pm on April 29, 2025: member

Opening as draft to facilitate discussions, this has some issues.

This special-case was introduced in #18044 to avoid hindering relay of witness transactions in adversarial situations where most connections on the network were still using txid-based relay. This is not a concern anymore. Since it is expensive to detect, get rid of the special case.

This may however cause issues for orphan resolution in adversarial situations, since fetching the parent is done using its txid. Greg Sanders pointed out to me on IRC this could be an issue for package relay. I’m opening here as draft to facilitate discussion.

DrahtBot commented at 2:23 pm on April 29, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32379.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32421 (test: refactor: overhaul (w)txid determination for CTransaction objects by theStack)
#32317 (kernel: Separate UTXO set access from validation functions by TheCharlatan)

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.

DrahtBot added the label P2P on Apr 29, 2025

in test/functional/p2p_orphan_handling.py:263 in 70e2b5f15b outdated

266         self.relay_transaction(peer2, child_invalid_witness["tx"])
267         assert child_invalid_witness["txid"] not in node.getrawmempool()
268-        assert tx_in_orphanage(node, child_invalid_witness["tx"])
269-
270-        # The parent should be requested since the unstripped wtxid would differ. Delayed because
271-        # it's by txid and this is not a preferred relay peer.

darosior commented at 2:27 pm on April 29, 2025:

It seems this comment was wrong in the first place. The parent is requested by txid, so the unstripped wtxid doesn’t matter. The only reason that this was working is because WITNESS_STRIPPED was special cased as a failure and we didn’t add the wtxid of the witness-stripped transaction (i.e. its txid) to the reject filters. Not special-casing this issue prevents requesting the parent since now its txid would be in the reject filter.

glozow commented at 4:32 pm on April 29, 2025:

Yep. Should say ‘parent should be requested since witness stripped-transaction rejections shouldn’t be cached at all’

glozow commented at 2:30 pm on April 29, 2025: member

This may however cause issues for orphan resolution in adversarial situations, since fetching the parent is done using its txid. Greg Sanders pointed out to me on IRC this could be an issue for package relay.

Just want to point out this is one of the motivations for (real) package relay, so we can get rid of txid-based fetching. https://github.com/bitcoin/bips/blob/master/bip-0331.mediawiki#handle-orphans-better

Today however, you’d be able to block (opportunistic) “package relay” by sending the parent witness-stripped first.

in src/node/txdownloadman_impl.cpp:464 in 70e2b5f15b outdated

459-        // We can remove this restriction (and always add wtxids to
460-        // the filter even for witness stripped transactions) once
461-        // wtxid-based relay is broadly deployed.
462-        // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
463-        // for concerns around weakening security of unupgraded nodes
464-        // if we start doing this too early.

glozow commented at 4:30 pm on April 29, 2025:

I don’t think you want to delete the entire comment, just the part about witness stripping

darosior commented at 5:54 pm on April 29, 2025:

Done.

tx download: add wtxid to reject filter even if witness was stripped

We previously didn't for witness-stripped transactions due to compatibility concerns with
non-wtxid-relay peers.

The issue we were trying to prevent is attacker node A stripping the witness of a transaction
broadcast by honest node B and submitting it stripped to a large part of the network. It would
result in those nodes including the txid of the transaction (== witness-stripped wtxid) to their
recent reject filter, which would significantly hinder the propagation of the honest non-stripped
transaction if most connections on the network are txid based relay.

Support for wtxid based relay was released with 0.21 in January 2021 and is now ubiquitous on the
network. Therefore this concern does not apply anymore and we can get rid of this special case.

07018a44f8

validation: avoid redundant scriptsig validation in PolicyScriptChecks

This was performed to identify transactions whose witness was stripped and is not necessary anymore.

f5f074eacc

Remove now-unused WITNESS_STRIPPED variant of TxValidationResult 25282b653b

darosior force-pushed on Apr 29, 2025

glozow commented at 8:50 pm on May 5, 2025: member

From the offline conversation, here’s a branch that drops the “don’t resolve orphans with a parent in RecentRejects” logic, meaning we’ll still keep an orphan of a witness stripped-transaction after this PR. This doesn’t fully fix the fact that 1p1c is disrupted, because the parent is still not requested (its txid is AlreadyHave). However, keeping the orphan means we still accept the 1p1c if we receive the parent via wtxid.

Also recommend squashing the “Remove now-unused WITNESS_STRIPPED variant of TxValidationResult” commit into “tx download: add wtxid to reject filter even if witness was stripped”

DrahtBot added the label Needs rebase on Jun 11, 2025

DrahtBot commented at 10:21 pm on June 11, 2025: contributor

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

darosior commented at 6:08 pm on June 18, 2025: member

It took me a while to remember the context here, since we had those discussions offline. I’ll try to put a short summary here for anybody else who did not participate in those discussions or doesn’t remember the details.

The current behaviour in master is the following. Upon Script validation failure of an unconfirmed transaction: https://github.com/bitcoin/bitcoin/blob/5e6dbfd14ea9eace1c7e5ee76b140be46a0ec855/src/validation.cpp#L1238-L1240

If the transaction has no witness attached, Script validation will be performed a second and third time to detect whether it is spending a witness program: https://github.com/bitcoin/bitcoin/blob/5e6dbfd14ea9eace1c7e5ee76b140be46a0ec855/src/validation.cpp#L1245-L1246

This 3-step logic was introduced along with Segwit in 8b49040854be2e26b66366aeae1cba4716f93d93, to avoid adding the txid of transactions with a mutated witness to the reject filter. This was changed in #8525, after which witness transactions which failed validation were never added to the reject filter at all. This is because it would otherwise trivial for an attacker to hinder the propagation of a witness transaction until wtxid-relay (see #8279).

The witness malleation issue was fixed with wtxid-relay in #18044, since at this point inserting the wtxid of a rejected transaction in the reject filter won’t prevent the same transaction with a different (valid) witness to be accepted in the future (if it is announced on wtxid relay connection). However, this 3-step script validation logic was still necessary until the network was mostly upgraded to wtxid relay. This is because otherwise one malicious peer could still interfere with the honest transaction relay of all txid-relay peers. The attack is as follows:

An honest node has N txid-relay peers and one malicious wtxid-relay peer (the malicious peer can be wtxid or txid it doesn’t matter).
A valid witness transaction is broadcast. Before any of its honest txid-relay peers sends the valid witness transaction to the node, the malicious peer sends it after stripping its witness.
The honest node now has the wtxid of the witness-stripped transaction, i.e. the txid of the transaction, in its reject filter. Any honest txid-relay peer trying to announce the valid witness transaction will hit the reject filter.

This is not a concern anymore because wtxid-relay was released on 2021-01-15 with 0.21.0. Nowadays, virtually all connections on the network are wtxid-relay. Therefore we can get rid of performing 3 times the expensive Script validation on non-witness transactions relayed to us for ~free. This is the argument made by OP.

However, this is not the full story. Since wtxid-relay was introduced a new ~~form of~~ dependency on “txid-based transaction resolution” was introduced with 1p1c package relay (h/t @instagibbs and @glozow). It is still possible for a malicious peer to interfere with parent resolution in this case. That is, if this resolution checks the reject filter, which @glozow shared a branch to stop doing: #32379#pullrequestreview-2816057986.

Therefore this change is in the end a tradeoff between getting rid of the 3x script validation and being able to cache rejected parents in 1p1c.

darosior commented at 6:17 pm on June 18, 2025: member

This doesn’t fully fix the fact that 1p1c is disrupted, because the parent is still not requested (its txid is AlreadyHave).

Couldn’t AlreadyHave() be tweaked to optionally not look up the reject filter similarly to include_reconsiderable: https://github.com/bitcoin/bitcoin/blob/e872a566f251c73908de8b6d243c94a6679c2eac/src/node/txdownloadman_impl.cpp#L147

It seems like a smaller change than not checking the reject filter anymore for parent resolution, so we might as well do both so as to not affect 1p1c behaviour at all?

glozow commented at 6:19 pm on June 18, 2025: member

Since wtxid-relay was introduced a new form of “txid-based transaction resolution” was introduced with 1p1c package relay

No, txid-based orphan resolution has been around for many years - I think before segwit. Opportunistic 1p1c just submits the parent and child together when the parent arrives.

darosior commented at 6:22 pm on June 18, 2025: member

Yes what i meant is “we now rely on something else for which it’s pretty bad if one peer can block us from seeing a transaction”. Changed the wording, thanks.

glozow commented at 6:27 pm on June 18, 2025: member

Couldn’t AlreadyHave() be tweaked to optionally not look up the reject filter:

I assume you mean just for the check that filters getdatas. That means we’ll then request transactions we’ve already rejected (we don’t know whether this is an orphan resolution or a normal tx). Although maybe that won’t lead to too many redownloads in practice, since the filter will still exist at inv receipt. Or maybe we also skip when the getdata is of type txid? Will have a think.

glozow commented at 6:33 pm on June 18, 2025: member

Thanks for writeup @darosior 👍

This is not a concern anymore because wtxid-relay was released on 2021-01-15 with 0.21.0. Nowadays, virtually all connections on the network are wtxid-relay.

Digging out some things from my (faint) memory, there was also some discussion about whether it’s problematic if some service uses txid-based relay and an attacker tries to censor their transactions by sending everybody witness stripped versions ahead of time. I think we concluded that this level of attack (where attacker is the first person to hear about it AND manages to frontrun every single connection made by the victim) is out of scope.

darosior commented at 7:53 pm on June 18, 2025: member

Yes, especially if we consider the costs to mitigating this. We shouldn’t impose to all Bitcoin nodes on the network to perform 3 times expensive Script validation just for some company that 1) doesn’t want to update their service to wtxid-relay after 5 years* while 2) running the risk of a somewhat sophisticated attack.

* If this is released in 30.0, by the time most nodes on the network upgrade it will have been more than 5 years since wtxid-relay was released.

glozow commented at 2:52 pm on July 10, 2025: member

Following up, this branch removes the logic to halt orphan-processing when there are rejected parents, and removes rejection cache checking by txid altogether. It also adds a new delay for peers that don’t support BIP 339 (distinct from the delay for requests by txid): https://github.com/glozow/bitcoin/tree/2025-07-wtxid-only-rej It should be sufficient to address the 1p1c issue.

There is a theoretical bandwidth increase from redownloading transactions, but my guess is it’s minimal. Nonsegwit transactions, peers that don’t support BIP 339, and rejections in general should be fairly rare. I’ll use this branch to gather some data on how big these numbers actually are and get back to you?

p2p: stop special-casing witness-stripped error for unconfirmed transactions #32379

Code Coverage & Benchmarks

Reviews

Conflicts