From e2170fa44e91da860286c75db14add4ccbc052df Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 31 Aug 2023 12:47:13 -0400 Subject: [PATCH 1/4] Merge bitcoin/bitcoin#28364: log: log wtxids when possible, add TXPACKAGES category a3b55c94b9ffde07386aa887149ddca91b364967 [doc] move comment about AlreadyHaveTx DoS score to the right place (glozow) 3b8c17838a561616fd7c933753c7b98b6c6c7c99 [log] add more logs related to orphan handling (glozow) 51b3275cd1de467933f13d8b71286bf5ebd12b4b [log] add category TXPACKAGES for orphanage and package relay (glozow) a33dde1e41d8923a46db84b50550175fa6149c48 [log] include wtxid in tx {relay,validation,orphanage} logging (glozow) Pull request description: This was taken from #28031 (see #27463 for project tracking). - Log wtxids in addition to txids when possible. This allows us to track the fate of a transaction from inv to mempool accept/reject through logs. - Additional orphan-related logging to make testing and debugging easier. Suggested in https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1531022386 and https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1269622220 - Add `TXPACKAGES` category for logging. - Move a nearby comment block that was in the wrong place. ACKs for top commit: instagibbs: reACK https://github.com/bitcoin/bitcoin/pull/28364/commits/a3b55c94b9ffde07386aa887149ddca91b364967 achow101: ACK a3b55c94b9ffde07386aa887149ddca91b364967 brunoerg: crACK a3b55c94b9ffde07386aa887149ddca91b364967 mzumsande: Code review ACK a3b55c94b9ffde07386aa887149ddca91b364967 Tree-SHA512: 21884ef7c2ea2fd006e715574a9dd3e6cbbe8f82d62c6187fe1d39aad5a834051203fda5f355a06ca40c3e2b9561aec50d7c922a662b1edc96f7b552c9f4b24d --- src/logging.cpp | 6 +++ src/logging.h | 2 + src/net_processing.cpp | 74 +++++++++++++++++++++++++++--- src/txorphanage.cpp | 16 +++++-- src/validation.cpp | 23 ++++++++++ test/functional/p2p_permissions.py | 6 +-- 6 files changed, 113 insertions(+), 14 deletions(-) diff --git a/src/logging.cpp b/src/logging.cpp index 2d98525a3a2a..30cbeeba94f2 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -179,6 +179,8 @@ const CLogCategoryDesc LogCategories[] = #endif {BCLog::BLOCKSTORE, "blockstorage"}, {BCLog::TXRECONCILIATION, "txreconciliation"}, + {BCLog::SCAN, "scan"}, + {BCLog::TXPACKAGES, "txpackages"}, {BCLog::ALL, "1"}, {BCLog::ALL, "all"}, @@ -296,6 +298,10 @@ std::string LogCategoryToStr(BCLog::LogFlags category) return "blockstorage"; case BCLog::LogFlags::TXRECONCILIATION: return "txreconciliation"; + case BCLog::LogFlags::SCAN: + return "scan"; + case BCLog::LogFlags::TXPACKAGES: + return "txpackages"; /* Start Dash */ case BCLog::LogFlags::CHAINLOCKS: return "chainlocks"; diff --git a/src/logging.h b/src/logging.h index 2488d056cd79..0d48b132a948 100644 --- a/src/logging.h +++ b/src/logging.h @@ -67,6 +67,8 @@ namespace BCLog { #endif BLOCKSTORE = (1 << 26), TXRECONCILIATION = (1 << 27), + SCAN = (1 << 28), + TXPACKAGES = (1 << 29), //Start Dash CHAINLOCKS = ((uint64_t)1 << 32), diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1d634907c5bc..aca1583d56e3 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3219,8 +3219,10 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx); const TxValidationState& state = result.m_state; + const uint256& orphan_wtxid = porphanTx->GetWitnessHash(); if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { + LogPrint(BCLog::TXPACKAGES, " accepted orphan tx %s (wtxid=%s)\n", orphanHash.ToString(), orphan_wtxid.ToString()); LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); _RelayTransaction(porphanTx->GetHash()); m_orphanage.AddChildrenToWorkSet(*porphanTx, orphan_work_set); @@ -3228,8 +3230,14 @@ 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 from peer=%d. %s\n", + LogPrint(BCLog::TXPACKAGES, " invalid orphan tx %s (wtxid=%s) from peer=%d. %s\n", orphanHash.ToString(), + orphan_wtxid.ToString(), + from_peer, + state.ToString()); + LogPrint(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n", + orphanHash.ToString(), + orphan_wtxid.ToString(), from_peer, state.ToString()); // Maybe punish peer that gave us an invalid orphan tx @@ -3237,8 +3245,39 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) } // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee + LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", orphanHash.ToString(), orphan_wtxid.ToString()); LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString()); m_recent_rejects.insert(orphanHash); + if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) { + // We can add the wtxid of this transaction to our reject filter. + // Do not add txids of witness transactions or witness-stripped + // transactions to the filter, as they can have been malleated; + // adding such txids to the reject filter would potentially + // interfere with relay of valid transactions from peers that + // do not support wtxid-based relay. See + // https://github.com/bitcoin/bitcoin/issues/8279 for details. + // We can remove this restriction (and always add wtxids to + // the filter even for witness stripped transactions) once + // wtxid-based relay is broadly deployed. + // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034 + // for concerns around weakening security of unupgraded nodes + // if we start doing this too early. + m_recent_rejects.insert(porphanTx->GetWitnessHash()); + // If the transaction failed for TX_INPUTS_NOT_STANDARD, + // then we know that the witness was irrelevant to the policy + // failure, since this check depends only on the txid + // (the scriptPubKey being spent is covered by the txid). + // Add the txid to the reject filter to prevent repeated + // processing of this transaction in the event that child + // transactions are later received (resulting in + // parent-fetching by txid via the orphan-handling logic). + if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && porphanTx->GetWitnessHash() != porphanTx->GetHash()) { + // We only add the txid if it differs from the wtxid, to + // avoid wasting entries in the rolling bloom filter. + m_recent_rejects.insert(porphanTx->GetHash()); + } + } +>>>>>>> 5666966dff (Merge bitcoin/bitcoin#28364: log: log wtxids when possible, add TXPACKAGES category) m_orphanage.EraseTx(orphanHash); break; } @@ -4478,12 +4517,30 @@ void PeerManagerImpl::ProcessMessage( // allowing the node to function as a gateway for // nodes hidden behind it. if (!m_mempool.exists(tx.GetHash())) { - LogPrintf("Not relaying non-mempool transaction %s from forcerelay peer=%d\n", tx.GetHash().ToString(), pfrom.GetId()); + LogPrintf("Not relaying non-mempool transaction %s (wtxid=%s) from forcerelay peer=%d\n", + tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), pfrom.GetId()); } else { - LogPrintf("Force relaying tx %s from peer=%d\n", tx.GetHash().ToString(), pfrom.GetId()); + LogPrintf("Force relaying tx %s (wtxid=%s) from peer=%d\n", + tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), pfrom.GetId()); _RelayTransaction(tx.GetHash()); + } } + // If a tx is detected by m_recent_rejects it is 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 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. return; } @@ -4503,9 +4560,10 @@ void PeerManagerImpl::ProcessMessage( pfrom.m_last_tx_time = GetTime(); - LogPrint(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (poolsz %u txn, %u kB)\n", + LogPrint(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (wtxid=%s) (poolsz %u txn, %u kB)\n", pfrom.GetId(), tx.GetHash().ToString(), + tx.GetWitnessHash().ToString(), m_mempool.size(), m_mempool.DynamicMemoryUsage() / 1000); // Recursively process any orphan transactions that depended on this one @@ -4555,7 +4613,9 @@ void PeerManagerImpl::ProcessMessage( LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted); } } else { - LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s\n",tx.GetHash().ToString()); + LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", + tx.GetHash().ToString(), + tx.GetWitnessHash().ToString()); // We will continue to reject this tx since it has rejected // parents so avoid re-requesting it from other peers. m_recent_rejects.insert(tx.GetHash()); @@ -4586,7 +4646,9 @@ void PeerManagerImpl::ProcessMessage( // regardless of false positives. if (state.IsInvalid()) { - LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n", tx.GetHash().ToString(), + LogPrint(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n", + tx.GetHash().ToString(), + tx.GetWitnessHash().ToString(), pfrom.GetId(), state.ToString()); MaybePunishNodeForTx(pfrom.GetId(), state); diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index ce9d6435ce9f..d6b0695914fc 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -23,6 +23,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) AssertLockHeld(g_cs_orphans); const uint256& hash = tx->GetHash(); + const uint256& wtxid = tx->GetWitnessHash(); if (m_orphans.count(hash)) return false; @@ -36,7 +37,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) unsigned int sz = GetSerializeSize(*tx, CTransaction::CURRENT_VERSION); if (sz > MAX_STANDARD_TX_SIZE) { - LogPrint(BCLog::MEMPOOL, "ignoring large orphan tx (size: %u, hash: %s)\n", sz, hash.ToString()); + LogPrint(BCLog::TXPACKAGES, "ignoring large orphan tx (size: %u, txid: %s, wtxid: %s)\n", sz, hash.ToString(), wtxid.ToString()); return false; } @@ -49,7 +50,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) m_orphan_tx_size += sz; - LogPrint(BCLog::MEMPOOL, "stored orphan tx %s (mapsz %u outsz %u)\n", hash.ToString(), + LogPrint(BCLog::TXPACKAGES, "stored orphan tx %s (wtxid=%s) (mapsz %u outsz %u)\n", hash.ToString(), wtxid.ToString(), m_orphans.size(), m_outpoint_to_orphan_it.size()); ::g_stats_client->inc("transactions.orphans.add", 1.0f); ::g_stats_client->gauge("transactions.orphans", m_orphans.size()); @@ -82,6 +83,8 @@ int TxOrphanage::EraseTx(const uint256& txid) m_orphan_list[old_pos] = it_last; it_last->second.list_pos = old_pos; } + const auto& wtxid = it->second.tx->GetWitnessHash(); + LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", txid.ToString(), wtxid.ToString()); m_orphan_list.pop_back(); assert(m_orphan_tx_size >= it->second.nTxSize); @@ -106,7 +109,7 @@ void TxOrphanage::EraseForPeer(NodeId peer) nErased += EraseTx(maybeErase->second.tx->GetHash()); } } - if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx from peer=%d\n", nErased, peer); + if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer); } unsigned int TxOrphanage::LimitOrphans(unsigned int max_orphans_size) @@ -132,7 +135,7 @@ unsigned int TxOrphanage::LimitOrphans(unsigned int max_orphans_size) } // Sweep again 5 minutes after the next entry that expires in order to batch the linear scan. nNextSweep = nMinExpTime + ORPHAN_TX_EXPIRE_INTERVAL; - if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx due to expiration\n", nErased); + if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx due to expiration\n", nErased); } FastRandomContext rng; while (!m_orphans.empty() && m_orphan_tx_size > max_orphans_size) @@ -142,6 +145,7 @@ unsigned int TxOrphanage::LimitOrphans(unsigned int max_orphans_size) EraseTx(m_orphan_list[randompos]->first); ++nEvicted; } + if (nEvicted > 0) LogPrint(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted); return nEvicted; } @@ -153,6 +157,8 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx, std::set if (it_by_prev != m_outpoint_to_orphan_it.end()) { for (const auto& elem : it_by_prev->second) { orphan_work_set.insert(elem->first); + LogPrint(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n", + tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), elem->second.fromPeer); } } } @@ -222,6 +228,6 @@ void TxOrphanage::EraseForBlock(const CBlock& block) for (const uint256& orphanHash : vOrphanErase) { nErased += EraseTx(orphanHash); } - LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx included or conflicted by block\n", nErased); + LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx included or conflicted by block\n", nErased); } } diff --git a/src/validation.cpp b/src/validation.cpp index 0df6f5aaa852..60567a87ae03 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1014,6 +1014,29 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) std::unique_ptr& entry = ws.m_entry; + // Remove conflicting transactions from the mempool + for (CTxMemPool::txiter it : ws.m_all_conflicting) + { + LogPrint(BCLog::MEMPOOL, "replacing tx %s (wtxid=%s) with %s (wtxid=%s) for %s additional fees, %d delta bytes\n", + it->GetTx().GetHash().ToString(), + it->GetTx().GetWitnessHash().ToString(), + hash.ToString(), + tx.GetWitnessHash().ToString(), + FormatMoney(ws.m_modified_fees - ws.m_conflicting_fees), + (int)entry->GetTxSize() - (int)ws.m_conflicting_size); + TRACE7(mempool, replaced, + it->GetTx().GetHash().data(), + it->GetTxSize(), + it->GetFee(), + std::chrono::duration_cast>(it->GetTime()).count(), + hash.data(), + entry->GetTxSize(), + entry->GetFee() + ); + ws.m_replaced_transactions.push_back(it->GetSharedTx()); + } + m_pool.RemoveStaged(ws.m_all_conflicting, false, MemPoolRemovalReason::REPLACED); + // This transaction should only count for fee estimation if: // - it's not being re-added during a reorg which bypasses typical mempool fee limits // - the node is not behind diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py index 53bf2e4f8a58..0c48e8b68f88 100755 --- a/test/functional/p2p_permissions.py +++ b/test/functional/p2p_permissions.py @@ -112,7 +112,7 @@ def in_mempool(): self.bump_mocktime(1) return txid in self.nodes[0].getrawmempool() - with self.nodes[1].assert_debug_log(["Force relaying tx {} from peer=0".format(txid)]): + with self.nodes[1].assert_debug_log(["Force relaying tx {} (wtxid={}) from peer=0".format(txid, tx.getwtxid())]): p2p_rebroadcast_wallet.send_txs_and_test([tx], self.nodes[1]) self.wait_until(in_mempool) @@ -125,14 +125,14 @@ def in_mempool(): [tx], self.nodes[1], success=False, - reject_reason='{} from peer=0 was not accepted: txn-mempool-conflict'.format(txid) + reject_reason='{} (wtxid={}) from peer=0 was not accepted: txn-mempool-conflict'.format(txid, tx.getwtxid()) ) p2p_rebroadcast_wallet.send_txs_and_test( [tx], self.nodes[1], success=False, - reject_reason='Not relaying non-mempool transaction {} from forcerelay peer=0'.format(txid) + reject_reason='Not relaying non-mempool transaction {} (wtxid={}) from forcerelay peer=0'.format(txid, tx.getwtxid()) ) def checkpermission(self, args, expectedPermissions): From 3b77f7b700ecbac7c913309b80a043a9acc22389 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 3 Aug 2025 01:41:21 -0500 Subject: [PATCH 2/4] validation: Fix incorrect transaction replacement block addition The Bitcoin PR #28364 was only adding logging to existing transaction replacement code. However, Dash doesn't have the transaction replacement infrastructure yet (m_all_conflicting, etc.), so this block was incorrectly added as new functionality instead of modifying existing code. This commit removes the incorrectly added block, keeping only the logging changes that are appropriate for this PR. --- src/validation.cpp | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 60567a87ae03..0df6f5aaa852 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1014,29 +1014,6 @@ bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws) std::unique_ptr& entry = ws.m_entry; - // Remove conflicting transactions from the mempool - for (CTxMemPool::txiter it : ws.m_all_conflicting) - { - LogPrint(BCLog::MEMPOOL, "replacing tx %s (wtxid=%s) with %s (wtxid=%s) for %s additional fees, %d delta bytes\n", - it->GetTx().GetHash().ToString(), - it->GetTx().GetWitnessHash().ToString(), - hash.ToString(), - tx.GetWitnessHash().ToString(), - FormatMoney(ws.m_modified_fees - ws.m_conflicting_fees), - (int)entry->GetTxSize() - (int)ws.m_conflicting_size); - TRACE7(mempool, replaced, - it->GetTx().GetHash().data(), - it->GetTxSize(), - it->GetFee(), - std::chrono::duration_cast>(it->GetTime()).count(), - hash.data(), - entry->GetTxSize(), - entry->GetFee() - ); - ws.m_replaced_transactions.push_back(it->GetSharedTx()); - } - m_pool.RemoveStaged(ws.m_all_conflicting, false, MemPoolRemovalReason::REPLACED); - // This transaction should only count for fee estimation if: // - it's not being re-added during a reorg which bypasses typical mempool fee limits // - the node is not behind From bb48fcaa0202994ddb95e09eb5849d2d2a10296f Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 3 Aug 2025 02:11:16 -0500 Subject: [PATCH 3/4] fix: remove Bitcoin-specific witness code and improve logging - Remove inappropriate wtxid logging throughout codebase - Simplify orphan transaction processing by removing witness-specific logic - Fix txorphanage logging to show correct orphan child hash - Update uint64_t casting for consistency in logging.h - Modernize Python test strings to use f-strings - Update test expectations to match simplified logging Addresses CodeRabbit feedback while preserving Bitcoin intent for Dash --- src/logging.h | 4 +- src/net_processing.cpp | 61 ++++++------------------------ src/txorphanage.cpp | 4 +- test/functional/p2p_permissions.py | 6 +-- 4 files changed, 19 insertions(+), 56 deletions(-) diff --git a/src/logging.h b/src/logging.h index 0d48b132a948..c1fbb3694929 100644 --- a/src/logging.h +++ b/src/logging.h @@ -67,8 +67,8 @@ namespace BCLog { #endif BLOCKSTORE = (1 << 26), TXRECONCILIATION = (1 << 27), - SCAN = (1 << 28), - TXPACKAGES = (1 << 29), + SCAN = (uint64_t)1 << 28, + TXPACKAGES = (uint64_t)1 << 29, //Start Dash CHAINLOCKS = ((uint64_t)1 << 32), diff --git a/src/net_processing.cpp b/src/net_processing.cpp index aca1583d56e3..8aef35018636 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3219,10 +3219,9 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx); const TxValidationState& state = result.m_state; - const uint256& orphan_wtxid = porphanTx->GetWitnessHash(); if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { - LogPrint(BCLog::TXPACKAGES, " accepted orphan tx %s (wtxid=%s)\n", orphanHash.ToString(), orphan_wtxid.ToString()); + LogPrint(BCLog::TXPACKAGES, " accepted orphan tx %s\n", orphanHash.ToString()); LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); _RelayTransaction(porphanTx->GetHash()); m_orphanage.AddChildrenToWorkSet(*porphanTx, orphan_work_set); @@ -3230,14 +3229,12 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) break; } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { if (state.IsInvalid()) { - LogPrint(BCLog::TXPACKAGES, " invalid orphan tx %s (wtxid=%s) from peer=%d. %s\n", + LogPrint(BCLog::TXPACKAGES, " invalid orphan tx %s from peer=%d. %s\n", orphanHash.ToString(), - orphan_wtxid.ToString(), from_peer, state.ToString()); - LogPrint(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n", + LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n", orphanHash.ToString(), - orphan_wtxid.ToString(), from_peer, state.ToString()); // Maybe punish peer that gave us an invalid orphan tx @@ -3245,39 +3242,9 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) } // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee - LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", orphanHash.ToString(), orphan_wtxid.ToString()); + LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s\n", orphanHash.ToString()); LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString()); m_recent_rejects.insert(orphanHash); - if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) { - // We can add the wtxid of this transaction to our reject filter. - // Do not add txids of witness transactions or witness-stripped - // transactions to the filter, as they can have been malleated; - // adding such txids to the reject filter would potentially - // interfere with relay of valid transactions from peers that - // do not support wtxid-based relay. See - // https://github.com/bitcoin/bitcoin/issues/8279 for details. - // We can remove this restriction (and always add wtxids to - // the filter even for witness stripped transactions) once - // wtxid-based relay is broadly deployed. - // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034 - // for concerns around weakening security of unupgraded nodes - // if we start doing this too early. - m_recent_rejects.insert(porphanTx->GetWitnessHash()); - // If the transaction failed for TX_INPUTS_NOT_STANDARD, - // then we know that the witness was irrelevant to the policy - // failure, since this check depends only on the txid - // (the scriptPubKey being spent is covered by the txid). - // Add the txid to the reject filter to prevent repeated - // processing of this transaction in the event that child - // transactions are later received (resulting in - // parent-fetching by txid via the orphan-handling logic). - if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && porphanTx->GetWitnessHash() != porphanTx->GetHash()) { - // We only add the txid if it differs from the wtxid, to - // avoid wasting entries in the rolling bloom filter. - m_recent_rejects.insert(porphanTx->GetHash()); - } - } ->>>>>>> 5666966dff (Merge bitcoin/bitcoin#28364: log: log wtxids when possible, add TXPACKAGES category) m_orphanage.EraseTx(orphanHash); break; } @@ -4517,13 +4484,12 @@ void PeerManagerImpl::ProcessMessage( // allowing the node to function as a gateway for // nodes hidden behind it. if (!m_mempool.exists(tx.GetHash())) { - LogPrintf("Not relaying non-mempool transaction %s (wtxid=%s) from forcerelay peer=%d\n", - tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), pfrom.GetId()); + LogPrintf("Not relaying non-mempool transaction %s from forcerelay peer=%d\n", + tx.GetHash().ToString(), pfrom.GetId()); } else { - LogPrintf("Force relaying tx %s (wtxid=%s) from peer=%d\n", - tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), pfrom.GetId()); + LogPrintf("Force relaying tx %s from peer=%d\n", + tx.GetHash().ToString(), pfrom.GetId()); _RelayTransaction(tx.GetHash()); - } } // If a tx is detected by m_recent_rejects it is ignored. Because we haven't @@ -4560,10 +4526,9 @@ void PeerManagerImpl::ProcessMessage( pfrom.m_last_tx_time = GetTime(); - LogPrint(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (wtxid=%s) (poolsz %u txn, %u kB)\n", + LogPrint(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (poolsz %u txn, %u kB)\n", pfrom.GetId(), tx.GetHash().ToString(), - tx.GetWitnessHash().ToString(), m_mempool.size(), m_mempool.DynamicMemoryUsage() / 1000); // Recursively process any orphan transactions that depended on this one @@ -4613,9 +4578,8 @@ void PeerManagerImpl::ProcessMessage( LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted); } } else { - LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", - tx.GetHash().ToString(), - tx.GetWitnessHash().ToString()); + LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s\n", + tx.GetHash().ToString()); // We will continue to reject this tx since it has rejected // parents so avoid re-requesting it from other peers. m_recent_rejects.insert(tx.GetHash()); @@ -4646,9 +4610,8 @@ void PeerManagerImpl::ProcessMessage( // regardless of false positives. if (state.IsInvalid()) { - LogPrint(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n", + LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n", tx.GetHash().ToString(), - tx.GetWitnessHash().ToString(), pfrom.GetId(), state.ToString()); MaybePunishNodeForTx(pfrom.GetId(), state); diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index d6b0695914fc..a4742a965db8 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -157,8 +157,8 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx, std::set if (it_by_prev != m_outpoint_to_orphan_it.end()) { for (const auto& elem : it_by_prev->second) { orphan_work_set.insert(elem->first); - LogPrint(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n", - tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), elem->second.fromPeer); + LogPrint(BCLog::TXPACKAGES, "added %s to peer %d workset\n", + elem->first.ToString(), elem->second.fromPeer); } } } diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py index 0c48e8b68f88..8a2c3ec1eff2 100755 --- a/test/functional/p2p_permissions.py +++ b/test/functional/p2p_permissions.py @@ -112,7 +112,7 @@ def in_mempool(): self.bump_mocktime(1) return txid in self.nodes[0].getrawmempool() - with self.nodes[1].assert_debug_log(["Force relaying tx {} (wtxid={}) from peer=0".format(txid, tx.getwtxid())]): + with self.nodes[1].assert_debug_log([f"Force relaying tx {txid} from peer=0"]): p2p_rebroadcast_wallet.send_txs_and_test([tx], self.nodes[1]) self.wait_until(in_mempool) @@ -125,14 +125,14 @@ def in_mempool(): [tx], self.nodes[1], success=False, - reject_reason='{} (wtxid={}) from peer=0 was not accepted: txn-mempool-conflict'.format(txid, tx.getwtxid()) + reject_reason=f"{txid} from peer=0 was not accepted: txn-mempool-conflict", ) p2p_rebroadcast_wallet.send_txs_and_test( [tx], self.nodes[1], success=False, - reject_reason='Not relaying non-mempool transaction {} (wtxid={}) from forcerelay peer=0'.format(txid, tx.getwtxid()) + reject_reason=f"Not relaying non-mempool transaction {txid} from forcerelay peer=0", ) def checkpermission(self, args, expectedPermissions): From d37d868897f5cadc199e04a4287a22118b72d822 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 3 Aug 2025 02:37:41 -0500 Subject: [PATCH 4/4] fix: remove redundant witness hash logging from orphan transactions In Dash, witness transaction IDs (wtxid) are identical to transaction IDs (txid), making the additional wtxid logging redundant and confusing. This commit removes all GetWitnessHash() references from orphan transaction logging to address CodeRabbit feedback and align with Dash's non-segwit architecture. Changes: - Remove wtxid variable declarations in TxOrphanage::AddTx and EraseTx - Simplify logging statements to only include txid - Address Bitcoin-specific witness code issues identified by review Resolves validation issues while preserving Bitcoin logging intent for Dash. --- src/txorphanage.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index a4742a965db8..85faeed55077 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -23,7 +23,6 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) AssertLockHeld(g_cs_orphans); const uint256& hash = tx->GetHash(); - const uint256& wtxid = tx->GetWitnessHash(); if (m_orphans.count(hash)) return false; @@ -37,7 +36,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) unsigned int sz = GetSerializeSize(*tx, CTransaction::CURRENT_VERSION); if (sz > MAX_STANDARD_TX_SIZE) { - LogPrint(BCLog::TXPACKAGES, "ignoring large orphan tx (size: %u, txid: %s, wtxid: %s)\n", sz, hash.ToString(), wtxid.ToString()); + LogPrint(BCLog::TXPACKAGES, "ignoring large orphan tx (size: %u, txid: %s)\n", sz, hash.ToString()); return false; } @@ -50,7 +49,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) m_orphan_tx_size += sz; - LogPrint(BCLog::TXPACKAGES, "stored orphan tx %s (wtxid=%s) (mapsz %u outsz %u)\n", hash.ToString(), wtxid.ToString(), + LogPrint(BCLog::TXPACKAGES, "stored orphan tx %s (mapsz %u outsz %u)\n", hash.ToString(), m_orphans.size(), m_outpoint_to_orphan_it.size()); ::g_stats_client->inc("transactions.orphans.add", 1.0f); ::g_stats_client->gauge("transactions.orphans", m_orphans.size()); @@ -83,8 +82,7 @@ int TxOrphanage::EraseTx(const uint256& txid) m_orphan_list[old_pos] = it_last; it_last->second.list_pos = old_pos; } - const auto& wtxid = it->second.tx->GetWitnessHash(); - LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", txid.ToString(), wtxid.ToString()); + LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s\n", txid.ToString()); m_orphan_list.pop_back(); assert(m_orphan_tx_size >= it->second.nTxSize);