-
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
txorphanage: add size accounting, use wtxids, support multiple announcers #28481
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
baddad7
to
581339f
Compare
581339f
to
6101c60
Compare
$ echo "Af//////Oqf/AP8TAf8AEPr/AQAApZL6/wEA/1sB/wBbAAClkpJ4eP//S0tL/Q==" | base64 -d > orphanage_crash
$ FUZZ=txorphan ./src/test/fuzz/fuzz orphanage_crash
...
test/fuzz/txorphan.cpp:98 operator(): Assertion `orphanage.BytesFromPeer(peer_id) >= ref->GetTotalSize()' failed.
... |
|
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.
initial comments, reviewed everything, also replicated the same fuzzer crash locally
@@ -33,8 +36,8 @@ class TxOrphanage { | |||
*/ | |||
CTransactionRef GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); | |||
|
|||
/** Erase an orphan by txid */ | |||
int EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); |
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.
Need to also rename arg for EraseTxNoLock
to wtxid
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.
txorphan.cpp
fuzzer needs to change for all EraseTx()
call sites because they're still using txid
@@ -118,6 +139,8 @@ void TxOrphanage::EraseForPeer(NodeId peer) | |||
} | |||
} | |||
if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer); | |||
if (!Assume(m_peer_bytes_used.count(peer) == 0)) m_peer_bytes_used.erase(peer); |
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.
leave a comment that this is belt-and-suspenders only, the condition should fail since the item should have been already erased
@@ -22,7 +22,7 @@ class TxOrphanage { | |||
public: | |||
/** Add a new orphan transaction. If the tx already exists, add this peer to its list of announcers. | |||
@returns true if the transaction was added as a new orphan. */ | |||
bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); | |||
bool AddTx(const CTransactionRef& tx, NodeId peer, const std::vector<uint256>& parent_txids) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); |
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.
Should describe the new argument as something like "all unique parent transaction txids". I kind of wish this was generated inside AddTx
but realize you're avoiding double the work...
@@ -92,6 +92,9 @@ class TxOrphanage { | |||
* remove this peer's entry from the map. */ | |||
void SubtractOrphanBytes(unsigned int size, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(m_mutex); | |||
|
|||
/** Get an orphan's parent_txids, or std::nullopt if the orphan is not present. */ |
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.
/** Get an orphan's parent_txids, or std::nullopt if the orphan is not present. */ | |
/** Get an orphan's unique parent_txids, or std::nullopt if the orphan is not present. */ |
|
||
/** Erase all orphans included in or invalidated by a new block */ | ||
void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); | ||
|
||
/** Limit the orphanage to the given maximum */ | ||
void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); | ||
/** Limit the orphanage to the given maximum. Returns all expired entries. */ |
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.
/** Limit the orphanage to the given maximum. Returns all expired entries. */ | |
/** Limit the orphanage to the given maximum. Returns all expired entries by wtxid. */ |
src/txorphanage.cpp
Outdated
{ | ||
LOCK(m_mutex); | ||
|
||
std::vector<uint256> expired; | ||
unsigned int nEvicted = 0; |
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.
can remove this and use size of expired(_wtxids)
set isntead
src/txorphanage.h
Outdated
void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); | ||
/** Maybe erase all orphans announced by a peer (eg, after that peer disconnects). If an orphan | ||
* has been announced by another peer, don't erase, just remove this peer from the list of announcers. */ | ||
std::vector<uint256> EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); |
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.
please remove this return #28031 (comment)
@@ -278,7 +278,7 @@ bool TxOrphanage::HaveTxToReconsider(NodeId peer) | |||
return false; | |||
} | |||
|
|||
void TxOrphanage::EraseForBlock(const CBlock& block) | |||
std::vector<uint256> TxOrphanage::EraseForBlock(const CBlock& block) |
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.
unused in this PR, but is exercised in tests so I'm happy for it to be used later 👍
tx.vin.emplace_back(CTxIn(outpoint)); | ||
} | ||
} | ||
// Ensure txid != wtxid |
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.
could this get mechanically asserted as extra-belt-and-suspender
// check EraseForBlock | ||
CBlock block; | ||
block.vtx.emplace_back(tx); | ||
orphanage.EraseForBlock(block); |
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.
might as well capture the return and check it here too
Putting this in draft to focus review attention on the mempool stuff. |
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.
add this to the first line of array or a bolleyan function and then to the first line of paragraph..
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.
make this a transidental function and then work upon it and mechanically assert it up than.
Going to resolve comments + fuzz failure, and then add a |
34de484
to
22e3c06
Compare
Wtxids are better because they disambiguate between transactions with the same txid but different witnesses. The package relay logic will work with wtxids, e.g. upon receipt of an ancpkginfo containing wtxids of transactions, it will look for those transactions in the orphanage.
No effect for now, just additional tracking. Enables: - load balance orphan resolution, limit per-peer orphanage usage - limit the total size of orphans instead of just the count
Add ability to add and track multiple announcers per orphan transaction, erasing announcers but not the entire orphan.
This information is already known; returning it is essentially free. A later commit uses these wtxids to remove conflicted transactions from the orphan resolution tracker.
22e3c06
to
56eef67
Compare
🐙 This pull request conflicts with the target branch and needs rebase. |
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
1 similar comment
⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
Closing for now. The first commit is superseded by #30000, and the rest will come after |
This PR contains changes to
TxOrphanage
needed for package relay stuff. See #27463 for project tracking and #28031 for how this code is used.Separating this PR was suggested in #28031 (comment).
Changes here:
CTransaction::GetTotalSize()
of orphans stored, as well as the size per peer.OrphanTx
so that we don't need to calculate them again later on.EraseForBlock
which includes orphans that conflicted with the block. This helps us delete those orphan resolution attempts from our tracker.