diff options
author | MarcoFalke <falke.marco@gmail.com> | 2020-11-16 07:51:21 +0100 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2020-11-16 07:50:27 +0100 |
commit | fa074d2c7b9c3d34876c428d12672a505d4ce4eb (patch) | |
tree | 26338f3d5945b898c8ea9dfbd13d32679ec81040 /src | |
parent | a339289c2ef9caffa1195436695a13f6e48e1bbc (diff) | |
download | bitcoin-fa074d2c7b9c3d34876c428d12672a505d4ce4eb.tar.xz |
Revert "Merge #19606: Backport wtxid relay to v0.20"
This reverts commit a339289c2ef9caffa1195436695a13f6e48e1bbc, reversing
changes made to b9ac31f2d29ae3e79c0f0cde5bab2d7213e6da51.
Diffstat (limited to 'src')
-rw-r--r-- | src/consensus/validation.h | 6 | ||||
-rw-r--r-- | src/net.h | 4 | ||||
-rw-r--r-- | src/net_processing.cpp | 282 | ||||
-rw-r--r-- | src/net_processing.h | 2 | ||||
-rw-r--r-- | src/node/transaction.cpp | 3 | ||||
-rw-r--r-- | src/protocol.cpp | 4 | ||||
-rw-r--r-- | src/protocol.h | 12 | ||||
-rw-r--r-- | src/txmempool.cpp | 14 | ||||
-rw-r--r-- | src/txmempool.h | 42 | ||||
-rw-r--r-- | src/validation.cpp | 2 | ||||
-rw-r--r-- | src/version.h | 5 |
11 files changed, 79 insertions, 297 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,16 +31,12 @@ 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) * Currently this is only used if the transaction already exists in the mempool or on chain. @@ -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<uint256, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans); -std::map<uint256, std::map<uint256, COrphanTx>::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<CRollingBloomFilter> 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<const CBlock>& 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::set<uin if (setMisbehaving.count(fromPeer)) continue; if (AcceptToMemoryPool(mempool, orphan_state, porphanTx, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); - RelayTransaction(orphanHash, porphanTx->GetWitnessHash(), *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::set<uin // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString()); - if (orphan_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(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<CTransactionRef> 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<std::chrono::microseconds>(); - 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<uint256>::iterator a, std::set<uint256>::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<std::string> 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<mempoolentry_txid, SaltedTxidHasher>, - // sorted by wtxid - boost::multi_index::hashed_unique< - boost::multi_index::tag<index_by_wtxid>, - mempoolentry_wtxid, - SaltedTxidHasher - >, // sorted by fee rate boost::multi_index::ordered_non_unique< boost::multi_index::tag<descendant_score>, @@ -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<uint256>& 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<index_by_wtxid>().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<index_by_wtxid>().find(wtxid)); - } - TxMempoolInfo info(const uint256& hash, bool wtxid=false) const; + TxMempoolInfo info(const uint256& hash) const; std::vector<TxMempoolInfo> 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 |