From 8602d8b2138a06d5db7e5b86b839f400b0a905a4 Mon Sep 17 00:00:00 2001
From: Suhas Daftuar <sdaftuar@gmail.com>
Date: Wed, 17 Apr 2019 12:46:45 -0400
Subject: [PATCH] Revert "Change in transaction pull scheduling to prevent
InvBlock-related attacks"
This reverts commit 1cff3d6cb017aea87d16cbda0768bbab256d16da.
---
src/net.cpp | 36 ++++++++
src/net.h | 10 +++
src/net_processing.cpp | 186 ++++------------------------------------
src/test/util_tests.cpp | 5 ++
src/util/time.cpp | 11 +++
src/util/time.h | 1 +
6 files changed, 81 insertions(+), 168 deletions(-)
diff --git a/src/net.cpp b/src/net.cpp
index ccab4a171..67c6a5c79 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -85,6 +85,8 @@ std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost);
static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {};
std::string strSubVersion;
+limitedmap<uint256, int64_t> mapAlreadyAskedFor(MAX_INV_SZ);
+
void CConnman::AddOneShot(const std::string& strDest)
{
LOCK(cs_vOneShots);
@@ -2648,6 +2650,40 @@ CNode::~CNode()
CloseSocket(hSocket);
}
+void CNode::AskFor(const CInv& inv)
+{
+ if (mapAskFor.size() > MAPASKFOR_MAX_SZ || setAskFor.size() > SETASKFOR_MAX_SZ)
+ return;
+ // a peer may not have multiple non-responded queue positions for a single inv item
+ if (!setAskFor.insert(inv.hash).second)
+ return;
+
+ // We're using mapAskFor as a priority queue,
+ // the key is the earliest time the request can be sent
+ int64_t nRequestTime;
+ limitedmap<uint256, int64_t>::const_iterator it = mapAlreadyAskedFor.find(inv.hash);
+ if (it != mapAlreadyAskedFor.end())
+ nRequestTime = it->second;
+ else
+ nRequestTime = 0;
+ LogPrint(BCLog::NET, "askfor %s %d (%s) peer=%d\n", inv.ToString(), nRequestTime, FormatISO8601Time(nRequestTime/1000000), id);
+
+ // Make sure not to reuse time indexes to keep things in the same order
+ int64_t nNow = GetTimeMicros() - 1000000;
+ static int64_t nLastTime;
+ ++nLastTime;
+ nNow = std::max(nNow, nLastTime);
+ nLastTime = nNow;
+
+ // Each retry is 2 minutes after the last
+ nRequestTime = std::max(nRequestTime + 2 * 60 * 1000000, nNow);
+ if (it != mapAlreadyAskedFor.end())
+ mapAlreadyAskedFor.update(it, nRequestTime);
+ else
+ mapAlreadyAskedFor.insert(std::make_pair(inv.hash, nRequestTime));
+ mapAskFor.insert(std::make_pair(nRequestTime, inv));
+}
+
bool CConnman::NodeFullyConnected(const CNode* pnode)
{
return pnode && pnode->fSuccessfullyConnected && !pnode->fDisconnect;
diff --git a/src/net.h b/src/net.h
index f1d09f593..db44ec633 100644
--- a/src/net.h
+++ b/src/net.h
@@ -67,6 +67,10 @@ static const bool DEFAULT_UPNP = USE_UPNP;
#else
static const bool DEFAULT_UPNP = false;
#endif
+/** The maximum number of entries in mapAskFor */
+static const size_t MAPASKFOR_MAX_SZ = MAX_INV_SZ;
+/** The maximum number of entries in setAskFor (larger due to getdata latency)*/
+static const size_t SETASKFOR_MAX_SZ = 2 * MAX_INV_SZ;
/** The maximum number of peer connections to maintain. */
static const unsigned int DEFAULT_MAX_PEER_CONNECTIONS = 125;
/** The default for -maxuploadtarget. 0 = Unlimited */
@@ -521,6 +525,8 @@ extern bool fDiscover;
extern bool fListen;
extern bool fRelayTxes;
+extern limitedmap<uint256, int64_t> mapAlreadyAskedFor;
+
/** Subversion as sent to the P2P network in `version` messages */
extern std::string strSubVersion;
@@ -709,6 +715,8 @@ public:
// and in the order requested.
std::vector<uint256> vInventoryBlockToSend GUARDED_BY(cs_inventory);
CCriticalSection cs_inventory;
+ std::set<uint256> setAskFor;
+ std::multimap<int64_t, CInv> mapAskFor;
int64_t nNextInvSend{0};
// Used for headers announcements - unfiltered blocks to relay
std::vector<uint256> vBlockHashesToAnnounce GUARDED_BY(cs_inventory);
@@ -857,6 +865,8 @@ public:
vBlockHashesToAnnounce.push_back(hash);
}
+ void AskFor(const CInv& inv);
+
void CloseSocketDisconnect();
void copyStats(CNodeStats &stats);
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 53ff6a52a..d524d626c 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -64,21 +64,6 @@ static constexpr int STALE_RELAY_AGE_LIMIT = 30 * 24 * 60 * 60;
/// Age after which a block is considered historical for purposes of rate
/// limiting block relay. Set to one week, denominated in seconds.
static constexpr int HISTORICAL_BLOCK_AGE = 7 * 24 * 60 * 60;
-/** Maximum number of in-flight transactions from a peer */
-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 from inbound peers */
-static constexpr int64_t INBOUND_PEER_TX_DELAY = 2 * 1000000;
-/** How long to wait (in microseconds) before downloading a transaction from an additional peer */
-static constexpr int64_t GETDATA_TX_INTERVAL = 60 * 1000000;
-/** Maximum delay (in microseconds) for transaction requests to avoid biasing some peers over others. */
-static constexpr int64_t MAX_GETDATA_RANDOM_DELAY = 2 * 1000000;
-static_assert(INBOUND_PEER_TX_DELAY >= MAX_GETDATA_RANDOM_DELAY,
-"To preserve security, MAX_GETDATA_RANDOM_DELAY should not exceed INBOUND_PEER_DELAY");
-/** Limit to avoid sending big packets. Not used in processing incoming GETDATA for compatibility */
-static const unsigned int MAX_GETDATA_SZ = 1000;
-
struct COrphanTx {
// When modifying, adapt the copy of this definition in tests/DoS_tests.
@@ -292,66 +277,6 @@ struct CNodeState {
//! Time of last new block announcement
int64_t m_last_block_announcement;
- /*
- * State associated with transaction download.
- *
- * Tx download algorithm:
- *
- * When inv comes in, queue up (process_time, txid) inside the peer's
- * CNodeState (m_tx_process_time) as long as m_tx_announced for the peer
- * isn't too big (MAX_PEER_TX_ANNOUNCEMENTS).
- *
- * The process_time for a transaction is set to nNow for outbound peers,
- * nNow + 2 seconds for inbound peers. This is the time at which we'll
- * consider trying to request the transaction from the peer in
- * SendMessages(). The delay for inbound peers is to allow outbound peers
- * a chance to announce before we request from inbound peers, to prevent
- * an adversary from using inbound connections to blind us to a
- * transaction (InvBlock).
- *
- * When we call SendMessages() for a given peer,
- * we will loop over the transactions in m_tx_process_time, looking
- * at the transactions whose process_time <= nNow. We'll request each
- * such transaction that we don't have already and that hasn't been
- * requested from another peer recently, up until we hit the
- * MAX_PEER_TX_IN_FLIGHT limit for the peer. Then we'll update
- * g_already_asked_for for each requested txid, storing the time of the
- * GETDATA request. We use g_already_asked_for to coordinate transaction
- * requests amongst our peers.
- *
- * For transactions that we still need but we have already recently
- * requested from some other peer, we'll reinsert (process_time, txid)
- * back into the peer's m_tx_process_time at the point in the future at
- * which the most recent GETDATA request would time out (ie
- * GETDATA_TX_INTERVAL + the request time stored in g_already_asked_for).
- * We add an additional delay for inbound peers, again to prefer
- * attempting download from outbound peers first.
- * We also add an extra small random delay up to 2 seconds
- * to avoid biasing some peers over others. (e.g., due to fixed ordering
- * of peer processing in ThreadMessageHandler).
- *
- * When we receive a transaction from a peer, we remove the txid from the
- * peer's m_tx_in_flight set and from their recently announced set
- * (m_tx_announced). We also clear g_already_asked_for for that entry, so
- * that if somehow the transaction is not accepted but also not added to
- * the reject filter, then we will eventually redownload from other
- * peers.
- */
- struct TxDownloadState {
- /* Track when to attempt download of announced transactions (process
- * time in micros -> txid)
- */
- std::multimap<int64_t, uint256> m_tx_process_time;
-
- //! Store all the transactions a peer has recently announced
- std::set<uint256> m_tx_announced;
-
- //! Store transactions which were requested by us
- std::set<uint256> m_tx_in_flight;
- };
-
- TxDownloadState m_tx_download;
-
CNodeState(CAddress addrIn, std::string addrNameIn) : address(addrIn), name(addrNameIn) {
fCurrentlyConnected = false;
nMisbehavior = 0;
@@ -379,9 +304,6 @@ struct CNodeState {
}
};
-// Keeps track of the time (in microseconds) when transactions were requested last time
-limitedmap<uint256, int64_t> g_already_asked_for GUARDED_BY(cs_main)(MAX_INV_SZ);
-
/** Map maintaining per-node state. */
static std::map<NodeId, CNodeState> mapNodeState GUARDED_BY(cs_main);
@@ -672,58 +594,6 @@ static void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vec
}
}
-void EraseTxRequest(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
-{
- g_already_asked_for.erase(txid);
-}
-
-int64_t GetTxRequestTime(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
-{
- auto it = g_already_asked_for.find(txid);
- if (it != g_already_asked_for.end()) {
- return it->second;
- }
- return 0;
-}
-
-void UpdateTxRequestTime(const uint256& txid, int64_t request_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
-{
- auto it = g_already_asked_for.find(txid);
- if (it == g_already_asked_for.end()) {
- g_already_asked_for.insert(std::make_pair(txid, request_time));
- } else {
- g_already_asked_for.update(it, request_time);
- }
-}
-
-
-void RequestTx(CNodeState* state, const uint256& txid, int64_t nNow) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
-{
- CNodeState::TxDownloadState& peer_download_state = state->m_tx_download;
- if (peer_download_state.m_tx_announced.size() >= MAX_PEER_TX_ANNOUNCEMENTS || peer_download_state.m_tx_announced.count(txid)) {
- // Too many queued announcements from this peer, or we already have
- // this announcement
- return;
- }
- peer_download_state.m_tx_announced.insert(txid);
-
- int64_t process_time;
- int64_t last_request_time = GetTxRequestTime(txid);
- // First time requesting this tx
- if (last_request_time == 0) {
- process_time = nNow;
- } else {
- // Randomize the delay to avoid biasing some peers over others (such as due to
- // fixed ordering of peer processing in ThreadMessageHandler)
- process_time = last_request_time + GETDATA_TX_INTERVAL + GetRand(MAX_GETDATA_RANDOM_DELAY);
- }
-
- // We delay processing announcements from non-preferred (eg inbound) peers
- if (!state->fPreferredDownload) process_time += INBOUND_PEER_TX_DELAY;
-
- peer_download_state.m_tx_process_time.emplace(process_time, txid);
-}
-
} // namespace
// This function is used for testing the stale tip eviction logic, see
@@ -2150,7 +2020,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
LOCK(cs_main);
uint32_t nFetchFlags = GetFetchFlags(pfrom);
- int64_t nNow = GetTimeMicros();
for (CInv &inv : vInv)
{
@@ -2182,7 +2051,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
if (fBlocksOnly) {
LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol peer=%d\n", inv.hash.ToString(), pfrom->GetId());
} else if (!fAlreadyHave && !fImporting && !fReindex && !IsInitialBlockDownload()) {
- RequestTx(State(pfrom->GetId()), inv.hash, nNow);
+ pfrom->AskFor(inv);
}
}
}
@@ -2415,10 +2284,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
bool fMissingInputs = false;
CValidationState state;
- 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);
+ pfrom->setAskFor.erase(inv.hash);
+ mapAlreadyAskedFor.erase(inv.hash);
std::list<CTransactionRef> lRemovedTxn;
@@ -2456,12 +2323,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
}
if (!fRejectedParents) {
uint32_t nFetchFlags = GetFetchFlags(pfrom);
- int64_t nNow = GetTimeMicros();
-
for (const CTxIn& txin : tx.vin) {
CInv _inv(MSG_TX | nFetchFlags, txin.prevout.hash);
pfrom->AddInventoryKnown(_inv);
- if (!AlreadyHave(_inv)) RequestTx(State(pfrom->GetId()), _inv.hash, nNow);
+ if (!AlreadyHave(_inv)) pfrom->AskFor(_inv);
}
AddOrphanTx(ptx, pfrom->GetId());
@@ -3899,39 +3764,24 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
//
// Message: getdata (non-blocks)
//
- auto& tx_process_time = state.m_tx_download.m_tx_process_time;
- while (!tx_process_time.empty() && tx_process_time.begin()->first <= nNow && state.m_tx_download.m_tx_in_flight.size() < MAX_PEER_TX_IN_FLIGHT) {
- const uint256& txid = tx_process_time.begin()->second;
- CInv inv(MSG_TX | GetFetchFlags(pto), txid);
- if (!AlreadyHave(inv)) {
- // If this transaction was last requested more than 1 minute ago,
- // then request.
- int64_t last_request_time = GetTxRequestTime(inv.hash);
- if (last_request_time <= nNow - GETDATA_TX_INTERVAL) {
- LogPrint(BCLog::NET, "Requesting %s peer=%d\n", inv.ToString(), pto->GetId());
- vGetData.push_back(inv);
- if (vGetData.size() >= MAX_GETDATA_SZ) {
- connman->PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));
- vGetData.clear();
- }
- UpdateTxRequestTime(inv.hash, nNow);
- state.m_tx_download.m_tx_in_flight.insert(inv.hash);
- } else {
- // This transaction is in flight from someone else; queue
- // up processing to happen after the download times out
- // (with a slight delay for inbound peers, to prefer
- // requests to outbound peers).
- RequestTx(&state, txid, nNow);
+ while (!pto->mapAskFor.empty() && (*pto->mapAskFor.begin()).first <= nNow)
+ {
+ const CInv& inv = (*pto->mapAskFor.begin()).second;
+ if (!AlreadyHave(inv))
+ {
+ LogPrint(BCLog::NET, "Requesting %s peer=%d\n", inv.ToString(), pto->GetId());
+ vGetData.push_back(inv);
+ if (vGetData.size() >= 1000)
+ {
+ connman->PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));
+ vGetData.clear();
}
} else {
- // We have already seen this transaction, no need to download.
- state.m_tx_download.m_tx_announced.erase(inv.hash);
- state.m_tx_download.m_tx_in_flight.erase(inv.hash);
+ //If we're not going to ask, don't expect a response.
+ pto->setAskFor.erase(inv.hash);
}
- tx_process_time.erase(tx_process_time.begin());
+ pto->mapAskFor.erase(pto->mapAskFor.begin());
}
-
-
if (!vGetData.empty())
connman->PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 860f64bb1..71b6ec742 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -169,6 +169,11 @@ BOOST_AUTO_TEST_CASE(util_FormatISO8601Date)
BOOST_CHECK_EQUAL(FormatISO8601Date(1317425777), "2011-09-30");
}
+BOOST_AUTO_TEST_CASE(util_FormatISO8601Time)
+{
+ BOOST_CHECK_EQUAL(FormatISO8601Time(1317425777), "23:36:17Z");
+}
+
struct TestArgsManager : public ArgsManager
{
TestArgsManager() { m_network_only_args.clear(); }
diff --git a/src/util/time.cpp b/src/util/time.cpp
index c0ede9870..83a7937d8 100644
--- a/src/util/time.cpp
+++ b/src/util/time.cpp
@@ -97,3 +97,14 @@ std::string FormatISO8601Date(int64_t nTime) {
#endif
return strprintf("%04i-%02i-%02i", ts.tm_year + 1900, ts.tm_mon + 1, ts.tm_mday);
}
+
+std::string FormatISO8601Time(int64_t nTime) {
+ struct tm ts;
+ time_t time_val = nTime;
+#ifdef _MSC_VER
+ gmtime_s(&ts, &time_val);
+#else
+ gmtime_r(&time_val, &ts);
+#endif
+ return strprintf("%02i:%02i:%02iZ", ts.tm_hour, ts.tm_min, ts.tm_sec);
+}
diff --git a/src/util/time.h b/src/util/time.h
index 68de1c156..f2e274743 100644
--- a/src/util/time.h
+++ b/src/util/time.h
@@ -33,5 +33,6 @@ void MilliSleep(int64_t n);
*/
std::string FormatISO8601DateTime(int64_t nTime);
std::string FormatISO8601Date(int64_t nTime);
+std::string FormatISO8601Time(int64_t nTime);
#endif // BITCOIN_UTIL_TIME_H