From a9bc5638031a29abaa40284273a3507b345c31e9 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 1 Jun 2020 10:53:03 -0700 Subject: [PATCH 1/5] Swap relay pool and mempool lookup This is in preparation to using the mempool entering time as part of the decision for relay, but does not change behavior on itself. --- src/net_processing.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f1a89a89364..07c12938c82 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1627,13 +1627,6 @@ CTransactionRef static FindTxForGetData(CNode& peer, const uint256& txid, const if (peer.m_tx_relay->setInventoryTxToSend.count(txid)) return {}; } - { - LOCK(cs_main); - // Look up transaction in relay pool - auto mi = mapRelay.find(txid); - if (mi != mapRelay.end()) return mi->second; - } - auto txinfo = mempool.info(txid); if (txinfo.tx) { // To protect privacy, do not answer getdata using the mempool when @@ -1644,6 +1637,13 @@ CTransactionRef static FindTxForGetData(CNode& peer, const uint256& txid, const } } + { + LOCK(cs_main); + // Look up transaction in relay pool + auto mi = mapRelay.find(txid); + if (mi != mapRelay.end()) return mi->second; + } + return {}; } From b24a17f03982c9cd8fd6ec665b16e022374c96f0 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 1 Jun 2020 19:38:16 -0700 Subject: [PATCH 2/5] Introduce constant for mempool-based relay separate from mapRelay caching This constant is set to 2 minutes, rather than 15. This is still many times larger than the transaction broadcast interval (2s for outbound, 5s for inbound), so it should be acceptable for peers to know what our contents of the mempool was that long ago. --- src/net_processing.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 07c12938c82..92217747614 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -38,7 +38,9 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60; /** Minimum time between orphan transactions expire time checks in seconds */ static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60; /** How long to cache transactions in mapRelay for normal relay */ -static constexpr std::chrono::seconds RELAY_TX_CACHE_TIME{15 * 60}; +static constexpr std::chrono::seconds RELAY_TX_CACHE_TIME = std::chrono::minutes{15}; +/** How long a transaction has to be in the mempool before it can unconditionally be relayed (even when not in mapRelay). */ +static constexpr std::chrono::seconds UNCONDITIONAL_RELAY_DELAY = std::chrono::minutes{2}; /** Headers download timeout expressed in microseconds * Timeout = base + per_header * (expected number of headers) */ static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_BASE = 15 * 60 * 1000000; // 15 minutes @@ -1617,7 +1619,7 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c } //! Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). -CTransactionRef static FindTxForGetData(CNode& peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds longlived_mempool_time) LOCKS_EXCLUDED(cs_main) +CTransactionRef static FindTxForGetData(const CNode& peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main) { // Check if the requested transaction is so recent that we're just // about to announce it to the peer; if so, they certainly shouldn't @@ -1631,8 +1633,8 @@ CTransactionRef static FindTxForGetData(CNode& peer, const uint256& txid, const if (txinfo.tx) { // 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. - if ((mempool_req.count() && txinfo.m_time <= mempool_req) || txinfo.m_time <= longlived_mempool_time) { + // and it's more recent than UNCONDITIONAL_RELAY_DELAY. + if ((mempool_req.count() && txinfo.m_time <= mempool_req) || txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY) { return txinfo.tx; } } @@ -1655,8 +1657,7 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm std::vector vNotFound; const CNetMsgMaker msgMaker(pfrom.GetSendVersion()); - // mempool entries added before this time have likely expired from mapRelay - const std::chrono::seconds longlived_mempool_time = GetTime() - RELAY_TX_CACHE_TIME; + const std::chrono::seconds now = GetTime(); // Get last mempool request time const std::chrono::seconds mempool_req = pfrom.m_tx_relay != nullptr ? pfrom.m_tx_relay->m_last_mempool_req.load() : std::chrono::seconds::min(); @@ -1677,7 +1678,7 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm continue; } - CTransactionRef tx = FindTxForGetData(pfrom, inv.hash, mempool_req, longlived_mempool_time); + CTransactionRef tx = FindTxForGetData(pfrom, inv.hash, mempool_req, now); if (tx) { int nSendFlags = (inv.type == MSG_TX ? SERIALIZE_TRANSACTION_NO_WITNESS : 0); connman->PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *tx)); From 43f02ccbff9b137d59458da7a8afdb0bf80e127f Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 29 May 2020 11:33:17 -0700 Subject: [PATCH 3/5] Only respond to requests for recently announced transactions ... unless they're UNCONDITIONAL_RELAY_DELAY old, or there has been a response to a MEMPOOL request in the mean time. This is accomplished using a rolling Bloom filter for the last 3500 announced transactions. The probability of seeing more than 100 broadcast events (which can be up to 35 txids each) in 2 minutes for an outbound peer (where the average frequency is one per minute), is less than 1 in a million. --- src/net_processing.cpp | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 92217747614..dc28d8cca5f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -121,9 +121,18 @@ static constexpr std::chrono::seconds AVG_ADDRESS_BROADCAST_INTERVAL{30}; /** Average delay between trickled inventory transmissions in seconds. * Blocks and whitelisted receivers bypass this, outbound peers get half this delay. */ static const unsigned int INVENTORY_BROADCAST_INTERVAL = 5; -/** Maximum number of inventory items to send per transmission. +/** Maximum rate of inventory items to send per second. * Limits the impact of low-fee transaction floods. */ -static constexpr unsigned int INVENTORY_BROADCAST_MAX = 7 * INVENTORY_BROADCAST_INTERVAL; +static constexpr unsigned int INVENTORY_BROADCAST_PER_SECOND = 7; +/** Maximum number of inventory items to send per transmission. */ +static constexpr unsigned int INVENTORY_BROADCAST_MAX = INVENTORY_BROADCAST_PER_SECOND * INVENTORY_BROADCAST_INTERVAL; +/** The number of most recently announced transactions a peer can request. */ +static constexpr unsigned int INVENTORY_MAX_RECENT_RELAY = 3500; +/** Verify that INVENTORY_MAX_RECENT_RELAY is enough to cache everything typically + * relayed before unconditional relay from the mempool kicks in. This is only a + * lower bound, and it should be larger to account for higher inv rate to outbound + * peers, and random variations in the broadcast mechanism. */ +static_assert(INVENTORY_MAX_RECENT_RELAY >= INVENTORY_BROADCAST_PER_SECOND * UNCONDITIONAL_RELAY_DELAY / std::chrono::seconds{1}, "INVENTORY_RELAY_MAX too low"); /** Average delay between feefilter broadcasts in seconds. */ static constexpr unsigned int AVG_FEEFILTER_BROADCAST_INTERVAL = 10 * 60; /** Maximum feefilter broadcast delay after significant change. */ @@ -397,6 +406,9 @@ struct CNodeState { //! Whether this peer is a manual connection bool m_is_manual_connection; + //! A rolling bloom filter of all announced tx CInvs to this peer. + CRollingBloomFilter m_recently_announced_invs = CRollingBloomFilter{INVENTORY_MAX_RECENT_RELAY, 0.000001}; + 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) @@ -424,6 +436,7 @@ struct CNodeState { fSupportsDesiredCmpctVersion = false; m_chain_sync = { 0, nullptr, false, false }; m_last_block_announcement = 0; + m_recently_announced_invs.reset(); } }; @@ -1631,19 +1644,25 @@ CTransactionRef static FindTxForGetData(const CNode& peer, const uint256& txid, auto txinfo = mempool.info(txid); if (txinfo.tx) { - // To protect privacy, do not answer getdata using the mempool when - // that TX couldn't have been INVed in reply to a MEMPOOL request, - // and it's more recent than UNCONDITIONAL_RELAY_DELAY. + // If a TX could have been INVed in reply to a MEMPOOL request, + // or is older than UNCONDITIONAL_RELAY_DELAY, permit the request + // unconditionally. if ((mempool_req.count() && txinfo.m_time <= mempool_req) || txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY) { - return txinfo.tx; + return std::move(txinfo.tx); } } { LOCK(cs_main); - // Look up transaction in relay pool - auto mi = mapRelay.find(txid); - if (mi != mapRelay.end()) return mi->second; + + // Otherwise, the transaction must have been announced recently. + if (State(peer.GetId())->m_recently_announced_invs.contains(txid)) { + // If it was, it can be relayed from either the mempool... + if (txinfo.tx) return std::move(txinfo.tx); + // ... or the relay pool. + auto mi = mapRelay.find(txid); + if (mi != mapRelay.end()) return mi->second; + } } return {}; @@ -4155,6 +4174,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) if (!pto->m_tx_relay->pfilter->IsRelevantAndUpdate(*txinfo.tx)) continue; } pto->m_tx_relay->filterInventoryKnown.insert(hash); + // Responses to MEMPOOL requests bypass the m_recently_announced_invs filter. vInv.push_back(inv); if (vInv.size() == MAX_INV_SZ) { connman->PushMessage(pto, msgMaker.Make(NetMsgType::INV, vInv)); @@ -4208,6 +4228,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto) } if (pto->m_tx_relay->pfilter && !pto->m_tx_relay->pfilter->IsRelevantAndUpdate(*txinfo.tx)) continue; // Send + State(pto->GetId())->m_recently_announced_invs.insert(hash); vInv.push_back(CInv(MSG_TX, hash)); nRelayedTransactions++; { From c4626bcd211af08c85b6567ef07eeae333edba47 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 1 Jun 2020 20:06:27 -0700 Subject: [PATCH 4/5] Drop setInventoryTxToSend based filtering --- src/net_processing.cpp | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index dc28d8cca5f..c8d03da6597 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1634,14 +1634,6 @@ void static ProcessGetBlockData(CNode& pfrom, const CChainParams& chainparams, c //! Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). CTransactionRef static FindTxForGetData(const CNode& peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main) { - // Check if the requested transaction is so recent that we're just - // about to announce it to the peer; if so, they certainly shouldn't - // know we already have it. - { - LOCK(peer.m_tx_relay->cs_tx_inventory); - if (peer.m_tx_relay->setInventoryTxToSend.count(txid)) return {}; - } - auto txinfo = mempool.info(txid); if (txinfo.tx) { // If a TX could have been INVed in reply to a MEMPOOL request, From f32c408f3a0b7e597977df2bc2cdc4ae298586e5 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Sat, 6 Jun 2020 10:25:21 -0700 Subject: [PATCH 5/5] Make sure unconfirmed parents are requestable --- src/net_processing.cpp | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c8d03da6597..9a4cd7d0d5f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1694,6 +1694,17 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm int nSendFlags = (inv.type == MSG_TX ? SERIALIZE_TRANSACTION_NO_WITNESS : 0); connman->PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *tx)); mempool.RemoveUnbroadcastTx(inv.hash); + // As we're going to send tx, make sure its unconfirmed parents are made requestable. + for (const auto& txin : tx->vin) { + auto txinfo = mempool.info(txin.prevout.hash); + if (txinfo.tx && txinfo.m_time > now - UNCONDITIONAL_RELAY_DELAY) { + // Relaying a transaction with a recent but unconfirmed parent. + if (WITH_LOCK(pfrom.m_tx_relay->cs_tx_inventory, return !pfrom.m_tx_relay->filterInventoryKnown.contains(txin.prevout.hash))) { + LOCK(cs_main); + State(pfrom.GetId())->m_recently_announced_invs.insert(txin.prevout.hash); + } + } + } } else { vNotFound.push_back(inv); }