From 51ced007f43b862e842908a4d6d6601ea12549bc Mon Sep 17 00:00:00 2001 From: abitmore Date: Thu, 15 Sep 2022 01:05:12 +0000 Subject: [PATCH 1/9] Verify inbound endpoints as soon as possible --- .../include/graphene/net/peer_connection.hpp | 4 ++- libraries/net/node.cpp | 32 +++++++++++++++++-- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/libraries/net/include/graphene/net/peer_connection.hpp b/libraries/net/include/graphene/net/peer_connection.hpp index 144878bb6f..25bd9a89a0 100644 --- a/libraries/net/include/graphene/net/peer_connection.hpp +++ b/libraries/net/include/graphene/net/peer_connection.hpp @@ -195,12 +195,14 @@ namespace graphene { namespace net fc::ip::address inbound_address; uint16_t inbound_port = 0; uint16_t outbound_port = 0; - /// The inbound endpoint of the remote peer + /// The inbound endpoint of the remote peer (our best guess) fc::optional remote_inbound_endpoint; /// Whether the inbound endpoint of the remote peer is verified bool inbound_endpoint_verified = false; /// Some nodes may be listening on multiple endpoints fc::flat_set additional_inbound_endpoints; + /// Potential inbound endpoints of the peer + fc::flat_map potential_inbound_endpoints; /// @} using item_to_time_map_type = std::unordered_map; diff --git a/libraries/net/node.cpp b/libraries/net/node.cpp index 9dedaf50f2..e24e7f4c41 100644 --- a/libraries/net/node.cpp +++ b/libraries/net/node.cpp @@ -1376,6 +1376,9 @@ namespace graphene { namespace net { namespace detail { ("endpoint", originating_peer->get_remote_endpoint())); // Gatekeeping code if( originating_peer->we_have_requested_close + // allow hello_message so we can learn more about the peer + && received_message.msg_type.value() != core_message_type_enum::hello_message_type + // allow closing_connection_message so we can finish disconnecting && received_message.msg_type.value() != core_message_type_enum::closing_connection_message_type ) { dlog( "Unexpected message from peer ${peer} while we have requested to close connection", @@ -1751,9 +1754,14 @@ namespace graphene { namespace net { namespace detail { auto old_inbound_endpoint = already_connected_peer->get_endpoint_for_connecting(); auto new_inbound_endpoint = originating_peer->get_remote_endpoint(); already_connected_peer->additional_inbound_endpoints.insert( *new_inbound_endpoint ); - if ( !already_connected_peer->inbound_endpoint_verified // which implies direction == inbound - || new_inbound_endpoint->get_address().is_public_address() - || !old_inbound_endpoint->get_address().is_public_address() ) + if( peer_connection_direction::inbound == already_connected_peer->direction ) + { + already_connected_peer->potential_inbound_endpoints[*new_inbound_endpoint] + = firewalled_state::not_firewalled; + } + if( !already_connected_peer->inbound_endpoint_verified // which implies direction == inbound + || new_inbound_endpoint->get_address().is_public_address() + || !old_inbound_endpoint->get_address().is_public_address() ) { ilog( "Saving verification result ${ep} for peer ${peer} with id ${id}", ("ep", new_inbound_endpoint) @@ -1836,6 +1844,8 @@ namespace graphene { namespace net { namespace detail { auto updated_peer_record = _potential_peer_db.lookup_or_create_entry_for_ep( ep ); updated_peer_record.last_seen_time = fc::time_point::now(); _potential_peer_db.update_entry( updated_peer_record ); + // mark as a potential inbound address + originating_peer->potential_inbound_endpoints[ep] = firewalled_state::unknown; } // Note: we don't update originating_peer->is_firewalled, because we might guess wrong @@ -3666,6 +3676,12 @@ namespace graphene { namespace net { namespace detail { _last_reported_number_of_conns = (uint32_t)_active_connections.size(); _delegate->connection_count_changed( _last_reported_number_of_conns ); } + // If it is an inbound connection, try to verify its inbound endpoint + if( peer_connection_direction::inbound == peer->direction ) + { + for( const auto& potential_inbound_endpoint : peer->potential_inbound_endpoints ) + _add_once_node_list.push_back( potential_peer_record( potential_inbound_endpoint.first ) ); + } } void node_impl::close() @@ -4129,6 +4145,10 @@ namespace graphene { namespace net { namespace detail { updated_peer_record.last_error = *connect_failed_exception; _potential_peer_db.update_entry(updated_peer_record); + // If this is for inbound endpoint verification, + // here we could try to find the original connection and update its firewalled state, + // but it doesn't seem necessary. + // if the connection failed, we want to disconnect now. _handshaking_connections.erase(new_peer); _terminating_connections.erase(new_peer); @@ -4666,6 +4686,12 @@ namespace graphene { namespace net { namespace detail { // the peer has already told us that it's ready to close the connection, so just close the connection peer_to_disconnect->close_connection(); } + else if( peer_to_disconnect->we_have_requested_close ) + { + dlog( "Disconnecting again from ${peer} for ${reason}, ignore", + ("peer",peer_to_disconnect->get_remote_endpoint()) ("reason",reason_for_disconnect)); + return; + } else { // we're the first to try to want to close the connection From cce56506d6cced597046231870f92aedeb2cff97 Mon Sep 17 00:00:00 2001 From: abitmore Date: Thu, 15 Sep 2022 01:39:49 +0000 Subject: [PATCH 2/9] Fix a code smell --- libraries/net/node.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/net/node.cpp b/libraries/net/node.cpp index e24e7f4c41..31c754c251 100644 --- a/libraries/net/node.cpp +++ b/libraries/net/node.cpp @@ -3680,7 +3680,7 @@ namespace graphene { namespace net { namespace detail { if( peer_connection_direction::inbound == peer->direction ) { for( const auto& potential_inbound_endpoint : peer->potential_inbound_endpoints ) - _add_once_node_list.push_back( potential_peer_record( potential_inbound_endpoint.first ) ); + _add_once_node_list.emplace_back( potential_inbound_endpoint.first ); } } From d331b4a1e1d762472b56e12b51d8884a63373505 Mon Sep 17 00:00:00 2001 From: abitmore Date: Thu, 15 Sep 2022 15:46:25 +0000 Subject: [PATCH 3/9] Review inbound peer's inbound endpoint --- libraries/net/node.cpp | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/libraries/net/node.cpp b/libraries/net/node.cpp index 31c754c251..5ecc9eb374 100644 --- a/libraries/net/node.cpp +++ b/libraries/net/node.cpp @@ -2089,7 +2089,22 @@ namespace graphene { namespace net { namespace detail { } } // else if this was an active connection, then this was just a reply to our periodic address requests. - // we've processed it, there's nothing else to do + // we've processed it. + // Now seems like a good time to review the peer's inbound endpoint and firewalled state + else if( !originating_peer->inbound_endpoint_verified // which implies direction == inbound + && originating_peer->inbound_port != 0 // Ignore if the peer is not listening + // We try not to update it a 2nd time + && originating_peer->get_remote_endpoint() != originating_peer->get_endpoint_for_connecting() ) + { + // Our best guess for the peer's inbound endpoint now is its remote endpoint, + // unless we are behind a reverse proxy, in which case we try to use a public address + if( originating_peer->get_remote_endpoint()->get_address().is_public_address() + || !originating_peer->get_endpoint_for_connecting()->get_address().is_public_address() ) + originating_peer->remote_inbound_endpoint = originating_peer->get_remote_endpoint(); + // else do nothing + + // We could reinitialize inbound endpoint verification here, but it doesn't seem necessary + } } void node_impl::on_fetch_blockchain_item_ids_message(peer_connection* originating_peer, From 0090b1fe341ee8c0778423f1e6b090cd09897109 Mon Sep 17 00:00:00 2001 From: abitmore Date: Thu, 15 Sep 2022 16:19:33 +0000 Subject: [PATCH 4/9] Fix a code smell --- libraries/net/node.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libraries/net/node.cpp b/libraries/net/node.cpp index 5ecc9eb374..f1ff9177de 100644 --- a/libraries/net/node.cpp +++ b/libraries/net/node.cpp @@ -2098,9 +2098,10 @@ namespace graphene { namespace net { namespace detail { { // Our best guess for the peer's inbound endpoint now is its remote endpoint, // unless we are behind a reverse proxy, in which case we try to use a public address - if( originating_peer->get_remote_endpoint()->get_address().is_public_address() + auto remote_endpoint = originating_peer->get_remote_endpoint(); // Note: this returns a copy + if( remote_endpoint->get_address().is_public_address() || !originating_peer->get_endpoint_for_connecting()->get_address().is_public_address() ) - originating_peer->remote_inbound_endpoint = originating_peer->get_remote_endpoint(); + originating_peer->remote_inbound_endpoint = remote_endpoint; // else do nothing // We could reinitialize inbound endpoint verification here, but it doesn't seem necessary From 326d44eafbb8fee108ebd997f7bf97dd6ce03493 Mon Sep 17 00:00:00 2001 From: abitmore Date: Thu, 15 Sep 2022 20:02:17 +0000 Subject: [PATCH 5/9] Guess inbound endpoint early to avoid revisiting --- libraries/net/node.cpp | 120 ++++++++++++++++++++--------------------- 1 file changed, 57 insertions(+), 63 deletions(-) diff --git a/libraries/net/node.cpp b/libraries/net/node.cpp index f1ff9177de..fae0cdec48 100644 --- a/libraries/net/node.cpp +++ b/libraries/net/node.cpp @@ -1513,6 +1513,7 @@ namespace graphene { namespace net { namespace detail { void node_impl::on_hello_message( peer_connection* originating_peer, const hello_message& hello_message_received ) { VERIFY_CORRECT_THREAD(); + auto remote_endpoint = originating_peer->get_remote_endpoint(); // Note: this returns a copy // Do gatekeeping first if( originating_peer->their_state != peer_connection::their_connection_state::just_connected ) { @@ -1528,7 +1529,7 @@ namespace graphene { namespace net { namespace detail { // probably need to think through that case. We're not attempting that // yet, though, so it's ok to just disconnect here. wlog( "Unexpected hello_message from peer ${peer}, disconnecting", - ("peer", originating_peer->get_remote_endpoint()) ); + ("peer", remote_endpoint) ); disconnect_from_peer( originating_peer, "Received an unexpected hello_message" ); return; } @@ -1537,7 +1538,7 @@ namespace graphene { namespace net { namespace detail { if( hello_message_received.chain_id != _chain_id ) { wlog( "Received hello message from peer ${peer} on a different chain: ${message}", - ("peer", originating_peer->get_remote_endpoint()) + ("peer", remote_endpoint) ("message", hello_message_received) ); // If it is an outbound connection, make sure we won't reconnect to the peer soon if( peer_connection_direction::outbound == originating_peer->direction ) @@ -1545,14 +1546,14 @@ namespace graphene { namespace net { namespace detail { // Note: deleting is not the best approach since it can be readded soon and we will reconnect soon. // Marking it "permanently rejected" is also not good enough since the peer can be "fixed". // It seems the best approach is to reduce its weight significantly. - greatly_delay_next_conn_to( this, *originating_peer->get_remote_endpoint() ); + greatly_delay_next_conn_to( this, *remote_endpoint ); } // Now reject std::ostringstream rejection_message; rejection_message << "You're on a different chain than I am. I'm on " << _chain_id.str() << " and you're on " << hello_message_received.chain_id.str(); connection_rejected_message connection_rejected( _user_agent_string, core_protocol_version, - *originating_peer->get_remote_endpoint(), + *remote_endpoint, rejection_reason_code::different_chain, rejection_message.str() ); originating_peer->their_state = peer_connection::their_connection_state::connection_rejected; @@ -1578,16 +1579,16 @@ namespace graphene { namespace net { namespace detail { catch( const fc::exception& e ) { wlog( "Error when validating signature in hello message from peer ${peer}: ${e}", - ("peer", originating_peer->get_remote_endpoint())("e", e.to_detail_string()) ); + ("peer", remote_endpoint)("e", e.to_detail_string()) ); } if( !expected_node_public_key || hello_message_received.node_public_key != expected_node_public_key->serialize() ) { wlog( "Invalid signature in hello message from peer ${peer}", - ("peer", originating_peer->get_remote_endpoint()) ); + ("peer", remote_endpoint) ); connection_rejected_message connection_rejected( _user_agent_string, core_protocol_version, - *originating_peer->get_remote_endpoint(), + *remote_endpoint, rejection_reason_code::invalid_hello_message, "Invalid signature in hello message" ); originating_peer->their_state = peer_connection::their_connection_state::connection_rejected; @@ -1607,14 +1608,14 @@ namespace graphene { namespace net { namespace detail { { // either it's not there or it's not a valid session id. either way, ignore. dlog( "Peer ${endpoint} sent us a hello message without a valid node_id in user_data", - ("endpoint", originating_peer->get_remote_endpoint() ) ); + ("endpoint", remote_endpoint ) ); } // The peer's node_id should not be null static const node_id_t null_node_id; if( null_node_id == peer_node_id ) { wlog( "The node_id in the hello_message from peer ${peer} is null, disconnecting", - ("peer", originating_peer->get_remote_endpoint()) ); + ("peer", remote_endpoint) ); disconnect_from_peer( originating_peer, "Your node_id in the hello_message is null" ); return; } @@ -1623,7 +1624,7 @@ namespace graphene { namespace net { namespace detail { { ilog( "Received a hello_message from peer ${peer} with id ${id} that is myself or claimed to be myself, " "rejection", - ("peer", originating_peer->get_remote_endpoint()) + ("peer", remote_endpoint) ("id", peer_node_id) ); // If it is an outbound connection, make sure we won't reconnect to the peer soon if( peer_connection_direction::outbound == originating_peer->direction ) @@ -1631,13 +1632,13 @@ namespace graphene { namespace net { namespace detail { // Note: deleting is not the best approach since it can be readded soon and we will reconnect soon. // Marking it "permanently rejected" is also not good enough since the peer can be "fixed". // It seems the best approach is to reduce its weight significantly. - greatly_delay_next_conn_to( this, *originating_peer->get_remote_endpoint() ); + greatly_delay_next_conn_to( this, *remote_endpoint ); } // Now reject // Note: this can happen in rare cases if the peer is not actually myself but another node. // Anyway, we see it as ourselves, reject it and disconnect it. connection_rejected_message connection_rejected( _user_agent_string, core_protocol_version, - *originating_peer->get_remote_endpoint(), + *remote_endpoint, rejection_reason_code::connected_to_self, "I'm connecting to myself" ); originating_peer->their_state = peer_connection::their_connection_state::connection_rejected; @@ -1651,14 +1652,11 @@ namespace graphene { namespace net { namespace detail { // store off the data provided in the hello message originating_peer->user_agent = hello_message_received.user_agent; originating_peer->node_public_key = hello_message_received.node_public_key; - // will probably be overwritten in parse_hello_user_data_for_peer() - originating_peer->node_id = hello_message_received.node_public_key; originating_peer->core_protocol_version = hello_message_received.core_protocol_version; originating_peer->inbound_address = hello_message_received.inbound_address; originating_peer->inbound_port = hello_message_received.inbound_port; originating_peer->outbound_port = hello_message_received.outbound_port; - - parse_hello_user_data_for_peer(originating_peer, hello_message_received.user_data); + // Note: more data is stored after initialized remote_inbound_endpoint // For an outbound connection, we know the remote_inbound_endpoint already, so keep it unchanged. // For an inbound connection, we initialize it here. @@ -1679,12 +1677,23 @@ namespace graphene { namespace net { namespace detail { // In addition, by now, our list or exclude list for peer advertisement only contains IP endpoints but not // nodes' public keys (we can't use node_id because it changes every time the node restarts). Using a valid // address is better for the purpose. - originating_peer->remote_inbound_endpoint - = fc::ip::endpoint( originating_peer->inbound_port != 0 ? originating_peer->inbound_address - : originating_peer->get_remote_endpoint()->get_address(), - originating_peer->inbound_port ); + if( originating_peer->inbound_port == 0 ) + originating_peer->remote_inbound_endpoint = fc::ip::endpoint( remote_endpoint->get_address() ); + else if( originating_peer->inbound_address.is_public_address() + || originating_peer->inbound_address == remote_endpoint->get_address() ) + originating_peer->remote_inbound_endpoint = fc::ip::endpoint( originating_peer->inbound_address, + originating_peer->inbound_port ); + else + originating_peer->remote_inbound_endpoint = remote_endpoint; } + // Note: store node_id after initialized remote_inbound_endpoint to avoid a race condition + + // will probably be overwritten in parse_hello_user_data_for_peer() + originating_peer->node_id = hello_message_received.node_public_key; + + parse_hello_user_data_for_peer(originating_peer, hello_message_received.user_data); + // if they didn't provide a last known fork, try to guess it if (originating_peer->last_known_fork_block_number == 0 && originating_peer->graphene_git_revision_unix_timestamp) @@ -1712,7 +1721,7 @@ namespace graphene { namespace net { namespace detail { std::ostringstream rejection_message; rejection_message << "Your client is outdated -- you can only understand blocks up to #" << next_fork_block_number << ", but I'm already on block #" << head_block_num; connection_rejected_message connection_rejected(_user_agent_string, core_protocol_version, - *originating_peer->get_remote_endpoint(), + *remote_endpoint, rejection_reason_code::unspecified, rejection_message.str() ); @@ -1734,9 +1743,10 @@ namespace graphene { namespace net { namespace detail { if( peer_connection_direction::outbound == originating_peer->direction && originating_peer->node_public_key == already_connected_peer->node_public_key ) { + auto already_connected_endpoint = already_connected_peer->get_remote_endpoint(); // This returns a copy ilog( "Verified that endpoint ${ep} is reachable and belongs to peer ${peer} with id ${id}", - ("ep", originating_peer->get_remote_endpoint()) - ("peer", already_connected_peer->get_remote_endpoint()) + ("ep", remote_endpoint) + ("peer", already_connected_endpoint) ("id", already_connected_peer->node_id) ); // Do not replace a verified public address with a private or local address. // Note: there is a scenario that some nodes in the same local network may have connected to each other, @@ -1751,40 +1761,38 @@ namespace graphene { namespace net { namespace detail { // that they are in the same local network and connected to each other. // On the other hand, when we skip updates in some cases, we may end up trying to reconnect soon // and endlessly (which is addressed with additional_inbound_endpoints). - auto old_inbound_endpoint = already_connected_peer->get_endpoint_for_connecting(); - auto new_inbound_endpoint = originating_peer->get_remote_endpoint(); - already_connected_peer->additional_inbound_endpoints.insert( *new_inbound_endpoint ); + already_connected_peer->additional_inbound_endpoints.insert( *remote_endpoint ); if( peer_connection_direction::inbound == already_connected_peer->direction ) { - already_connected_peer->potential_inbound_endpoints[*new_inbound_endpoint] + already_connected_peer->potential_inbound_endpoints[*remote_endpoint] = firewalled_state::not_firewalled; } if( !already_connected_peer->inbound_endpoint_verified // which implies direction == inbound - || new_inbound_endpoint->get_address().is_public_address() - || !old_inbound_endpoint->get_address().is_public_address() ) + || remote_endpoint->get_address().is_public_address() + || !already_connected_peer->get_endpoint_for_connecting()->get_address().is_public_address() ) { ilog( "Saving verification result ${ep} for peer ${peer} with id ${id}", - ("ep", new_inbound_endpoint) - ("peer", already_connected_peer->get_remote_endpoint()) + ("ep", remote_endpoint) + ("peer", already_connected_endpoint) ("id", already_connected_peer->node_id) ); - already_connected_peer->remote_inbound_endpoint = new_inbound_endpoint; + already_connected_peer->remote_inbound_endpoint = remote_endpoint; already_connected_peer->inbound_endpoint_verified = true; already_connected_peer->is_firewalled = firewalled_state::not_firewalled; } // If the already connected peer is in the active connections list, save the endpoint to the peer db if( peer_connection::connection_negotiation_status::negotiation_complete == already_connected_peer->negotiation_status ) - save_successful_address( this, *new_inbound_endpoint ); + save_successful_address( this, *remote_endpoint ); } // Now reject connection_rejected_message connection_rejected( _user_agent_string, core_protocol_version, - *originating_peer->get_remote_endpoint(), + *remote_endpoint, rejection_reason_code::already_connected, "I'm already connected to you" ); originating_peer->their_state = peer_connection::their_connection_state::connection_rejected; originating_peer->send_message( message(connection_rejected) ); ilog("Received a hello_message from peer ${peer} that I'm already connected to (with id ${id}), rejection", - ("peer", originating_peer->get_remote_endpoint()) + ("peer", remote_endpoint) ("id", originating_peer->node_id)); // If already connected, we disconnect disconnect_from_peer( originating_peer, connection_rejected.reason_string ); @@ -1794,12 +1802,13 @@ namespace graphene { namespace net { namespace detail { _allowed_peers.find(originating_peer->node_id) == _allowed_peers.end()) { connection_rejected_message connection_rejected(_user_agent_string, core_protocol_version, - *originating_peer->get_remote_endpoint(), + *remote_endpoint, rejection_reason_code::blocked, "you are not in my allowed_peers list"); originating_peer->their_state = peer_connection::their_connection_state::connection_rejected; originating_peer->send_message( message(connection_rejected ) ); - dlog( "Received a hello_message from peer ${peer} who isn't in my allowed_peers list, rejection", ("peer", originating_peer->get_remote_endpoint() ) ); + dlog( "Received a hello_message from peer ${peer} who isn't in my allowed_peers list, rejection", + ("peer", remote_endpoint ) ); } #endif // ENABLE_P2P_DEBUGGING_API else @@ -1814,7 +1823,7 @@ namespace graphene { namespace net { namespace detail { else if( 0 == originating_peer->inbound_port ) { ilog( "peer ${peer} did not give an inbound port so I'm treating them as if they are firewalled.", - ("peer", originating_peer->get_remote_endpoint()) ); + ("peer", remote_endpoint) ); originating_peer->is_firewalled = firewalled_state::firewalled; } else @@ -1823,20 +1832,20 @@ namespace graphene { namespace net { namespace detail { // First, we add the inbound endpoint that the peer told us it is listening on. fc::flat_set endpoints_to_save; - endpoints_to_save.insert( *originating_peer->get_endpoint_for_connecting() ); + endpoints_to_save.insert( fc::ip::endpoint( originating_peer->inbound_address, + originating_peer->inbound_port ) ); // Second, we add the address and port we see. // It might be the same as above, but that's OK. - fc::ip::endpoint peers_actual_outbound_endpoint = *originating_peer->get_remote_endpoint(); - endpoints_to_save.insert( peers_actual_outbound_endpoint ); + endpoints_to_save.insert( *remote_endpoint ); // Third, we add the address we see, with the inbound port the peer told us. // It might be the same as above, but that's OK. - endpoints_to_save.insert( fc::ip::endpoint( peers_actual_outbound_endpoint.get_address(), + endpoints_to_save.insert( fc::ip::endpoint( remote_endpoint->get_address(), originating_peer->inbound_port ) ); ilog( "Saving potential endpoints to the peer database for peer ${peer}: ${endpoints}", - ("peer", originating_peer->get_remote_endpoint()) ("endpoints", endpoints_to_save) ); + ("peer", remote_endpoint) ("endpoints", endpoints_to_save) ); for( const auto& ep : endpoints_to_save ) { @@ -1855,20 +1864,20 @@ namespace graphene { namespace net { namespace detail { if (!is_accepting_new_connections()) { connection_rejected_message connection_rejected(_user_agent_string, core_protocol_version, - *originating_peer->get_remote_endpoint(), + *remote_endpoint, rejection_reason_code::not_accepting_connections, "not accepting any more incoming connections"); originating_peer->their_state = peer_connection::their_connection_state::connection_rejected; originating_peer->send_message(message(connection_rejected)); ilog("Received a hello_message from peer ${peer}, but I'm not accepting any more connections, rejection", - ("peer", originating_peer->get_remote_endpoint())); + ("peer", remote_endpoint)); } else { originating_peer->their_state = peer_connection::their_connection_state::connection_accepted; originating_peer->send_message(message(connection_accepted_message())); ilog("Received a hello_message from peer ${peer}, sending reply to accept connection", - ("peer", originating_peer->get_remote_endpoint())); + ("peer", remote_endpoint)); } } } @@ -2089,23 +2098,8 @@ namespace graphene { namespace net { namespace detail { } } // else if this was an active connection, then this was just a reply to our periodic address requests. - // we've processed it. - // Now seems like a good time to review the peer's inbound endpoint and firewalled state - else if( !originating_peer->inbound_endpoint_verified // which implies direction == inbound - && originating_peer->inbound_port != 0 // Ignore if the peer is not listening - // We try not to update it a 2nd time - && originating_peer->get_remote_endpoint() != originating_peer->get_endpoint_for_connecting() ) - { - // Our best guess for the peer's inbound endpoint now is its remote endpoint, - // unless we are behind a reverse proxy, in which case we try to use a public address - auto remote_endpoint = originating_peer->get_remote_endpoint(); // Note: this returns a copy - if( remote_endpoint->get_address().is_public_address() - || !originating_peer->get_endpoint_for_connecting()->get_address().is_public_address() ) - originating_peer->remote_inbound_endpoint = remote_endpoint; - // else do nothing - - // We could reinitialize inbound endpoint verification here, but it doesn't seem necessary - } + // we've processed it, there's nothing else to do + // Note: we could reinitialize inbound endpoint verification here, but it doesn't seem necessary } void node_impl::on_fetch_blockchain_item_ids_message(peer_connection* originating_peer, From f7023e5e42f0990263f121ee16399e3237d99cc3 Mon Sep 17 00:00:00 2001 From: abitmore Date: Thu, 15 Sep 2022 20:26:22 +0000 Subject: [PATCH 6/9] Simplify code --- libraries/net/node.cpp | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/libraries/net/node.cpp b/libraries/net/node.cpp index fae0cdec48..39c1502baf 100644 --- a/libraries/net/node.cpp +++ b/libraries/net/node.cpp @@ -4549,15 +4549,11 @@ namespace graphene { namespace net { namespace detail { if( peer_connection_direction::outbound == active_peer->direction && endpoint_for_this_peer && *endpoint_for_this_peer == remote_endpoint ) return active_peer; - if( peer_connection_direction::inbound == active_peer->direction - && active_peer->inbound_endpoint_verified // which implies get_endpoint_for_connecting().valid() - && *active_peer->get_endpoint_for_connecting() == remote_endpoint ) + // Note: if it is an inbound connection and its inbound endpoint is verified already, + // the inbound endpoint should be in additional_inbound_endpoints + if( active_peer->additional_inbound_endpoints.find( remote_endpoint ) + != active_peer->additional_inbound_endpoints.end() ) return active_peer; - for( const auto& ep : active_peer->additional_inbound_endpoints ) - { - if( ep == remote_endpoint ) - return active_peer; - } } return peer_connection_ptr(); } @@ -4577,15 +4573,11 @@ namespace graphene { namespace net { namespace detail { fc::optional endpoint_for_this_peer( handshaking_peer->get_remote_endpoint() ); if( endpoint_for_this_peer && *endpoint_for_this_peer == remote_endpoint ) return handshaking_peer; - if( peer_connection_direction::inbound == handshaking_peer->direction - && handshaking_peer->inbound_endpoint_verified // which implies get_endpoint_for_connecting().valid() - && *handshaking_peer->get_endpoint_for_connecting() == remote_endpoint ) + // Note: if it is an inbound connection and its inbound endpoint is verified already, + // the inbound endpoint should be in additional_inbound_endpoints + if( handshaking_peer->additional_inbound_endpoints.find( remote_endpoint ) + != handshaking_peer->additional_inbound_endpoints.end() ) return handshaking_peer; - for( const auto& ep : handshaking_peer->additional_inbound_endpoints ) - { - if( ep == remote_endpoint ) - return handshaking_peer; - } } return peer_connection_ptr(); } From 28704f1f05abfa1d83fd6275713437b7d0ce1f12 Mon Sep 17 00:00:00 2001 From: abitmore Date: Thu, 15 Sep 2022 20:47:09 +0000 Subject: [PATCH 7/9] Remove redundant member: inbound_endpoint_verified --- libraries/net/include/graphene/net/peer_connection.hpp | 2 -- libraries/net/node.cpp | 7 +++---- libraries/net/peer_connection.cpp | 1 - tests/tests/p2p_node_tests.cpp | 1 - 4 files changed, 3 insertions(+), 8 deletions(-) diff --git a/libraries/net/include/graphene/net/peer_connection.hpp b/libraries/net/include/graphene/net/peer_connection.hpp index 25bd9a89a0..8f87485880 100644 --- a/libraries/net/include/graphene/net/peer_connection.hpp +++ b/libraries/net/include/graphene/net/peer_connection.hpp @@ -197,8 +197,6 @@ namespace graphene { namespace net uint16_t outbound_port = 0; /// The inbound endpoint of the remote peer (our best guess) fc::optional remote_inbound_endpoint; - /// Whether the inbound endpoint of the remote peer is verified - bool inbound_endpoint_verified = false; /// Some nodes may be listening on multiple endpoints fc::flat_set additional_inbound_endpoints; /// Potential inbound endpoints of the peer diff --git a/libraries/net/node.cpp b/libraries/net/node.cpp index 39c1502baf..51e6aa3785 100644 --- a/libraries/net/node.cpp +++ b/libraries/net/node.cpp @@ -1767,7 +1767,7 @@ namespace graphene { namespace net { namespace detail { already_connected_peer->potential_inbound_endpoints[*remote_endpoint] = firewalled_state::not_firewalled; } - if( !already_connected_peer->inbound_endpoint_verified // which implies direction == inbound + if( already_connected_peer->is_firewalled != firewalled_state::not_firewalled // implies it's inbound || remote_endpoint->get_address().is_public_address() || !already_connected_peer->get_endpoint_for_connecting()->get_address().is_public_address() ) { @@ -1776,7 +1776,6 @@ namespace graphene { namespace net { namespace detail { ("peer", already_connected_endpoint) ("id", already_connected_peer->node_id) ); already_connected_peer->remote_inbound_endpoint = remote_endpoint; - already_connected_peer->inbound_endpoint_verified = true; already_connected_peer->is_firewalled = firewalled_state::not_firewalled; } // If the already connected peer is in the active connections list, save the endpoint to the peer db @@ -4628,12 +4627,12 @@ namespace graphene { namespace net { namespace detail { fc::scoped_lock lock(_active_connections.get_mutex()); for( const peer_connection_ptr& peer : _active_connections ) { - ilog( " active peer ${endpoint} [${direction}] (${inbound_ep} ${verified}) " + ilog( " active peer ${endpoint} [${direction}] (${inbound_ep} ${is_firewalled}) " "peer_is_in_sync_with_us:${in_sync_with_us} we_are_in_sync_with_peer:${in_sync_with_them}", ( "endpoint", peer->get_remote_endpoint() ) ( "direction", peer->direction ) ( "inbound_ep", peer->get_endpoint_for_connecting() ) - ( "verified", peer->inbound_endpoint_verified ? "verified" : "not_verified" ) + ( "is_firewalled", peer->is_firewalled) ( "in_sync_with_us", !peer->peer_needs_sync_items_from_us ) ( "in_sync_with_them", !peer->we_need_sync_items_from_peer ) ); if( peer->we_need_sync_items_from_peer ) diff --git a/libraries/net/peer_connection.cpp b/libraries/net/peer_connection.cpp index 2ca826fdb9..ad4a99bfb2 100644 --- a/libraries/net/peer_connection.cpp +++ b/libraries/net/peer_connection.cpp @@ -293,7 +293,6 @@ namespace graphene { namespace net their_state = their_connection_state::just_connected; our_state = our_connection_state::just_connected; remote_inbound_endpoint = remote_endpoint; - inbound_endpoint_verified = true; ilog( "established outbound connection to ${remote_endpoint}", ("remote_endpoint", remote_endpoint ) ); } catch ( fc::exception& e ) diff --git a/tests/tests/p2p_node_tests.cpp b/tests/tests/p2p_node_tests.cpp index a759fd044f..0faa8844df 100644 --- a/tests/tests/p2p_node_tests.cpp +++ b/tests/tests/p2p_node_tests.cpp @@ -800,7 +800,6 @@ BOOST_AUTO_TEST_CASE( hello_already_connected ) { fc::ip::endpoint peer3_ep = fc::ip::endpoint::from_string( std::string("1.2.3.4:5678") ); BOOST_CHECK( *node2_ptr->remote_inbound_endpoint == peer3_ep ); - BOOST_CHECK( node2_ptr->inbound_endpoint_verified ); BOOST_CHECK( graphene::net::firewalled_state::not_firewalled == node2_ptr->is_firewalled ); BOOST_REQUIRE_EQUAL( node2_ptr->additional_inbound_endpoints.size(), 1u ); BOOST_CHECK( *node2_ptr->additional_inbound_endpoints.begin() == peer3_ep ); From 120ea6f99e3b339526ecbd1ab676720ca28af5ab Mon Sep 17 00:00:00 2001 From: abitmore Date: Thu, 15 Sep 2022 21:13:45 +0000 Subject: [PATCH 8/9] Avoid unnecessary copying of data --- libraries/net/node.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/net/node.cpp b/libraries/net/node.cpp index 51e6aa3785..0388bdddd4 100644 --- a/libraries/net/node.cpp +++ b/libraries/net/node.cpp @@ -4569,7 +4569,7 @@ namespace graphene { namespace net { namespace detail { // For an inbound handshaking connection, there is a race condition since we might not know its node_id yet, // so be stricter here. // Even so, there may be situations that we end up having multiple active connections with them. - fc::optional endpoint_for_this_peer( handshaking_peer->get_remote_endpoint() ); + fc::optional endpoint_for_this_peer = handshaking_peer->get_remote_endpoint(); if( endpoint_for_this_peer && *endpoint_for_this_peer == remote_endpoint ) return handshaking_peer; // Note: if it is an inbound connection and its inbound endpoint is verified already, From c0174e8cac7f3565a4c00547626e7d144b335ecd Mon Sep 17 00:00:00 2001 From: abitmore Date: Thu, 15 Sep 2022 22:43:30 +0000 Subject: [PATCH 9/9] Fix a code smell --- libraries/net/node.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/net/node.cpp b/libraries/net/node.cpp index 0388bdddd4..f69bb98cd2 100644 --- a/libraries/net/node.cpp +++ b/libraries/net/node.cpp @@ -1677,7 +1677,7 @@ namespace graphene { namespace net { namespace detail { // In addition, by now, our list or exclude list for peer advertisement only contains IP endpoints but not // nodes' public keys (we can't use node_id because it changes every time the node restarts). Using a valid // address is better for the purpose. - if( originating_peer->inbound_port == 0 ) + if( 0 == originating_peer->inbound_port ) originating_peer->remote_inbound_endpoint = fc::ip::endpoint( remote_endpoint->get_address() ); else if( originating_peer->inbound_address.is_public_address() || originating_peer->inbound_address == remote_endpoint->get_address() )