From 2599277e9cb51e3619582978cba9bf03325c0cb6 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 30 Jan 2020 09:35:00 -0500 Subject: Add support for tx-relay via wtxid This adds a field to CNodeState that tracks whether to relay transactions with that peer via wtxid, instead of txid. As of this commit the field will always be false, but in a later commit we will add a way to negotiate turning this on via p2p messages exchanged with the peer. --- src/net_processing.cpp | 124 ++++++++++++++++++++++++++++++------------ src/net_processing.h | 2 +- src/node/transaction.cpp | 3 +- src/protocol.cpp | 2 + src/protocol.h | 4 +- test/functional/p2p_segwit.py | 2 +- 6 files changed, 98 insertions(+), 39 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f8b0f98e23..106f061d03 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -362,6 +362,9 @@ 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) @@ -1332,6 +1335,7 @@ 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) @@ -1346,7 +1350,11 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO { LOCK(g_cs_orphans); - if (mapOrphanTransactions.count(inv.hash)) return true; + 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; + } } { @@ -1354,8 +1362,8 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO if (g_recent_confirmed_transactions->contains(inv.hash)) return true; } - return recentRejects->contains(inv.hash) || - mempool.exists(inv.hash); + const bool by_wtxid = (inv.type == MSG_WTX); + return recentRejects->contains(inv.hash) || mempool.exists(inv.hash, by_wtxid); } case MSG_BLOCK: case MSG_WITNESS_BLOCK: @@ -1365,12 +1373,17 @@ bool static AlreadyHave(const CInv& inv, const CTxMemPool& mempool) EXCLUSIVE_LO return true; } -void RelayTransaction(const uint256& txid, const CConnman& connman) +void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) { - CInv inv(MSG_TX, txid); - connman.ForEachNode([&inv](CNode* pnode) + connman.ForEachNode([&txid, &wtxid](CNode* pnode) { - pnode->PushInventory(inv); + 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}); + } }); } @@ -1585,7 +1598,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)) { + while (it != pfrom->vRecvGetData.end() && (it->type == MSG_TX || it->type == MSG_WITNESS_TX || it->type == MSG_WTX)) { if (interruptMsgProc) return; // The send buffer provides backpressure. If there's no space in @@ -1608,7 +1621,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); + auto txinfo = mempool.info(inv.hash, inv.type == MSG_WTX); // 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. @@ -1888,7 +1901,7 @@ void static ProcessOrphanTx(CConnman* connman, CTxMemPool& mempool, std::setGetWitnessHash(), *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()) { @@ -2559,23 +2572,47 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec const CTransaction& tx = *ptx; const uint256& txid = ptx->GetHash(); - pfrom->AddInventoryKnown(txid); + const uint256& wtxid = ptx->GetWitnessHash(); LOCK2(cs_main, g_cs_orphans); + CNodeState* nodestate = State(pfrom->GetId()); + + const uint256& hash = nodestate->m_wtxid_relay ? wtxid : txid; + pfrom->AddInventoryKnown(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->AddInventoryKnown(txid); + } + TxValidationState state; - CNodeState* nodestate = State(pfrom->GetId()); - nodestate->m_tx_download.m_tx_announced.erase(txid); - nodestate->m_tx_download.m_tx_in_flight.erase(txid); - EraseTxRequest(txid); + nodestate->m_tx_download.m_tx_announced.erase(hash); + nodestate->m_tx_download.m_tx_in_flight.erase(hash); + EraseTxRequest(hash); std::list lRemovedTxn; - if (!AlreadyHave(CInv(MSG_TX, txid), mempool) && + // 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) && AcceptToMemoryPool(mempool, state, ptx, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { mempool.check(&::ChainstateActive().CoinsTip()); - RelayTransaction(tx.GetHash(), *connman); + RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), *connman); for (unsigned int i = 0; i < tx.vout.size(); i++) { auto it_by_prev = mapOrphanTransactionsByPrev.find(COutPoint(txid, i)); if (it_by_prev != mapOrphanTransactionsByPrev.end()) { @@ -2608,10 +2645,17 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec uint32_t nFetchFlags = GetFetchFlags(pfrom); const auto current_time = GetTime(); - for (const CTxIn& txin : tx.vin) { - CInv _inv(MSG_TX | nFetchFlags, txin.prevout.hash); - pfrom->AddInventoryKnown(txin.prevout.hash); - if (!AlreadyHave(_inv, mempool)) RequestTx(State(pfrom->GetId()), _inv.hash, current_time); + 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->AddInventoryKnown(txin.prevout.hash); + if (!AlreadyHave(_inv, mempool)) RequestTx(State(pfrom->GetId()), _inv.hash, current_time); + } } AddOrphanTx(ptx, pfrom->GetId()); @@ -2672,7 +2716,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(), *connman); + RelayTransaction(tx.GetHash(), tx.GetWitnessHash(), *connman); } } } @@ -3288,7 +3332,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) { + if (inv.type == MSG_TX || inv.type == MSG_WITNESS_TX || inv.type == MSG_WTX) { // 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); @@ -3580,17 +3624,19 @@ namespace { class CompareInvMempoolOrder { CTxMemPool *mp; + bool m_wtxid_relay; public: - explicit CompareInvMempoolOrder(CTxMemPool *_mempool) + explicit CompareInvMempoolOrder(CTxMemPool *_mempool, bool use_wtxid) { 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); + return mp->CompareDepthAndScore(*b, *a, m_wtxid_relay); } }; } @@ -3897,8 +3943,8 @@ bool PeerLogicValidation::SendMessages(CNode* pto) LOCK(pto->m_tx_relay->cs_filter); for (const auto& txinfo : vtxinfo) { - const uint256& hash = txinfo.tx->GetHash(); - CInv inv(MSG_TX, hash); + const uint256& hash = state.m_wtxid_relay ? txinfo.tx->GetWitnessHash() : txinfo.tx->GetHash(); + CInv inv(state.m_wtxid_relay ? MSG_WTX : 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)) { @@ -3932,7 +3978,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); + CompareInvMempoolOrder compareInvMempoolOrder(&m_mempool, state.m_wtxid_relay); 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. @@ -3951,17 +3997,19 @@ bool PeerLogicValidation::SendMessages(CNode* pto) continue; } // Not in the mempool anymore? don't bother sending it. - auto txinfo = m_mempool.info(hash); + auto txinfo = m_mempool.info(hash, state.m_wtxid_relay); 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(MSG_TX, hash)); + vInv.push_back(CInv(state.m_wtxid_relay ? MSG_WTX : MSG_TX, hash)); nRelayedTransactions++; { // Expire old relay messages @@ -3971,12 +4019,12 @@ bool PeerLogicValidation::SendMessages(CNode* pto) vRelayExpiration.pop_front(); } - auto ret = mapRelay.insert(std::make_pair(hash, std::move(txinfo.tx))); + auto ret = mapRelay.emplace(txid, std::move(txinfo.tx)); if (ret.second) { - vRelayExpiration.push_back(std::make_pair(nNow + std::chrono::microseconds{RELAY_TX_CACHE_TIME}.count(), ret.first)); + 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(ret.first->second->GetWitnessHash(), ret.first->second); + 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); } @@ -3986,6 +4034,14 @@ 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); + } } } } @@ -4110,7 +4166,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(MSG_TX | GetFetchFlags(pto), txid); + CInv inv(state.m_wtxid_relay ? MSG_WTX : (MSG_TX | GetFetchFlags(pto)), txid); if (!AlreadyHave(inv, m_mempool)) { // If this transaction was last requested more than 1 minute ago, // then request. diff --git a/src/net_processing.h b/src/net_processing.h index ddb1780148..79a3ccd4ed 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&, const CConnman& connman); +void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(cs_main); #endif // BITCOIN_NET_PROCESSING_H diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 201406ce3b..17878eeb17 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -78,7 +78,8 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t } if (relay) { - RelayTransaction(hashTx, *node.connman); + LOCK(cs_main); + RelayTransaction(hashTx, tx->GetWitnessHash(), *node.connman); } return TransactionError::OK; diff --git a/src/protocol.cpp b/src/protocol.cpp index bd3ed25a8a..f5cf9e6de8 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -183,6 +183,8 @@ 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 6639ae2aac..1d14585f97 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -362,11 +362,11 @@ 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 -{ +enum GetDataMsg : uint32_t { 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 or BLOCK. MSG_FILTERED_BLOCK = 3, //!< Defined in BIP37 MSG_CMPCT_BLOCK = 4, //!< Defined in BIP152 diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index a7cfefc485..f7255d1911 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -1296,7 +1296,7 @@ class SegWitTest(BitcoinTestFramework): 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') + test_transaction_acceptance(self.nodes[1], self.std_node, tx3, True, False) # 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])) -- cgit v1.2.3