p2p: index TxOrphanage by wtxid, allow entries with same txid #30000
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2295,7 +2295,23 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside | |
|
|
||
| const uint256& hash = gtxid.GetHash(); | ||
|
|
||
| if (gtxid.IsWtxid()) { | ||
| // Normal query by wtxid. | ||
| if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; | ||
| } else { | ||
| // Never query by txid: it is possible that the transaction in the orphanage has the same | ||
| // txid but a different witness, which would give us a false positive result. If we decided | ||
| // not to request the transaction based on this result, an attacker could prevent us from | ||
| // downloading a transaction by intentionally creating a malleated version of it. While | ||
| // only one (or none!) of these transactions can ultimately be confirmed, we have no way of | ||
| // discerning which one that is, so the orphanage can store multiple transactions with the | ||
| // same txid. | ||
| // | ||
| // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid. | ||
| // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will | ||
| // help us find non-segwit transactions, saving bandwidth, and should have no false positives. | ||
| if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; | ||
| } | ||
|
Comment on lines
+2298
to
+2314
There was a problem hiding this comment. (since @sr-gi mentioned this offline) Yes, the last push turned this 1 line into 2 identical lines. The purpose is to illustrate to current reviewers and future readers more clearly... if wtxid, we do this. if txid, we also do this, but notice that we are actually doing a "cast" and that's ok because we are guessing what the wtxid is. Also, perhaps these will diverge again in the future if |
||
|
|
||
| if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true; | ||
|
|
||
|
|
@@ -3219,7 +3235,7 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx | |
|
|
||
| // If the tx failed in ProcessOrphanTx, it should be removed from the orphanage unless the | ||
| // tx was still missing inputs. If the tx was not in the orphanage, EraseTx does nothing and returns 0. | ||
| if (Assume(state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) && m_orphanage.EraseTx(ptx->GetWitnessHash()) > 0) { | ||
| LogDebug(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); | ||
| } | ||
| } | ||
|
|
@@ -3237,7 +3253,7 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c | |
|
|
||
| m_orphanage.AddChildrenToWorkSet(*tx); | ||
| // If it came from the orphanage, remove it. No-op if the tx is not in txorphanage. | ||
| m_orphanage.EraseTx(tx->GetWitnessHash()); | ||
|
|
||
| LogDebug(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (wtxid=%s) (poolsz %u txn, %u kB)\n", | ||
| nodeid, | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -23,7 +23,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) | |||||
|
|
||||||
| const Txid& hash = tx->GetHash(); | ||||||
glozow marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| const Wtxid& wtxid = tx->GetWitnessHash(); | ||||||
| if (m_orphans.count(wtxid)) | ||||||
|
There was a problem hiding this comment. c++-20 nit:
Suggested change
|
||||||
| return false; | ||||||
|
|
||||||
| // Ignore big transactions, to avoid a | ||||||
|
|
@@ -40,30 +40,28 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) | |||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| auto ret = m_orphans.emplace(wtxid, OrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size()}); | ||||||
| assert(ret.second); | ||||||
| m_orphan_list.push_back(ret.first); | ||||||
| for (const CTxIn& txin : tx->vin) { | ||||||
| m_outpoint_to_orphan_it[txin.prevout].insert(ret.first); | ||||||
| } | ||||||
|
|
||||||
| LogPrint(BCLog::TXPACKAGES, "stored orphan tx %s (wtxid=%s), weight: %u (mapsz %u outsz %u)\n", hash.ToString(), wtxid.ToString(), sz, | ||||||
| m_orphans.size(), m_outpoint_to_orphan_it.size()); | ||||||
| return true; | ||||||
| } | ||||||
|
|
||||||
| int TxOrphanage::EraseTx(const Wtxid& wtxid) | ||||||
| { | ||||||
| LOCK(m_mutex); | ||||||
| return EraseTxNoLock(wtxid); | ||||||
| } | ||||||
|
|
||||||
| int TxOrphanage::EraseTxNoLock(const Wtxid& wtxid) | ||||||
| { | ||||||
| AssertLockHeld(m_mutex); | ||||||
| std::map<Wtxid, OrphanTx>::iterator it = m_orphans.find(wtxid); | ||||||
| if (it == m_orphans.end()) | ||||||
| return 0; | ||||||
glozow marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| for (const CTxIn& txin : it->second.tx->vin) | ||||||
|
|
@@ -85,10 +83,12 @@ int TxOrphanage::EraseTxNoLock(const Txid& txid) | |||||
| m_orphan_list[old_pos] = it_last; | ||||||
| it_last->second.list_pos = old_pos; | ||||||
| } | ||||||
| const auto& txid = it->second.tx->GetHash(); | ||||||
| // Time spent in orphanage = difference between current and entry time. | ||||||
| // Entry time is equal to ORPHAN_TX_EXPIRE_TIME earlier than entry's expiry. | ||||||
| LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s) after %ds\n", txid.ToString(), wtxid.ToString(), | ||||||
| GetTime() + ORPHAN_TX_EXPIRE_TIME - it->second.nTimeExpire); | ||||||
| m_orphan_list.pop_back(); | ||||||
|
|
||||||
| m_orphans.erase(it); | ||||||
| return 1; | ||||||
|
|
@@ -101,16 +101,16 @@ void TxOrphanage::EraseForPeer(NodeId peer) | |||||
| m_peer_work_set.erase(peer); | ||||||
|
|
||||||
| int nErased = 0; | ||||||
| std::map<Wtxid, OrphanTx>::iterator iter = m_orphans.begin(); | ||||||
| while (iter != m_orphans.end()) | ||||||
| { | ||||||
| // increment to avoid iterator becoming invalid after erasure | ||||||
| const auto& [wtxid, orphan] = *iter++; | ||||||
| if (orphan.fromPeer == peer) { | ||||||
| nErased += EraseTxNoLock(wtxid); | ||||||
| } | ||||||
| } | ||||||
| if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) from peer=%d\n", nErased, peer); | ||||||
| } | ||||||
|
|
||||||
| void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) | ||||||
|
|
@@ -124,12 +124,12 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) | |||||
| // Sweep out expired orphan pool entries: | ||||||
| int nErased = 0; | ||||||
| int64_t nMinExpTime = nNow + ORPHAN_TX_EXPIRE_TIME - ORPHAN_TX_EXPIRE_INTERVAL; | ||||||
| std::map<Wtxid, OrphanTx>::iterator iter = m_orphans.begin(); | ||||||
| while (iter != m_orphans.end()) | ||||||
| { | ||||||
| std::map<Wtxid, OrphanTx>::iterator maybeErase = iter++; | ||||||
| if (maybeErase->second.nTimeExpire <= nNow) { | ||||||
| nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash()); | ||||||
| } else { | ||||||
| nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime); | ||||||
| } | ||||||
|
|
@@ -142,7 +142,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) | |||||
| { | ||||||
| // Evict a random orphan: | ||||||
| size_t randompos = rng.randrange(m_orphan_list.size()); | ||||||
| EraseTxNoLock(m_orphan_list[randompos]->second.tx->GetWitnessHash()); | ||||||
| ++nEvicted; | ||||||
| } | ||||||
| if (nEvicted > 0) LogPrint(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted); | ||||||
|
|
@@ -159,7 +159,7 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) | |||||
| for (const auto& elem : it_by_prev->second) { | ||||||
| // Get this source peer's work set, emplacing an empty set if it didn't exist | ||||||
| // (note: if this peer wasn't still connected, we would have removed the orphan tx already) | ||||||
| std::set<Wtxid>& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second; | ||||||
| // Add this tx to the work set | ||||||
| orphan_work_set.insert(elem->first); | ||||||
| LogPrint(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n", | ||||||
glozow marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
@@ -169,14 +169,10 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| bool TxOrphanage::HaveTx(const Wtxid& wtxid) const | ||||||
| { | ||||||
| LOCK(m_mutex); | ||||||
| return m_orphans.count(wtxid); | ||||||
|
There was a problem hiding this comment. c++20-nit:
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) | ||||||
|
|
@@ -187,10 +183,10 @@ CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) | |||||
| if (work_set_it != m_peer_work_set.end()) { | ||||||
| auto& work_set = work_set_it->second; | ||||||
| while (!work_set.empty()) { | ||||||
| Wtxid wtxid = *work_set.begin(); | ||||||
| work_set.erase(work_set.begin()); | ||||||
|
|
||||||
| const auto orphan_it = m_orphans.find(wtxid); | ||||||
| if (orphan_it != m_orphans.end()) { | ||||||
| return orphan_it->second.tx; | ||||||
| } | ||||||
|
|
@@ -215,7 +211,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block) | |||||
| { | ||||||
| LOCK(m_mutex); | ||||||
|
|
||||||
| std::vector<Wtxid> vOrphanErase; | ||||||
|
|
||||||
| for (const CTransactionRef& ptx : block.vtx) { | ||||||
| const CTransaction& tx = *ptx; | ||||||
|
|
@@ -226,8 +222,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block) | |||||
| if (itByPrev == m_outpoint_to_orphan_it.end()) continue; | ||||||
| for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) { | ||||||
| const CTransaction& orphanTx = *(*mi)->second.tx; | ||||||
| vOrphanErase.push_back(orphanTx.GetWitnessHash()); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -238,7 +233,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block) | |||||
| for (const auto& orphanHash : vOrphanErase) { | ||||||
| nErased += EraseTxNoLock(orphanHash); | ||||||
| } | ||||||
| LogPrint(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) included or conflicted by block\n", nErased); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,8 +23,8 @@ class TxOrphanage { | |
| /** Add a new orphan transaction */ | ||
| bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); | ||
|
|
||
| /** Check if we already have an orphan transaction (by wtxid only) */ | ||
| bool HaveTx(const Wtxid& wtxid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); | ||
|
|
||
| /** Extract a transaction from a peer's work set | ||
| * Returns nullptr if there are no transactions to work on. | ||
|
|
@@ -33,8 +33,8 @@ class TxOrphanage { | |
| */ | ||
| CTransactionRef GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); | ||
|
|
||
| /** Erase an orphan by wtxid */ | ||
| int EraseTx(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); | ||
|
|
||
| /** Erase all orphans announced by a peer (eg, after that peer disconnects) */ | ||
| void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); | ||
|
|
@@ -77,12 +77,12 @@ class TxOrphanage { | |
| size_t list_pos; | ||
| }; | ||
|
|
||
| /** Map from wtxid to orphan transaction record. Limited by | ||
| * -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */ | ||
| std::map<Wtxid, OrphanTx> m_orphans GUARDED_BY(m_mutex); | ||
|
|
||
| /** Which peer provided the orphans that need to be reconsidered */ | ||
| std::map<NodeId, std::set<Wtxid>> m_peer_work_set GUARDED_BY(m_mutex); | ||
|
|
||
| using OrphanMap = decltype(m_orphans); | ||
|
|
||
|
|
@@ -102,12 +102,8 @@ class TxOrphanage { | |
| /** Orphan transactions in vector for quick random eviction */ | ||
| std::vector<OrphanMap::iterator> m_orphan_list GUARDED_BY(m_mutex); | ||
|
|
||
| /** Erase an orphan by wtxid */ | ||
| int EraseTxNoLock(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex); | ||
| }; | ||
|
|
||
| #endif // BITCOIN_TXORPHANAGE_H | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 8923edf:
nit: I feel like documentation on why
TxOrphanageindexes by wtxid/allows multiple versions of the same transaction should be documented inTxOrphanageinstead of here.