From fa074d2c7b9c3d34876c428d12672a505d4ce4eb Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 16 Nov 2020 07:51:21 +0100 Subject: Revert "Merge #19606: Backport wtxid relay to v0.20" This reverts commit a339289c2ef9caffa1195436695a13f6e48e1bbc, reversing changes made to b9ac31f2d29ae3e79c0f0cde5bab2d7213e6da51. --- src/consensus/validation.h | 6 +- src/net.h | 4 +- src/net_processing.cpp | 282 +++++---------------- src/net_processing.h | 2 +- src/node/transaction.cpp | 3 +- src/protocol.cpp | 4 - src/protocol.h | 12 +- src/txmempool.cpp | 14 +- src/txmempool.h | 42 +-- src/validation.cpp | 2 +- src/version.h | 5 +- test/functional/mempool_packages.py | 5 - test/functional/p2p_blocksonly.py | 2 +- test/functional/p2p_feefilter.py | 4 +- test/functional/p2p_segwit.py | 99 +------- test/functional/p2p_tx_download.py | 12 +- test/functional/test_framework/messages.py | 25 +- test/functional/test_framework/mininode.py | 5 - test/functional/wallet_resendwallettransactions.py | 17 +- 19 files changed, 101 insertions(+), 444 deletions(-) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index f7d476951b..3a90cd69b3 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -31,15 +31,11 @@ enum class TxValidationResult { TX_MISSING_INPUTS, //!< transaction was missing some of its inputs TX_PREMATURE_SPEND, //!< transaction spends a coinbase too early, or violates locktime/sequence locks /** - * Transaction might have a witness prior to SegWit + * Transaction might be missing a witness, have a witness prior to SegWit * activation, or witness may have been malleated (which includes * non-standard witnesses). */ TX_WITNESS_MUTATED, - /** - * Transaction is missing a witness. - */ - TX_WITNESS_STRIPPED, /** * Tx already in mempool or conflicts with a tx in the chain * (if it conflicts with another tx in mempool, we use MEMPOOL_POLICY as it failed to reach the RBF threshold) diff --git a/src/net.h b/src/net.h index 8822e4e35d..0dbd5e0549 100644 --- a/src/net.h +++ b/src/net.h @@ -969,11 +969,11 @@ public: } - void AddKnownTx(const uint256& hash) + void AddInventoryKnown(const CInv& inv) { if (m_tx_relay != nullptr) { LOCK(m_tx_relay->cs_tx_inventory); - m_tx_relay->filterInventoryKnown.insert(hash); + m_tx_relay->filterInventoryKnown.insert(inv.hash); } } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 79a0ce7559..8572ebb9f7 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -68,8 +68,6 @@ static constexpr int HISTORICAL_BLOCK_AGE = 7 * 24 * 60 * 60; static constexpr int32_t MAX_PEER_TX_IN_FLIGHT = 100; /** Maximum number of announced transactions from a peer */ static constexpr int32_t MAX_PEER_TX_ANNOUNCEMENTS = 2 * MAX_INV_SZ; -/** How many microseconds to delay requesting transactions via txids, if we have wtxid-relaying peers */ -static constexpr std::chrono::microseconds TXID_RELAY_DELAY{std::chrono::seconds{2}}; /** How many microseconds to delay requesting transactions from inbound peers */ static constexpr std::chrono::microseconds INBOUND_PEER_TX_DELAY{std::chrono::seconds{2}}; /** How long to wait (in microseconds) before downloading a transaction from an additional peer */ @@ -93,7 +91,6 @@ struct COrphanTx { }; RecursiveMutex g_cs_orphans; std::map mapOrphanTransactions GUARDED_BY(g_cs_orphans); -std::map::iterator> g_orphans_by_wtxid GUARDED_BY(g_cs_orphans); void EraseOrphansFor(NodeId peer); @@ -145,15 +142,6 @@ namespace { * million to make it highly unlikely for users to have issues with this * filter. * - * We only need to add wtxids to this filter. For non-segwit - * transactions, the txid == wtxid, so this only prevents us from - * re-downloading non-segwit transactions when communicating with - * non-wtxidrelay peers -- which is important for avoiding malleation - * attacks that could otherwise interfere with transaction relay from - * non-wtxidrelay peers. For communicating with wtxidrelay peers, having - * the reject filter store wtxids is exactly what we want to avoid - * redownload of a rejected transaction. - * * Memory used: 1.3 MB */ std::unique_ptr recentRejects GUARDED_BY(cs_main); @@ -185,9 +173,6 @@ namespace { /** Number of peers from which we're downloading blocks. */ int nPeersWithValidatedDownloads GUARDED_BY(cs_main) = 0; - /** Number of peers with wtxid relay. */ - int g_wtxid_relay_peers GUARDED_BY(cs_main) = 0; - /** Number of outbound peers with m_chain_sync.m_protect. */ int g_outbound_peers_with_protect_from_disconnect GUARDED_BY(cs_main) = 0; @@ -376,9 +361,6 @@ struct CNodeState { //! Whether this peer is a manual connection bool m_is_manual_connection; - //! Whether this peer relays txs via wtxid - bool m_wtxid_relay{false}; - CNodeState(CAddress addrIn, std::string addrNameIn, bool is_inbound, bool is_manual) : address(addrIn), name(std::move(addrNameIn)), m_is_inbound(is_inbound), m_is_manual_connection (is_manual) @@ -729,7 +711,7 @@ void UpdateTxRequestTime(const uint256& txid, std::chrono::microseconds request_ } } -std::chrono::microseconds CalculateTxGetDataTime(const uint256& txid, std::chrono::microseconds current_time, bool use_inbound_delay, bool use_txid_delay) EXCLUSIVE_LOCKS_REQUIRED(cs_main) +std::chrono::microseconds CalculateTxGetDataTime(const uint256& txid, std::chrono::microseconds current_time, bool use_inbound_delay) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { std::chrono::microseconds process_time; const auto last_request_time = GetTxRequestTime(txid); @@ -745,9 +727,6 @@ std::chrono::microseconds CalculateTxGetDataTime(const uint256& txid, std::chron // We delay processing announcements from inbound peers if (use_inbound_delay) process_time += INBOUND_PEER_TX_DELAY; - // We delay processing announcements from peers that use txid-relay (instead of wtxid) - if (use_txid_delay) process_time += TXID_RELAY_DELAY; - return process_time; } @@ -765,7 +744,7 @@ void RequestTx(CNodeState* state, const uint256& txid, std::chrono::microseconds // Calculate the time to try requesting this transaction. Use // fPreferredDownload as a proxy for outbound peers. - const auto process_time = CalculateTxGetDataTime(txid, current_time, !state->fPreferredDownload, !state->m_wtxid_relay && g_wtxid_relay_peers > 0); + const auto process_time = CalculateTxGetDataTime(txid, current_time, !state->fPreferredDownload); peer_download_state.m_tx_process_time.emplace(process_time, txid); } @@ -822,8 +801,6 @@ void PeerLogicValidation::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTim assert(nPeersWithValidatedDownloads >= 0); g_outbound_peers_with_protect_from_disconnect -= state->m_chain_sync.m_protect; assert(g_outbound_peers_with_protect_from_disconnect >= 0); - g_wtxid_relay_peers -= state->m_wtxid_relay; - assert(g_wtxid_relay_peers >= 0); mapNodeState.erase(nodeid); @@ -833,7 +810,6 @@ void PeerLogicValidation::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTim assert(nPreferredDownload == 0); assert(nPeersWithValidatedDownloads == 0); assert(g_outbound_peers_with_protect_from_disconnect == 0); - assert(g_wtxid_relay_peers == 0); } LogPrint(BCLog::NET, "Cleared nodestate for peer=%d\n", nodeid); } @@ -892,8 +868,6 @@ bool AddOrphanTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRE auto ret = mapOrphanTransactions.emplace(hash, COrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, g_orphan_list.size()}); assert(ret.second); g_orphan_list.push_back(ret.first); - // Allow for lookups in the orphan pool by wtxid, as well as txid - g_orphans_by_wtxid.emplace(tx->GetWitnessHash(), ret.first); for (const CTxIn& txin : tx->vin) { mapOrphanTransactionsByPrev[txin.prevout].insert(ret.first); } @@ -930,7 +904,6 @@ int static EraseOrphanTx(uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) it_last->second.list_pos = old_pos; } g_orphan_list.pop_back(); - g_orphans_by_wtxid.erase(it->second.tx->GetWitnessHash()); mapOrphanTransactions.erase(it); return 1; @@ -1101,7 +1074,6 @@ static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, case TxValidationResult::TX_MISSING_INPUTS: case TxValidationResult::TX_PREMATURE_SPEND: case TxValidationResult::TX_WITNESS_MUTATED: - case TxValidationResult::TX_WITNESS_STRIPPED: case TxValidationResult::TX_CONFLICT: case TxValidationResult::TX_MEMPOOL_POLICY: break; @@ -1141,15 +1113,14 @@ PeerLogicValidation::PeerLogicValidation(CConnman* connmanIn, BanMan* banman, CS recentRejects.reset(new CRollingBloomFilter(120000, 0.000001)); // Blocks don't typically have more than 4000 transactions, so this should - // be at least six blocks (~1 hr) worth of transactions that we can store, - // inserting both a txid and wtxid for every observed transaction. + // be at least six blocks (~1 hr) worth of transactions that we can store. // If the number of transactions appearing in a block goes up, or if we are // seeing getdata requests more than an hour after initial announcement, we // can increase this number. // The false positive rate of 1/1M should come out to less than 1 // transaction per day that would be inadvertently ignored (which is the // same probability that we have in the reject filter). - g_recent_confirmed_transactions.reset(new CRollingBloomFilter(48000, 0.000001)); + g_recent_confirmed_transactions.reset(new CRollingBloomFilter(24000, 0.000001)); const Consensus::Params& consensusParams = Params().GetConsensus(); // Stale tip checking and peer eviction are on two different timers, but we @@ -1201,9 +1172,6 @@ void PeerLogicValidation::BlockConnected(const std::shared_ptr& pb LOCK(g_cs_recent_confirmed_transactions); for (const auto& ptx : pblock->vtx) { g_recent_confirmed_transactions->insert(ptx->GetHash()); - if (ptx->GetHash() != ptx->GetWitnessHash()) { - g_recent_confirmed_transactions->insert(ptx->GetWitnessHash()); - } } } } @@ -1356,7 +1324,6 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO { case MSG_TX: case MSG_WITNESS_TX: - case MSG_WTX: { assert(recentRejects); if (::ChainActive().Tip()->GetBlockHash() != hashRecentRejectsChainTip) @@ -1371,11 +1338,7 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO { LOCK(g_cs_orphans); - if (inv.type != MSG_WTX && mapOrphanTransactions.count(inv.hash)) { - return true; - } else if (inv.type == MSG_WTX && g_orphans_by_wtxid.count(inv.hash)) { - return true; - } + if (mapOrphanTransactions.count(inv.hash)) return true; } { @@ -1383,8 +1346,8 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO if (g_recent_confirmed_transactions->contains(inv.hash)) return true; } - const bool by_wtxid = (inv.type == MSG_WTX); - return recentRejects->contains(inv.hash) || mempool.exists(inv.hash, by_wtxid); + return recentRejects->contains(inv.hash) || + mempool.exists(inv.hash); } case MSG_BLOCK: case MSG_WITNESS_BLOCK: @@ -1394,17 +1357,12 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO return true; } -void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) +void RelayTransaction(const uint256& txid, const CConnman& connman) { - connman.ForEachNode([&txid, &wtxid](CNode* pnode) + CInv inv(MSG_TX, txid); + connman.ForEachNode([&inv](CNode* pnode) { - AssertLockHeld(cs_main); - CNodeState &state = *State(pnode->GetId()); - if (state.m_wtxid_relay) { - pnode->PushInventory({MSG_TX, wtxid}); // inv type is MSG_TX even for wtxid relay - } else { - pnode->PushInventory({MSG_TX, txid}); - } + pnode->PushInventory(inv); }); } @@ -1619,7 +1577,7 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm // Process as many TX items from the front of the getdata queue as // possible, since they're common and it's efficient to batch process // them. - while (it != pfrom->vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX || it->type == MSG_WTX)) { + while (it != pfrom->vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX)) { if (interruptMsgProc) return; // The send buffer provides backpressure. If there's no space in @@ -1642,7 +1600,7 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *mi->second)); push = true; } else { - auto txinfo = mempool.info(inv.hash, inv.type == MSG_WTX); + auto txinfo = mempool.info(inv.hash); // To protect privacy, do not answer getdata using the mempool when // that TX couldn't have been INVed in reply to a MEMPOOL request, // or when it's too recent to have expired from mapRelay. @@ -1922,7 +1880,7 @@ void static ProcessOrphanTx(CConnman* connman, CTxMemPool& mempool, std::setGetWitnessHash(), *connman); + RelayTransaction(orphanHash, *connman); for (unsigned int i = 0; i < orphanTx.vout.size(); i++) { auto it_by_prev = mapOrphanTransactionsByPrev.find(COutPoint(orphanHash, i)); if (it_by_prev != mapOrphanTransactionsByPrev.end()) { @@ -1944,35 +1902,17 @@ void static ProcessOrphanTx(CConnman* connman, CTxMemPool& mempool, std::setinsert(orphanTx.GetWitnessHash()); - // If the transaction failed for TX_INPUTS_NOT_STANDARD, + if ((!orphanTx.HasWitness() && orphan_state.GetResult() != TxValidationResult::TX_WITNESS_MUTATED) || + orphan_state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD) { + // Do not use rejection cache for witness transactions or + // witness-stripped transactions, as they can have been malleated. + // See https://github.com/bitcoin/bitcoin/issues/8279 for details. + // However, 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 (orphan_state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && orphanTx.GetWitnessHash() != orphanTx.GetHash()) { - // We only add the txid if it differs from the wtxid, to - // avoid wasting entries in the rolling bloom filter. - recentRejects->insert(orphanTx.GetHash()); - } + assert(recentRejects); + recentRejects->insert(orphanHash); } EraseOrphanTx(orphanHash); done = true; @@ -2076,10 +2016,6 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec if (pfrom->fInbound) PushNodeVersion(pfrom, connman, GetAdjustedTime()); - if (nVersion >= WTXID_RELAY_VERSION) { - connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::WTXIDRELAY)); - } - connman->PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK)); pfrom->nServices = nServices; @@ -2219,25 +2155,6 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec return true; } - // Feature negotiation of wtxidrelay should happen between VERSION and - // VERACK, to avoid relay problems from switching after a connection is up - if (msg_type == NetMsgType::WTXIDRELAY) { - if (pfrom->fSuccessfullyConnected) { - // Disconnect peers that send wtxidrelay message after VERACK; this - // must be negotiated between VERSION and VERACK. - pfrom->fDisconnect = true; - return false; - } - if (pfrom->nVersion >= WTXID_RELAY_VERSION) { - LOCK(cs_main); - if (!State(pfrom->GetId())->m_wtxid_relay) { - State(pfrom->GetId())->m_wtxid_relay = true; - g_wtxid_relay_peers++; - } - } - return false; - } - if (!pfrom->fSuccessfullyConnected) { // Must have a verack message before anything else LOCK(cs_main); @@ -2358,13 +2275,6 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec if (interruptMsgProc) return true; - // ignore INVs that don't match wtxidrelay setting - if (State(pfrom->GetId())->m_wtxid_relay) { - if (inv.type == MSG_TX) continue; - } else { - if (inv.type == MSG_WTX) continue; - } - bool fAlreadyHave = AlreadyHave(inv, mempool); LogPrint(BCLog::NET, "got inv: %s %s peer=%d\n", inv.ToString(), fAlreadyHave ? "have" : "new", pfrom->GetId()); @@ -2383,7 +2293,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec best_block = &inv.hash; } } else { - pfrom->AddKnownTx(inv.hash); + pfrom->AddInventoryKnown(inv); if (fBlocksOnly) { LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom->GetId()); pfrom->fDisconnect = true; @@ -2622,50 +2532,26 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec vRecv >> ptx; const CTransaction& tx = *ptx; - const uint256& txid = ptx->GetHash(); - const uint256& wtxid = ptx->GetWitnessHash(); + CInv inv(MSG_TX, tx.GetHash()); + pfrom->AddInventoryKnown(inv); LOCK2(cs_main, g_cs_orphans); - CNodeState* nodestate = State(pfrom->GetId()); - - const uint256& hash = nodestate->m_wtxid_relay ? wtxid : txid; - pfrom->AddKnownTx(hash); - if (nodestate->m_wtxid_relay && txid != wtxid) { - // Insert txid into filterInventoryKnown, even for - // wtxidrelay peers. This prevents re-adding of - // unconfirmed parents to the recently_announced - // filter, when a child tx is requested. See - // ProcessGetData(). - pfrom->AddKnownTx(txid); - } - TxValidationState state; - nodestate->m_tx_download.m_tx_announced.erase(hash); - nodestate->m_tx_download.m_tx_in_flight.erase(hash); - EraseTxRequest(hash); + CNodeState* nodestate = State(pfrom->GetId()); + nodestate->m_tx_download.m_tx_announced.erase(inv.hash); + nodestate->m_tx_download.m_tx_in_flight.erase(inv.hash); + EraseTxRequest(inv.hash); std::list lRemovedTxn; - // We do the AlreadyHave() check using wtxid, rather than txid - in the - // absence of witness malleation, this is strictly better, because the - // recent rejects filter may contain the wtxid but will never contain - // the txid of a segwit transaction that has been rejected. - // In the presence of witness malleation, it's possible that by only - // doing the check with wtxid, we could overlook a transaction which - // was confirmed with a different witness, or exists in our mempool - // with a different witness, but this has limited downside: - // mempool validation does its own lookup of whether we have the txid - // already; and an adversary can already relay us old transactions - // (older than our recency filter) if trying to DoS us, without any need - // for witness malleation. - if (!AlreadyHave(CInv(MSG_WTX, wtxid), mempool) && + if (!AlreadyHave(inv, mempool) && AcceptToMemoryPool(mempool, state, ptx, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { mempool.check(&::ChainstateActive().CoinsTip()); - RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), *connman); + RelayTransaction(tx.GetHash(), *connman); for (unsigned int i = 0; i < tx.vout.size(); i++) { - auto it_by_prev = mapOrphanTransactionsByPrev.find(COutPoint(txid, i)); + auto it_by_prev = mapOrphanTransactionsByPrev.find(COutPoint(inv.hash, i)); if (it_by_prev != mapOrphanTransactionsByPrev.end()) { for (const auto& elem : it_by_prev->second) { pfrom->orphan_work_set.insert(elem->first); @@ -2696,17 +2582,10 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec uint32_t nFetchFlags = GetFetchFlags(pfrom); const auto current_time = GetTime(); - if (!State(pfrom->GetId())->m_wtxid_relay) { - for (const CTxIn& txin : tx.vin) { - // Here, we only have the txid (and not wtxid) of the - // inputs, so we only request parents from - // non-wtxid-relay peers. - // Eventually we should replace this with an improved - // protocol for getting all unconfirmed parents. - CInv _inv(MSG_TX | nFetchFlags, txin.prevout.hash); - pfrom->AddKnownTx(txin.prevout.hash); - if (!AlreadyHave(_inv, mempool)) RequestTx(State(pfrom->GetId()), _inv.hash, current_time); - } + for (const CTxIn& txin : tx.vin) { + CInv _inv(MSG_TX | nFetchFlags, txin.prevout.hash); + pfrom->AddInventoryKnown(_inv); + if (!AlreadyHave(_inv, mempool)) RequestTx(State(pfrom->GetId()), _inv.hash, current_time); } AddOrphanTx(ptx, pfrom->GetId()); @@ -2720,41 +2599,20 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec 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. - // Here we add both the txid and the wtxid, as we know that - // regardless of what witness is provided, we will not accept - // this, so we don't need to allow for redownload of this txid - // from any of our non-wtxidrelay peers. recentRejects->insert(tx.GetHash()); - recentRejects->insert(tx.GetWitnessHash()); } } else { - 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. - assert(recentRejects); - recentRejects->insert(tx.GetWitnessHash()); - // If the transaction failed for TX_INPUTS_NOT_STANDARD, + if ((!tx.HasWitness() && state.GetResult() != TxValidationResult::TX_WITNESS_MUTATED) || + state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD) { + // Do not use rejection cache for witness transactions or + // witness-stripped transactions, as they can have been malleated. + // See https://github.com/bitcoin/bitcoin/issues/8279 for details. + // However, 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 && tx.GetWitnessHash() != tx.GetHash()) { - recentRejects->insert(tx.GetHash()); - } + assert(recentRejects); + recentRejects->insert(tx.GetHash()); if (RecursiveDynamicUsage(*ptx) < 100000) { AddToCompactExtraTransactions(ptx); } @@ -2771,7 +2629,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec LogPrintf("Not relaying non-mempool transaction %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId()); } else { LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId()); - RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), *connman); + RelayTransaction(tx.GetHash(), *connman); } } } @@ -3387,7 +3245,7 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec vRecv >> vInv; if (vInv.size() <= MAX_PEER_TX_IN_FLIGHT + MAX_BLOCKS_IN_TRANSIT_PER_PEER) { for (CInv &inv : vInv) { - if (inv.type == MSG_TX || inv.type == MSG_WITNESS_TX || inv.type == MSG_WTX) { + if (inv.type == MSG_TX || inv.type == MSG_WITNESS_TX) { // If we receive a NOTFOUND message for a txid we requested, erase // it from our data structures for this peer. auto in_flight_it = state->m_tx_download.m_tx_in_flight.find(inv.hash); @@ -3679,19 +3537,17 @@ namespace { class CompareInvMempoolOrder { CTxMemPool *mp; - bool m_wtxid_relay; public: - explicit CompareInvMempoolOrder(CTxMemPool *_mempool, bool use_wtxid) + explicit CompareInvMempoolOrder(CTxMemPool *_mempool) { mp = _mempool; - m_wtxid_relay = use_wtxid; } bool operator()(std::set::iterator a, std::set::iterator b) { /* As std::make_heap produces a max-heap, we want the entries with the * fewest ancestors/highest fee to sort later. */ - return mp->CompareDepthAndScore(*b, *a, m_wtxid_relay); + return mp->CompareDepthAndScore(*b, *a); } }; } @@ -3998,8 +3854,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto) LOCK(pto->m_tx_relay->cs_filter); for (const auto& txinfo : vtxinfo) { - const uint256& hash = state.m_wtxid_relay ? txinfo.tx->GetWitnessHash() : txinfo.tx->GetHash(); - CInv inv(state.m_wtxid_relay ? MSG_WTX : MSG_TX, hash); + const uint256& hash = txinfo.tx->GetHash(); + CInv inv(MSG_TX, hash); pto->m_tx_relay->setInventoryTxToSend.erase(hash); // Don't send transactions that peers will not put into their mempool if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) { @@ -4033,7 +3889,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) } // Topologically and fee-rate sort the inventory we send for privacy and priority reasons. // A heap is used so that not all items need sorting if only a few are being sent. - CompareInvMempoolOrder compareInvMempoolOrder(&m_mempool, state.m_wtxid_relay); + CompareInvMempoolOrder compareInvMempoolOrder(&m_mempool); std::make_heap(vInvTx.begin(), vInvTx.end(), compareInvMempoolOrder); // No reason to drain out at many times the network's capacity, // especially since we have many peers and some will draw much shorter delays. @@ -4052,19 +3908,17 @@ bool PeerLogicValidation::SendMessages(CNode* pto) continue; } // Not in the mempool anymore? don't bother sending it. - auto txinfo = m_mempool.info(hash, state.m_wtxid_relay); + auto txinfo = m_mempool.info(hash); if (!txinfo.tx) { continue; } - auto txid = txinfo.tx->GetHash(); - auto wtxid = txinfo.tx->GetWitnessHash(); // Peer told you to not send transactions at that feerate? Don't bother sending it. if (txinfo.fee < filterrate.GetFee(txinfo.vsize)) { continue; } if (pto->m_tx_relay->pfilter && !pto->m_tx_relay->pfilter->IsRelevantAndUpdate(*txinfo.tx)) continue; // Send - vInv.push_back(CInv(state.m_wtxid_relay ? MSG_WTX : MSG_TX, hash)); + vInv.push_back(CInv(MSG_TX, hash)); nRelayedTransactions++; { // Expire old relay messages @@ -4074,14 +3928,9 @@ bool PeerLogicValidation::SendMessages(CNode* pto) vRelayExpiration.pop_front(); } - auto ret = mapRelay.emplace(txid, std::move(txinfo.tx)); + auto ret = mapRelay.insert(std::make_pair(hash, std::move(txinfo.tx))); if (ret.second) { - vRelayExpiration.emplace_back(nNow + std::chrono::microseconds{RELAY_TX_CACHE_TIME}.count(), ret.first); - } - // Add wtxid-based lookup into mapRelay as well, so that peers can request by wtxid - auto ret2 = mapRelay.emplace(wtxid, ret.first->second); - if (ret2.second) { - vRelayExpiration.emplace_back(nNow + std::chrono::microseconds{RELAY_TX_CACHE_TIME}.count(), ret2.first); + vRelayExpiration.push_back(std::make_pair(nNow + std::chrono::microseconds{RELAY_TX_CACHE_TIME}.count(), ret.first)); } } if (vInv.size() == MAX_INV_SZ) { @@ -4089,14 +3938,6 @@ bool PeerLogicValidation::SendMessages(CNode* pto) vInv.clear(); } pto->m_tx_relay->filterInventoryKnown.insert(hash); - if (hash != txid) { - // Insert txid into filterInventoryKnown, even for - // wtxidrelay peers. This prevents re-adding of - // unconfirmed parents to the recently_announced - // filter, when a child tx is requested. See - // ProcessGetData(). - pto->m_tx_relay->filterInventoryKnown.insert(txid); - } } } } @@ -4221,7 +4062,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) // Erase this entry from tx_process_time (it may be added back for // processing at a later time, see below) tx_process_time.erase(tx_process_time.begin()); - CInv inv(state.m_wtxid_relay ? MSG_WTX : (MSG_TX | GetFetchFlags(pto)), txid); + CInv inv(MSG_TX | GetFetchFlags(pto), txid); if (!AlreadyHave(inv, m_mempool)) { // If this transaction was last requested more than 1 minute ago, // then request. @@ -4240,15 +4081,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) // up processing to happen after the download times out // (with a slight delay for inbound peers, to prefer // requests to outbound peers). - // Don't apply the txid-delay to re-requests of a - // transaction; the heuristic of delaying requests to - // txid-relay peers is to save bandwidth on initial - // announcement of a transaction, and doesn't make sense - // for a followup request if our first peer times out (and - // would open us up to an attacker using inbound - // wtxid-relay to prevent us from requesting transactions - // from outbound txid-relay peers). - const auto next_process_time = CalculateTxGetDataTime(txid, current_time, !state.fPreferredDownload, false); + const auto next_process_time = CalculateTxGetDataTime(txid, current_time, !state.fPreferredDownload); tx_process_time.emplace(next_process_time, txid); } } else { @@ -4301,7 +4134,6 @@ public: // orphan transactions mapOrphanTransactions.clear(); mapOrphanTransactionsByPrev.clear(); - g_orphans_by_wtxid.clear(); } }; static CNetProcessingCleanup instance_of_cnetprocessingcleanup; diff --git a/src/net_processing.h b/src/net_processing.h index 79a3ccd4ed..ddb1780148 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -92,6 +92,6 @@ struct CNodeStateStats { bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats); /** Relay transaction to every node */ -void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +void RelayTransaction(const uint256&, const CConnman& connman); #endif // BITCOIN_NET_PROCESSING_H diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 17878eeb17..201406ce3b 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -78,8 +78,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t } if (relay) { - LOCK(cs_main); - RelayTransaction(hashTx, tx->GetWitnessHash(), *node.connman); + RelayTransaction(hashTx, *node.connman); } return TransactionError::OK; diff --git a/src/protocol.cpp b/src/protocol.cpp index 8b2043fda4..bd3ed25a8a 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -40,7 +40,6 @@ const char *SENDCMPCT="sendcmpct"; const char *CMPCTBLOCK="cmpctblock"; const char *GETBLOCKTXN="getblocktxn"; const char *BLOCKTXN="blocktxn"; -const char *WTXIDRELAY="wtxidrelay"; } // namespace NetMsgType /** All known message types. Keep this in the same order as the list of @@ -72,7 +71,6 @@ const static std::string allNetMessageTypes[] = { NetMsgType::CMPCTBLOCK, NetMsgType::GETBLOCKTXN, NetMsgType::BLOCKTXN, - NetMsgType::WTXIDRELAY, }; const static std::vector allNetMessageTypesVec(allNetMessageTypes, allNetMessageTypes+ARRAYLEN(allNetMessageTypes)); @@ -185,8 +183,6 @@ std::string CInv::GetCommand() const switch (masked) { case MSG_TX: return cmd.append(NetMsgType::TX); - // WTX is not a message type, just an inv type - case MSG_WTX: return cmd.append("wtx"); case MSG_BLOCK: return cmd.append(NetMsgType::BLOCK); case MSG_FILTERED_BLOCK: return cmd.append(NetMsgType::MERKLEBLOCK); case MSG_CMPCT_BLOCK: return cmd.append(NetMsgType::CMPCTBLOCK); diff --git a/src/protocol.h b/src/protocol.h index a27ca56495..6639ae2aac 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -234,12 +234,6 @@ extern const char *GETBLOCKTXN; * @since protocol version 70014 as described by BIP 152 */ extern const char *BLOCKTXN; -/** - * Indicates that a node prefers to relay transactions via wtxid, rather than - * txid. - * @since protocol version 70016 as described by BIP 339. - */ -extern const char *WTXIDRELAY; }; /* Get a vector of all valid message types (see above) */ @@ -368,12 +362,12 @@ const uint32_t MSG_TYPE_MASK = 0xffffffff >> 2; * These numbers are defined by the protocol. When adding a new value, be sure * to mention it in the respective BIP. */ -enum GetDataMsg : uint32_t { +enum GetDataMsg +{ UNDEFINED = 0, MSG_TX = 1, MSG_BLOCK = 2, - MSG_WTX = 5, //!< Defined in BIP 339 - // The following can only occur in getdata. Invs always use TX/WTX or BLOCK. + // The following can only occur in getdata. Invs always use TX or BLOCK. MSG_FILTERED_BLOCK = 3, //!< Defined in BIP37 MSG_CMPCT_BLOCK = 4, //!< Defined in BIP152 MSG_WITNESS_BLOCK = MSG_BLOCK | MSG_WITNESS_FLAG, //!< Defined in BIP144 diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 8de225ba19..c14417a847 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -724,12 +724,12 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const assert(innerUsage == cachedInnerUsage); } -bool CTxMemPool::CompareDepthAndScore(const uint256& hasha, const uint256& hashb, bool wtxid) +bool CTxMemPool::CompareDepthAndScore(const uint256& hasha, const uint256& hashb) { LOCK(cs); - indexed_transaction_set::const_iterator i = wtxid ? get_iter_from_wtxid(hasha) : mapTx.find(hasha); + indexed_transaction_set::const_iterator i = mapTx.find(hasha); if (i == mapTx.end()) return false; - indexed_transaction_set::const_iterator j = wtxid ? get_iter_from_wtxid(hashb) : mapTx.find(hashb); + indexed_transaction_set::const_iterator j = mapTx.find(hashb); if (j == mapTx.end()) return true; uint64_t counta = i->GetCountWithAncestors(); uint64_t countb = j->GetCountWithAncestors(); @@ -809,10 +809,10 @@ CTransactionRef CTxMemPool::get(const uint256& hash) const return i->GetSharedTx(); } -TxMempoolInfo CTxMemPool::info(const uint256& hash, bool wtxid) const +TxMempoolInfo CTxMemPool::info(const uint256& hash) const { LOCK(cs); - indexed_transaction_set::const_iterator i = (wtxid ? get_iter_from_wtxid(hash) : mapTx.find(hash)); + indexed_transaction_set::const_iterator i = mapTx.find(hash); if (i == mapTx.end()) return TxMempoolInfo(); return GetInfo(i); @@ -915,8 +915,8 @@ bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const { size_t CTxMemPool::DynamicMemoryUsage() const { LOCK(cs); - // Estimate the overhead of mapTx to be 15 pointers + an allocation, as no exact formula for boost::multi_index_contained is implemented. - return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 15 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(mapLinks) + memusage::DynamicUsage(vTxHashes) + cachedInnerUsage; + // Estimate the overhead of mapTx to be 12 pointers + an allocation, as no exact formula for boost::multi_index_contained is implemented. + return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 12 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(mapLinks) + memusage::DynamicUsage(vTxHashes) + cachedInnerUsage; } void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants, MemPoolRemovalReason reason) { diff --git a/src/txmempool.h b/src/txmempool.h index 40986f13a3..3dae0a04c7 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -198,22 +198,6 @@ struct mempoolentry_txid } }; -// extracts a transaction witness-hash from CTxMemPoolEntry or CTransactionRef -struct mempoolentry_wtxid -{ - typedef uint256 result_type; - result_type operator() (const CTxMemPoolEntry &entry) const - { - return entry.GetTx().GetWitnessHash(); - } - - result_type operator() (const CTransactionRef& tx) const - { - return tx->GetWitnessHash(); - } -}; - - /** \class CompareTxMemPoolEntryByDescendantScore * * Sort an entry by max(score/size of entry's tx, score/size with all descendants). @@ -334,7 +318,6 @@ public: struct descendant_score {}; struct entry_time {}; struct ancestor_score {}; -struct index_by_wtxid {}; class CBlockPolicyEstimator; @@ -400,9 +383,8 @@ public: * * CTxMemPool::mapTx, and CTxMemPoolEntry bookkeeping: * - * mapTx is a boost::multi_index that sorts the mempool on 5 criteria: - * - transaction hash (txid) - * - witness-transaction hash (wtxid) + * mapTx is a boost::multi_index that sorts the mempool on 4 criteria: + * - transaction hash * - descendant feerate [we use max(feerate of tx, feerate of tx with all descendants)] * - time in mempool * - ancestor feerate [we use min(feerate of tx, feerate of tx with all unconfirmed ancestors)] @@ -487,12 +469,6 @@ public: boost::multi_index::indexed_by< // sorted by txid boost::multi_index::hashed_unique, - // sorted by wtxid - boost::multi_index::hashed_unique< - boost::multi_index::tag, - mempoolentry_wtxid, - SaltedTxidHasher - >, // sorted by fee rate boost::multi_index::ordered_non_unique< boost::multi_index::tag, @@ -607,7 +583,7 @@ public: void clear(); void _clear() EXCLUSIVE_LOCKS_REQUIRED(cs); //lock free - bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb, bool wtxid=false); + bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb); void queryHashes(std::vector& vtxid) const; bool isSpent(const COutPoint& outpoint) const; unsigned int GetTransactionsUpdated() const; @@ -710,22 +686,14 @@ public: return totalTxSize; } - bool exists(const uint256& hash, bool wtxid=false) const + bool exists(const uint256& hash) const { LOCK(cs); - if (wtxid) { - return (mapTx.get().count(hash) != 0); - } return (mapTx.count(hash) != 0); } CTransactionRef get(const uint256& hash) const; - txiter get_iter_from_wtxid(const uint256& wtxid) const EXCLUSIVE_LOCKS_REQUIRED(cs) - { - AssertLockHeld(cs); - return mapTx.project<0>(mapTx.get().find(wtxid)); - } - TxMempoolInfo info(const uint256& hash, bool wtxid=false) const; + TxMempoolInfo info(const uint256& hash) const; std::vector infoAll() const; size_t DynamicMemoryUsage() const; diff --git a/src/validation.cpp b/src/validation.cpp index a4adc81b5a..5a98b2cb92 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -916,7 +916,7 @@ bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, Workspace& ws, Precompute if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, txdata) && !CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) { // Only the witness is missing, so the transaction itself may be fine. - state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED, + state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, state.GetRejectReason(), state.GetDebugMessage()); } return false; // state filled in by CheckInputScripts diff --git a/src/version.h b/src/version.h index e55871fc41..d932b512d4 100644 --- a/src/version.h +++ b/src/version.h @@ -9,7 +9,7 @@ * network protocol versioning */ -static const int PROTOCOL_VERSION = 70016; +static const int PROTOCOL_VERSION = 70015; //! initial proto version, to be increased after version/verack negotiation static const int INIT_PROTO_VERSION = 209; @@ -42,7 +42,4 @@ static const int SHORT_IDS_BLOCKS_VERSION = 70014; //! not banning for invalid compact blocks starts with this version static const int INVALID_CB_NO_BAN_VERSION = 70015; -//! "wtxidrelay" command for wtxid-based relay starts with this version -static const int WTXID_RELAY_VERSION = 70016; - #endif // BITCOIN_VERSION_H diff --git a/test/functional/mempool_packages.py b/test/functional/mempool_packages.py index 5a1a73d16e..a07dad18d6 100755 --- a/test/functional/mempool_packages.py +++ b/test/functional/mempool_packages.py @@ -67,15 +67,10 @@ class MempoolPackagesTest(BitcoinTestFramework): fee = Decimal("0.0001") # MAX_ANCESTORS transactions off a confirmed tx should be fine chain = [] - witness_chain = [] for i in range(MAX_ANCESTORS): (txid, sent_value) = self.chain_transaction(self.nodes[0], txid, 0, value, fee, 1) value = sent_value chain.append(txid) - # We need the wtxids to check P2P announcements - fulltx = self.nodes[0].getrawtransaction(txid) - witnesstx = self.nodes[0].decoderawtransaction(fulltx, True) - witness_chain.append(witnesstx['hash']) # Check mempool has MAX_ANCESTORS transactions in it, and descendant and ancestor # count and fees should look correct diff --git a/test/functional/p2p_blocksonly.py b/test/functional/p2p_blocksonly.py index 1069702da7..3258a38e3c 100755 --- a/test/functional/p2p_blocksonly.py +++ b/test/functional/p2p_blocksonly.py @@ -52,7 +52,7 @@ class P2PBlocksOnly(BitcoinTestFramework): self.log.info('Check that txs from rpc are not rejected and relayed to other peers') assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], True) txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid'] - with self.nodes[0].assert_debug_log(['received getdata for: wtx {} peer=1'.format(txid)]): + with self.nodes[0].assert_debug_log(['received getdata for: tx {} peer=1'.format(txid)]): self.nodes[0].sendrawtransaction(sigtx) self.nodes[0].p2p.wait_for_tx(txid) assert_equal(self.nodes[0].getmempoolinfo()['size'], 1) diff --git a/test/functional/p2p_feefilter.py b/test/functional/p2p_feefilter.py index f77937d726..4f242bd94a 100755 --- a/test/functional/p2p_feefilter.py +++ b/test/functional/p2p_feefilter.py @@ -7,7 +7,7 @@ from decimal import Decimal import time -from test_framework.messages import MSG_TX, MSG_WTX, msg_feefilter +from test_framework.messages import msg_feefilter from test_framework.mininode import mininode_lock, P2PInterface from test_framework.test_framework import BitcoinTestFramework @@ -31,7 +31,7 @@ class TestP2PConn(P2PInterface): def on_inv(self, message): for i in message.inv: - if (i.type == MSG_TX) or (i.type == MSG_WTX): + if (i.type == 1): self.txinvs.append(hashToHex(i.hash)) def clear_invs(self): diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 40925ae8c9..a7cfefc485 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -22,9 +22,7 @@ from test_framework.messages import ( CTxOut, CTxWitness, MAX_BLOCK_BASE_SIZE, - MSG_TX, MSG_WITNESS_FLAG, - MSG_WTX, NODE_NETWORK, NODE_WITNESS, msg_no_witness_block, @@ -34,7 +32,6 @@ from test_framework.messages import ( msg_tx, msg_block, msg_no_witness_tx, - msg_verack, ser_uint256, ser_vector, sha256, @@ -82,7 +79,6 @@ from test_framework.util import ( softfork_active, hex_str_to_bytes, assert_raises_rpc_error, - wait_until, ) # The versionbit bit used to signal activation of SegWit @@ -145,42 +141,23 @@ def test_witness_block(node, p2p, block, accepted, with_witness=True, reason=Non class TestP2PConn(P2PInterface): - def __init__(self, wtxidrelay=False): + def __init__(self): super().__init__() self.getdataset = set() - self.last_wtxidrelay = [] - self.lastgetdata = [] - self.wtxidrelay = wtxidrelay # Avoid sending out msg_getdata in the mininode thread as a reply to invs. # They are not needed and would only lead to races because we send msg_getdata out in the test thread def on_inv(self, message): pass - def on_version(self, message): - if self.wtxidrelay: - super().on_version(message) - else: - self.send_message(msg_verack()) - self.nServices = message.nServices - def on_getdata(self, message): - self.lastgetdata = message.inv for inv in message.inv: self.getdataset.add(inv.hash) - def on_wtxidrelay(self, message): - self.last_wtxidrelay.append(message) - - def announce_tx_and_wait_for_getdata(self, tx, timeout=60, success=True, use_wtxid=False): + def announce_tx_and_wait_for_getdata(self, tx, timeout=60, success=True): with mininode_lock: self.last_message.pop("getdata", None) - if use_wtxid: - wtxid = tx.calc_sha256(True) - self.send_message(msg_inv(inv=[CInv(MSG_WTX, wtxid)])) - else: - self.send_message(msg_inv(inv=[CInv(MSG_TX, tx.sha256)])) - + self.send_message(msg_inv(inv=[CInv(1, tx.sha256)])) if success: self.wait_for_getdata(timeout) else: @@ -298,7 +275,6 @@ class SegWitTest(BitcoinTestFramework): self.test_upgrade_after_activation() self.test_witness_sigops() self.test_superfluous_witness() - self.test_wtxid_relay() # Individual tests @@ -1292,6 +1268,7 @@ class SegWitTest(BitcoinTestFramework): test_transaction_acceptance(self.nodes[0], self.test_node, tx, with_witness=True, accepted=False) # Verify that removing the witness succeeds. + self.test_node.announce_tx_and_wait_for_getdata(tx) test_transaction_acceptance(self.nodes[0], self.test_node, tx, with_witness=False, accepted=True) # Now try to add extra witness data to a valid witness tx. @@ -1318,6 +1295,8 @@ class SegWitTest(BitcoinTestFramework): # Node will not be blinded to the transaction self.std_node.announce_tx_and_wait_for_getdata(tx3) test_transaction_acceptance(self.nodes[1], self.std_node, tx3, True, False, 'tx-size') + self.std_node.announce_tx_and_wait_for_getdata(tx3) + test_transaction_acceptance(self.nodes[1], self.std_node, tx3, True, False, 'tx-size') # Remove witness stuffing, instead add extra witness push on stack tx3.vout[0] = CTxOut(tx2.vout[0].nValue - 1000, CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])) @@ -2044,11 +2023,6 @@ class SegWitTest(BitcoinTestFramework): # TODO: test p2sh sigop counting - # Cleanup and prep for next test - self.utxo.pop(0) - self.utxo.append(UTXO(tx2.sha256, 0, tx2.vout[0].nValue)) - - @subtest # type: ignore def test_superfluous_witness(self): # Serialization of tx that puts witness flag to 3 always def serialize_with_bogus_witness(tx): @@ -2092,67 +2066,6 @@ class SegWitTest(BitcoinTestFramework): with self.nodes[0].assert_debug_log(['Unknown transaction optional data']): self.nodes[0].p2p.send_and_ping(msg_bogus_tx(tx)) - @subtest # type: ignore - def test_wtxid_relay(self): - # Use brand new nodes to avoid contamination from earlier tests - self.wtx_node = self.nodes[0].add_p2p_connection(TestP2PConn(wtxidrelay=True), services=NODE_NETWORK | NODE_WITNESS) - self.tx_node = self.nodes[0].add_p2p_connection(TestP2PConn(wtxidrelay=False), services=NODE_NETWORK | NODE_WITNESS) - - # Check wtxidrelay feature negotiation message through connecting a new peer - def received_wtxidrelay(): - return (len(self.wtx_node.last_wtxidrelay) > 0) - wait_until(received_wtxidrelay, timeout=60, lock=mininode_lock) - - # Create a Segwit output from the latest UTXO - # and announce it to the network - witness_program = CScript([OP_TRUE]) - witness_hash = sha256(witness_program) - script_pubkey = CScript([OP_0, witness_hash]) - - tx = CTransaction() - tx.vin.append(CTxIn(COutPoint(self.utxo[0].sha256, self.utxo[0].n), b"")) - tx.vout.append(CTxOut(self.utxo[0].nValue - 1000, script_pubkey)) - tx.rehash() - - # Create a Segwit transaction - tx2 = CTransaction() - tx2.vin.append(CTxIn(COutPoint(tx.sha256, 0), b"")) - tx2.vout.append(CTxOut(tx.vout[0].nValue - 1000, script_pubkey)) - tx2.wit.vtxinwit.append(CTxInWitness()) - tx2.wit.vtxinwit[0].scriptWitness.stack = [witness_program] - tx2.rehash() - - # Announce Segwit transaction with wtxid - # and wait for getdata - self.wtx_node.announce_tx_and_wait_for_getdata(tx2, use_wtxid=True) - with mininode_lock: - lgd = self.wtx_node.lastgetdata[:] - assert_equal(lgd, [CInv(MSG_WTX, tx2.calc_sha256(True))]) - - # Announce Segwit transaction from non wtxidrelay peer - # and wait for getdata - self.tx_node.announce_tx_and_wait_for_getdata(tx2, use_wtxid=False) - with mininode_lock: - lgd = self.tx_node.lastgetdata[:] - assert_equal(lgd, [CInv(MSG_TX | MSG_WITNESS_FLAG, tx2.sha256)]) - - # Send tx2 through; it's an orphan so won't be accepted - with mininode_lock: - self.tx_node.last_message.pop("getdata", None) - test_transaction_acceptance(self.nodes[0], self.tx_node, tx2, with_witness=True, accepted=False) - - # Expect a request for parent (tx) due to use of non-WTX peer - self.tx_node.wait_for_getdata(60) - with mininode_lock: - lgd = self.tx_node.lastgetdata[:] - assert_equal(lgd, [CInv(MSG_TX | MSG_WITNESS_FLAG, tx.sha256)]) - - # Send tx through - test_transaction_acceptance(self.nodes[0], self.tx_node, tx, with_witness=False, accepted=True) - - # Check tx2 is there now - assert_equal(tx2.hash in self.nodes[0].getrawmempool(), True) - if __name__ == '__main__': SegWitTest().main() diff --git a/test/functional/p2p_tx_download.py b/test/functional/p2p_tx_download.py index 7e7df1442e..b56dc994e7 100755 --- a/test/functional/p2p_tx_download.py +++ b/test/functional/p2p_tx_download.py @@ -12,7 +12,6 @@ from test_framework.messages import ( FromHex, MSG_TX, MSG_TYPE_MASK, - MSG_WTX, msg_inv, msg_notfound, ) @@ -37,7 +36,7 @@ class TestP2PConn(P2PInterface): def on_getdata(self, message): for i in message.inv: - if i.type & MSG_TYPE_MASK == MSG_TX or i.type & MSG_TYPE_MASK == MSG_WTX: + if i.type & MSG_TYPE_MASK == MSG_TX: self.tx_getdata_count += 1 @@ -45,13 +44,12 @@ class TestP2PConn(P2PInterface): GETDATA_TX_INTERVAL = 60 # seconds MAX_GETDATA_RANDOM_DELAY = 2 # seconds INBOUND_PEER_TX_DELAY = 2 # seconds -TXID_RELAY_DELAY = 2 # seconds MAX_GETDATA_IN_FLIGHT = 100 TX_EXPIRY_INTERVAL = GETDATA_TX_INTERVAL * 10 # Python test constants NUM_INBOUND = 10 -MAX_GETDATA_INBOUND_WAIT = GETDATA_TX_INTERVAL + MAX_GETDATA_RANDOM_DELAY + INBOUND_PEER_TX_DELAY + TXID_RELAY_DELAY +MAX_GETDATA_INBOUND_WAIT = GETDATA_TX_INTERVAL + MAX_GETDATA_RANDOM_DELAY + INBOUND_PEER_TX_DELAY class TxDownloadTest(BitcoinTestFramework): @@ -65,7 +63,7 @@ class TxDownloadTest(BitcoinTestFramework): txid = 0xdeadbeef self.log.info("Announce the txid from each incoming peer to node 0") - msg = msg_inv([CInv(t=MSG_WTX, h=txid)]) + msg = msg_inv([CInv(t=1, h=txid)]) for p in self.nodes[0].p2ps: p.send_and_ping(msg) @@ -137,13 +135,13 @@ class TxDownloadTest(BitcoinTestFramework): with mininode_lock: p.tx_getdata_count = 0 - p.send_message(msg_inv([CInv(t=MSG_WTX, h=i) for i in txids])) + p.send_message(msg_inv([CInv(t=1, h=i) for i in txids])) wait_until(lambda: p.tx_getdata_count >= MAX_GETDATA_IN_FLIGHT, lock=mininode_lock) with mininode_lock: assert_equal(p.tx_getdata_count, MAX_GETDATA_IN_FLIGHT) self.log.info("Now check that if we send a NOTFOUND for a transaction, we'll get one more request") - p.send_message(msg_notfound(vec=[CInv(t=MSG_WTX, h=txids[0])])) + p.send_message(msg_notfound(vec=[CInv(t=1, h=txids[0])])) wait_until(lambda: p.tx_getdata_count >= MAX_GETDATA_IN_FLIGHT + 1, timeout=10, lock=mininode_lock) with mininode_lock: assert_equal(p.tx_getdata_count, MAX_GETDATA_IN_FLIGHT + 1) diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index c8c38e017a..5f8fcc6fd8 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -31,7 +31,7 @@ from test_framework.siphash import siphash256 from test_framework.util import hex_str_to_bytes, assert_equal MIN_VERSION_SUPPORTED = 60001 -MY_VERSION = 70016 # past wtxid relay +MY_VERSION = 70014 # past bip-31 for ping/pong MY_SUBVERSION = b"/python-mininode-tester:0.0.3/" MY_RELAY = 1 # from version 70001 onwards, fRelay should be appended to version messages (BIP37) @@ -52,7 +52,6 @@ NODE_NETWORK_LIMITED = (1 << 10) MSG_TX = 1 MSG_BLOCK = 2 MSG_FILTERED_BLOCK = 3 -MSG_WTX = 5 MSG_WITNESS_FLAG = 1 << 30 MSG_TYPE_MASK = 0xffffffff >> 2 @@ -232,8 +231,7 @@ class CInv: MSG_TX | MSG_WITNESS_FLAG: "WitnessTx", MSG_BLOCK | MSG_WITNESS_FLAG: "WitnessBlock", MSG_FILTERED_BLOCK: "filtered Block", - 4: "CompactBlock", - 5: "WTX", + 4: "CompactBlock" } def __init__(self, t=0, h=0): @@ -254,9 +252,6 @@ class CInv: return "CInv(type=%s hash=%064x)" \ % (self.typemap[self.type], self.hash) - def __eq__(self, other): - return isinstance(other, CInv) and self.hash == other.hash and self.type == other.type - class CBlockLocator: __slots__ = ("nVersion", "vHave") @@ -1118,22 +1113,6 @@ class msg_tx: def __repr__(self): return "msg_tx(tx=%s)" % (repr(self.tx)) -class msg_wtxidrelay: - __slots__ = () - command = b"wtxidrelay" - - def __init__(self): - pass - - def deserialize(self, f): - pass - - def serialize(self): - return b"" - - def __repr__(self): - return "msg_wtxidrelay()" - class msg_no_witness_tx(msg_tx): __slots__ = () diff --git a/test/functional/test_framework/mininode.py b/test/functional/test_framework/mininode.py index 383b340118..ad330f2a93 100755 --- a/test/functional/test_framework/mininode.py +++ b/test/functional/test_framework/mininode.py @@ -52,7 +52,6 @@ from test_framework.messages import ( MSG_TYPE_MASK, msg_verack, msg_version, - msg_wtxidrelay, NODE_NETWORK, NODE_WITNESS, sha256, @@ -87,7 +86,6 @@ MESSAGEMAP = { b"tx": msg_tx, b"verack": msg_verack, b"version": msg_version, - b"wtxidrelay": msg_wtxidrelay, } MAGIC_BYTES = { @@ -345,7 +343,6 @@ class P2PInterface(P2PConnection): def on_sendcmpct(self, message): pass def on_sendheaders(self, message): pass def on_tx(self, message): pass - def on_wtxidrelay(self, message): pass def on_inv(self, message): want = msg_getdata() @@ -363,8 +360,6 @@ class P2PInterface(P2PConnection): def on_version(self, message): assert message.nVersion >= MIN_VERSION_SUPPORTED, "Version {} received. Test framework only supports versions greater than {}".format(message.nVersion, MIN_VERSION_SUPPORTED) - if message.nVersion >= 70016: - self.send_message(msg_wtxidrelay()) self.send_message(msg_verack()) self.nServices = message.nServices diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py index fe670ddaee..d122e3db52 100755 --- a/test/functional/wallet_resendwallettransactions.py +++ b/test/functional/wallet_resendwallettransactions.py @@ -7,11 +7,7 @@ from collections import defaultdict import time from test_framework.blocktools import create_block, create_coinbase -from test_framework.messages import ( - MSG_TX, - MSG_WTX, - ToHex, -) +from test_framework.messages import ToHex from test_framework.mininode import P2PInterface, mininode_lock from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal, wait_until @@ -25,7 +21,7 @@ class P2PStoreTxInvs(P2PInterface): def on_inv(self, message): # Store how many times invs have been received for each tx. for i in message.inv: - if i.type == MSG_TX or i.type == MSG_WTX: + if i.type == 1: # save txid self.tx_invs_received[i.hash] += 1 @@ -43,8 +39,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework): node.add_p2p_connection(P2PStoreTxInvs()) self.log.info("Create a new transaction and wait until it's broadcast") - txid = node.sendtoaddress(node.getnewaddress(), 1) - wtxid = int(node.getrawtransaction(txid, 1)['hash'], 16) + txid = int(node.sendtoaddress(node.getnewaddress(), 1), 16) # Wallet rebroadcast is first scheduled 1 sec after startup (see # nNextResend in ResendWalletTransactions()). Sleep for just over a @@ -53,7 +48,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework): time.sleep(1.1) # Can take a few seconds due to transaction trickling - wait_until(lambda: node.p2p.tx_invs_received[wtxid] >= 1, lock=mininode_lock) + wait_until(lambda: node.p2p.tx_invs_received[txid] >= 1, lock=mininode_lock) # Add a second peer since txs aren't rebroadcast to the same peer (see filterInventoryKnown) node.add_p2p_connection(P2PStoreTxInvs()) @@ -72,13 +67,13 @@ class ResendWalletTransactionsTest(BitcoinTestFramework): # Transaction should not be rebroadcast node.syncwithvalidationinterfacequeue() node.p2ps[1].sync_with_ping() - assert_equal(node.p2ps[1].tx_invs_received[wtxid], 0) + assert_equal(node.p2ps[1].tx_invs_received[txid], 0) self.log.info("Transaction should be rebroadcast after 30 minutes") # Use mocktime and give an extra 5 minutes to be sure. rebroadcast_time = int(time.time()) + 41 * 60 node.setmocktime(rebroadcast_time) - wait_until(lambda: node.p2ps[1].tx_invs_received[wtxid] >= 1, lock=mininode_lock) + wait_until(lambda: node.p2ps[1].tx_invs_received[txid] >= 1, lock=mininode_lock) if __name__ == '__main__': -- cgit v1.2.3