Based on a commit by Anthony Towns.
Otherwise log that an unknown inv type was received.
In inv processing, when handling transaction type inv messages,
ToGenTxid() expects that we constructed the CInv ourselves or that we
verified that it is for a transaction type CInv.
Therefore, change this 'else' branch into an 'else if tx in inv type' to
make this safer and log any inv that fall through.
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.
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?
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 (review), 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.
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 42ca5618cae0fd9ef97d2006b17d896bc58cc17c) and then fixed later by adding the sanity check (commit fb56d37612dea6666e7da73d671311a697570dae) in the same refactoring pr.
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).
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: https://github.com/bitcoin/bitcoin/blob/9efd86a908cf09d9ddbadd3195f202635117d505/src/net_processing.cpp#L2681
MSG_WITNESS_BLOCK and in 42ca561 in works with any unknown type.
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.
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.
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.
If reverting wtxid relay from the 0.20 branch is easy, that’s probably the best option.
Done in #20399
Milestone
0.20.2