Backport wtxid orphan fetch to v0.20

[jnewbery]

wtxid relay (#18044) was backported to 0.20 in #19606. This PR backports the follow-up "Enable fetching of orphan parents from wtxid peers" #19569.

Also included is commit p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing from #19610 to prevent a possible remote crash bug.

[gmaxwell]

I don't understand why this should be done at this point, I believe #19620 addressed the (limited) justification for backporting wtxid relay. Am I confused?

[jnewbery]

This was discussed in the IRC meeting on July 30th: http://www.erisian.com.au/meetbot/bitcoin-core-dev/2020/bitcoin-core-dev.2020-07-30-19.00.log.html#l-47

19:25 < wumpus> so the plan is to backport #19606 to 0.20 and #19620 to 0.19 and 0.20?
[...]
19:25 < jnewbery> wumpus: sounds good to me
[...]
19:25 < sdaftuar> sounds good
19:26 < sipa> sounds reasonable if wtxid relay (and followups) are easy to do in 0.20

#19569 should really have been a part of the original wtxid relay PR. It makes sense to backport them together. I only didn't include this in #19606 to make review easier.

[fanquake]

It's too late for #19606, however this PR is also missing the GitHub-Pull: # and Rebased-From: metadata. See #20166 for an example.

[gmaxwell]

I am confused by this discussion.

19:16:35 we can make an exception to normallly not backporting p2p features as it's requirement for taproot

Wtxid relay is not a requirement for taproot. Without #19620 there was a narrow justification, though I don't think too big of one, but with 19620 in place there isn't any duplicated fetching-- so there is no connection to taproot anymore.

19:15:08 my opinion is that there's no strong reason not to backport wtxid relay to 0.20.

At the time of this statement people weren't aware that this change relay introduced a crash vulnerability (which was accidentally fixed by a separate re-factoring change). But people didn't need to be aware, because that's the reason that features aren't normally backported unless there is a really good reason.

But even after reading that discussion, I don't see any reason why this or wtxid relay should be backported to 0.20. Unless I'm missing something this (and 19606) is a pure feature backport, but I don't know the reason for doing it because I don't see it in the discussion-- what I see given (that it's needed for taproot) just isn't true.

Am I missing something in the discussion?

[jnewbery]

19:16:35 we can make an exception to normallly not backporting p2p features as it's requirement for taproot

That wasn't the reason given for backporting. The reason for backporting was not that it was a requirement for taproot. It was discussed as being an independently useful change. wtxid relay has been wanted since segwit was introduced (#8279), the feature was developed against 0.20, and missed feature freeze by a few weeks, so was a trivial backport.

people weren't aware that this change relay introduced a crash vulnerability (which was accidentally fixed by a separate re-factoring change)

The crash vulnerability that you're refering to wasn't introduced in #18044. It came in commit 9efd86a (#19569) in the follow-up PR. It wasn't 'accidentally fixed by a separate refactoring change' - I explicitly pointed out the problem in review here: #19569 (comment), and wrote a fix here: fb56d37 (#19610), which was included in the follow-up PR. I've also included that commit in this backport.

I still think it's useful to have wtxid relay widely deployed (and also have the ability to request orphan parents from wtxid peers). If other people disagree, then we can close this PR.

[maflcko]

The crash bug has been introduced in neither of those feature prs. It has been introduced (and fixed) in the refactoring pr. If it was introduced in a pr different from one that fixed it, it would be highly alarming because our fuzzers are meaningless and can't even find the most trivial crash seed.

A side effect of AlreadyHave was to (also) skip requesting of the tx with unknown type flags. AlreadyHave is still used in 0.20, so the last commit does not necessarily need backport. As in: The commit doesn't fix an exploitable bug that exists anywhere prior to it.

The bug has been introduced by splitting AlreadyHave (commit 42ca561) and then fixed later by adding the sanity check (commit fb56d37) in the same refactoring pr.

[practicalswift]

The crash bug has been introduced in neither of those feature prs. It has been introduced (and fixed) in the refactoring pr.

As the person who identified the condition which triggers the bug and reported it to security@bitcoincore.org (with a working PoC) I can tell you with certainty that the bug was introduced in #19569 and fixed in #19610 :)

If it was introduced in a pr different from one that fixed it, it would be highly alarming because our fuzzers are meaningless and can't even find the most trivial crash seed.

That seems somewhat hyperbolic :) FWIW I found the bug by fuzzing using one of the fuzzers in our repo (with a one line downstream change).

[jnewbery]

The bug has been introduced by splitting AlreadyHave (commit 42ca561) and then fixed later by adding the sanity check (commit fb56d37) in the same refactoring pr.

No, the bug was introduced in 9efd86a (#19569). After that commit, if we receive an inv message with type MSG_WITNESS_BLOCK, then we assert when calling ToGenTxid(inv) here:

bitcoin/src/net_processing.cpp

Line 2681 in 9efd86a

RequestTx(State(pfrom.GetId()), ToGenTxid(inv), current_time);

[maflcko]

Oh, I see my bad. So in 9efd86a it works with only type MSG_WITNESS_BLOCK and in 42ca561 in works with any unknown type.

[gmaxwell]

I still think it's useful to have wtxid relay widely deployed

Why? I'm not following why this bundle of features is atypically important. Wtxid relay is a fine change overall in the long term, but part of the reason it took a long time to do is because it wasn't especially important.

[ajtowns]

I don't understand why this should be done at this point, I believe #19620 addressed the (limited) justification for backporting wtxid relay. Am I confused?

That matches my understanding -- the risk was that if taproot was deployed, then nodes running 0.20 and earlier would see soft-rejects txs spending taproot UTXOs, but only due to the witness being non-standard, so would re-request the tx from each other peer that advertised the tx, wasting bandwidth. But #19681 and #19680 fixed this in 0.19 and 0.20 already.

But that's an argument for why #19606 wasn't necessary; this PR fixes a regression in orphan handling introduced by that change.

[maflcko]

Due to the other bugfixes in the 0.20 branch, we should be aiming for a release soon. wtxid relay and this followup are not a regression bugfix, so if they don't receive enough review to make it in soon, or are controversial, we should drop them or postpone them to the next minor release in the 0.20 series.

[sipa]

I probably contributed to the initial sentiment that wtxid relay was particularly important as a feature. Since the immediate concern about BIP340-342 deployment is addressed through #19620, that's not really the case (anymore).

So I think we should take both wtxid relay and this PR in 0.20, or neither. I believe that just wtxid relay may be a slight regression as it may hurt orphan handling once wtxid peers are widespread.

If reverting wtxid relay from the 0.20 branch is easy, that's probably the best option.

[maflcko]

If reverting wtxid relay from the 0.20 branch is easy, that's probably the best option.

Done in #20399

[maflcko]

I am catching up on the IRC backlog: https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2020/bitcoin-core-dev.2020-11-17-15.00.moin.txt

@jnewbery Is this still relevant or can this be closed?

[jnewbery]

This can be closed. We agreed at the meeting to revert 19606.