We currently concentrate the validation of all the assigned peer’s reconsiderable orphans that are still missing inputs in a single message-processing turn. This ties orphan processing to validation result, and adds latency to processing other peers' messages.
Always process a single reconsiderable orphan per turn instead. This removes the need for an immediate validation result, and shifts the latency from other peers to the assigned peer.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | fametrano |
| Concept ACK | instagibbs, ViniciusCestarii |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
712 | @@ -713,13 +713,12 @@ class PeerManagerImpl final : public PeerManager
713 | /**
714 | * Reconsider orphan transactions after a parent has been accepted to the mempool.
715 | *
commit msg: citing the wrong hash, should be 866c8058a7 ?
Yeah, looks like a copy paste mistake.
Corrected commit message, thanks.
concept ACK
probably slap on a unit test or smth and/or fuzz to cap the amount of reconsidered stuff in a step
Orphan processing used to be uninterruptible, which could be exploited
as a severe CPU DoS (see CVE-2024-52914). 866c8058a7 mitigated this by
interrupting orphan processing after the first transaction validation
that did not result in a missing input error.
This commit moves the interruption point to after each transaction
validation, whatever the result. This unties it from the validation
result and reduces the latency before processing other peers' messages,
at the cost of increasing it for the messages of the peer the orphans
are assigned to.
Added a unit test that demonstrates the behaviour: the test will fail on master, as both child would be reconsidered there (since validation returns a missing-input error).
Concept ACK
Was the missing-inputs exception in 866c8058a706931f025335b3e794ed2f4d287918 there because failing in PreChecks was considered cheap? The only hint I found is "after every actual transaction validation attempt" in the 866c8058a706931f025335b3e794ed2f4d287918 commit message.
It seems the focus was likely on avoiding too much script validation, and wiping from the orphanage eagerly to avoid a single announcer dominating the orphanage. The orphanage and how announcements are handled across peers has changed quite a bit since then.
tACK 83003cafaa81bb116526419246608658ede824b6
I measured the trade-off the description names with a scenario of my own. One peer sends four orphans that each spend a parent output and an outpoint that does not exist, then sends the parent and an unrelated valid transaction back to back. On master the unrelated transaction is processed on the peer's second turn, because all four orphans are retried in one turn. With this PR it takes six turns: the parent, one per orphan, then the transaction. So the wait moves onto the assigned peer, as described, and each turn validates at most one orphan.
The new case in denialofservice_tests fails on master, at the ProcessMessagesOnce check in its loop, and passes here.
I tested the head merged into master a773e1e33b, in a Debug build, with the denialofservice, orphanage, txdownload and txpackage unit tests and the p2p_orphan_handling and p2p_opportunistic_1p1c functional tests. I ran them again with #36437 and #36578 merged on top; the three PRs merge without conflicts. I can share the scenario, a Boost test case, as a gist.