Merge pull request #2866

cf5f6236 Corrections in rate limiting / trottle code, especially in 'out' direction (rbrunner7)

7 files changed, 21 insertions, 36 deletions

diff --git a/src/cryptonote_protocol/cryptonote_protocol_handler-base.cpp b/src/cryptonote_protocol/cryptonote_protocol_handler-base.cpp
index 3bda50c22..094e4fc95 100644
--- a/src/cryptonote_protocol/cryptonote_protocol_handler-base.cpp
+++ b/src/cryptonote_protocol/cryptonote_protocol_handler-base.cpp
@@ -140,7 +140,7 @@ void cryptonote_protocol_handler_base::handler_response_blocks_now(size_t packet
 
 		{ 
 	  	CRITICAL_REGION_LOCAL(	network_throttle_manager::m_lock_get_global_throttle_out );
-			delay = network_throttle_manager::get_global_throttle_out().get_sleep_time_after_tick( packet_size ); // decission from global
+			delay = network_throttle_manager::get_global_throttle_out().get_sleep_time_after_tick( packet_size );
 		}
 
 		
diff --git a/src/p2p/connection_basic.cpp b/src/p2p/connection_basic.cpp
index 8edd75b3e..06baa7893 100644
--- a/src/p2p/connection_basic.cpp
+++ b/src/p2p/connection_basic.cpp
@@ -173,14 +173,9 @@ connection_basic::~connection_basic() noexcept(false) {
 }
 
 void connection_basic::set_rate_up_limit(uint64_t limit) {
-	
-	// TODO remove __SCALING_FACTOR...
-	const double SCALING_FACTOR = 2.1; // to acheve the best performance
-	limit *= SCALING_FACTOR;
 	{
 		CRITICAL_REGION_LOCAL(	network_throttle_manager::m_lock_get_global_throttle_out );
 		network_throttle_manager::get_global_throttle_out().set_target_speed(limit);
-		network_throttle_manager::get_global_throttle_out().set_real_target_speed(limit / SCALING_FACTOR);
 	}
 	save_limit_to_file(limit);
 }
@@ -238,7 +233,7 @@ void connection_basic::sleep_before_packet(size_t packet_size, int phase,  int q
 
 		{ 
 			CRITICAL_REGION_LOCAL(	network_throttle_manager::m_lock_get_global_throttle_out );
-			delay = network_throttle_manager::get_global_throttle_out().get_sleep_time_after_tick( packet_size ); // decission from global
+			delay = network_throttle_manager::get_global_throttle_out().get_sleep_time_after_tick( packet_size );
 		}
 
 		delay *= 0.50;
@@ -252,7 +247,7 @@ void connection_basic::sleep_before_packet(size_t packet_size, int phase,  int q
 // XXX LATER XXX
 	{
 	  CRITICAL_REGION_LOCAL(	network_throttle_manager::m_lock_get_global_throttle_out );
-		network_throttle_manager::get_global_throttle_out().handle_trafic_exact( packet_size * 700); // increase counter - global
+		network_throttle_manager::get_global_throttle_out().handle_trafic_exact( packet_size ); // increase counter - global
 	}
 
 }
@@ -262,13 +257,13 @@ void connection_basic::set_start_time() {
 }
 
 void connection_basic::do_send_handler_write(const void* ptr , size_t cb ) {
-	sleep_before_packet(cb,1,-1);
+        // No sleeping here; sleeping is done once and for all in connection<t_protocol_handler>::handle_write
 	MTRACE("handler_write (direct) - before ASIO write, for packet="<<cb<<" B (after sleep)");
 	set_start_time();
 }
 
 void connection_basic::do_send_handler_write_from_queue( const boost::system::error_code& e, size_t cb, int q_len ) {
-	sleep_before_packet(cb,2,q_len);
+        // No sleeping here; sleeping is done once and for all in connection<t_protocol_handler>::handle_write
 	MTRACE("handler_write (after write, from queue="<<q_len<<") - before ASIO write, for packet="<<cb<<" B (after sleep)");
 
 	set_start_time();
diff --git a/src/p2p/net_node.inl b/src/p2p/net_node.inl
index f64b29c1f..e32eb0c6b 100644
--- a/src/p2p/net_node.inl
+++ b/src/p2p/net_node.inl
@@ -72,8 +72,8 @@ namespace nodetool
 {
   namespace
   {
-    const int64_t default_limit_up = 2048;
-    const int64_t default_limit_down = 8192;
+    const int64_t default_limit_up = 2048;   // kB/s
+    const int64_t default_limit_down = 8192; // kB/s
     const command_line::arg_descriptor<std::string> arg_p2p_bind_ip        = {"p2p-bind-ip", "Interface for p2p network protocol", "0.0.0.0"};
     const command_line::arg_descriptor<std::string> arg_p2p_bind_port = {
         "p2p-bind-port"
@@ -1844,9 +1844,8 @@ namespace nodetool
       this->islimitup=false;
     }
 
-    limit *= 1024;
     epee::net_utils::connection<epee::levin::async_protocol_handler<p2p_connection_context> >::set_rate_up_limit( limit );
-    MINFO("Set limit-up to " << limit/1024 << " kB/s");
+    MINFO("Set limit-up to " << limit << " kB/s");
     return true;
   }
 
@@ -1858,9 +1857,8 @@ namespace nodetool
       limit=default_limit_down;
       this->islimitdown=false;
     }
-    limit *= 1024;
     epee::net_utils::connection<epee::levin::async_protocol_handler<p2p_connection_context> >::set_rate_down_limit( limit );
-    MINFO("Set limit-down to " << limit/1024 << " kB/s");
+    MINFO("Set limit-down to " << limit << " kB/s");
     return true;
   }
 
@@ -1872,21 +1870,21 @@ namespace nodetool
 
     if(limit == -1)
     {
-      limit_up = default_limit_up * 1024;
-      limit_down = default_limit_down * 1024;
+      limit_up = default_limit_up;
+      limit_down = default_limit_down;
     }
     else
     {
-      limit_up = limit * 1024;
-      limit_down = limit * 1024;
+      limit_up = limit;
+      limit_down = limit;
     }
     if(!this->islimitup) {
       epee::net_utils::connection<epee::levin::async_protocol_handler<p2p_connection_context> >::set_rate_up_limit(limit_up);
-      MINFO("Set limit-up to " << limit_up/1024 << " kB/s");
+      MINFO("Set limit-up to " << limit_up << " kB/s");
     }
     if(!this->islimitdown) {
       epee::net_utils::connection<epee::levin::async_protocol_handler<p2p_connection_context> >::set_rate_down_limit(limit_down);
-      MINFO("Set limit-down to " << limit_down/1024 << " kB/s");
+      MINFO("Set limit-down to " << limit_down << " kB/s");
     }
 
     return true;
diff --git a/src/p2p/network_throttle-detail.cpp b/src/p2p/network_throttle-detail.cpp
index 1df48ee26..651e01e6b 100644
--- a/src/p2p/network_throttle-detail.cpp
+++ b/src/p2p/network_throttle-detail.cpp
@@ -160,17 +160,11 @@ void network_throttle::set_target_speed( network_speed_kbps target )
 {
     m_target_speed = target * 1024;
 	MINFO("Setting LIMIT: " << target << " kbps");
-	set_real_target_speed(target);
-}
-
-void network_throttle::set_real_target_speed( network_speed_kbps real_target )
-{
-	m_real_target_speed = real_target * 1024;
 }
 
 network_speed_kbps network_throttle::get_target_speed()
 {
-	return m_real_target_speed / 1024;
+	return m_target_speed / 1024;
 }
 			
 void network_throttle::tick()
diff --git a/src/p2p/network_throttle-detail.hpp b/src/p2p/network_throttle-detail.hpp
index 27caa85d3..676d4341a 100644
--- a/src/p2p/network_throttle-detail.hpp
+++ b/src/p2p/network_throttle-detail.hpp
@@ -52,8 +52,7 @@ class network_throttle : public i_network_throttle {
 		};
 
 
-		network_speed_kbps m_target_speed;
-		network_speed_kbps m_real_target_speed;
+		network_speed_bps m_target_speed;
 		size_t m_network_add_cost; // estimated add cost of headers 
 		size_t m_network_minimal_segment; // estimated minimal cost of sending 1 byte to round up to
 		size_t m_network_max_segment; // recommended max size of 1 TCP transmission
@@ -76,7 +75,6 @@ class network_throttle : public i_network_throttle {
 		virtual ~network_throttle();
 		virtual void set_name(const std::string &name);
 		virtual void set_target_speed( network_speed_kbps target );
-		virtual void set_real_target_speed( network_speed_kbps real_target ); // only for throttle_out
 		virtual network_speed_kbps get_target_speed();
 
 		// add information about events:
diff --git a/src/p2p/network_throttle.hpp b/src/p2p/network_throttle.hpp
index 9853df5e1..bf1f93859 100644
--- a/src/p2p/network_throttle.hpp
+++ b/src/p2p/network_throttle.hpp
@@ -80,7 +80,8 @@ namespace net_utils
 {
 
 // just typedefs to in code define the units used. TODO later it will be enforced that casts to other numericals are only explicit to avoid mistakes? use boost::chrono?
-typedef double network_speed_kbps;
+typedef double network_speed_kbps;   // externally, for parameters and return values, all defined in kilobytes per second
+typedef double network_speed_bps;    // throttle-internally, bytes per second
 typedef double network_time_seconds;
 typedef double network_MB;
 
@@ -137,7 +138,6 @@ class i_network_throttle {
 	public:
 		virtual void set_name(const std::string &name)=0;
 		virtual void set_target_speed( network_speed_kbps target )=0;
-		virtual void set_real_target_speed(network_speed_kbps real_target)=0;
 		virtual network_speed_kbps get_target_speed()=0;
 
 		virtual void handle_trafic_exact(size_t packet_size) =0; // count the new traffic/packet; the size is exact considering all network costs
diff --git a/src/rpc/core_rpc_server.cpp b/src/rpc/core_rpc_server.cpp
index 3a3d9f664..34c7afa6d 100644
--- a/src/rpc/core_rpc_server.cpp
+++ b/src/rpc/core_rpc_server.cpp
@@ -1549,7 +1549,7 @@ namespace cryptonote
         res.status = CORE_RPC_ERROR_CODE_WRONG_PARAM;
         return false;
       }
-      epee::net_utils::connection_basic::set_rate_down_limit(nodetool::default_limit_down * 1024);
+      epee::net_utils::connection_basic::set_rate_down_limit(nodetool::default_limit_down);
     }
 
     if (req.limit_up > 0)
@@ -1563,7 +1563,7 @@ namespace cryptonote
         res.status = CORE_RPC_ERROR_CODE_WRONG_PARAM;
         return false;
       }
-      epee::net_utils::connection_basic::set_rate_up_limit(nodetool::default_limit_up * 1024);
+      epee::net_utils::connection_basic::set_rate_up_limit(nodetool::default_limit_up);
     }
 
     res.limit_down = epee::net_utils::connection_basic::get_rate_down_limit();