-
Notifications
You must be signed in to change notification settings - Fork 35.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
p2p: opportunistically accept 1-parent-1-child packages #28970
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.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
5351320
to
8bbcc92
Compare
Rebased and fixed CI. This is in draft because I'm focusing on v3 stuff, can be reviewed for its approach. |
ready for un-draft? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I focused on the first two refactoring commits. Might be helpful zeroing in on these since we probably need to justify each small behavior change or make sure we're aligning with old behavior.
src/net_processing.cpp
Outdated
// regardless of what witness is provided, we will not accept | ||
// this, so we don't need to allow for redownload of this txid | ||
// from any of our non-wtxidrelay peers. | ||
m_recent_rejects.insert(tx->GetHash().ToUint256()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
future work: might make sense to delete this transaction from the orphanage as well in this case if it exists so we don't try it with other peer?
Working on pulling the first 2 commits out into a separate PR. |
It's very hard to randomly construct a transaction that would be the parent of an existing orphanage tx. For functions like AddChildrenToWorkSet and GetChildren that take orphan parents, use a tx that was previously constructed.
6a00cad
to
850a2fc
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reACK e518a8b
non-blocking comments
reviewed via git range-diff master 30c9e6b e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f
void ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, const std::vector<NodeId>& senders) | ||
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main); | ||
|
||
/** A package to validate */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant comment
@@ -3198,6 +3237,117 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c | |||
} | |||
} | |||
|
|||
void PeerManagerImpl::ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, const std::vector<NodeId>& senders) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that we have PackageToValidate
, just directly pass here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #30012
m_senders {parent_sender, child_sender} | ||
{} | ||
|
||
std::string ToString() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useful log message for stats 👍
tACK e518a8b All comments have been addressed/replied to. Tests are now passing even in this case: #28970 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-review ACK e518a8b 📦
Since there are a few ACKs now, listing followups. I plan to open a PR for the first two immediately:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
light Code review ACK e518a8b
@@ -4467,6 +4646,19 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, | |||
if (state.IsInvalid()) { | |||
ProcessInvalidTx(pfrom.GetId(), ptx, state, /*maybe_add_extra_compact_tx=*/true); | |||
} | |||
// When a transaction fails for TX_RECONSIDERABLE, look for a matching child in the | |||
// orphanage, as it is possible that they succeed as a package. | |||
if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think attempting to accept a 1p1c package could also be done when reconsidering transactions from the orphanage in ProcessOrphanTx
. E.g. you might have a 1p1c package in your orphanage (P -> C) and if P is reconsidered but rejected as TX_RECONSIDERABLE
you might want to look for C in the orphanage and if found try 1p1c acceptance.
Maybe its not worth adding right now (any scenarios I can think of seem somewhat rare) but perhaps worthwhile later if the data suggests so.
// Erase duplicates | ||
std::sort(iters.begin(), iters.end(), IteratorComparator()); | ||
iters.erase(std::unique(iters.begin(), iters.end()), iters.end()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this sort comparison different from the one in GetChildrenFromSamePeer
?
These functions are almost identical as well. Perhaps a more generic std::vector<std::pair<CTransactionRef, NodeId>> GetChildren(const CTransactionRef&)
that returns all children and their senders would be less code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, would be nice to avoid this code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made a similar point at #28970 (comment) fwiw (with a response attached)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this sort comparison different from the one in
GetChildrenFromSamePeer
?
The idea is to return GetChildrenFromSamePeer
in recency order. This one could be returned that way as well, as the goal is to have a random order (and we randomize the indices), but all I wanted to do here was deduplicate.
These functions are almost identical as well.
As for the 2 functions having duplicate code, for a longer explanation: I'm hoping we can remove 1p1cs with different providers by tracking all orphan announcers, which would mean deleting GetChildrenFromDifferentPeer
. I think it'll be easier to delete the function + unit tests wholesale if it's a different function. But it seems I'm in the minority here, and apologies for resolving the first comment about this. Will avoid this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK e518a8b
[](const auto& tx){ return tx->GetWitnessHash(); }); | ||
|
||
// Sort in ascending order | ||
std::sort(wtxids_copy.begin(), wtxids_copy.end(), [](const auto& lhs, const auto& rhs) { return lhs.GetHex() < rhs.GetHex(); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 092c978 "[txpackages] add canonical way to get hash of package"
Is there a reason to convert the wtxids into hex strings for this comparison? That seems kind of expensive, especially when Wtxid
already has an operator<
.
I surmise that this has to do with the "concatenated in lexicographical order (treating the wtxids as little endian encoded uint256, smallest to largest)" and BIP 331 defines the same thing, but it's not clear to me why it must be like that (and couldn't also be changed in the BIP).
If this must be done in reverse byte order, then I think it would at be better to use std::lexicographical_compare
with reverse iterators, rather than turning the wtxids into strings:
std::sort(wtxids_copy.begin(), wtxids_copy.end(), [](const auto& lhs, const auto& rhs) { return lhs.GetHex() < rhs.GetHex(); }); | |
std::sort(wtxids_copy.begin(), wtxids_copy.end(), [](const auto& lhs, const auto& rhs) { return std::lexicographical_compare(std::make_reverse_iterator(lhs.end()), std::make_reverse_iterator(lhs.begin()), std::make_reverse_iterator(rhs.end()), std::make_reverse_iterator(rhs.begin())); }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to convert the wtxids into hex strings for this comparison? That seems kind of expensive, especially when Wtxid already has an operator<.
I surmise that this has to do with the "concatenated in lexicographical order (treating the wtxids as little endian encoded uint256, smallest to largest)" and BIP 331 defines the same thing, but it's not clear to me why it must be like that (and couldn't also be changed in the BIP).
The code was originally using the operator< defined on uint256
, and the opposite switch happened 😅 A few people commented on lexicographical being underspecified or different from how it's usually defined (e.g. #28970 (comment)). Mostly I felt like reverse byte order was much more intuitive, so I changed it here and in the BIP. Yes this doesn't have to match the BIP since it's only used internally, but I figured I might as well write something that's reusable.
Thanks for the std::lexicographical_compare
suggestion, I'll add that in a followup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in #30012
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Light ACK e518a8b
I didn't review the functional tests in much detail, but the p2p code looks good to me.
// If the result is TX_RECONSIDERABLE, add it to m_recent_rejects_reconsiderable | ||
// because we should not download or submit this transaction by itself again, but may | ||
// submit it as part of a package later. | ||
m_recent_rejects_reconsiderable.insert(ptx->GetWitnessHash().ToUint256()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think that 6c51e1d changes behavior such that txns which are now in m_recent_rejects_reconsiderable
instead of m_recent_rejects
will be ignored when calling m_recent_rejects.contains(...)
. That is probably an undesired change of behavior - however it's just for 1 commit 🤷♂️
// Erase duplicates | ||
std::sort(iters.begin(), iters.end(), IteratorComparator()); | ||
iters.erase(std::unique(iters.begin(), iters.end()), iters.end()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, would be nice to avoid this code duplication.
|
||
// No package results to look through for PCKG_POLICY or PCKG_MEMPOOL_ERROR | ||
if (package_result.m_state.GetResult() == PackageValidationResult::PCKG_POLICY || | ||
package_result.m_state.GetResult() == PackageValidationResult::PCKG_MEMPOOL_ERROR) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there are situations in which we fail on a package level and therefore return here and never call ProcessInvalidTx()
to remove the child from the orphanage. Would that be possible, for example, with v3 transactions that violate PackageV3Checks()
, and is there something we could do against it? (or should, maybe it's not that terrible?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PCKG_POLICY
doesn't populate m_tx_results
so we wouldn't have results for those transactions - it's meant for situations in which the package is invalid, but the individual transactions could be if submitted in a different context. However, going over the failures that are marked as PCKG_POLICY
, I agree there's a few failures that are classified as PCKG_POLICY
but are actually more definitively invalid, and we should bubble that up to p2p.
Having both mempool + package v3 parents, v2-spending-v3, and v3-spending-v2 are failures that can't change in another context, so we can delete the tx from orphanage.
We shouldn't do this for package-mempool-limits
and package-not-child-with-unconfirmed-parents
, since there could be a parent with a smaller witness that makes this child acceptable.
The 2 changes we'd make are:
- update validation-side things to return a
MempoolAcceptResult
for the results where we should mark transactions as invalid. Perhaps we can reclassify these failures asPCKG_TX
failures, or allowPCKG_POLICY
failures to populatem_tx_results
. cc @instagibbs ? - update the p2p logic here to continue when it's
PCKG_POLICY
and callProcessInvalidTx
for theMempoolAcceptResult
s we find.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PCKG_POLICY doesn't populate m_tx_results so we wouldn't have results for those transactions
From my reading of the code, package-mempool-limits
populates m_tx_results
? We also allow those cases in our fuzz target(which I'm guessing is there because I hit it!).
We could probably be more aggressive about filling out results when possible and passing those results along, sounds like future work for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe we should just do this?
Update the p2p logic here to continue when it's PCKG_POLICY and call ProcessInvalidTx for the MempoolAcceptResults we find.
(and remove the Assume
that they always exist)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^I added this to #30012
7f6fb73 [refactor] use reference in for loop through iters (glozow) 6119f76 Process every MempoolAcceptResult regardless of PackageValidationResult (glozow) 2b482dc [refactor] have ProcessPackageResult take a PackageToValidate (glozow) c2ada05 [doc] remove redundant PackageToValidate comment (glozow) 9a762ef [txpackages] use std::lexicographical_compare instead of sorting hex strings (glozow) 8496f69 [refactor] make MempoolAcceptResult::m_replaced_transactions non-optional (glozow) Pull request description: Followups from #28970: - #28970 (comment) - #28970 (comment) - #28970 (comment) - #28970 (comment) - #28970 (comment) ACKs for top commit: instagibbs: reACK 7f6fb73 Tree-SHA512: 9d8393d5f2fedbc6ebce582ff2a8ed074a02dd8e7dbf562c14d48b439fdc1ee6c3203b3609366d3c883d44655cc1a5c83a75ca56e490d25c1a34d95a0622d458
58594c7 fuzz: txorphan tests fixups (Sergi Delgado Segura) Pull request description: Motivated by #28970 (comment) Adds the following fixups in txorphan fuzz tests: - Don't bond the output count of the created orphans to the number of available coins - Allow duplicate inputs but don't store duplicate outpoints Most significantly, this gets rid of the `duplicate_input` flag altogether, making the test easier to reason about. Notice how, under normal conditions, duplicate inputs would be caught by `MemPoolAccept::PreChecks`, however, no validations checks are run on the test before adding data to the orphanage (neither were they before this patch) ## Rationale The way the test is currently written, duplicate inputs are allowed based on a random flag (`duplicate_input`). If the flag is unset, upon selecting an outpoint as input for a new transaction, the input is popped to prevent re-selection and later re-added to the collection (once all inputs have been picked). However, the re-addition to the collection is performed independently of whether the flag was set or not. This means that, if the flag is set, the selected inputs are duplicated which in turn makes these inputs more likely to be re-picked in the following iteration of the loop. Additionally, both the input and output count of the transaction are bonded to the number of available outpoints. This makes sense for the former, but the latter shouldn't be. ACKs for top commit: maflcko: utACK 58594c7 glozow: ACK 58594c7 instagibbs: ACK 58594c7 Tree-SHA512: e97cc2a43e388f87b64d2e4e45f929dd5b0dd85d668dd693b75e4c9ceea734cd7645952385d428208d07b70e1aafbec84cc2ec264a2e07d36fc8ba3e97885a8d
…e txid 0fb17bf [log] updates in TxOrphanage (glozow) b16da7e [functional test] attackers sending mutated orphans (glozow) 6675f64 [unit test] TxOrphanage handling of same-txid-different-witness txns (glozow) 8923edf [p2p] allow entries with the same txid in TxOrphanage (glozow) c31f148 [refactor] TxOrphanage::EraseTx by wtxid (glozow) efcc593 [refactor] TxOrphanage::HaveTx only by wtxid (glozow) 7e475b9 [p2p] don't query orphanage by txid (glozow) Pull request description: Part of #27463 in the "make orphan handling more robust" section. Currently the main map in `TxOrphanage` is indexed by txid; we do not allow 2 transactions with the same txid into TxOrphanage. This means that if we receive a transaction and want to store it in orphanage, we'll fail to do so if a same-txid-different-witness version of the tx already exists in the orphanage. The existing orphanage entry can stay until it expires 20 minutes later, or until we find that it is invalid. This means an attacker can try to block/delay us accepting an orphan transaction by sending a mutated version of the child ahead of time. See included test. Prior to #28970, we don't rely on the orphanage for anything and it would be relatively difficult to guess what transaction will go to a node's orphanage. After the parent(s) are accepted, if anybody sends us the correct transaction, we'll end up accepting it. However, this is a bit more painful for 1p1c: it's easier for an attacker to tell when a tx is going to hit a node's orphanage, and we need to store the correct orphan + receive the parent before we'll consider the package. If we start out with a bad orphan, we can't evict it until we receive the parent + try the 1p1c, and then we'll need to download the real child, put it in orphanage, download the parent again, and then retry 1p1c. ACKs for top commit: AngusP: ACK 0fb17bf itornaza: trACK 0fb17bf instagibbs: ACK 0fb17bf theStack: ACK 0fb17bf sr-gi: crACK [0fb17bf](0fb17bf) stickies-v: ACK 0fb17bf Tree-SHA512: edcbac7287c628bc27036920c2d4e4f63ec65087fbac1de9319c4f541515d669fc4e5fdc30c8b9a248b720da42b89153d388e91c7bf5caf4bc5b3b931ded1f59
This enables 1p1c packages to propagate in the "happy case" (i.e. not reliable if there are adversaries) and contains a lot of package relay-related code. See #27463 for overall package relay tracking.
Rationale: This is "non-robust 1-parent-1-child package relay" which is immediately useful.
The first 2 commits are followups from #29619:
Q: What makes this short of a more full package relay feature?
(1) it only supports packages in which 1 of the parents needs to be CPFP'd by the child. That includes 1-parent-1-child packages and situations in which the other parents already pay for themselves (and are thus in mempool already when the package is submitted). More general package relay is a future improvement that requires more engineering in mempool and validation - see #27463.
(2) We rely on having kept the child in orphanage, and don't make any attempt to protect it while we wait to receive the parent. If we are experiencing a lot of orphanage churn (e.g. an adversary is purposefully sending us a lot of transactions with missing inputs), we will fail to submit packages. This limitation has been around for 12+ years, see #27742 which adds a token bucket scheme for protecting package-related orphans at a limited rate per peer.
(3) Our orphan-handling logic is somewhat opportunistic; we don't make much effort to resolve an orphan beyond asking the child's sender for the parents. This means we may miss packages if the first sender fails to give us the parent (intentionally or unintentionally). To make this more robust, we need receiver-side logic to retry orphan resolution with multiple peers. This is also an existing problem which has a proposed solution in #28031.
[1]: see this writeup and its links https://github.com/bitcoin/bips/blob/02ec218c7857ef60914e9a3d383b68caf987f70b/bip-0331.mediawiki#propagate-high-feerate-transactions