diff options
| -rw-r--r-- | src/net_processing.cpp | 22 | ||||
| -rw-r--r-- | src/test/fuzz/txorphan.cpp | 15 | ||||
| -rw-r--r-- | src/test/orphanage_tests.cpp | 57 | ||||
| -rw-r--r-- | src/txorphanage.cpp | 65 | ||||
| -rw-r--r-- | src/txorphanage.h | 22 | ||||
| -rwxr-xr-x | test/functional/p2p_invalid_tx.py | 6 | ||||
| -rwxr-xr-x | test/functional/p2p_orphan_handling.py | 174 |
7 files changed, 296 insertions, 65 deletions
diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 749ddf740f..6374cb52c1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2293,7 +2293,23 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside const uint256& hash = gtxid.GetHash(); - if (m_orphanage.HaveTx(gtxid)) return true; + if (gtxid.IsWtxid()) { + // Normal query by wtxid. + if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; + } else { + // Never query by txid: it is possible that the transaction in the orphanage has the same + // txid but a different witness, which would give us a false positive result. If we decided + // not to request the transaction based on this result, an attacker could prevent us from + // downloading a transaction by intentionally creating a malleated version of it. While + // only one (or none!) of these transactions can ultimately be confirmed, we have no way of + // discerning which one that is, so the orphanage can store multiple transactions with the + // same txid. + // + // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid. + // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will + // help us find non-segwit transactions, saving bandwidth, and should have no false positives. + if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; + } if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true; @@ -3239,7 +3255,7 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& 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) { + if (Assume(state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) && m_orphanage.EraseTx(ptx->GetWitnessHash()) > 0) { LogDebug(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString()); } } @@ -3257,7 +3273,7 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c m_orphanage.AddChildrenToWorkSet(*tx); // If it came from the orphanage, remove it. No-op if the tx is not in txorphanage. - m_orphanage.EraseTx(tx->GetHash()); + m_orphanage.EraseTx(tx->GetWitnessHash()); LogDebug(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (wtxid=%s) (poolsz %u txn, %u kB)\n", nodeid, diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 037c7af77f..7bebb987b1 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -104,13 +104,12 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) { CTransactionRef ref = orphanage.GetTxToReconsider(peer_id); if (ref) { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetWitnessHash())); - Assert(have_tx); + Assert(orphanage.HaveTx(ref->GetWitnessHash())); } } }, [&] { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); + bool have_tx = orphanage.HaveTx(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 { @@ -118,7 +117,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) // have_tx == true -> add_tx == false Assert(!have_tx || !add_tx); } - have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); + have_tx = orphanage.HaveTx(tx->GetWitnessHash()); { bool add_tx = orphanage.AddTx(tx, peer_id); // if have_tx is still false, it must be too big @@ -127,15 +126,15 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) } }, [&] { - bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); + bool have_tx = orphanage.HaveTx(tx->GetWitnessHash()); // EraseTx should return 0 if m_orphans doesn't have the tx { - Assert(have_tx == orphanage.EraseTx(tx->GetHash())); + Assert(have_tx == orphanage.EraseTx(tx->GetWitnessHash())); } - have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash())); + have_tx = orphanage.HaveTx(tx->GetWitnessHash()); // have_tx should be false and EraseTx should fail { - Assert(!have_tx && !orphanage.EraseTx(tx->GetHash())); + Assert(!have_tx && !orphanage.EraseTx(tx->GetWitnessHash())); } }, [&] { diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index b2643cf678..450bf6a4fc 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -30,8 +30,8 @@ public: CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { LOCK(m_mutex); - std::map<Txid, OrphanTx>::iterator it; - it = m_orphans.lower_bound(Txid::FromUint256(InsecureRand256())); + std::map<Wtxid, OrphanTx>::iterator it; + it = m_orphans.lower_bound(Wtxid::FromUint256(InsecureRand256())); if (it == m_orphans.end()) it = m_orphans.begin(); return it->second.tx; @@ -70,6 +70,16 @@ static CTransactionRef MakeTransactionSpending(const std::vector<COutPoint>& out return MakeTransactionRef(tx); } +// Make another (not necessarily valid) tx with the same txid but different wtxid. +static CTransactionRef MakeMutation(const CTransactionRef& ptx) +{ + CMutableTransaction tx(*ptx); + tx.vin[0].scriptWitness.stack.push_back({5}); + auto mutated_tx = MakeTransactionRef(tx); + assert(ptx->GetHash() == mutated_tx->GetHash()); + return mutated_tx; +} + static bool EqualTxns(const std::set<CTransactionRef>& set_txns, const std::vector<CTransactionRef>& vec_txns) { if (vec_txns.size() != set_txns.size()) return false; @@ -180,6 +190,49 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) BOOST_CHECK(orphanage.CountOrphans() == 0); } +BOOST_AUTO_TEST_CASE(same_txid_diff_witness) +{ + FastRandomContext det_rand{true}; + TxOrphanage orphanage; + NodeId peer{0}; + + std::vector<COutPoint> empty_outpoints; + auto parent = MakeTransactionSpending(empty_outpoints, det_rand); + + // Create children to go into orphanage. + auto child_normal = MakeTransactionSpending({{parent->GetHash(), 0}}, det_rand); + auto child_mutated = MakeMutation(child_normal); + + const auto& normal_wtxid = child_normal->GetWitnessHash(); + const auto& mutated_wtxid = child_mutated->GetWitnessHash(); + BOOST_CHECK(normal_wtxid != mutated_wtxid); + + BOOST_CHECK(orphanage.AddTx(child_normal, peer)); + // EraseTx fails as transaction by this wtxid doesn't exist. + BOOST_CHECK_EQUAL(orphanage.EraseTx(mutated_wtxid), 0); + BOOST_CHECK(orphanage.HaveTx(normal_wtxid)); + BOOST_CHECK(!orphanage.HaveTx(mutated_wtxid)); + + // Must succeed. Both transactions should be present in orphanage. + BOOST_CHECK(orphanage.AddTx(child_mutated, peer)); + BOOST_CHECK(orphanage.HaveTx(normal_wtxid)); + BOOST_CHECK(orphanage.HaveTx(mutated_wtxid)); + + // Outpoints map should track all entries: check that both are returned as children of the parent. + std::set<CTransactionRef> expected_children{child_normal, child_mutated}; + BOOST_CHECK(EqualTxns(expected_children, orphanage.GetChildrenFromSamePeer(parent, peer))); + + // Erase by wtxid: mutated first + BOOST_CHECK_EQUAL(orphanage.EraseTx(mutated_wtxid), 1); + BOOST_CHECK(orphanage.HaveTx(normal_wtxid)); + BOOST_CHECK(!orphanage.HaveTx(mutated_wtxid)); + + BOOST_CHECK_EQUAL(orphanage.EraseTx(normal_wtxid), 1); + BOOST_CHECK(!orphanage.HaveTx(normal_wtxid)); + BOOST_CHECK(!orphanage.HaveTx(mutated_wtxid)); +} + + BOOST_AUTO_TEST_CASE(get_children) { FastRandomContext det_rand{true}; diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 92055ded72..13ef96f3be 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -23,7 +23,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) const Txid& hash = tx->GetHash(); const Wtxid& wtxid = tx->GetWitnessHash(); - if (m_orphans.count(hash)) + if (m_orphans.count(wtxid)) return false; // Ignore big transactions, to avoid a @@ -40,30 +40,28 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) return false; } - auto ret = m_orphans.emplace(hash, OrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size()}); + auto ret = m_orphans.emplace(wtxid, OrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size()}); assert(ret.second); m_orphan_list.push_back(ret.first); - // Allow for lookups in the orphan pool by wtxid, as well as txid - m_wtxid_to_orphan_it.emplace(tx->GetWitnessHash(), ret.first); for (const CTxIn& txin : tx->vin) { m_outpoint_to_orphan_it[txin.prevout].insert(ret.first); } - 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 (wtxid=%s), weight: %u (mapsz %u outsz %u)\n", hash.ToString(), wtxid.ToString(), sz, m_orphans.size(), m_outpoint_to_orphan_it.size()); return true; } -int TxOrphanage::EraseTx(const Txid& txid) +int TxOrphanage::EraseTx(const Wtxid& wtxid) { LOCK(m_mutex); - return EraseTxNoLock(txid); + return EraseTxNoLock(wtxid); } -int TxOrphanage::EraseTxNoLock(const Txid& txid) +int TxOrphanage::EraseTxNoLock(const Wtxid& wtxid) { AssertLockHeld(m_mutex); - std::map<Txid, OrphanTx>::iterator it = m_orphans.find(txid); + std::map<Wtxid, OrphanTx>::iterator it = m_orphans.find(wtxid); if (it == m_orphans.end()) return 0; for (const CTxIn& txin : it->second.tx->vin) @@ -85,10 +83,12 @@ int TxOrphanage::EraseTxNoLock(const Txid& 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()); + const auto& txid = it->second.tx->GetHash(); + // Time spent in orphanage = difference between current and entry time. + // Entry time is equal to ORPHAN_TX_EXPIRE_TIME earlier than entry's expiry. + LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s) after %ds\n", txid.ToString(), wtxid.ToString(), + GetTime() + ORPHAN_TX_EXPIRE_TIME - it->second.nTimeExpire); m_orphan_list.pop_back(); - m_wtxid_to_orphan_it.erase(it->second.tx->GetWitnessHash()); m_orphans.erase(it); return 1; @@ -101,16 +101,16 @@ void TxOrphanage::EraseForPeer(NodeId peer) m_peer_work_set.erase(peer); int nErased = 0; - std::map<Txid, OrphanTx>::iterator iter = m_orphans.begin(); + std::map<Wtxid, OrphanTx>::iterator iter = m_orphans.begin(); while (iter != m_orphans.end()) { - std::map<Txid, OrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid - if (maybeErase->second.fromPeer == peer) - { - nErased += EraseTxNoLock(maybeErase->second.tx->GetHash()); + // increment to avoid iterator becoming invalid after erasure + const auto& [wtxid, orphan] = *iter++; + if (orphan.fromPeer == peer) { + nErased += EraseTxNoLock(wtxid); } } - if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer); + if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) from peer=%d\n", nErased, peer); } void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) @@ -124,12 +124,12 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) // Sweep out expired orphan pool entries: int nErased = 0; int64_t nMinExpTime = nNow + ORPHAN_TX_EXPIRE_TIME - ORPHAN_TX_EXPIRE_INTERVAL; - std::map<Txid, OrphanTx>::iterator iter = m_orphans.begin(); + std::map<Wtxid, OrphanTx>::iterator iter = m_orphans.begin(); while (iter != m_orphans.end()) { - std::map<Txid, OrphanTx>::iterator maybeErase = iter++; + std::map<Wtxid, OrphanTx>::iterator maybeErase = iter++; if (maybeErase->second.nTimeExpire <= nNow) { - nErased += EraseTxNoLock(maybeErase->second.tx->GetHash()); + nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash()); } else { nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime); } @@ -142,7 +142,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) { // Evict a random orphan: size_t randompos = rng.randrange(m_orphan_list.size()); - EraseTxNoLock(m_orphan_list[randompos]->first); + EraseTxNoLock(m_orphan_list[randompos]->second.tx->GetWitnessHash()); ++nEvicted; } if (nEvicted > 0) LogPrint(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted); @@ -159,7 +159,7 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) for (const auto& elem : it_by_prev->second) { // Get this source peer's work set, emplacing an empty set if it didn't exist // (note: if this peer wasn't still connected, we would have removed the orphan tx already) - std::set<Txid>& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second; + std::set<Wtxid>& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second; // Add this tx to the work set orphan_work_set.insert(elem->first); LogPrint(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n", @@ -169,14 +169,10 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) } } -bool TxOrphanage::HaveTx(const GenTxid& gtxid) const +bool TxOrphanage::HaveTx(const Wtxid& wtxid) const { LOCK(m_mutex); - if (gtxid.IsWtxid()) { - return m_wtxid_to_orphan_it.count(Wtxid::FromUint256(gtxid.GetHash())); - } else { - return m_orphans.count(Txid::FromUint256(gtxid.GetHash())); - } + return m_orphans.count(wtxid); } CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) @@ -187,10 +183,10 @@ CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) if (work_set_it != m_peer_work_set.end()) { auto& work_set = work_set_it->second; while (!work_set.empty()) { - Txid txid = *work_set.begin(); + Wtxid wtxid = *work_set.begin(); work_set.erase(work_set.begin()); - const auto orphan_it = m_orphans.find(txid); + const auto orphan_it = m_orphans.find(wtxid); if (orphan_it != m_orphans.end()) { return orphan_it->second.tx; } @@ -215,7 +211,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block) { LOCK(m_mutex); - std::vector<Txid> vOrphanErase; + std::vector<Wtxid> vOrphanErase; for (const CTransactionRef& ptx : block.vtx) { const CTransaction& tx = *ptx; @@ -226,8 +222,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block) if (itByPrev == m_outpoint_to_orphan_it.end()) continue; for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) { const CTransaction& orphanTx = *(*mi)->second.tx; - const auto& orphanHash = orphanTx.GetHash(); - vOrphanErase.push_back(orphanHash); + vOrphanErase.push_back(orphanTx.GetWitnessHash()); } } } @@ -238,7 +233,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block) for (const auto& orphanHash : vOrphanErase) { nErased += EraseTxNoLock(orphanHash); } - LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx included or conflicted by block\n", nErased); + LogPrint(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) included or conflicted by block\n", nErased); } } diff --git a/src/txorphanage.h b/src/txorphanage.h index a3c8edaa2a..5f9888c969 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -23,8 +23,8 @@ public: /** Add a new orphan transaction */ bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); - /** Check if we already have an orphan transaction (by txid or wtxid) */ - bool HaveTx(const GenTxid& gtxid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** Check if we already have an orphan transaction (by wtxid only) */ + bool HaveTx(const Wtxid& wtxid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Extract a transaction from a peer's work set * Returns nullptr if there are no transactions to work on. @@ -33,8 +33,8 @@ public: */ CTransactionRef GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); - /** Erase an orphan by txid */ - int EraseTx(const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** Erase an orphan by wtxid */ + int EraseTx(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Erase all orphans announced by a peer (eg, after that peer disconnects) */ void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); @@ -77,12 +77,12 @@ protected: size_t list_pos; }; - /** Map from txid to orphan transaction record. Limited by + /** Map from wtxid to orphan transaction record. Limited by * -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */ - std::map<Txid, OrphanTx> m_orphans GUARDED_BY(m_mutex); + std::map<Wtxid, OrphanTx> m_orphans GUARDED_BY(m_mutex); /** Which peer provided the orphans that need to be reconsidered */ - std::map<NodeId, std::set<Txid>> m_peer_work_set GUARDED_BY(m_mutex); + std::map<NodeId, std::set<Wtxid>> m_peer_work_set GUARDED_BY(m_mutex); using OrphanMap = decltype(m_orphans); @@ -102,12 +102,8 @@ protected: /** Orphan transactions in vector for quick random eviction */ std::vector<OrphanMap::iterator> m_orphan_list GUARDED_BY(m_mutex); - /** Index from wtxid into the m_orphans to lookup orphan - * transactions using their witness ids. */ - std::map<Wtxid, OrphanMap::iterator> m_wtxid_to_orphan_it GUARDED_BY(m_mutex); - - /** Erase an orphan by txid */ - int EraseTxNoLock(const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex); + /** Erase an orphan by wtxid */ + int EraseTxNoLock(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex); }; #endif // BITCOIN_TXORPHANAGE_H diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py index ae9dc816ab..0ae05d4b0b 100755 --- a/test/functional/p2p_invalid_tx.py +++ b/test/functional/p2p_invalid_tx.py @@ -165,7 +165,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): node.p2ps[0].send_txs_and_test([rejected_parent], node, success=False) self.log.info('Test that a peer disconnection causes erase its transactions from the orphan pool') - with node.assert_debug_log(['Erased 100 orphan tx from peer=25']): + with node.assert_debug_log(['Erased 100 orphan transaction(s) from peer=25']): self.reconnect_p2p(num_connections=1) self.log.info('Test that a transaction in the orphan pool is included in a new tip block causes erase this transaction from the orphan pool') @@ -190,7 +190,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): block_A.solve() self.log.info('Send the block that includes the previous orphan ... ') - with node.assert_debug_log(["Erased 1 orphan tx included or conflicted by block"]): + with node.assert_debug_log(["Erased 1 orphan transaction(s) included or conflicted by block"]): node.p2ps[0].send_blocks_and_test([block_A], node, success=True) self.log.info('Test that a transaction in the orphan pool conflicts with a new tip block causes erase this transaction from the orphan pool') @@ -219,7 +219,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): block_B.solve() self.log.info('Send the block that includes a transaction which conflicts with the previous orphan ... ') - with node.assert_debug_log(["Erased 1 orphan tx included or conflicted by block"]): + with node.assert_debug_log(["Erased 1 orphan transaction(s) included or conflicted by block"]): node.p2ps[0].send_blocks_and_test([block_B], node, success=True) diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index 6166c62aa2..f525d87cca 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -7,6 +7,7 @@ import time from test_framework.messages import ( CInv, + CTxInWitness, MSG_TX, MSG_WITNESS_TX, MSG_WTX, @@ -21,6 +22,7 @@ from test_framework.p2p import ( NONPREF_PEER_TX_DELAY, OVERLOADED_PEER_TX_DELAY, p2p_lock, + P2PInterface, P2PTxInvStore, TXID_RELAY_DELAY, ) @@ -127,6 +129,22 @@ class OrphanHandlingTest(BitcoinTestFramework): peer.wait_for_getdata([wtxid]) peer.send_and_ping(msg_tx(tx)) + def create_malleated_version(self, tx): + """ + Create a malleated version of the tx where the witness is replaced with garbage data. + Returns a CTransaction object. + """ + tx_bad_wit = tx_from_hex(tx["hex"]) + tx_bad_wit.wit.vtxinwit = [CTxInWitness()] + # Add garbage data to witness 0. We cannot simply strip the witness, as the node would + # classify it as a transaction in which the witness was missing rather than wrong. + tx_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage'] + + assert_equal(tx["txid"], tx_bad_wit.rehash()) + assert tx["wtxid"] != tx_bad_wit.getwtxid() + + return tx_bad_wit + @cleanup def test_arrival_timing_orphan(self): self.log.info("Test missing parents that arrive during delay are not requested") @@ -284,8 +302,8 @@ class OrphanHandlingTest(BitcoinTestFramework): # doesn't give up on the orphan. Once all of the missing parents are received, it should be # submitted to mempool. peer.send_message(msg_notfound(vec=[CInv(MSG_WITNESS_TX, int(txid_conf_old, 16))])) + # Sync with ping to ensure orphans are reconsidered peer.send_and_ping(msg_tx(missing_tx["tx"])) - peer.sync_with_ping() assert_equal(node.getmempoolentry(orphan["txid"])["ancestorcount"], 3) @cleanup @@ -394,10 +412,161 @@ class OrphanHandlingTest(BitcoinTestFramework): peer2.assert_never_requested(child["tx"].getwtxid()) # The child should never be requested, even if announced again with potentially different witness. + # Sync with ping to ensure orphans are reconsidered peer3.send_and_ping(msg_inv([CInv(t=MSG_TX, h=int(child["txid"], 16))])) self.nodes[0].bumpmocktime(TXREQUEST_TIME_SKIP) peer3.assert_never_requested(child["txid"]) + @cleanup + def test_same_txid_orphan(self): + self.log.info("Check what happens when orphan with same txid is already in orphanage") + node = self.nodes[0] + + tx_parent = self.wallet.create_self_transfer() + + # Create the real child + tx_child = self.wallet.create_self_transfer(utxo_to_spend=tx_parent["new_utxo"]) + + # Create a fake version of the child + tx_orphan_bad_wit = self.create_malleated_version(tx_child) + + bad_peer = node.add_p2p_connection(P2PInterface()) + honest_peer = node.add_p2p_connection(P2PInterface()) + + # 1. Fake orphan is received first. It is missing an input. + bad_peer.send_and_ping(msg_tx(tx_orphan_bad_wit)) + + # 2. Node requests the missing parent by txid. + parent_txid_int = int(tx_parent["txid"], 16) + node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + bad_peer.wait_for_getdata([parent_txid_int]) + + # 3. Honest peer relays the real child, which is also missing parents and should be placed + # in the orphanage. + with node.assert_debug_log(["missingorspent", "stored orphan tx"]): + honest_peer.send_and_ping(msg_tx(tx_child["tx"])) + + # Time out the previous request for the parent (node will not request the same transaction + # from multiple nodes at the same time) + node.bumpmocktime(GETDATA_TX_INTERVAL) + + # 4. The parent is requested. Honest peer sends it. + honest_peer.wait_for_getdata([parent_txid_int]) + # Sync with ping to ensure orphans are reconsidered + honest_peer.send_and_ping(msg_tx(tx_parent["tx"])) + + # 5. After parent is accepted, orphans should be reconsidered. + # The real child should be accepted and the fake one rejected. + node_mempool = node.getrawmempool() + assert tx_parent["txid"] in node_mempool + assert tx_child["txid"] in node_mempool + assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"]) + + @cleanup + def test_same_txid_orphan_of_orphan(self): + self.log.info("Check what happens when orphan's parent with same txid is already in orphanage") + node = self.nodes[0] + + tx_grandparent = self.wallet.create_self_transfer() + + # Create middle tx (both parent and child) which will be in orphanage. + tx_middle = self.wallet.create_self_transfer(utxo_to_spend=tx_grandparent["new_utxo"]) + + # Create a fake version of the middle tx + tx_orphan_bad_wit = self.create_malleated_version(tx_middle) + + # Create grandchild spending from tx_middle (and spending from tx_orphan_bad_wit since they + # have the same txid). + tx_grandchild = self.wallet.create_self_transfer(utxo_to_spend=tx_middle["new_utxo"]) + + bad_peer = node.add_p2p_connection(P2PInterface()) + honest_peer = node.add_p2p_connection(P2PInterface()) + + # 1. Fake orphan is received first. It is missing an input. + bad_peer.send_and_ping(msg_tx(tx_orphan_bad_wit)) + + # 2. Node requests missing tx_grandparent by txid. + grandparent_txid_int = int(tx_grandparent["txid"], 16) + node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + bad_peer.wait_for_getdata([grandparent_txid_int]) + + # 3. Honest peer relays the grandchild, which is missing a parent. The parent by txid already + # exists in orphanage, but should be re-requested because the node shouldn't assume that the + # witness data is the same. In this case, a same-txid-different-witness transaction exists! + with node.assert_debug_log(["stored orphan tx"]): + honest_peer.send_and_ping(msg_tx(tx_grandchild["tx"])) + middle_txid_int = int(tx_middle["txid"], 16) + node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + honest_peer.wait_for_getdata([middle_txid_int]) + + # 4. Honest peer relays the real child, which is also missing parents and should be placed + # in the orphanage. + with node.assert_debug_log(["stored orphan tx"]): + honest_peer.send_and_ping(msg_tx(tx_middle["tx"])) + assert_equal(len(node.getrawmempool()), 0) + + # 5. Honest peer sends tx_grandparent + honest_peer.send_and_ping(msg_tx(tx_grandparent["tx"])) + + # 6. After parent is accepted, orphans should be reconsidered. + # The real child should be accepted and the fake one rejected. + node_mempool = node.getrawmempool() + assert tx_grandparent["txid"] in node_mempool + assert tx_middle["txid"] in node_mempool + assert tx_grandchild["txid"] in node_mempool + assert_equal(node.getmempoolentry(tx_middle["txid"])["wtxid"], tx_middle["wtxid"]) + + @cleanup + def test_orphan_txid_inv(self): + self.log.info("Check node does not ignore announcement with same txid as tx in orphanage") + node = self.nodes[0] + + tx_parent = self.wallet.create_self_transfer() + + # Create the real child and fake version + tx_child = self.wallet.create_self_transfer(utxo_to_spend=tx_parent["new_utxo"]) + tx_orphan_bad_wit = self.create_malleated_version(tx_child) + + bad_peer = node.add_p2p_connection(PeerTxRelayer()) + # Must not send wtxidrelay because otherwise the inv(TX) will be ignored later + honest_peer = node.add_p2p_connection(P2PInterface(wtxidrelay=False)) + + # 1. Fake orphan is received first. It is missing an input. + bad_peer.send_and_ping(msg_tx(tx_orphan_bad_wit)) + + # 2. Node requests the missing parent by txid. + parent_txid_int = int(tx_parent["txid"], 16) + node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + bad_peer.wait_for_getdata([parent_txid_int]) + + # 3. Honest peer announces the real child, by txid (this isn't common but the node should + # still keep track of it). + child_txid_int = int(tx_child["txid"], 16) + honest_peer.send_and_ping(msg_inv([CInv(t=MSG_TX, h=child_txid_int)])) + + # 4. The child is requested. Honest peer sends it. + node.bumpmocktime(TXREQUEST_TIME_SKIP) + honest_peer.wait_for_getdata([child_txid_int]) + with node.assert_debug_log(["stored orphan tx"]): + honest_peer.send_and_ping(msg_tx(tx_child["tx"])) + + # 5. After first parent request times out, the node sends another one for the missing parent + # of the real orphan child. + node.bumpmocktime(GETDATA_TX_INTERVAL) + honest_peer.wait_for_getdata([parent_txid_int]) + honest_peer.send_and_ping(msg_tx(tx_parent["tx"])) + + # 6. After parent is accepted, orphans should be reconsidered. + # The real child should be accepted and the fake one rejected. This may happen in either + # order since the message-processing is randomized. If tx_orphan_bad_wit is validated first, + # its consensus error leads to disconnection of bad_peer. If tx_child is validated first, + # tx_orphan_bad_wit is rejected for txn-same-nonwitness-data-in-mempool (no punishment). + node_mempool = node.getrawmempool() + assert tx_parent["txid"] in node_mempool + assert tx_child["txid"] in node_mempool + assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"]) + + def run_test(self): self.nodes[0].setmocktime(int(time.time())) self.wallet_nonsegwit = MiniWallet(self.nodes[0], mode=MiniWalletMode.RAW_P2PK) @@ -410,6 +579,9 @@ class OrphanHandlingTest(BitcoinTestFramework): self.test_orphans_overlapping_parents() self.test_orphan_of_orphan() self.test_orphan_inherit_rejection() + self.test_same_txid_orphan() + self.test_same_txid_orphan_of_orphan() + self.test_orphan_txid_inv() if __name__ == '__main__': |