diff options
author | MacroFake <falke.marco@gmail.com> | 2022-09-02 09:50:40 +0200 |
---|---|---|
committer | MacroFake <falke.marco@gmail.com> | 2022-09-02 09:50:46 +0200 |
commit | ea67232cdb80c4bc3f16fcd823f6f811fd8903e1 (patch) | |
tree | dbe54bc7f534a71c7b1c064c1e63c338dad57095 | |
parent | 7281fac2e0a41685adff024669b44c5d6586e941 (diff) | |
parent | 377e9ccda469731d535829f184b70c73ed46b6ef (diff) | |
download | bitcoin-ea67232cdb80c4bc3f16fcd823f6f811fd8903e1.tar.xz |
Merge bitcoin/bitcoin#25962: net: Add CNodeOptions and increase constness
377e9ccda469731d535829f184b70c73ed46b6ef scripted-diff: net: rename permissionFlags to permission_flags (Anthony Towns)
0a7fc428978c4db416fdcf9bf0b79de17d0558d7 net: make CNode::m_prefer_evict const (Anthony Towns)
d394156b99d6b9a99aedee78658310d169ca188d net: make CNode::m_permissionFlags const (Anthony Towns)
9dccc3328eeaf9cd66518d812c878def5d014e36 net: add CNodeOptions for optional CNode constructor params (Anthony Towns)
Pull request description:
Adds CNodeOptions to make it easier to add optional parameters to the CNode constructor, and makes prefer_evict and m_permissionFlags actually const.
ACKs for top commit:
naumenkogs:
ACK 377e9ccda469731d535829f184b70c73ed46b6ef
jonatack:
ACK 377e9ccda469731d535829f184b70c73ed46b6ef per `git range-diff 52dcb1d 2f3602b 377e9cc`
vasild:
ACK 377e9ccda469731d535829f184b70c73ed46b6ef
ryanofsky:
Code review ACK 377e9ccda469731d535829f184b70c73ed46b6ef. Looks good and feel free to ignore suggestions!
Tree-SHA512: 06fd6748770bad75ec8c966fdb73b7534c10bd61838f6f1b36b3f3d6a438e58f6a7d0edb011977e5c118ed7ea85325fac35e10dde520fef249f7a780cf500a85
-rw-r--r-- | src/net.cpp | 46 | ||||
-rw-r--r-- | src/net.h | 21 | ||||
-rw-r--r-- | src/qt/rpcconsole.cpp | 4 | ||||
-rw-r--r-- | src/rpc/net.cpp | 2 | ||||
-rw-r--r-- | src/test/denialofservice_tests.cpp | 1 | ||||
-rw-r--r-- | src/test/fuzz/util.cpp | 1 | ||||
-rw-r--r-- | src/test/fuzz/util.h | 7 | ||||
-rw-r--r-- | src/test/util/net.cpp | 2 | ||||
-rw-r--r-- | src/test/util/net.h | 1 |
9 files changed, 47 insertions, 38 deletions
diff --git a/src/net.cpp b/src/net.cpp index 535a5ce8e2..6659b64246 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -556,7 +556,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo pszDest ? pszDest : "", conn_type, /*inbound_onion=*/false, - std::move(i2p_transient_session)); + CNodeOptions{ .i2p_sam_session = std::move(i2p_transient_session) }); pnode->AddRef(); // We're making a new connection, harvest entropy from the time (and our peer count) @@ -637,7 +637,7 @@ void CNode::CopyStats(CNodeStats& stats) X(mapRecvBytesPerMsgType); X(nRecvBytes); } - X(m_permissionFlags); + X(m_permission_flags); X(m_last_ping_time); X(m_min_ping_time); @@ -936,27 +936,27 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) { const CAddress addr_bind{MaybeFlipIPv6toCJDNS(GetBindAddress(*sock)), NODE_NONE}; - NetPermissionFlags permissionFlags = NetPermissionFlags::None; - hListenSocket.AddSocketPermissionFlags(permissionFlags); + NetPermissionFlags permission_flags = NetPermissionFlags::None; + hListenSocket.AddSocketPermissionFlags(permission_flags); - CreateNodeFromAcceptedSocket(std::move(sock), permissionFlags, addr_bind, addr); + CreateNodeFromAcceptedSocket(std::move(sock), permission_flags, addr_bind, addr); } void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock, - NetPermissionFlags permissionFlags, + NetPermissionFlags permission_flags, const CAddress& addr_bind, const CAddress& addr) { int nInbound = 0; int nMaxInbound = nMaxConnections - m_max_outbound; - AddWhitelistPermissionFlags(permissionFlags, addr); - if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::Implicit)) { - NetPermissions::ClearFlag(permissionFlags, NetPermissionFlags::Implicit); - if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::ForceRelay); - if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::Relay); - NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::Mempool); - NetPermissions::AddFlag(permissionFlags, NetPermissionFlags::NoBan); + AddWhitelistPermissionFlags(permission_flags, addr); + if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::Implicit)) { + NetPermissions::ClearFlag(permission_flags, NetPermissionFlags::Implicit); + if (gArgs.GetBoolArg("-whitelistforcerelay", DEFAULT_WHITELISTFORCERELAY)) NetPermissions::AddFlag(permission_flags, NetPermissionFlags::ForceRelay); + if (gArgs.GetBoolArg("-whitelistrelay", DEFAULT_WHITELISTRELAY)) NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Relay); + NetPermissions::AddFlag(permission_flags, NetPermissionFlags::Mempool); + NetPermissions::AddFlag(permission_flags, NetPermissionFlags::NoBan); } { @@ -987,7 +987,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock, // Don't accept connections from banned peers. bool banned = m_banman && m_banman->IsBanned(addr); - if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::NoBan) && banned) + if (!NetPermissions::HasFlag(permission_flags, NetPermissionFlags::NoBan) && banned) { LogPrint(BCLog::NET, "connection from %s dropped (banned)\n", addr.ToString()); return; @@ -995,7 +995,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock, // Only accept connections from discouraged peers if our inbound slots aren't (almost) full. bool discouraged = m_banman && m_banman->IsDiscouraged(addr); - if (!NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::NoBan) && nInbound + 1 >= nMaxInbound && discouraged) + if (!NetPermissions::HasFlag(permission_flags, NetPermissionFlags::NoBan) && nInbound + 1 >= nMaxInbound && discouraged) { LogPrint(BCLog::NET, "connection from %s dropped (discouraged)\n", addr.ToString()); return; @@ -1014,7 +1014,7 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock, uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); ServiceFlags nodeServices = nLocalServices; - if (NetPermissions::HasFlag(permissionFlags, NetPermissionFlags::BloomFilter)) { + if (NetPermissions::HasFlag(permission_flags, NetPermissionFlags::BloomFilter)) { nodeServices = static_cast<ServiceFlags>(nodeServices | NODE_BLOOM); } @@ -1027,10 +1027,12 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock, addr_bind, /*addrNameIn=*/"", ConnectionType::INBOUND, - inbound_onion); + inbound_onion, + CNodeOptions{ + .permission_flags = permission_flags, + .prefer_evict = discouraged, + }); pnode->AddRef(); - pnode->m_permissionFlags = permissionFlags; - pnode->m_prefer_evict = discouraged; m_msgproc->InitializeNode(*pnode, nodeServices); LogPrint(BCLog::NET, "connection from %s accepted\n", addr.ToString()); @@ -2722,20 +2724,22 @@ CNode::CNode(NodeId idIn, const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, - std::unique_ptr<i2p::sam::Session>&& i2p_sam_session) + CNodeOptions&& node_opts) : m_deserializer{std::make_unique<V1TransportDeserializer>(V1TransportDeserializer(Params(), idIn, SER_NETWORK, INIT_PROTO_VERSION))}, m_serializer{std::make_unique<V1TransportSerializer>(V1TransportSerializer())}, + m_permission_flags{node_opts.permission_flags}, m_sock{sock}, m_connected{GetTime<std::chrono::seconds>()}, addr{addrIn}, addrBind{addrBindIn}, m_addr_name{addrNameIn.empty() ? addr.ToStringIPPort() : addrNameIn}, m_inbound_onion{inbound_onion}, + m_prefer_evict{node_opts.prefer_evict}, nKeyedNetGroup{nKeyedNetGroupIn}, id{idIn}, nLocalHostNonce{nLocalHostNonceIn}, m_conn_type{conn_type_in}, - m_i2p_sam_session{std::move(i2p_sam_session)} + m_i2p_sam_session{std::move(node_opts.i2p_sam_session)} { if (inbound_onion) assert(conn_type_in == ConnectionType::INBOUND); @@ -204,7 +204,7 @@ public: mapMsgTypeSize mapSendBytesPerMsgType; uint64_t nRecvBytes; mapMsgTypeSize mapRecvBytesPerMsgType; - NetPermissionFlags m_permissionFlags; + NetPermissionFlags m_permission_flags; std::chrono::microseconds m_last_ping_time; std::chrono::microseconds m_min_ping_time; // Our address, as reported by the peer @@ -334,6 +334,13 @@ public: void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) const override; }; +struct CNodeOptions +{ + NetPermissionFlags permission_flags = NetPermissionFlags::None; + std::unique_ptr<i2p::sam::Session> i2p_sam_session = nullptr; + bool prefer_evict = false; +}; + /** Information about a peer */ class CNode { @@ -344,7 +351,7 @@ public: const std::unique_ptr<TransportDeserializer> m_deserializer; // Used only by SocketHandler thread const std::unique_ptr<const TransportSerializer> m_serializer; - NetPermissionFlags m_permissionFlags{NetPermissionFlags::None}; // treated as const outside of fuzz tester + const NetPermissionFlags m_permission_flags; /** * Socket used for communication with the node. @@ -393,9 +400,9 @@ public: * from the wire. This cleaned string can safely be logged or displayed. */ std::string cleanSubVer GUARDED_BY(m_subver_mutex){}; - bool m_prefer_evict{false}; // This peer is preferred for eviction. (treated as const) + const bool m_prefer_evict{false}; // This peer is preferred for eviction. bool HasPermission(NetPermissionFlags permission) const { - return NetPermissions::HasFlag(m_permissionFlags, permission); + return NetPermissions::HasFlag(m_permission_flags, permission); } /** fSuccessfullyConnected is set to true on receiving VERACK from the peer. */ std::atomic_bool fSuccessfullyConnected{false}; @@ -522,7 +529,7 @@ public: const std::string& addrNameIn, ConnectionType conn_type_in, bool inbound_onion, - std::unique_ptr<i2p::sam::Session>&& i2p_sam_session = nullptr); + CNodeOptions&& node_opts = {}); CNode(const CNode&) = delete; CNode& operator=(const CNode&) = delete; @@ -890,12 +897,12 @@ private: * Create a `CNode` object from a socket that has just been accepted and add the node to * the `m_nodes` member. * @param[in] sock Connected socket to communicate with the peer. - * @param[in] permissionFlags The peer's permissions. + * @param[in] permission_flags The peer's permissions. * @param[in] addr_bind The address and port at our side of the connection. * @param[in] addr The address and port at the peer's side of the connection. */ void CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock, - NetPermissionFlags permissionFlags, + NetPermissionFlags permission_flags, const CAddress& addr_bind, const CAddress& addr); diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp index 8c0b8cc3ab..295450d6b7 100644 --- a/src/qt/rpcconsole.cpp +++ b/src/qt/rpcconsole.cpp @@ -1182,11 +1182,11 @@ void RPCConsole::updateDetailWidget() ui->peerSubversion->setText(QString::fromStdString(stats->nodeStats.cleanSubVer)); ui->peerConnectionType->setText(GUIUtil::ConnectionTypeToQString(stats->nodeStats.m_conn_type, /*prepend_direction=*/true)); ui->peerNetwork->setText(GUIUtil::NetworkToQString(stats->nodeStats.m_network)); - if (stats->nodeStats.m_permissionFlags == NetPermissionFlags::None) { + if (stats->nodeStats.m_permission_flags == NetPermissionFlags::None) { ui->peerPermissions->setText(ts.na); } else { QStringList permissions; - for (const auto& permission : NetPermissions::ToStrings(stats->nodeStats.m_permissionFlags)) { + for (const auto& permission : NetPermissions::ToStrings(stats->nodeStats.m_permission_flags)) { permissions.append(QString::fromStdString(permission)); } ui->peerPermissions->setText(permissions.join(" & ")); diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 88584ff25f..e221b3462d 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -242,7 +242,7 @@ static RPCHelpMan getpeerinfo() obj.pushKV("addr_rate_limited", statestats.m_addr_rate_limited); } UniValue permissions(UniValue::VARR); - for (const auto& permission : NetPermissions::ToStrings(stats.m_permissionFlags)) { + for (const auto& permission : NetPermissions::ToStrings(stats.m_permission_flags)) { permissions.push_back(permission); } obj.pushKV("permissions", permissions); diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 66c4605afa..7889156d51 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -68,7 +68,6 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) /*successfully_connected=*/true, /*remote_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS), /*local_services=*/ServiceFlags(NODE_NETWORK | NODE_WITNESS), - /*permission_flags=*/NetPermissionFlags::None, /*version=*/PROTOCOL_VERSION, /*relay_txs=*/true); TestOnlyResetTimeData(); diff --git a/src/test/fuzz/util.cpp b/src/test/fuzz/util.cpp index ba1a634e41..38626d4bcf 100644 --- a/src/test/fuzz/util.cpp +++ b/src/test/fuzz/util.cpp @@ -295,7 +295,6 @@ void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, /*successfully_connected=*/fuzzed_data_provider.ConsumeBool(), /*remote_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS), /*local_services=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_SERVICE_FLAGS), - /*permission_flags=*/ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS), /*version=*/fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max()), /*relay_txs=*/fuzzed_data_provider.ConsumeBool()); } diff --git a/src/test/fuzz/util.h b/src/test/fuzz/util.h index 33d9ab3cc3..6d652c922b 100644 --- a/src/test/fuzz/util.h +++ b/src/test/fuzz/util.h @@ -301,6 +301,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N const std::string addr_name = fuzzed_data_provider.ConsumeRandomLengthString(64); const ConnectionType conn_type = fuzzed_data_provider.PickValueInArray(ALL_CONNECTION_TYPES); const bool inbound_onion{conn_type == ConnectionType::INBOUND ? fuzzed_data_provider.ConsumeBool() : false}; + NetPermissionFlags permission_flags = ConsumeWeakEnum(fuzzed_data_provider, ALL_NET_PERMISSION_FLAGS); if constexpr (ReturnUniquePtr) { return std::make_unique<CNode>(node_id, sock, @@ -310,7 +311,8 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N addr_bind, addr_name, conn_type, - inbound_onion); + inbound_onion, + CNodeOptions{ .permission_flags = permission_flags }); } else { return CNode{node_id, sock, @@ -320,7 +322,8 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N addr_bind, addr_name, conn_type, - inbound_onion}; + inbound_onion, + CNodeOptions{ .permission_flags = permission_flags }}; } } inline std::unique_ptr<CNode> ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional<NodeId>& node_id_in = std::nullopt) { return ConsumeNode<true>(fdp, node_id_in); } diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp index 223db16c6c..21273ac5c1 100644 --- a/src/test/util/net.cpp +++ b/src/test/util/net.cpp @@ -17,7 +17,6 @@ void ConnmanTestMsg::Handshake(CNode& node, bool successfully_connected, ServiceFlags remote_services, ServiceFlags local_services, - NetPermissionFlags permission_flags, int32_t version, bool relay_txs) { @@ -56,7 +55,6 @@ void ConnmanTestMsg::Handshake(CNode& node, assert(peerman.GetNodeStateStats(node.GetId(), statestats)); assert(statestats.m_relay_txs == (relay_txs && !node.IsBlockOnlyConn())); assert(statestats.their_services == remote_services); - node.m_permissionFlags = permission_flags; if (successfully_connected) { CSerializedNetMsg msg_verack{mm.Make(NetMsgType::VERACK)}; (void)connman.ReceiveMsgFrom(node, msg_verack); diff --git a/src/test/util/net.h b/src/test/util/net.h index ec6b4e6e88..b339bee32a 100644 --- a/src/test/util/net.h +++ b/src/test/util/net.h @@ -43,7 +43,6 @@ struct ConnmanTestMsg : public CConnman { bool successfully_connected, ServiceFlags remote_services, ServiceFlags local_services, - NetPermissionFlags permission_flags, int32_t version, bool relay_txs); |