diff --git a/src/node/txdownload_impl.cpp b/src/node/txdownload_impl.cpp index 0ac2d9399d3bed..d152d3bad66f1e 100644 --- a/src/node/txdownload_impl.cpp +++ b/src/node/txdownload_impl.cpp @@ -272,6 +272,7 @@ std::vector TxDownloadImpl::GetRequestsToSend(NodeId nodeid, std::chron // Schedule with no delay instead of using ReceivedTxInv. This means it's scheduled // for request immediately unless there is already a request out for the same txhash // (e.g. if there is another orphan that needs this parent). + if (m_orphanage.HaveTxAndPeer(GenTxid::Txid(txid), nodeid)) continue; m_txrequest.ReceivedInv(nodeid, GenTxid::Txid(txid), info.m_preferred, current_time); LogPrint(BCLog::TXPACKAGES, "scheduled parent request %s from peer=%d for orphan %s\n", txid.ToString(), nodeid, orphan_gtxid.GetHash().ToString()); @@ -294,7 +295,8 @@ std::vector TxDownloadImpl::GetRequestsToSend(NodeId nodeid, std::chron entry.second.GetHash().ToString(), entry.first); } for (const GenTxid& gtxid : requestable) { - Assume(!m_orphanage.HaveTxAndPeer(gtxid, nodeid)); + Assume(!m_orphanage.HaveTxAndPeer(gtxid, nodeid)); // FIXME NewOrphanTx should ForgetTxHash so it's not requested anymore? + // Stands to reason we ReceivedInv at some point after ForgetTxHash? Hash could be embedded with a "ready" orphan above? if (!AlreadyHaveTxLocked(gtxid)) { LogPrint(BCLog::NET, "Requesting %s %s peer=%d\n", gtxid.IsWtxid() ? "wtx" : "tx", gtxid.GetHash().ToString(), nodeid); diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index a883c19c9cac29..4e1d32e382874a 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -94,15 +94,15 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) CTransactionRef ref = orphanage.GetTxToReconsider(peer_id); if (ref) { Assert(orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || - orphanage.HaveTx(GenTxid::Wtxid(ref->GetHash()))); + orphanage.HaveTx(GenTxid::Wtxid(ref->GetWitnessHash()))); Assert(orphanage.HaveTxAndPeer(GenTxid::Txid(ref->GetHash()), peer_id) || - orphanage.HaveTxAndPeer(GenTxid::Wtxid(ref->GetHash()), peer_id)); + orphanage.HaveTxAndPeer(GenTxid::Wtxid(ref->GetWitnessHash()), peer_id)); Assert(orphanage.BytesFromPeer(peer_id) >= ref->GetTotalSize()); } } }, [&] { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); // AddTx should return false if tx is too big or already have it // tx weight is unknown, we only check when tx is already in orphanage { @@ -116,7 +116,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) Assert(orphanage.TotalOrphanBytes() == total_bytes_before + tx->GetTotalSize()); } } - have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); { bool add_tx = orphanage.AddTx(tx, peer_id, {}); // if have_tx is still false, it must be too big @@ -125,12 +125,12 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) } }, [&] { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); // EraseTx should return 0 if m_orphans doesn't have the tx { Assert(have_tx == orphanage.EraseTx(tx->GetWitnessHash())); } - have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash())); + have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); // have_tx should be false and EraseTx should fail { Assert(!have_tx && !orphanage.EraseTx(tx->GetWitnessHash())); diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index a5606002a6b840..8803d572cad207 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -183,6 +183,15 @@ void TxOrphanage::EraseForPeer(NodeId peer) if (!Assume(m_peer_bytes_used.count(peer) == 0)) m_peer_bytes_used.erase(peer); } +void TxOrphanage::remove_work_from_all_sets(std::map>& peer_work_set, const uint256& txid) +{ + for (auto& [nodeid, work_set]: peer_work_set) { + if (work_set.count(txid) > 0) { + work_set.erase(txid); + } + } +} + std::vector TxOrphanage::LimitOrphans(unsigned int max_orphans, unsigned int max_total_size) { LOCK(m_mutex); @@ -202,7 +211,8 @@ std::vector TxOrphanage::LimitOrphans(unsigned int max_orphans, unsigne const auto& wtxid = maybeErase->second.tx->GetWitnessHash(); if (maybeErase->second.nTimeExpire <= nNow) { expired_and_evicted.emplace_back(wtxid); - nErased += EraseTxNoLock(wtxid); + nErased += EraseTxNoLock(wtxid); // FIXME m_peer_work_set still persisted? + remove_work_from_all_sets(m_peer_work_set, maybeErase->second.tx->GetHash()); } else { nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime); } @@ -218,7 +228,8 @@ std::vector TxOrphanage::LimitOrphans(unsigned int max_orphans, unsigne size_t randompos = rng.randrange(m_orphan_list.size()); const auto& wtxid = m_orphan_list[randompos]->second.tx->GetWitnessHash(); expired_and_evicted.emplace_back(wtxid); - EraseTxNoLock(wtxid); + EraseTxNoLock(wtxid); // FIXME m_peer_work_set still persisted? + remove_work_from_all_sets(m_peer_work_set, m_orphan_list[randompos]->second.tx->GetHash()); ++nEvicted; } if (nEvicted > 0) LogPrint(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted); @@ -283,6 +294,7 @@ CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) const auto orphan_it = m_orphans.find(txid); if (orphan_it != m_orphans.end()) { + Assume(orphan_it->second.announcers.count(peer) > 0); return orphan_it->second.tx; } } @@ -307,6 +319,8 @@ std::vector TxOrphanage::EraseForBlock(const CBlock& block) LOCK(m_mutex); std::vector vOrphanErase; + std::vector vOrphanErase_txid; + size_t index = 0; for (const CTransactionRef& ptx : block.vtx) { const CTransaction& tx = *ptx; @@ -319,6 +333,7 @@ std::vector TxOrphanage::EraseForBlock(const CBlock& block) const CTransaction& orphanTx = *(*mi)->second.tx; const uint256& orphan_wtxid = orphanTx.GetWitnessHash(); vOrphanErase.push_back(orphan_wtxid); + vOrphanErase_txid.push_back(orphanTx.GetHash()); } } } @@ -327,7 +342,8 @@ std::vector TxOrphanage::EraseForBlock(const CBlock& block) if (vOrphanErase.size()) { int nErased = 0; for (const uint256& orphan_wtxid : vOrphanErase) { - nErased += EraseTxNoLock(orphan_wtxid); + nErased += EraseTxNoLock(orphan_wtxid); // FIXME m_peer_work_set still persisted? + remove_work_from_all_sets(m_peer_work_set, vOrphanErase_txid[index]); } LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx included or conflicted by block\n", nErased); } diff --git a/src/txorphanage.h b/src/txorphanage.h index f9de6d44af65be..fd46cf3cc44319 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -24,6 +24,8 @@ static constexpr unsigned int DEFAULT_MAX_ORPHAN_TOTAL_SIZE{100 * MAX_STANDARD_T */ class TxOrphanage { public: + void remove_work_from_all_sets(std::map>& peer_work_set, const uint256& txid); + /** Add a new orphan transaction. If the tx already exists, add this peer to its list of announcers. * parent_txids should contain a (de-duplicated) list of txids of this transaction's missing parents. @returns true if the transaction was added as a new orphan. */