Skip to content

Merge bitcoin/bitcoin#29619: refactor: consolidate MempoolAcceptResult processing #1118

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.

Sign up for GitHub

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

Closed
DashCoreAutoGuix wants to merge 1 commit into from
+88 −51
Closed

Merge bitcoin/bitcoin#29619: refactor: consolidate MempoolAcceptResult processing #1118

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 88 additions & 51 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,20 @@ class PeerManagerImpl final : public PeerManager
*/
bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer);

/** Handle a transaction whose result was not MempoolAcceptResult::ResultType::VALID.
* @param[in] maybe_add_extra_compact_tx Whether this tx should be added to vExtraTxnForCompact.
* Set to false if the tx has already been rejected before,
* e.g. is an orphan, to avoid adding duplicate entries.
* Updates m_txrequest, m_recent_rejects, m_orphanage, and vExtraTxnForCompact. */
void ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result,
bool maybe_add_extra_compact_tx)
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);

/** Handle a transaction whose result was MempoolAcceptResult::ResultType::VALID.
* Updates m_txrequest, m_orphanage, and vExtraTxnForCompact. Also queues the tx for relay. */
void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);

/**
* Reconsider orphan transactions after a parent has been accepted to the mempool.
*
Expand Down Expand Up @@ -3204,6 +3218,65 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
return;
}

void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx, const TxValidationState& state,
bool maybe_add_extra_compact_tx)
{
AssertLockNotHeld(m_peer_mutex);
AssertLockHeld(g_msgproc_mutex);
AssertLockHeld(cs_main);

LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n",
ptx->GetHash().ToString(),
nodeid,
state.ToString());

if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
return;
} else {
m_recent_rejects.insert(ptx->GetHash());
m_txrequest.ForgetTxHash(ptx->GetHash());

if (maybe_add_extra_compact_tx && RecursiveDynamicUsage(*ptx) < 100000) {
AddToCompactExtraTransactions(ptx);
}
}

MaybePunishNodeForTx(nodeid, state);
m_llmq_ctx->isman->TransactionRemovedFromMempool(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->GetHash()) > 0) {
LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", ptx->GetHash().ToString());
}
}

void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
{
AssertLockNotHeld(m_peer_mutex);
AssertLockHeld(g_msgproc_mutex);
AssertLockHeld(cs_main);

// As this version of the transaction was acceptable, we can forget about any requests for it.
// No-op if the tx is not in txrequest.
m_txrequest.ForgetTxHash(tx->GetHash());

m_orphanage.AddChildrenToWorkSet(*tx, nodeid);
// If it came from the orphanage, remove it. No-op if the tx is not in txorphanage.
m_orphanage.EraseTx(tx->GetHash());

LogPrint(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (poolsz %u txn, %u kB)\n",
nodeid,
tx->GetHash().ToString(),
m_mempool.size(), m_mempool.DynamicMemoryUsage() / 1000);

_RelayTransaction(tx->GetHash());

for (const CTransactionRef& removedTx : replaced_transactions) {
AddToCompactExtraTransactions(removedTx);
}
}

bool PeerManagerImpl::ProcessOrphanTx(NodeId node_id)
{
AssertLockHeld(cs_main);
Expand All @@ -3219,24 +3292,22 @@ bool PeerManagerImpl::ProcessOrphanTx(NodeId node_id)

if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString());
_RelayTransaction(porphanTx->GetHash());
m_orphanage.AddChildrenToWorkSet(*porphanTx, node_id);
m_orphanage.EraseTx(orphanHash);
Assume(result.m_replaced_transactions.has_value());
std::list<CTransactionRef> empty_replacement_list;
ProcessValidTx(node_id, porphanTx, result.m_replaced_transactions.value_or(empty_replacement_list));
break;
} else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) {
if (state.IsInvalid()) {
LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s from peer=%d. %s\n",
orphanHash.ToString(),
from_peer,
state.ToString());
// Maybe punish peer that gave us an invalid orphan tx
MaybePunishNodeForTx(from_peer, state);
LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s from peer=%d. %s\n",
orphanHash.ToString(),
from_peer,
state.ToString());

if (Assume(state.IsInvalid() &&
state.GetResult() != TxValidationResult::TX_UNKNOWN &&
state.GetResult() != TxValidationResult::TX_NO_MEMPOOL &&
state.GetResult() != TxValidationResult::TX_RESULT_UNSET)) {
ProcessInvalidTx(from_peer, porphanTx, state, /*maybe_add_extra_compact_tx=*/false);
Copy link

@cursor cursor bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Mutex Assertion Failures in Transaction Processing

ProcessValidTx and ProcessInvalidTx assert that g_msgproc_mutex is held. However, ProcessOrphanTx calls these functions, and ProcessOrphanTx is invoked from BlockConnected without g_msgproc_mutex held, leading to assertion failures and potential data races. Additionally, Assume(...) is incorrectly used within a boolean expression in ProcessOrphanTx, causing a compile error.

Additional Locations (2)
Fix in Cursor Fix in Web

}
// Has inputs but not accepted to mempool
// Probably non-standard or insufficient fee
LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString());
m_recent_rejects.insert(orphanHash);
m_orphanage.EraseTx(orphanHash);
break;
}
}
Expand Down Expand Up @@ -4499,16 +4570,9 @@ void PeerManagerImpl::ProcessMessage(
m_cj_ctx->dstxman->AddDSTX(dstx);
}

_RelayTransaction(tx.GetHash());
m_orphanage.AddChildrenToWorkSet(tx, peer->m_id);

ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value());
pfrom.m_last_tx_time = GetTime<std::chrono::seconds>();

LogPrint(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (poolsz %u txn, %u kB)\n",
pfrom.GetId(),
tx.GetHash().ToString(),
m_mempool.size(), m_mempool.DynamicMemoryUsage() / 1000);

// Recursively process any orphan transactions that depended on this one
ProcessOrphanTx(peer->m_id);
}
Expand Down Expand Up @@ -4559,36 +4623,9 @@ void PeerManagerImpl::ProcessMessage(
m_recent_rejects.insert(tx.GetHash());
m_llmq_ctx->isman->TransactionRemovedFromMempool(ptx);
}
} else {
m_recent_rejects.insert(tx.GetHash());
if (RecursiveDynamicUsage(*ptx) < 100000) {
AddToCompactExtraTransactions(ptx);
}
}

// If a tx has been detected by m_recent_rejects, we will have reached
// this point and the tx will have been ignored. Because we haven't
// submitted the tx to our mempool, we won't have computed a DoS
// score for it or determined exactly why we consider it invalid.
//
// This means we won't penalize any peer subsequently relaying a DoSy
// tx (even if we penalized the first peer who gave it to us) because
// we have to account for m_recent_rejects showing false positives. In
// other words, we shouldn't penalize a peer if we aren't *sure* they
// submitted a DoSy tx.
//
// Note that m_recent_rejects doesn't just record DoSy or invalid
// transactions, but any tx not accepted by the m_mempool, which may be
// due to node policy (vs. consensus). So we can't blanket penalize a
// peer simply for relaying a tx that our m_recent_rejects has caught,
// regardless of false positives.

if (state.IsInvalid()) {
LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n", tx.GetHash().ToString(),
pfrom.GetId(),
state.ToString());
MaybePunishNodeForTx(pfrom.GetId(), state);
m_llmq_ctx->isman->TransactionRemovedFromMempool(ptx);
ProcessInvalidTx(pfrom.GetId(), ptx, state, /*maybe_add_extra_compact_tx=*/true);
Copy link

@cursor cursor bot Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Orphan Transaction Handling Regression

The commit introduces a compilation error in ProcessOrphanTx by using Assume(...) as a boolean condition. Furthermore, policy-rejected orphan transactions (not missing inputs and not invalid) are no longer added to m_recent_rejects or removed from the orphanage, nor are they added to vExtraTxnForCompact. This is because the new ProcessInvalidTx function, which handles these actions, is only called for state.IsInvalid() results. This omission leads to repeated reconsideration of these orphans, potential orphanage memory growth, and repeated re-requests from peers.

Additional Locations (1)
Fix in Cursor Fix in Web

}
return;
}
Expand Down
Loading