Skip to content

Commit

Permalink
random fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@instagibbs
instagibbs committed Oct 13, 2023
1 parent 36f4d0d commit 5388830
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 10 deletions.
4 changes: 3 additions & 1 deletion src/node/txdownload_impl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ std::vector<GenTxid> 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());
Expand All @@ -294,7 +295,8 @@ std::vector<GenTxid> 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);
Expand Down
12 changes: 6 additions & 6 deletions src/test/fuzz/txorphan.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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
Expand All @@ -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()));
Expand Down
22 changes: 19 additions & 3 deletions src/txorphanage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<NodeId, std::set<uint256>>& 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<uint256> TxOrphanage::LimitOrphans(unsigned int max_orphans, unsigned int max_total_size)
{
LOCK(m_mutex);
Expand All @@ -202,7 +211,8 @@ std::vector<uint256> 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);
}
Expand All @@ -218,7 +228,8 @@ std::vector<uint256> 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);
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -307,6 +319,8 @@ std::vector<uint256> TxOrphanage::EraseForBlock(const CBlock& block)
LOCK(m_mutex);

std::vector<uint256> vOrphanErase;
std::vector<uint256> vOrphanErase_txid;
size_t index = 0;

for (const CTransactionRef& ptx : block.vtx) {
const CTransaction& tx = *ptx;
Expand All @@ -319,6 +333,7 @@ std::vector<uint256> 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());
}
}
}
Expand All @@ -327,7 +342,8 @@ std::vector<uint256> 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);
}
Expand Down
2 changes: 2 additions & 0 deletions src/txorphanage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<NodeId, std::set<uint256>>& 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. */
Expand Down

0 comments on commit 5388830

Please sign in to comment.