highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-18 22:35:39 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge #20187: Addrman: test-before-evict bugfix and improvements for block-relay-only peers

Browse Source 
16d9bfc417 Avoid test-before-evict evictions of current peers (Suhas Daftuar)
e8b215a086 Refactor test for existing peer connection into own function (Suhas Daftuar)
4fe338ab3e Call CAddrMan::Good() on block-relay-only peer addresses (Suhas Daftuar)
daf5553126 Avoid calling CAddrMan::Connected() on block-relay-only peer addresses (Suhas Daftuar)

Pull request description:

  This PR does two things:

  * Block-relay-only interaction with addrman.
    * Calling `CAddrMan::Connected()` on an address that was a block-relay-only peer causes the time we report in `addr` messages containing that peer to be updated; particularly now that we use anchor connections with a our block-relay-only peers, this risks leaking information about those peers.  So, stop this.
    * Avoiding calling `CAddrMan::Good()` on block-relay-only peer addresses causes the addrman logic around maintaining the new and tried table to be less good, and in particular makes it so that block-relay-only peer addresses are more likely to be evicted from the addrman (for no good reason I can think of).  So, mark those addresses as good when we connect.

  * Fix test-before-evict bug. There's a bug where if we get a collision in the tried table with an existing address that is one of our current peers, and the connection is long-lived enough, then `SelectTriedCollisions()` might return that existing peer address to us as a test-before-evict connection candidate. However, our logic for new outbound connections would later prevent us from actually making a connection; the result would be that when we get a collision with a long-lived current peer, that peer's address is likely to get evicted from the tried table.  Fix this by checking to see if a test-before-evict candidate is a peer we're currently connected to, and if so, mark it as `Good()`.

ACKs for top commit:
  sipa:
    utACK 16d9bfc417
  amitiuttarwar:
    code review ACK 16d9bfc417
  mzumsande:
    Code-Review ACK 16d9bfc417.
  jnewbery:
    utACK 16d9bfc417
  ariard:
    Code Review ACK 16d9bfc.
  jonatack:
    Tested ACK 16d9bfc417

Tree-SHA512: 188ccb814e436937cbb91d29d73c316ce83f4b9c22f1cda56747f0949a093e10161ae724e87e4a2d85ac40f85f5f6b4e87e97d350a1ac44f80c57783f4423324
This commit is contained in:
fanquake
2020-11-03 08:12:43 +08:00
parent ca18860563 16d9bfc417
commit 8387f832d6
5 changed files with 65 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

34
src/net_processing.cpp
View File

@@ -837,7 +837,8 @@ void PeerManager::ReattemptInitialBroadcast(CScheduler& scheduler) const
scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta);
}
void PeerManager::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) {
void PeerManager::FinalizeNode(const CNode& node, bool& fUpdateConnectionTime) {
NodeId nodeid = node.GetId();
fUpdateConnectionTime = false;
LOCK(cs_main);
int misbehavior{0};
@@ -854,7 +855,8 @@ void PeerManager::FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) {
if (state->fSyncStarted)
nSyncStarted--;
if (misbehavior == 0 && state->fCurrentlyConnected) {
if (misbehavior == 0 && state->fCurrentlyConnected && !node.IsBlockOnlyConn()) {
// Note: we avoid changing visible addrman state for block-relay-only peers
fUpdateConnectionTime = true;
}
@@ -2405,14 +2407,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
// empty and no one will know who we are, so these mechanisms are
// important to help us connect to the network.
//
// We also update the addrman to record connection success for
// these peers (which include OUTBOUND_FULL_RELAY and FEELER
// connections) so that addrman will have an up-to-date notion of
// which peers are online and available.
//
// We skip these operations for BLOCK_RELAY peers to avoid
// potentially leaking information about our BLOCK_RELAY
// connections via the addrman or address relay.
// We skip this for BLOCK_RELAY peers to avoid potentially leaking
// information about our BLOCK_RELAY connections via address relay.
if (fListen && !::ChainstateActive().IsInitialBlockDownload())
{
CAddress addr = GetLocalAddress(&pfrom.addr, pfrom.GetLocalServices());
@@ -2431,9 +2427,23 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
// Get recent addresses
m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make(NetMsgType::GETADDR));
pfrom.fGetAddr = true;
}
// Moves address from New to Tried table in Addrman, resolves
// tried-table collisions, etc.
if (!pfrom.IsInboundConn()) {
// For non-inbound connections, we update the addrman to record
// connection success so that addrman will have an up-to-date
// notion of which peers are online and available.
//
// While we strive to not leak information about block-relay-only
// connections via the addrman, not moving an address to the tried
// table is also potentially detrimental because new-table entries
// are subject to eviction in the event of addrman collisions. We
// mitigate the information-leak by never calling
// CAddrMan::Connected() on block-relay-only peers; see
// FinalizeNode().
//
// This moves an address from New to Tried table in Addrman,
// resolves tried-table collisions, etc.
m_connman.MarkAddressGood(pfrom.addr);
}