Merge pull request #6243

4771a7ae p2p: remove obsolete local time in handshake (moneromooo-monero)
2fbbc4a2 p2p: avoid sending the same peer list over and over (moneromooo-monero)
3004835b epee: remove backward compatible endian specific address serialization (moneromooo-monero)
39a343d7 p2p: remove backward compatible peer list (moneromooo-monero)
60631802 p2p: simplify last_seen serialization now we have optional stores (moneromooo-monero)
9467b2e4 cryptonote_protocol: omit top 64 bits of difficulty when 0 (moneromooo-monero)
b595583f serialization: do not write optional fields with default value (moneromooo-monero)
5f98b46d p2p: remove obsolete local time from TIMED_SYNC (moneromooo-monero)

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<is_store>::serialize(ip, stg, hparent_section, "m_ip");
 			}
 			else
 			{
-				if (!epee::serialization::selector<is_store>::serialize_t_val_as_blob(this_ref.m_ip, stg, hparent_section, "ip"))
+				KV_SERIALIZE(m_ip)
-				{
+				const_cast<ipv4_network_address&>(this_ref).m_ip = SWAP32LE(this_ref.m_ip);
-					KV_SERIALIZE(m_ip)
-					const_cast<ipv4_network_address&>(this_ref).m_ip = SWAP32LE(this_ref.m_ip);
-				}
 			}
 			KV_SERIALIZE(m_port)
 		END_KV_SERIALIZE_MAP()

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<is_store>::serialize(this_ref.variable, stg, hparent_section, val_name)) \
       epee::serialize_default(this_ref.variable, default_value); \
   } while (0);

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()

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<epee::net_utils::network_address> sent_addresses;
   };
 
   template<class t_payload_net_handler>
@@ -362,7 +363,7 @@ namespace nodetool
         const boost::program_options::variables_map& vm
       );
     bool idle_worker();
-    bool handle_remote_peerlist(const std::vector<peerlist_entry>& peerlist, time_t local_time, const epee::net_utils::connection_context_base& context);
+    bool handle_remote_peerlist(const std::vector<peerlist_entry>& 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);
 

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<class t_payload_net_handler>
-  bool node_server<t_payload_net_handler>::handle_remote_peerlist(const std::vector<peerlist_entry>& peerlist, time_t local_time, const epee::net_utils::connection_context_base& context)
+  bool node_server<t_payload_net_handler>::handle_remote_peerlist(const std::vector<peerlist_entry>& peerlist, const epee::net_utils::connection_context_base& context)
   {
     std::vector<peerlist_entry> peerlist_ = peerlist;
     if(!sanitize_peerlist(peerlist_))
@@ -1918,9 +1918,6 @@ namespace nodetool
   template<class t_payload_net_handler>
   bool node_server<t_payload_net_handler>::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;
@@ -2291,12 +2288,20 @@ 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);
 
-    zone.m_peerlist.get_peerlist_head(rsp.local_peerlist_new, true);
+    std::vector<peerlist_entry> 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)
@@ -2418,6 +2423,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");

src/p2p/net_peerlist.h

@@ -269,19 +269,19 @@ namespace nodetool
     peers_indexed::index<by_time>::type& by_time_index=m_peers_white.get<by_time>();
     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))
     {

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)
@@ -166,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;
@@ -175,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)
@@ -214,35 +211,7 @@ namespace nodetool
       BEGIN_KV_SERIALIZE_MAP()
         KV_SERIALIZE(node_data)
         KV_SERIALIZE(payload_data)
-        if (is_store)
+        KV_SERIALIZE(local_peerlist_new)
-        {
-          // saving: save both, so old and new peers can understand it
-          KV_SERIALIZE(local_peerlist_new)
-          std::vector<peerlist_entry_base<network_address_old>> 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<const epee::net_utils::ipv4_network_address>();
-              local_peerlist.push_back(peerlist_entry_base<network_address_old>({{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<is_store>::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<is_store>::serialize(this_ref.local_peerlist_new, stg, hparent_section, "local_peerlist_new"))
-          {
-            std::vector<peerlist_entry_base<network_address_old>> local_peerlist;
-            epee::serialization::selector<is_store>::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}));
-          }
-        }
       END_KV_SERIALIZE_MAP()
     };
     typedef epee::misc_utils::struct_init<response_t> response;
@@ -268,42 +237,12 @@ namespace nodetool
 
     struct response_t
     {
-      uint64_t local_time;
       t_playload_type payload_data;
       std::vector<peerlist_entry> local_peerlist_new;
 
       BEGIN_KV_SERIALIZE_MAP()
-        KV_SERIALIZE(local_time)
         KV_SERIALIZE(payload_data)
-        if (is_store)
+        KV_SERIALIZE(local_peerlist_new)
-        {
-          // saving: save both, so old and new peers can understand it
-          KV_SERIALIZE(local_peerlist_new)
-          std::vector<peerlist_entry_base<network_address_old>> 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<const epee::net_utils::ipv4_network_address>();
-              local_peerlist.push_back(peerlist_entry_base<network_address_old>({{ipv4.ip(), ipv4.port()}, p.id, p.last_seen}));
-            }
-            else
-              MDEBUG("Not including in legacy peer list: " << p.adr.str());
-          }
-          epee::serialization::selector<is_store>::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<is_store>::serialize(this_ref.local_peerlist_new, stg, hparent_section, "local_peerlist_new"))
-          {
-            std::vector<peerlist_entry_base<network_address_old>> local_peerlist;
-            epee::serialization::selector<is_store>::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}));
-          }
-        }
       END_KV_SERIALIZE_MAP()
     };
     typedef epee::misc_utils::struct_init<response_t> response;

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