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)
#32080 (OP_CHECKCONTRACTVERIFY by bigspider)

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”

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

Code Coverage & Benchmarks

Reviews

Conflicts