Skip to content

Commit 07cd510

Browse files
glozowglozow
committed
[refactor] consolidate invalid MempoolAcceptResult processing
Deduplicate code that exists in both tx processing and ProcessOrphanTx. Additionally, this can be reused in a function that handles multiple MempoolAcceptResults from package submission.
1 parent 9353aa4 commit 07cd510

File tree

1 file changed

+78
-86
lines changed

1 file changed

+78
-86
lines changed

‎src/net_processing.cpp‎

Lines changed: 78 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,15 @@ class PeerManagerImpl final : public PeerManager
582582
*/
583583
bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer);
584584

585+
/** Handle a transaction whose result was not MempoolAcceptResult::ResultType::VALID.
586+
* @param[in] maybe_add_extra_compact_tx Whether this tx should be added to vExtraTxnForCompact.
587+
* Set to false if the tx has already been rejected before,
588+
* e.g. is an orphan, to avoid adding duplicate entries.
589+
* Updates m_txrequest, m_recent_rejects, m_orphanage, and vExtraTxnForCompact. */
590+
void ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result,
591+
bool maybe_add_extra_compact_tx)
592+
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
593+
585594
/** Handle a transaction whose result was MempoolAcceptResult::ResultType::VALID.
586595
* Updates m_txrequest, m_orphanage, and vExtraTxnForCompact. Also queues the tx for relay. */
587596
void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
@@ -3037,6 +3046,63 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
30373046
return;
30383047
}
30393048

3049+
void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx, const TxValidationState& state,
3050+
bool maybe_add_extra_compact_tx)
3051+
{
3052+
AssertLockNotHeld(m_peer_mutex);
3053+
AssertLockHeld(g_msgproc_mutex);
3054+
AssertLockHeld(cs_main);
3055+
3056+
LogDebug(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n",
3057+
ptx->GetHash().ToString(),
3058+
ptx->GetWitnessHash().ToString(),
3059+
nodeid,
3060+
state.ToString());
3061+
3062+
if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
3063+
return;
3064+
} else if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) {
3065+
// We can add the wtxid of this transaction to our reject filter.
3066+
// Do not add txids of witness transactions or witness-stripped
3067+
// transactions to the filter, as they can have been malleated;
3068+
// adding such txids to the reject filter would potentially
3069+
// interfere with relay of valid transactions from peers that
3070+
// do not support wtxid-based relay. See
3071+
// https://github.com/bitcoin/bitcoin/issues/8279 for details.
3072+
// We can remove this restriction (and always add wtxids to
3073+
// the filter even for witness stripped transactions) once
3074+
// wtxid-based relay is broadly deployed.
3075+
// See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
3076+
// for concerns around weakening security of unupgraded nodes
3077+
// if we start doing this too early.
3078+
m_recent_rejects.insert(ptx->GetWitnessHash().ToUint256());
3079+
m_txrequest.ForgetTxHash(ptx->GetWitnessHash());
3080+
// If the transaction failed for TX_INPUTS_NOT_STANDARD,
3081+
// then we know that the witness was irrelevant to the policy
3082+
// failure, since this check depends only on the txid
3083+
// (the scriptPubKey being spent is covered by the txid).
3084+
// Add the txid to the reject filter to prevent repeated
3085+
// processing of this transaction in the event that child
3086+
// transactions are later received (resulting in
3087+
// parent-fetching by txid via the orphan-handling logic).
3088+
if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && ptx->HasWitness()) {
3089+
m_recent_rejects.insert(ptx->GetHash().ToUint256());
3090+
m_txrequest.ForgetTxHash(ptx->GetHash());
3091+
}
3092+
if (maybe_add_extra_compact_tx && RecursiveDynamicUsage(*ptx) < 100000) {
3093+
AddToCompactExtraTransactions(ptx);
3094+
}
3095+
}
3096+
3097+
MaybePunishNodeForTx(nodeid, state);
3098+
3099+
// If the tx failed in ProcessOrphanTx, it should be removed from the orphanage unless the
3100+
// tx was still missing inputs. If the tx was not in the orphanage, EraseTx does nothing and returns 0.
3101+
if (Assume(state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) && m_orphanage.EraseTx(ptx->GetHash()) > 0) {
3102+
LogDebug(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString());
3103+
}
3104+
}
3105+
30403106
void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
30413107
{
30423108
AssertLockNotHeld(m_peer_mutex);
@@ -3085,53 +3151,18 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
30853151
ProcessValidTx(peer.m_id, porphanTx, result.m_replaced_transactions.value_or(empty_replacement_list));
30863152
return true;
30873153
} else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) {
3088-
if (state.IsInvalid()) {
3089-
LogPrint(BCLog::TXPACKAGES, " invalid orphan tx %s (wtxid=%s) from peer=%d. %s\n",
3090-
orphanHash.ToString(),
3091-
orphan_wtxid.ToString(),
3092-
peer.m_id,
3093-
state.ToString());
3094-
LogPrint(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n",
3095-
orphanHash.ToString(),
3096-
orphan_wtxid.ToString(),
3097-
peer.m_id,
3098-
state.ToString());
3099-
// Maybe punish peer that gave us an invalid orphan tx
3100-
MaybePunishNodeForTx(peer.m_id, state);
3101-
}
3102-
// Has inputs but not accepted to mempool
3103-
// Probably non-standard or insufficient fee
3104-
LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", orphanHash.ToString(), orphan_wtxid.ToString());
3105-
if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) {
3106-
// We can add the wtxid of this transaction to our reject filter.
3107-
// Do not add txids of witness transactions or witness-stripped
3108-
// transactions to the filter, as they can have been malleated;
3109-
// adding such txids to the reject filter would potentially
3110-
// interfere with relay of valid transactions from peers that
3111-
// do not support wtxid-based relay. See
3112-
// https://github.com/bitcoin/bitcoin/issues/8279 for details.
3113-
// We can remove this restriction (and always add wtxids to
3114-
// the filter even for witness stripped transactions) once
3115-
// wtxid-based relay is broadly deployed.
3116-
// See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
3117-
// for concerns around weakening security of unupgraded nodes
3118-
// if we start doing this too early.
3119-
m_recent_rejects.insert(porphanTx->GetWitnessHash().ToUint256());
3120-
// If the transaction failed for TX_INPUTS_NOT_STANDARD,
3121-
// then we know that the witness was irrelevant to the policy
3122-
// failure, since this check depends only on the txid
3123-
// (the scriptPubKey being spent is covered by the txid).
3124-
// Add the txid to the reject filter to prevent repeated
3125-
// processing of this transaction in the event that child
3126-
// transactions are later received (resulting in
3127-
// parent-fetching by txid via the orphan-handling logic).
3128-
if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && porphanTx->HasWitness()) {
3129-
// We only add the txid if it differs from the wtxid, to
3130-
// avoid wasting entries in the rolling bloom filter.
3131-
m_recent_rejects.insert(porphanTx->GetHash().ToUint256());
3132-
}
3154+
LogPrint(BCLog::TXPACKAGES, " invalid orphan tx %s (wtxid=%s) from peer=%d. %s\n",
3155+
orphanHash.ToString(),
3156+
orphan_wtxid.ToString(),
3157+
peer.m_id,
3158+
state.ToString());
3159+
3160+
if (Assume(state.IsInvalid() &&
3161+
state.GetResult() != TxValidationResult::TX_UNKNOWN &&
3162+
state.GetResult() != TxValidationResult::TX_NO_MEMPOOL &&
3163+
state.GetResult() != TxValidationResult::TX_RESULT_UNSET)) {
3164+
ProcessInvalidTx(peer.m_id, porphanTx, state, /*maybe_add_extra_compact_tx=*/false);
31333165
}
3134-
m_orphanage.EraseTx(orphanHash);
31353166
return true;
31363167
}
31373168
}
@@ -4363,48 +4394,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
43634394
m_txrequest.ForgetTxHash(tx.GetHash());
43644395
m_txrequest.ForgetTxHash(tx.GetWitnessHash());
43654396
}
4366-
} else {
4367-
if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) {
4368-
// We can add the wtxid of this transaction to our reject filter.
4369-
// Do not add txids of witness transactions or witness-stripped
4370-
// transactions to the filter, as they can have been malleated;
4371-
// adding such txids to the reject filter would potentially
4372-
// interfere with relay of valid transactions from peers that
4373-
// do not support wtxid-based relay. See
4374-
// https://github.com/bitcoin/bitcoin/issues/8279 for details.
4375-
// We can remove this restriction (and always add wtxids to
4376-
// the filter even for witness stripped transactions) once
4377-
// wtxid-based relay is broadly deployed.
4378-
// See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
4379-
// for concerns around weakening security of unupgraded nodes
4380-
// if we start doing this too early.
4381-
m_recent_rejects.insert(tx.GetWitnessHash().ToUint256());
4382-
m_txrequest.ForgetTxHash(tx.GetWitnessHash());
4383-
// If the transaction failed for TX_INPUTS_NOT_STANDARD,
4384-
// then we know that the witness was irrelevant to the policy
4385-
// failure, since this check depends only on the txid
4386-
// (the scriptPubKey being spent is covered by the txid).
4387-
// Add the txid to the reject filter to prevent repeated
4388-
// processing of this transaction in the event that child
4389-
// transactions are later received (resulting in
4390-
// parent-fetching by txid via the orphan-handling logic).
4391-
if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && tx.HasWitness()) {
4392-
m_recent_rejects.insert(tx.GetHash().ToUint256());
4393-
m_txrequest.ForgetTxHash(tx.GetHash());
4394-
}
4395-
if (RecursiveDynamicUsage(*ptx) < 100000) {
4396-
AddToCompactExtraTransactions(ptx);
4397-
}
4398-
}
43994397
}
4400-
44014398
if (state.IsInvalid()) {
4402-
LogPrint(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n",
4403-
tx.GetHash().ToString(),
4404-
tx.GetWitnessHash().ToString(),
4405-
pfrom.GetId(),
4406-
state.ToString());
4407-
MaybePunishNodeForTx(pfrom.GetId(), state);
4399+
ProcessInvalidTx(pfrom.GetId(), ptx, state, /*maybe_add_extra_compact_tx=*/true);
44084400
}
44094401
return;
44104402
}

0 commit comments

Comments
 (0)