p2p: stop special-casing witness-stripped error for unconfirmed transactions #32379
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32379. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| # The parent should be requested since the unstripped wtxid would differ. Delayed because | ||
| # it's by txid and this is not a preferred relay peer. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yep. Should say 'parent should be requested since witness stripped-transaction rejections shouldn't be cached at all'
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. |
| # The parent should be requested since the unstripped wtxid would differ. Delayed because | ||
| # it's by txid and this is not a preferred relay peer. |
There was a problem hiding this comment.
Yep. Should say 'parent should be requested since witness stripped-transaction rejections shouldn't be cached at all'
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.
This was performed to identify transactions whose witness was stripped and is not necessary anymore.
70e2b5f to
25282b6
Compare
There was a problem hiding this comment.
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"
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
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: Lines 1238 to 1240 in 5e6dbfd 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: Lines 1245 to 1246 in 5e6dbfd This 3-step logic was introduced along with Segwit in 8b49040, 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:
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 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. |
Couldn't bitcoin/src/node/txdownloadman_impl.cpp Line 147 in e872a56 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? |
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. |
|
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. |
I assume you mean just for the check that filters |
|
Thanks for writeup @darosior 👍
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. |
|
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. |
|
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? |
|
I ran the above patch for a while and added logging to collect some stats and see how much more redownloading we’d do. TLDR it seems like we can definitely go ahead with it. Caveats: Traffic has been very low so I set my mempool expiry to 1 hour to try to induce more orphan-fetching. I’m a default policy node so I don’t really expect to be relayed many transactions I’ll reject. There isn’t much opportunity to test reconsiderable rejections right now because of how low transaction traffic is. My stats in the last 7 days*:
The impact of this patch is that we’ll redownload a transaction that is (1) nonsegwit (2) when we are requesting by txid (in practice, always orphan resolution) and (3) we’ve already rejected it. |
|
Can this be closed? |
|
I was waiting to see the outcome of #33066 to see what to do of this, but i'm happy to close and reopen if necessary. 🤷♂️ |
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.