From 5f98b46d58e37dfe409578d48e15966acf7c4560 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 4 Dec 2019 12:51:45 +0000 Subject: [PATCH 1/8] p2p: remove obsolete local time from TIMED_SYNC --- src/p2p/net_node.h | 2 +- src/p2p/net_node.inl | 8 +++----- src/p2p/p2p_protocol_defs.h | 2 -- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/src/p2p/net_node.h b/src/p2p/net_node.h index 0e9c1c942..1ef6b9028 100644 --- a/src/p2p/net_node.h +++ b/src/p2p/net_node.h @@ -362,7 +362,7 @@ namespace nodetool const boost::program_options::variables_map& vm ); bool idle_worker(); - bool handle_remote_peerlist(const std::vector& peerlist, time_t local_time, const epee::net_utils::connection_context_base& context); + bool handle_remote_peerlist(const std::vector& peerlist, const epee::net_utils::connection_context_base& context); bool get_local_node_data(basic_node_data& node_data, const network_zone& zone); //bool get_local_handshake_data(handshake_data& hshd); diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl index 263cecfa2..7a2feddc8 100644 --- a/src/p2p/net_node.inl +++ b/src/p2p/net_node.inl @@ -1041,7 +1041,7 @@ namespace nodetool return; } - if(!handle_remote_peerlist(rsp.local_peerlist_new, rsp.node_data.local_time, context)) + if(!handle_remote_peerlist(rsp.local_peerlist_new, context)) { LOG_WARNING_CC(context, "COMMAND_HANDSHAKE: failed to handle_remote_peerlist(...), closing connection."); add_host_fail(context.m_remote_address); @@ -1119,7 +1119,7 @@ namespace nodetool return; } - if(!handle_remote_peerlist(rsp.local_peerlist_new, rsp.local_time, context)) + if(!handle_remote_peerlist(rsp.local_peerlist_new, context)) { LOG_WARNING_CC(context, "COMMAND_TIMED_SYNC: failed to handle_remote_peerlist(...), closing connection."); m_network_zones.at(context.m_remote_address.get_zone()).m_net_server.get_config_object().close(context.m_connection_id ); @@ -1894,7 +1894,7 @@ namespace nodetool } //----------------------------------------------------------------------------------- template - bool node_server::handle_remote_peerlist(const std::vector& peerlist, time_t local_time, const epee::net_utils::connection_context_base& context) + bool node_server::handle_remote_peerlist(const std::vector& peerlist, const epee::net_utils::connection_context_base& context) { std::vector peerlist_ = peerlist; if(!sanitize_peerlist(peerlist_)) @@ -2291,8 +2291,6 @@ namespace nodetool } //fill response - rsp.local_time = time(NULL); - const epee::net_utils::zone zone_type = context.m_remote_address.get_zone(); network_zone& zone = m_network_zones.at(zone_type); diff --git a/src/p2p/p2p_protocol_defs.h b/src/p2p/p2p_protocol_defs.h index 44b278589..178382547 100644 --- a/src/p2p/p2p_protocol_defs.h +++ b/src/p2p/p2p_protocol_defs.h @@ -268,12 +268,10 @@ namespace nodetool struct response_t { - uint64_t local_time; t_playload_type payload_data; std::vector local_peerlist_new; BEGIN_KV_SERIALIZE_MAP() - KV_SERIALIZE(local_time) KV_SERIALIZE(payload_data) if (is_store) { From b595583f3dde9d7a6ef4c6a30d38d541eb6c6cd9 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 4 Dec 2019 21:19:04 +0000 Subject: [PATCH 2/8] serialization: do not write optional fields with default value --- contrib/epee/include/serialization/keyvalue_serialization.h | 2 ++ tests/functional_tests/transfer.py | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/contrib/epee/include/serialization/keyvalue_serialization.h b/contrib/epee/include/serialization/keyvalue_serialization.h index 78d294d05..fd343865c 100644 --- a/contrib/epee/include/serialization/keyvalue_serialization.h +++ b/contrib/epee/include/serialization/keyvalue_serialization.h @@ -89,6 +89,8 @@ public: \ #define KV_SERIALIZE_OPT_N(variable, val_name, default_value) \ do { \ + if (is_store && this_ref.variable == default_value) \ + break; \ if (!epee::serialization::selector::serialize(this_ref.variable, stg, hparent_section, val_name)) \ epee::serialize_default(this_ref.variable, default_value); \ } while (0); diff --git a/tests/functional_tests/transfer.py b/tests/functional_tests/transfer.py index 0c942f48b..3bed69b98 100755 --- a/tests/functional_tests/transfer.py +++ b/tests/functional_tests/transfer.py @@ -169,7 +169,7 @@ class TransferTest(): assert e.subaddr_indices == [{'major': 0, 'minor': 0}] assert e.address == '42ey1afDFnn4886T7196doS9GPMzexD9gXpsZJDwVjeRVdFCSoHnv7KPbBeGpzJBzHRCAs9UxqeoyFQMYbqSWYTfJJQAWDm' assert e.double_spend_seen == False - assert e.confirmations == 0 + assert not 'confirmations' in e or e.confirmations == 0 running_balances[0] -= 1000000000000 + fee @@ -282,7 +282,7 @@ class TransferTest(): assert e.subaddr_indices == [{'major': 0, 'minor': 0}] assert e.address == '44Kbx4sJ7JDRDV5aAhLJzQCjDz2ViLRduE3ijDZu3osWKBjMGkV1XPk4pfDUMqt1Aiezvephdqm6YD19GKFD9ZcXVUTp6BW' assert e.double_spend_seen == False - assert e.confirmations == 0 + assert not 'confirmations' in e or e.confirmations == 0 assert e.amount == amount assert e.fee == fee From 9467b2e44cdc05fd578b26ac76d73461778613dd Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 4 Dec 2019 21:19:40 +0000 Subject: [PATCH 3/8] cryptonote_protocol: omit top 64 bits of difficulty when 0 --- src/cryptonote_protocol/cryptonote_protocol_defs.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/cryptonote_protocol/cryptonote_protocol_defs.h b/src/cryptonote_protocol/cryptonote_protocol_defs.h index 201001c8e..ee7e69eb7 100644 --- a/src/cryptonote_protocol/cryptonote_protocol_defs.h +++ b/src/cryptonote_protocol/cryptonote_protocol_defs.h @@ -257,7 +257,10 @@ namespace cryptonote BEGIN_KV_SERIALIZE_MAP() KV_SERIALIZE(current_height) KV_SERIALIZE(cumulative_difficulty) - KV_SERIALIZE(cumulative_difficulty_top64) + if (is_store) + KV_SERIALIZE(cumulative_difficulty_top64) + else + KV_SERIALIZE_OPT(cumulative_difficulty_top64, (uint64_t)0) KV_SERIALIZE_VAL_POD_AS_BLOB(top_id) KV_SERIALIZE_OPT(top_version, (uint8_t)0) KV_SERIALIZE_OPT(pruning_seed, (uint32_t)0) @@ -298,7 +301,10 @@ namespace cryptonote KV_SERIALIZE(start_height) KV_SERIALIZE(total_height) KV_SERIALIZE(cumulative_difficulty) - KV_SERIALIZE(cumulative_difficulty_top64) + if (is_store) + KV_SERIALIZE(cumulative_difficulty_top64) + else + KV_SERIALIZE_OPT(cumulative_difficulty_top64, (uint64_t)0) KV_SERIALIZE_CONTAINER_POD_AS_BLOB(m_block_ids) KV_SERIALIZE_CONTAINER_POD_AS_BLOB(m_block_weights) END_KV_SERIALIZE_MAP() From 606318026e0a21fa77a2dac33510c01ff14f5522 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 4 Dec 2019 21:20:55 +0000 Subject: [PATCH 4/8] p2p: simplify last_seen serialization now we have optional stores --- src/p2p/p2p_protocol_defs.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/p2p/p2p_protocol_defs.h b/src/p2p/p2p_protocol_defs.h index 178382547..4acf05ef8 100644 --- a/src/p2p/p2p_protocol_defs.h +++ b/src/p2p/p2p_protocol_defs.h @@ -82,8 +82,7 @@ namespace nodetool BEGIN_KV_SERIALIZE_MAP() KV_SERIALIZE(adr) KV_SERIALIZE(id) - if (!is_store || this_ref.last_seen != 0) - KV_SERIALIZE_OPT(last_seen, (int64_t)0) + KV_SERIALIZE_OPT(last_seen, (int64_t)0) KV_SERIALIZE_OPT(pruning_seed, (uint32_t)0) KV_SERIALIZE_OPT(rpc_port, (uint16_t)0) KV_SERIALIZE_OPT(rpc_credits_per_hash, (uint32_t)0) From 39a343d76ed984ef03a4d2b57777b207b85fb461 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 4 Dec 2019 21:21:47 +0000 Subject: [PATCH 5/8] p2p: remove backward compatible peer list --- src/p2p/p2p_protocol_defs.h | 60 ++----------------------------------- 1 file changed, 2 insertions(+), 58 deletions(-) diff --git a/src/p2p/p2p_protocol_defs.h b/src/p2p/p2p_protocol_defs.h index 4acf05ef8..3684c317a 100644 --- a/src/p2p/p2p_protocol_defs.h +++ b/src/p2p/p2p_protocol_defs.h @@ -213,35 +213,7 @@ namespace nodetool BEGIN_KV_SERIALIZE_MAP() KV_SERIALIZE(node_data) KV_SERIALIZE(payload_data) - if (is_store) - { - // saving: save both, so old and new peers can understand it - KV_SERIALIZE(local_peerlist_new) - std::vector> local_peerlist; - for (const auto &p: this_ref.local_peerlist_new) - { - if (p.adr.get_type_id() == epee::net_utils::ipv4_network_address::get_type_id()) - { - const epee::net_utils::network_address &na = p.adr; - const epee::net_utils::ipv4_network_address &ipv4 = na.as(); - local_peerlist.push_back(peerlist_entry_base({{ipv4.ip(), ipv4.port()}, p.id, p.last_seen, p.pruning_seed, p.rpc_port, p.rpc_credits_per_hash})); - } - else - MDEBUG("Not including in legacy peer list: " << p.adr.str()); - } - epee::serialization::selector::serialize_stl_container_pod_val_as_blob(local_peerlist, stg, hparent_section, "local_peerlist"); - } - else - { - // loading: load old list only if there is no new one - if (!epee::serialization::selector::serialize(this_ref.local_peerlist_new, stg, hparent_section, "local_peerlist_new")) - { - std::vector> local_peerlist; - epee::serialization::selector::serialize_stl_container_pod_val_as_blob(local_peerlist, stg, hparent_section, "local_peerlist"); - for (const auto &p: local_peerlist) - ((response&)this_ref).local_peerlist_new.push_back(peerlist_entry({epee::net_utils::ipv4_network_address(p.adr.ip, p.adr.port), p.id, p.last_seen, p.pruning_seed, p.rpc_port, p.rpc_credits_per_hash})); - } - } + KV_SERIALIZE(local_peerlist_new) END_KV_SERIALIZE_MAP() }; typedef epee::misc_utils::struct_init response; @@ -272,35 +244,7 @@ namespace nodetool BEGIN_KV_SERIALIZE_MAP() KV_SERIALIZE(payload_data) - if (is_store) - { - // saving: save both, so old and new peers can understand it - KV_SERIALIZE(local_peerlist_new) - std::vector> local_peerlist; - for (const auto &p: this_ref.local_peerlist_new) - { - if (p.adr.get_type_id() == epee::net_utils::ipv4_network_address::get_type_id()) - { - const epee::net_utils::network_address &na = p.adr; - const epee::net_utils::ipv4_network_address &ipv4 = na.as(); - local_peerlist.push_back(peerlist_entry_base({{ipv4.ip(), ipv4.port()}, p.id, p.last_seen})); - } - else - MDEBUG("Not including in legacy peer list: " << p.adr.str()); - } - epee::serialization::selector::serialize_stl_container_pod_val_as_blob(local_peerlist, stg, hparent_section, "local_peerlist"); - } - else - { - // loading: load old list only if there is no new one - if (!epee::serialization::selector::serialize(this_ref.local_peerlist_new, stg, hparent_section, "local_peerlist_new")) - { - std::vector> local_peerlist; - epee::serialization::selector::serialize_stl_container_pod_val_as_blob(local_peerlist, stg, hparent_section, "local_peerlist"); - for (const auto &p: local_peerlist) - ((response&)this_ref).local_peerlist_new.push_back(peerlist_entry({epee::net_utils::ipv4_network_address(p.adr.ip, p.adr.port), p.id, p.last_seen})); - } - } + KV_SERIALIZE(local_peerlist_new) END_KV_SERIALIZE_MAP() }; typedef epee::misc_utils::struct_init response; From 3004835b517425159e9fc30c5ead4d807d006738 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 4 Dec 2019 21:22:11 +0000 Subject: [PATCH 6/8] epee: remove backward compatible endian specific address serialization --- contrib/epee/include/net/net_utils_base.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/contrib/epee/include/net/net_utils_base.h b/contrib/epee/include/net/net_utils_base.h index 028e605d7..d86c62c17 100644 --- a/contrib/epee/include/net/net_utils_base.h +++ b/contrib/epee/include/net/net_utils_base.h @@ -94,17 +94,13 @@ namespace net_utils BEGIN_KV_SERIALIZE_MAP() if (is_store) { - KV_SERIALIZE_VAL_POD_AS_BLOB_N(m_ip, "ip") uint32_t ip = SWAP32LE(this_ref.m_ip); epee::serialization::selector::serialize(ip, stg, hparent_section, "m_ip"); } else { - if (!epee::serialization::selector::serialize_t_val_as_blob(this_ref.m_ip, stg, hparent_section, "ip")) - { - KV_SERIALIZE(m_ip) - const_cast(this_ref).m_ip = SWAP32LE(this_ref.m_ip); - } + KV_SERIALIZE(m_ip) + const_cast(this_ref).m_ip = SWAP32LE(this_ref.m_ip); } KV_SERIALIZE(m_port) END_KV_SERIALIZE_MAP() From 2fbbc4a2d3ebfd6fca1d9d7687cef261491c0e1f Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Wed, 4 Dec 2019 21:22:55 +0000 Subject: [PATCH 7/8] p2p: avoid sending the same peer list over and over Nodes remember which connections have been sent which peer addresses and won't send it again. This causes more addresses to be sent as the connection lifetime grows, since there is no duplication anymore, which increases the diffusion speed of peer addresses. The whole white list is now considered for sending, not just the most recent seen peers. This further hardens against topology discovery, though it will more readily send peers that have been last seen earlier than it otherwise would. While this does save a fair amount of net bandwidth, it makes heavy use of std::set lookups, which does bring network_address::less up the profile, though not too aggressively. --- src/p2p/net_node.h | 1 + src/p2p/net_node.inl | 14 +++++++++++++- src/p2p/net_peerlist.h | 6 +++--- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/p2p/net_node.h b/src/p2p/net_node.h index 1ef6b9028..9b9ffbe2a 100644 --- a/src/p2p/net_node.h +++ b/src/p2p/net_node.h @@ -123,6 +123,7 @@ namespace nodetool peerid_type peer_id; uint32_t support_flags; bool m_in_timedsync; + std::set sent_addresses; }; template diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl index 7a2feddc8..13da53b1d 100644 --- a/src/p2p/net_node.inl +++ b/src/p2p/net_node.inl @@ -2294,7 +2294,17 @@ namespace nodetool const epee::net_utils::zone zone_type = context.m_remote_address.get_zone(); network_zone& zone = m_network_zones.at(zone_type); - zone.m_peerlist.get_peerlist_head(rsp.local_peerlist_new, true); + std::vector local_peerlist_new; + zone.m_peerlist.get_peerlist_head(local_peerlist_new, true, P2P_DEFAULT_PEERS_IN_HANDSHAKE); + + //only include out peers we did not already send + rsp.local_peerlist_new.reserve(local_peerlist_new.size()); + for (auto &pe: local_peerlist_new) + { + if (!context.sent_addresses.insert(pe.adr).second) + continue; + rsp.local_peerlist_new.push_back(std::move(pe)); + } m_payload_handler.get_payload_sync_data(rsp.payload_data); /* Tor/I2P nodes receiving connections via forwarding (from tor/i2p daemon) @@ -2416,6 +2426,8 @@ namespace nodetool //fill response zone.m_peerlist.get_peerlist_head(rsp.local_peerlist_new, true); + for (const auto &e: rsp.local_peerlist_new) + context.sent_addresses.insert(e.adr); get_local_node_data(rsp.node_data, zone); m_payload_handler.get_payload_sync_data(rsp.payload_data); LOG_DEBUG_CC(context, "COMMAND_HANDSHAKE"); diff --git a/src/p2p/net_peerlist.h b/src/p2p/net_peerlist.h index 58b704f73..8225ad2fa 100644 --- a/src/p2p/net_peerlist.h +++ b/src/p2p/net_peerlist.h @@ -269,19 +269,19 @@ namespace nodetool peers_indexed::index::type& by_time_index=m_peers_white.get(); uint32_t cnt = 0; - // picks a random set of peers within the first 120%, rather than a set of the first 100%. + // picks a random set of peers within the whole set, rather pick the first depth elements. // The intent is that if someone asks twice, they can't easily tell: // - this address was not in the first list, but is in the second, so the only way this can be // is if its last_seen was recently reset, so this means the target node recently had a new // connection to that address // - this address was in the first list, and not in the second, which means either the address - // was moved to the gray list (if it's not accessibe, which the attacker can check if + // was moved to the gray list (if it's not accessible, which the attacker can check if // the address accepts incoming connections) or it was the oldest to still fit in the 250 items, // so its last_seen is old. // // See Cao, Tong et al. "Exploring the Monero Peer-to-Peer Network". https://eprint.iacr.org/2019/411 // - const uint32_t pick_depth = anonymize ? depth + depth / 5 : depth; + const uint32_t pick_depth = anonymize ? m_peers_white.size() : depth; bs_head.reserve(pick_depth); for(const peers_indexed::value_type& vl: boost::adaptors::reverse(by_time_index)) { From 4771a7aec198421fd6c7875b88f496b6c689ade8 Mon Sep 17 00:00:00 2001 From: moneromooo-monero Date: Thu, 5 Dec 2019 12:27:57 +0000 Subject: [PATCH 8/8] p2p: remove obsolete local time in handshake Also removes a potential fingerprinting vector --- src/p2p/net_node.inl | 3 --- src/p2p/p2p_protocol_defs.h | 2 -- 2 files changed, 5 deletions(-) diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl index 13da53b1d..998406c91 100644 --- a/src/p2p/net_node.inl +++ b/src/p2p/net_node.inl @@ -1918,9 +1918,6 @@ namespace nodetool template bool node_server::get_local_node_data(basic_node_data& node_data, const network_zone& zone) { - time_t local_time; - time(&local_time); - node_data.local_time = local_time; // \TODO This can be an identifying value across zones (public internet to tor/i2p) ... node_data.peer_id = zone.m_config.m_peer_id; if(!m_hide_my_port && zone.m_can_pingback) node_data.my_port = m_external_port ? m_external_port : m_listening_port; diff --git a/src/p2p/p2p_protocol_defs.h b/src/p2p/p2p_protocol_defs.h index 3684c317a..53defe9bd 100644 --- a/src/p2p/p2p_protocol_defs.h +++ b/src/p2p/p2p_protocol_defs.h @@ -165,7 +165,6 @@ namespace nodetool struct basic_node_data { uuid network_id; - uint64_t local_time; uint32_t my_port; uint16_t rpc_port; uint32_t rpc_credits_per_hash; @@ -174,7 +173,6 @@ namespace nodetool BEGIN_KV_SERIALIZE_MAP() KV_SERIALIZE_VAL_POD_AS_BLOB(network_id) KV_SERIALIZE(peer_id) - KV_SERIALIZE(local_time) KV_SERIALIZE(my_port) KV_SERIALIZE_OPT(rpc_port, (uint16_t)(0)) KV_SERIALIZE_OPT(rpc_credits_per_hash, (uint32_t)0)