From 4ebe24985182ee4dc27f20a0e782015f77f3c8e0 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 14 Jul 2020 16:14:29 +0200 Subject: [PATCH] Merge #18990: log: Properly log txs rejected from mempool fa9f20b6477a206adf5089398803b45d1a114b6f log: Properly log txs rejected from mempool (MarcoFalke) Pull request description: Currently `CheckTxInputs` rejections from the mempool are the only rejections that log directly and unconditionally to debug.log instead of leaving it to the caller. This has multiple issues: * A rejected RPC transaction will log a redundant failure reason to debug log. All other failures are merely reported to the RPC user. * A rejected p2p transaction will log the failure twice. Once with the `MEMPOOLREJ` flag, and once unconditionally. * A rejected orphan transaction will log no failure. Fix all issues by simply returning the state to the caller, like it is done for all other rejections. The patch includes whitespace fixups to highlight relevant parts of the codebase and simplify review. ACKs for top commit: naumenkogs: utACK fa9f20b6477a206adf5089398803b45d1a114b6f rajarshimaitra: Concept ACK. Compiled and ran tests. `fa9f20b` jnewbery: code review ACK fa9f20b6477a206adf5089398803b45d1a114b6f Tree-SHA512: 86cc17b2a9239c01c4fc3f254ad48ee1d3883266966b9811030176338b9ac3deaea7ea5babfb8bbf739d7440154e30011fede8f9313175f199d4a062af6494f7 --- src/net_processing.cpp | 8 +++++--- src/validation.cpp | 5 +++-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6b632ad62ad8fa..ceeee786f15951 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2582,7 +2582,10 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) break; } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { if (state.IsInvalid()) { - LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s\n", orphanHash.ToString()); + LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s from peer=%d. %s\n", + orphanHash.ToString(), + orphan_it->second.fromPeer, + state.ToString()); // Maybe punish peer that gave us an invalid orphan tx MaybePunishNodeForTx(orphan_it->second.fromPeer, state); } @@ -3736,8 +3739,7 @@ void PeerManagerImpl::ProcessMessage( // peer simply for relaying a tx that our m_recent_rejects has caught, // regardless of false positives. - if (state.IsInvalid()) - { + if (state.IsInvalid()) { LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n", tx.GetHash().ToString(), pfrom.GetId(), state.ToString()); diff --git a/src/validation.cpp b/src/validation.cpp index dc1f5242676df8..dae414e5456b3b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -648,8 +648,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) std::unique_ptr& entry = ws.m_entry; CAmount& nModifiedFees = ws.m_modified_fees; - if (!CheckTransaction(tx, state)) + if (!CheckTransaction(tx, state)) { return false; // state filled in by CheckTransaction + } assert(std::addressof(::ChainstateActive()) == std::addressof(m_active_chainstate)); if (!ContextualCheckTransaction(tx, state, chainparams.GetConsensus(), m_active_chainstate.m_chain.Tip())) @@ -761,7 +762,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) assert(std::addressof(g_chainman.m_blockman) == std::addressof(m_active_chainstate.m_blockman)); if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_blockman.GetSpendHeight(m_view), ws.m_base_fees)) { - return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), state.ToString()); + return false; // state filled in by CheckTxInputs } // Check for non-standard pay-to-script-hash in inputs