p2p: Allow 1P1C to fetch parent from compact block extra_txn #30032
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. 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. |
instagibbs
force-pushed
the
2024-05-opportunistic-extratxn
branch
from
501220c
to
24618bc
Compare
One relatively common pattern of 1P1C relay is receiving a just-below-minfee parent, dropping it and marking it as reconsiderable, then fetching it again from the peer once the child is added to the orphanage. A cache of dropped parents would be useful, and it turns out we're already opportunistically storing transactions like this for compact blocks as "extra transactions". Use this size-limited cache to potentially fetch a reconsiderable parent, and submit for validation.
instagibbs
force-pushed
the
2024-05-opportunistic-extratxn
branch
from
24618bc
to
4fa5543
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
|
||
| /** Orphan/conflicted/etc transactions that are kept for compact block reconstruction. | ||
| * The last -blockreconstructionextratxn/DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN of | ||
| * these are kept in a ring buffer */ | ||
| std::vector<CTransactionRef> vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex); | ||
| std::vector<NodeId> m_extra_txn_senders GUARDED_BY(g_msgproc_mutex); |
| const auto& maybe_parent = vExtraTxnForCompact[i]; | ||
| // We stopped checking it earlier due to fee reasons, try again? | ||
| if (maybe_parent && m_recent_rejects_reconsiderable.contains(maybe_parent->GetWitnessHash().ToUint256())) { | ||
| Package maybe_cpfp_package{maybe_parent, orphan_child}; | ||
| if (IsChildWithParents(maybe_cpfp_package) && !m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) { |
There was a problem hiding this comment.
Feels inefficient/expensive to construct a package + call these checks for every tx every time there's a missing input. Maybe parameterize by the txid of the missing parent (i.e. just 1 exists) and first check that the txid matches?
| self.extra_args = [[ | ||
| "-datacarriersize=100000", | ||
| "-maxmempool=5", | ||
| "-blockreconstructionextratxn=0", # require fetching parent even if cached |
I'll do some logging 👍 I suspect it'll be quite above non-0 because from my log-reading just-below-minfee is quite commonly a reason for missing the parent. edit: this isn't ready for prime-time, have to more carefully think about handling the results. This new case is triggered even if we would consider continuing on failure in various cases, so I likely have bugs here. |
|
from my non-listening node, looks like ~3% of the time of package evaluation is being considered it triggers |
|
Don't think I find these results compelling vs the complexity in how we would need to handle the packages |
One relatively common pattern of 1P1C relay is receiving
a just-below-minfee parent, dropping it and marking it
as reconsiderable, then fetching it again from the peer
once the child is added to the orphanage.
A cache of dropped parents would be useful, and it turns
out we're already opportunistically storing transactions
like this for compact blocks as "extra transactions".
Use this size-limited cache to potentially fetch a
reconsiderable parent from memory, and submit for validation.