Follow-up to #36437 which resolves a few key weaknesses in 1p1c relay, focusing on issues that can be easily achieved on average cpfp behavior.
Resolves two fuzz harness "holes" and narrows a third.
This is a proposed stopping point on resolving the holes, as it's unclear if further work is worth the churn vs a sender-initiated package relay protocol update that would resolve this directly.
Known issues doc: https://gist.github.com/instagibbs/880a76535fd802fced9f9a5253cdbb40
Check that if an honest peer announces the child of an acceptable
parent and child, the child ends up in the mempool within a bounded
time, whatever other peers do. The target drives a real PeerManager
with real messages and validation, which the existing targets cannot,
as they model validation results.
After an adversarial prefix the honest peer only answers requests. With
an outbound honest peer the adversaries keep acting; with an inbound
one they go quiet. Adversary connections are limited, and adversaries
do not refill outbound slots.
Known liveness holes are marked in the target and turn the check off
once opened, including the lost orphan reconsideration fixed in the
next commit.
A child that arrives before its parent is kept as an orphan. Once the
parent is accepted on its own, the orphan is put in one randomly chosen
announcer's work set to be retried. If that announcer disconnects or is
evicted first, the orphan is never retried, and re-announcements by the
other announcers don't fetch it again since it is already held. This
keeps e.g. a CPFP child of a parent that pays for itself out of the
mempool until the orphan is evicted, and an announcer can cause it on
purpose.
Hand reconsideration over to the remaining announcer with the lowest
NodeId instead.
Remove its hole from the liveness target.
<!--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 |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
@marcofleon @mzumsande this will be the ultimate target PR to fuzz
Tested c8abd9ee41: each of the four fixes is caught by a test this PR adds or extends. I reverted each fix on its own, keeping the tests, and each revert made txdownload_tests, orphanage_tests or p2p_opportunistic_1p1c.py fail, while the unchanged branch passes them all. I did not review the C++ changes. Details on request.
ACK c8abd9ee41. I have now reviewed the four fixes commit by commit and found nothing blocking. One gap remains for a follow-up: in the 1p1c package path, a DIFFERENT_WITNESS result still goes through ProcessInvalidTx, so a witness-stripped copy of a mempool parent puts the parent's txid in the reject filter, as TX_CONFLICT did before 3a2b5539a1. A functional test shows it end to end: an attacker peer re-sends the stripped copy with an orphan of its own, and after the parent leaves the mempool, an honest child is dropped as having a rejected parent. Test on request.
TX_CONFLICT means the transaction is already known, in the mempool or
confirmed, not that it is invalid. Its wtxid still went into the reject
filter. For a witnessless copy that is also the txid, so if the known
parent then left the mempool before the next block, a child arriving
afterwards was dropped as having a rejected parent. For the honest
version of a parent whose other-witness version is in the mempool, the
honest parent itself was ignored once that version left.
Don't add TX_CONFLICT results to the reject filter. The entry only saved
re-downloading and re-checking a transaction we already have. Do the
same for a DIFFERENT_WITNESS result in 1p1c package evaluation, which
ProcessPackageResult passed to ProcessInvalidTx with a valid state and
so reached the filter by another path.
This also keeps a witnessless copy of a confirmed transaction out of
the filter, as it is rejected as already known when one of its outputs
is in the coins cache, so children spending that transaction are no
longer dropped. A copy of a coinbase is rejected as TX_CONSENSUS
instead, before any input lookup, and still poisons the coinbase's
txid.
Update the rejection-type table for TX_CONFLICT. In the liveness
target, remove the replayed-parent hole and narrow the stripped
confirmed-parent hole to children funded by a coinbase.
When a 1p1c package fails the package feerate check, the child gets a
TX_RECONSIDERABLE result, and ProcessPackageResult passed it to
ProcessInvalidTx, which erased the child from the orphanage for all of
its announcers.
The failure is a property of that pair of wtxids, which
MempoolRejectedPackage already caches, not of the child. The child can
still succeed with another version of the parent, such as the one an
honest announcer holds, so it should stay available.
Skip ProcessInvalidTx for package members with a TX_RECONSIDERABLE
result. A child that is too low feerate on its own is still rejected
when its parent's work set is processed. Remove the hole from the
liveness target.
A reconsiderable rejection of a witnessless copy of a transaction forgot its
wtxid in TxRequestTracker regardless of announcement type, and since that
wtxid equals the txid, it also cancelled every peer's txid-typed request for
the transaction, including orphan resolution requests for a missing parent.
Now only wtxid-typed announcements of that hash are forgotten, so witnessless
parents are treated like witnessed ones, which were never affected since
their wtxid differs from the txid.
reACK 8d44ac8909. The new DIFFERENT_WITNESS case closes the 1p1c gap I noted at c8abd9ee41, and the only other code change is its test, known_parent_in_package_does_not_poison_after_eviction. I built test_bitcoin and ran txdownload_tests, orphanage_tests and txpackage_tests: the new test passes, and fails with that case moved back next to INVALID.
@fametrano fuzzer hit it, which is a good sign