Skip to content

Commit

Permalink
Merge bitcoin/bitcoin#25514: net processing: Move CNode::nServices an…
Browse files Browse the repository at this point in the history 
…d CNode::nLocalServices to Peer

8d8eeb4 [net processing] Remove CNode::nLocalServices (John Newbery)
5961f8e [net] Return CService from GetLocalAddrForPeer and GetLocalAddress (dergoegge)
d9079fe [net processing] Remove CNode::nServices (John Newbery)
7d1c036 [net processing] Replace fHaveWitness with CanServeWitnesses() (John Newbery)
f65e83d [net processing] Remove fClient and m_limited_node (John Newbery)
fc5eb52 [tests] Connect peer in outbound_slow_chain_eviction by sending p2p messages (John Newbery)
1f52c47 [net processing] Add m_our_services and m_their_services to Peer (John Newbery)

Pull request description:

  Another step in #19398. Which services we offer to a peer and which services they offer to us is application layer data and should not be stored on `CNode`.

  This is also a prerequisite for adding `PeerManager` unit tests (See #25515).

ACKs for top commit:
  MarcoFalke:
    ACK 8d8eeb4 🔑
  jnewbery:
    utACK 8d8eeb4
  mzumsande:
    Code Review ACK 8d8eeb4

Tree-SHA512: e772eb2a0a85db346dd7b453a41011a12756fc7cbfda6a9ef6daa9633b9a47b9770ab3dc02377690f9d02127301c3905ff22905977f758bf90b17a9a35b37523
  • Loading branch information
MacroFake committed Jul 19, 2022
2 parents 8d4a058 + 8d8eeb4 commit 2bdce7f
Show file tree
Hide file tree
Showing 13 changed files with 199 additions and 203 deletions.
43 changes: 20 additions & 23 deletions src/net.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -206,15 +206,13 @@ static std::vector<CAddress> ConvertSeeds(const std::vector<uint8_t> &vSeedsIn)
// Otherwise, return the unroutable 0.0.0.0 but filled in with
// the normal parameters, since the IP may be changed to a useful
// one by discovery.
CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices)
CService GetLocalAddress(const CNetAddr& addrPeer)
{
CAddress ret(CService(CNetAddr(),GetListenPort()), nLocalServices);
CService ret{CNetAddr(), GetListenPort()};
CService addr;
if (GetLocal(addr, paddrPeer))
{
ret = CAddress(addr, nLocalServices);
if (GetLocal(addr, &addrPeer)) {
ret = CService{addr};
}
ret.nTime = GetAdjustedTime();
return ret;
}

Expand All @@ -233,35 +231,35 @@ bool IsPeerAddrLocalGood(CNode *pnode)
IsReachable(addrLocal.GetNetwork());
}

std::optional<CAddress> GetLocalAddrForPeer(CNode *pnode)
std::optional<CService> GetLocalAddrForPeer(CNode& node)
{
CAddress addrLocal = GetLocalAddress(&pnode->addr, pnode->GetLocalServices());
CService addrLocal{GetLocalAddress(node.addr)};
if (gArgs.GetBoolArg("-addrmantest", false)) {
// use IPv4 loopback during addrmantest
addrLocal = CAddress(CService(LookupNumeric("127.0.0.1", GetListenPort())), pnode->GetLocalServices());
addrLocal = CService(LookupNumeric("127.0.0.1", GetListenPort()));
}
// If discovery is enabled, sometimes give our peer the address it
// tells us that it sees us as in case it has a better idea of our
// address than we do.
FastRandomContext rng;
if (IsPeerAddrLocalGood(pnode) && (!addrLocal.IsRoutable() ||
if (IsPeerAddrLocalGood(&node) && (!addrLocal.IsRoutable() ||
rng.randbits((GetnScore(addrLocal) > LOCAL_MANUAL) ? 3 : 1) == 0))
{
if (pnode->IsInboundConn()) {
if (node.IsInboundConn()) {
// For inbound connections, assume both the address and the port
// as seen from the peer.
addrLocal = CAddress{pnode->GetAddrLocal(), addrLocal.nServices, addrLocal.nTime};
addrLocal = CService{node.GetAddrLocal()};
} else {
// For outbound connections, assume just the address as seen from
// the peer and leave the port in `addrLocal` as returned by
// `GetLocalAddress()` above. The peer has no way to observe our
// listening port when we have initiated the connection.
addrLocal.SetIP(pnode->GetAddrLocal());
addrLocal.SetIP(node.GetAddrLocal());
}
}
if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false))
{
LogPrint(BCLog::NET, "Advertising address %s to peer=%d\n", addrLocal.ToString(), pnode->GetId());
LogPrint(BCLog::NET, "Advertising address %s to peer=%d\n", addrLocal.ToString(), node.GetId());
return addrLocal;
}
// Address is unroutable. Don't advertise.
Expand Down Expand Up @@ -543,7 +541,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
addr_bind = GetBindAddress(*sock);
}
CNode* pnode = new CNode(id,
nLocalServices,
std::move(sock),
addrConnect,
CalculateKeyedNetGroup(addrConnect),
Expand Down Expand Up @@ -603,7 +600,6 @@ Network CNode::ConnectedThroughNetwork() const
void CNode::CopyStats(CNodeStats& stats)
{
stats.nodeid = this->GetId();
X(nServices);
X(addr);
X(addrBind);
stats.m_network = ConnectedThroughNetwork();
Expand Down Expand Up @@ -880,7 +876,7 @@ bool CConnman::AttemptToEvictConnection()
.m_min_ping_time = node->m_min_ping_time,
.m_last_block_time = node->m_last_block_time,
.m_last_tx_time = node->m_last_tx_time,
.fRelevantServices = HasAllDesirableServiceFlags(node->nServices),
.fRelevantServices = node->m_has_all_wanted_services,
.m_relay_txs = node->m_relays_txs.load(),
.fBloomFilter = node->m_bloom_filter_loaded.load(),
.nKeyedNetGroup = node->nKeyedNetGroup,
Expand Down Expand Up @@ -1014,7 +1010,6 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,

const bool inbound_onion = std::find(m_onion_binds.begin(), m_onion_binds.end(), addr_bind) != m_onion_binds.end();
CNode* pnode = new CNode(id,
nodeServices,
std::move(sock),
addr,
CalculateKeyedNetGroup(addr),
Expand All @@ -1026,7 +1021,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
pnode->AddRef();
pnode->m_permissionFlags = permissionFlags;
pnode->m_prefer_evict = discouraged;
m_msgproc->InitializeNode(pnode);
m_msgproc->InitializeNode(*pnode, nodeServices);

LogPrint(BCLog::NET, "connection from %s accepted\n", addr.ToString());

Expand Down Expand Up @@ -1964,7 +1959,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
if (grantOutbound)
grantOutbound->MoveTo(pnode->grantOutbound);

m_msgproc->InitializeNode(pnode);
m_msgproc->InitializeNode(*pnode, nLocalServices);
{
LOCK(m_nodes_mutex);
m_nodes.push_back(pnode);
Expand Down Expand Up @@ -2708,7 +2703,10 @@ ServiceFlags CConnman::GetLocalServices() const

unsigned int CConnman::GetReceiveFloodSize() const { return nReceiveFloodSize; }

CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion)
CNode::CNode(NodeId idIn, std::shared_ptr<Sock> sock, const CAddress& addrIn,
uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn,
const CAddress& addrBindIn, const std::string& addrNameIn,
ConnectionType conn_type_in, bool inbound_onion)
: m_sock{sock},
m_connected{GetTime<std::chrono::seconds>()},
addr(addrIn),
Expand All @@ -2718,8 +2716,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> s
nKeyedNetGroup(nKeyedNetGroupIn),
id(idIn),
nLocalHostNonce(nLocalHostNonceIn),
m_conn_type(conn_type_in),
nLocalServices(nLocalServicesIn)
m_conn_type(conn_type_in)
{
if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND);

Expand Down
52 changes: 15 additions & 37 deletions src/net.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ enum
};

bool IsPeerAddrLocalGood(CNode *pnode);
/** Returns a local address that we should advertise to this peer */
std::optional<CAddress> GetLocalAddrForPeer(CNode *pnode);
/** Returns a local address that we should advertise to this peer. */
std::optional<CService> GetLocalAddrForPeer(CNode& node);

/**
* Mark a network as reachable or unreachable (no automatic connects to it)
Expand All @@ -163,7 +163,7 @@ void RemoveLocal(const CService& addr);
bool SeenLocal(const CService& addr);
bool IsLocal(const CService& addr);
bool GetLocal(CService &addr, const CNetAddr *paddrPeer = nullptr);
CAddress GetLocalAddress(const CNetAddr *paddrPeer, ServiceFlags nLocalServices);
CService GetLocalAddress(const CNetAddr& addrPeer);


extern bool fDiscover;
Expand All @@ -187,7 +187,6 @@ class CNodeStats
{
public:
NodeId nodeid;
ServiceFlags nServices;
std::chrono::seconds m_last_send;
std::chrono::seconds m_last_recv;
std::chrono::seconds m_last_tx_time;
Expand Down Expand Up @@ -346,7 +345,6 @@ class CNode
std::unique_ptr<TransportSerializer> m_serializer;

NetPermissionFlags m_permissionFlags{NetPermissionFlags::None};
std::atomic<ServiceFlags> nServices{NODE_NONE};

/**
* Socket used for communication with the node.
Expand Down Expand Up @@ -399,8 +397,6 @@ class CNode
bool HasPermission(NetPermissionFlags permission) const {
return NetPermissions::HasFlag(m_permissionFlags, permission);
}
bool fClient{false}; // set by version message
bool m_limited_node{false}; //after BIP159, set by version message
/** fSuccessfullyConnected is set to true on receiving VERACK from the peer. */
std::atomic_bool fSuccessfullyConnected{false};
// Setting fDisconnect to true will cause the node to be disconnected the
Expand Down Expand Up @@ -484,6 +480,9 @@ class CNode
// Peer selected us as (compact blocks) high-bandwidth peer (BIP152)
std::atomic<bool> m_bip152_highbandwidth_from{false};

/** Whether this peer provides all services that we want. Used for eviction decisions */
std::atomic_bool m_has_all_wanted_services{false};

/** Whether we should relay transactions to this peer (their version
* message did not include fRelay=false and this is not a block-relay-only
* connection). This only changes from false to true. It will never change
Expand Down Expand Up @@ -514,7 +513,10 @@ class CNode
* criterium in CConnman::AttemptToEvictConnection. */
std::atomic<std::chrono::microseconds> m_min_ping_time{std::chrono::microseconds::max()};

CNode(NodeId id, ServiceFlags nLocalServicesIn, std::shared_ptr<Sock> sock, const CAddress& addrIn, uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn, const CAddress& addrBindIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion);
CNode(NodeId id, std::shared_ptr<Sock> sock, const CAddress& addrIn,
uint64_t nKeyedNetGroupIn, uint64_t nLocalHostNonceIn,
const CAddress& addrBindIn, const std::string& addrNameIn,
ConnectionType conn_type_in, bool inbound_onion);
CNode(const CNode&) = delete;
CNode& operator=(const CNode&) = delete;

Expand Down Expand Up @@ -572,11 +574,6 @@ class CNode

void CopyStats(CNodeStats& stats) EXCLUSIVE_LOCKS_REQUIRED(!m_subver_mutex, !m_addr_local_mutex, !cs_vSend, !cs_vRecv);

ServiceFlags GetLocalServices() const
{
return nLocalServices;
}

std::string ConnectionTypeAsString() const { return ::ConnectionTypeAsString(m_conn_type); }

/** A ping-pong round trip has completed successfully. Update latest and minimum ping times. */
Expand All @@ -591,23 +588,6 @@ class CNode
const ConnectionType m_conn_type;
std::atomic<int> m_greatest_common_version{INIT_PROTO_VERSION};

//! Services offered to this peer.
//!
//! This is supplied by the parent CConnman during peer connection
//! (CConnman::ConnectNode()) from its attribute of the same name.
//!
//! This is const because there is no protocol defined for renegotiating
//! services initially offered to a peer. The set of local services we
//! offer should not change after initialization.
//!
//! An interesting example of this is NODE_NETWORK and initial block
//! download: a node which starts up from scratch doesn't have any blocks
//! to serve, but still advertises NODE_NETWORK because it will eventually
//! fulfill this role after IBD completes. P2P code is written in such a
//! way that it can gracefully handle peers who don't make good on their
//! service advertisements.
const ServiceFlags nLocalServices;

std::list<CNetMessage> vRecvMsg; // Used only by SocketHandler thread

// Our address, as reported by the peer
Expand All @@ -625,7 +605,7 @@ class NetEventsInterface
{
public:
/** Initialize a peer (setup state, queue any initial messages) */
virtual void InitializeNode(CNode* pnode) = 0;
virtual void InitializeNode(CNode& node, ServiceFlags our_services) = 0;

/** Handle removal of a peer (clear state) */
virtual void FinalizeNode(const CNode& node) = 0;
Expand Down Expand Up @@ -1035,16 +1015,14 @@ class CConnman
std::map<uint64_t, CachedAddrResponse> m_addr_response_caches;

/**
* Services this instance offers.
* Services this node offers.
*
* This data is replicated in each CNode instance we create during peer
* connection (in ConnectNode()) under a member also called
* nLocalServices.
* This data is replicated in each Peer instance we create.
*
* This data is not marked const, but after being set it should not
* change. See the note in CNode::nLocalServices documentation.
* change.
*
* \sa CNode::nLocalServices
* \sa Peer::our_services
*/
ServiceFlags nLocalServices;

Expand Down
Loading

0 comments on commit 2bdce7f

Please sign in to comment.