Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

randomize GETDATA(tx) request order and introduce bias toward outbound #14897

Merged
merged 1 commit into from Feb 8, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -85,8 +85,6 @@ 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);
@@ -2644,40 +2642,6 @@ 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);
This conversation was marked as resolved by naumenkogs

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 7, 2019

Member

This is the last use of FormatISO8601Time. Please remove it from src/util/time.cpp.


// 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;
@@ -67,10 +67,6 @@ 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 */
@@ -514,8 +510,6 @@ 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;

@@ -704,8 +698,6 @@ class CNode
// 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);
@@ -852,8 +844,6 @@ class CNode
vBlockHashesToAnnounce.push_back(hash);
}

void AskFor(const CInv& inv);

void CloseSocketDisconnect();

void copyStats(CNodeStats &stats);
@@ -64,6 +64,21 @@ 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.
@@ -274,6 +289,66 @@ 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.
*/
This conversation was marked as resolved by naumenkogs

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jan 30, 2019

Member

nit: This comment should be updated to refer to the new variables (eg mapAlreadyAskedFor was renamed), new constants (1 second is now incorrect), and the randomization behavior/delay change.

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;
@@ -301,6 +376,9 @@ 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);
This conversation was marked as resolved by naumenkogs

This comment has been minimized.

Copy link
@jamesob

jamesob Feb 4, 2019

Member

Presumably the MAX_PEER_TX_ANNOUNCEMENTS limitation prevents any peer from triggering unnecessary evictions by overrunning this map with junk announcements.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Feb 4, 2019

Member

I think the limit on the number of in-flight requests to a peer is what prevents this from being taken over.

This comment has been minimized.

Copy link
@naumenkogs

naumenkogs Feb 4, 2019

Author Contributor

Right, and you need as many as 500 sybil connections to overrun this map :)

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Feb 4, 2019

Member

I forgot to add that the downside to this map being overrun is that we'll just request a transaction sooner than we otherwise would have (in the event that it was removed due to this data structure filling up). So if a zillion peers manage to take over this data structure, they're just wasting our bandwidth a bit -- which they can already do.

This comment has been minimized.

Copy link
@jamesob

jamesob Feb 4, 2019

Member

I think the limit on the number of in-flight requests to a peer is what prevents this from being taken over.

Ah right, per the third clause in this while condition: https://github.com/bitcoin/bitcoin/pull/14897/files#diff-eff7adeaec73a769788bb78858815c91R3864


/** Map maintaining per-node state. */
static std::map<NodeId, CNodeState> mapNodeState GUARDED_BY(cs_main);

@@ -591,6 +669,58 @@ 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);
This conversation was marked as resolved by sipa

This comment has been minimized.

Copy link
@sipa

sipa Feb 4, 2019

Member

If g_already_asked_for turns into a normal map, this whole function can become g_already_asked_for[txid] = request_time;.

This comment has been minimized.

Copy link
@naumenkogs

naumenkogs Feb 5, 2019

Author Contributor

To be clear: are you suggesting switching to normal map and relying on the fact that we are not gonna have 500 sybils (so that it results in the same limiting properties due to checks discussed here?

This comment has been minimized.

Copy link
@sipa

sipa Feb 6, 2019

Member

No, I'm suggesting that if we'd determine that switching to a normal map is safe, this function can be simplified.

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);

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 11, 2019

Member

style-nit: Could make this if (!peer_download_state.m_tx_announced.insert(txid).second) return; and remove the count() above to avoid two lookups. (This will be in line with the previous code in AskFor)


This conversation was marked as resolved by naumenkogs

This comment has been minimized.

Copy link
@jamesob

jamesob Feb 4, 2019

Member

Is it worth a belt-and-suspenders early return here if txid is present in peer_download_state.m_tx_announced or .m_in_flight, a la the old AskFor() definition? Or is that change in behavior intentional?

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Feb 4, 2019

Member

I believe the || peer_download_state.m_tx_announced.count(txid)) { captures the check you're asking about? I don't think this should be a change in behavior compared with AskFor().

Because we check against m_tx_announced, I don't think we also need to check with what's in flight.

This comment has been minimized.

Copy link
@jamesob

jamesob Feb 4, 2019

Member

D'oh, yep you're totally right.

int64_t process_time;
int64_t last_request_time = GetTxRequestTime(txid);

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 11, 2019

Member

style-nit: Could be const

// 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);
}
This conversation was marked as resolved by naumenkogs

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 5, 2019

Member

In this function you are checking against the size of m_tx_announced to prevent DoS. However, a remote peer can control whether entries are added to m_tx_announced or not (see the if condition before peer_download_state.m_tx_announced.insert)

So you can eat all memory by slowly filling m_tx_process_time without touching the other tx download state and thus avoid the DoS protection.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Feb 5, 2019

Member

Good catch! Looks like it's just a mistake that we are only adding to m_tx_announced if last_request_time == 0, rather than always.


} // namespace

// This function is used for testing the stale tip eviction logic, see
@@ -1945,6 +2075,7 @@ 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)
{
@@ -1976,7 +2107,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()) {
pfrom->AskFor(inv);
RequestTx(State(pfrom->GetId()), inv.hash, nNow);
}
}
}
@@ -2211,8 +2342,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
bool fMissingInputs = false;
CValidationState state;

pfrom->setAskFor.erase(inv.hash);
mapAlreadyAskedFor.erase(inv.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;

@@ -2303,10 +2436,12 @@ 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)) pfrom->AskFor(_inv);
if (!AlreadyHave(_inv)) RequestTx(State(pfrom->GetId()), _inv.hash, nNow);
}
AddOrphanTx(ptx, pfrom->GetId());

@@ -3731,24 +3866,39 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
//
// Message: getdata (non-blocks)
//
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();
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) {

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Feb 11, 2019

Member

style-nit: Could inline GetTxRequestTime here to avoid the named symbol last_request_time.

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);
}
} else {
//If we're not going to ask, don't expect a response.
pto->setAskFor.erase(inv.hash);
// 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);
}
pto->mapAskFor.erase(pto->mapAskFor.begin());
tx_process_time.erase(tx_process_time.begin());
}


if (!vGetData.empty())
connman->PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));

@@ -169,11 +169,6 @@ 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(); }
@@ -97,14 +97,3 @@ 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);
}
@@ -33,6 +33,5 @@ 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
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.