revert handshake round-trip optimization (#797)

revert handshake round-trip optimization. it prevents taking advantage of FAST extensions since the bitfield is sent before receiving the other peer's handshake

ChangeLog

@@ -21,6 +21,7 @@
 
 1.1.1 release
 
+	* fix issue where FAST extension messages were not used during handshake
 	* fixed crash on invalid input in http_parser
 	* upgraded to libtommath 1.0
 	* fixed parsing of IPv6 endpoint with invalid port character separator

include/libtorrent/bt_peer_connection.hpp

@@ -223,7 +223,7 @@ namespace libtorrent
 		void write_dont_have(int index) override;
 		void write_piece(peer_request const& r, disk_buffer_holder& buffer) override;
 		void write_keepalive() override;
-		void write_handshake(bool plain_handshake = false);
+		void write_handshake();
 #ifndef TORRENT_DISABLE_EXTENSIONS
 		void write_extensions();
 		void write_upload_only();

src/bt_peer_connection.cpp

@@ -783,7 +783,7 @@ namespace libtorrent
 		}
 	}
 
-	void bt_peer_connection::write_handshake(bool plain_handshake)
+	void bt_peer_connection::write_handshake()
 	{
 		INVARIANT_CHECK;
 
@@ -871,33 +871,6 @@ namespace libtorrent
 			, "ih: %s", aux::to_hex(ih.to_string()).c_str());
 #endif
 		send_buffer(handshake, sizeof(handshake));
-
-		// for encrypted peers, just send a plain handshake. We
-		// don't know at this point if the rest should be
-		// obfuscated or not, we have to wait for the other end's
-		// response first.
-		if (plain_handshake) return;
-
-		// we don't know how many pieces there are until we
-		// have the metadata
-		if (t->ready_for_connections())
-		{
-			write_bitfield();
-#ifndef TORRENT_DISABLE_DHT
-			if (m_supports_dht_port && m_ses.has_dht())
-				write_dht_port(m_ses.external_udp_port());
-#endif
-
-			// if we don't have any pieces, don't do any preemptive
-			// unchoking at all.
-			if (t->num_have() > 0)
-			{
-				// if the peer is ignoring unchoke slots, or if we have enough
-				// unused slots, unchoke this peer right away, to save a round-trip
-				// in case it's interested.
-				maybe_unchoke_this_peer();
-			}
-		}
 	}
 
 	boost::optional<piece_block_progress> bt_peer_connection::downloading_piece_progress() const
@@ -2137,6 +2110,10 @@ namespace libtorrent
 	{
 		INVARIANT_CHECK;
 
+		// if we have not received the other peer's extension bits yet, how do we
+		// know whether to send a have-all or have-none?
+		TORRENT_ASSERT(m_state >= read_peer_id);
+
 		boost::shared_ptr<torrent> t = associated_torrent().lock();
 		TORRENT_ASSERT(t);
 		TORRENT_ASSERT(m_sent_handshake);
@@ -2719,7 +2696,7 @@ namespace libtorrent
 				// always rc4 if sent here. m_rc4_encrypted is flagged
 				// again according to peer selection.
 				switch_send_crypto(m_rc4);
-				write_handshake(true);
+				write_handshake();
 				switch_send_crypto(boost::shared_ptr<crypto_plugin>());
 
 				// vc,crypto_select,len(pad),pad, encrypt(handshake)
@@ -3159,25 +3136,6 @@ namespace libtorrent
 			bytes_transferred = 0;
 			TORRENT_ASSERT(m_encrypted);
 
-			if (is_outgoing() && t->ready_for_connections())
-			{
-				write_bitfield();
-#ifndef TORRENT_DISABLE_DHT
-				if (m_supports_dht_port && m_ses.has_dht())
-					write_dht_port(m_ses.external_udp_port());
-#endif
-
-				// if we don't have any pieces, don't do any preemptive
-				// unchoking at all.
-				if (t->num_have() > 0)
-				{
-					// if the peer is ignoring unchoke slots, or if we have enough
-					// unused slots, unchoke this peer right away, to save a round-trip
-					// in case it's interested.
-					maybe_unchoke_this_peer();
-				}
-			}
-
 			// decrypt remaining received bytes
 			if (m_rc4_encrypted)
 			{
@@ -3505,6 +3463,28 @@ namespace libtorrent
 			}
 #endif
 
+			// complete the handshake
+			// we don't know how many pieces there are until we
+			// have the metadata
+			if (t->ready_for_connections())
+			{
+				write_bitfield();
+#ifndef TORRENT_DISABLE_DHT
+				if (m_supports_dht_port && m_ses.has_dht())
+					write_dht_port(m_ses.external_udp_port());
+#endif
+
+				// if we don't have any pieces, don't do any preemptive
+				// unchoking at all.
+				if (t->num_have() > 0)
+				{
+					// if the peer is ignoring unchoke slots, or if we have enough
+					// unused slots, unchoke this peer right away, to save a round-trip
+					// in case it's interested.
+					maybe_unchoke_this_peer();
+				}
+			}
+
 			m_state = read_packet_size;
 			m_recv_buffer.reset(5);
 

src/peer_connection.cpp

@@ -574,10 +574,10 @@ namespace libtorrent
 			return;
 		}
 
-		int num_allowed_pieces = m_settings.get_int(settings_pack::allowed_fast_set_size);
+		int const num_allowed_pieces = m_settings.get_int(settings_pack::allowed_fast_set_size);
 		if (num_allowed_pieces == 0) return;
 
-		int num_pieces = t->torrent_file().num_pieces();
+		int const num_pieces = t->torrent_file().num_pieces();
 
 		if (num_allowed_pieces >= num_pieces)
 		{

test/test_fast_extension.cpp

@@ -270,14 +270,20 @@ void do_handshake(tcp::socket& s, sha1_hash const& ih, char* buffer)
 	// check for fast extension support
 	TEST_CHECK(extensions[7] & 0x4);
 
-#ifndef TORRENT_DISABLE_EXTENSIONS
 	// check for extension protocol support
-	TEST_CHECK(extensions[5] & 0x10);
+	bool const lt_extension_protocol = (extensions[5] & 0x10) != 0;
+#ifndef TORRENT_DISABLE_EXTENSIONS
+	TEST_CHECK(lt_extension_protocol == true);
+#else
+	TEST_CHECK(lt_extension_protocol == false);
 #endif
 
-#ifndef TORRENT_DISABLE_DHT
 	// check for DHT support
-	TEST_CHECK(extensions[7] & 0x1);
+	bool const dht_support = (extensions[7] & 0x1) != 0;
+#ifndef TORRENT_DISABLE_DHT
+	TEST_CHECK(dht_support == true);
+#else
+	TEST_CHECK(dht_support == false);
 #endif
 
 	TEST_CHECK(std::memcmp(buffer + 28, ih.begin(), 20) == 0);
@@ -399,7 +405,8 @@ entry read_ut_metadata_msg(tcp::socket& s, char* recv_buffer, int size)
 }
 
 boost::shared_ptr<torrent_info> setup_peer(tcp::socket& s, sha1_hash& ih
-	, boost::shared_ptr<lt::session>& ses, torrent_handle* th = NULL)
+	, boost::shared_ptr<lt::session>& ses, bool incoming = true
+	, int flags = 0, torrent_handle* th = NULL)
 {
 	boost::shared_ptr<torrent_info> t = ::create_torrent();
 	ih = t->info_hash();
@@ -410,12 +417,17 @@ boost::shared_ptr<torrent_info> setup_peer(tcp::socket& s, sha1_hash& ih
 	sett.set_bool(settings_pack::enable_natpmp, false);
 	sett.set_bool(settings_pack::enable_lsd, false);
 	sett.set_bool(settings_pack::enable_dht, false);
+	sett.set_int(settings_pack::in_enc_policy, settings_pack::pe_disabled);
+	sett.set_int(settings_pack::out_enc_policy, settings_pack::pe_disabled);
+	sett.set_bool(settings_pack::enable_outgoing_utp, false);
+	sett.set_bool(settings_pack::enable_incoming_utp, false);
 	ses.reset(new lt::session(sett, lt::session::add_default_plugins));
 
 	error_code ec;
 	add_torrent_params p;
 	p.flags &= ~add_torrent_params::flag_paused;
 	p.flags &= ~add_torrent_params::flag_auto_managed;
+	p.flags |= flags;
 	p.ti = t;
 	p.save_path = "./tmp1_fast";
 
@@ -426,12 +438,29 @@ boost::shared_ptr<torrent_info> setup_peer(tcp::socket& s, sha1_hash& ih
 	if (th) *th = ret;
 
 	// wait for the torrent to be ready
+	if ((flags & add_torrent_params::flag_seed_mode) == 0)
+	{
 		wait_for_downloading(*ses, "ses");
+	}
 
-	int const port = ses->listen_port();
+	if (incoming)
-	std::fprintf(stderr, "listen port: %d\n", port);
+	{
-	s.connect(tcp::endpoint(address::from_string("127.0.0.1", ec), port), ec);
+		s.connect(tcp::endpoint(address::from_string("127.0.0.1", ec), ses->listen_port()), ec);
 		if (ec) TEST_ERROR(ec.message());
+	}
+	else
+	{
+		tcp::acceptor l(s.get_io_service());
+		l.open(tcp::v4());
+		l.bind(tcp::endpoint(address_v4::from_string("127.0.0.1")
+			, 3000 + rand() % 60000));
+		l.listen();
+		tcp::endpoint addr = l.local_endpoint();
+
+		ret.connect_peer(addr);
+		print_session_log(*ses);
+		l.accept(s);
+	}
 
 	print_session_log(*ses);
 
@@ -705,7 +734,7 @@ TORRENT_TEST(dont_have)
 	boost::shared_ptr<lt::session> ses;
 	io_service ios;
 	tcp::socket s(ios);
-	boost::shared_ptr<torrent_info> ti = setup_peer(s, ih, ses, &th);
+	boost::shared_ptr<torrent_info> ti = setup_peer(s, ih, ses, true, 0, &th);
 
 	char recv_buffer[1000];
 	do_handshake(s, ih, recv_buffer);
@@ -874,6 +903,55 @@ TORRENT_TEST(invalid_request)
 	send_request(s, req);
 }
 
+void have_all_test(bool const incoming)
+{
+	sha1_hash ih;
+	boost::shared_ptr<lt::session> ses;
+	io_service ios;
+	tcp::socket s(ios);
+	setup_peer(s, ih, ses, incoming, add_torrent_params::flag_seed_mode);
+
+	char recv_buffer[1000];
+	do_handshake(s, ih, recv_buffer);
+	print_session_log(*ses);
+
+	// expect to receive a have-all (not a bitfield)
+	// since we advertised support for FAST extensions
+	for (;;)
+	{
+		int const len = read_message(s, recv_buffer, sizeof(recv_buffer));
+		if (len == -1)
+		{
+			TEST_ERROR("failed to receive have-all despite advertising support for FAST");
+			break;
+		}
+		print_message(recv_buffer, len);
+		int const msg = recv_buffer[0];
+		if (msg == 0xe) // have-all
+		{
+			// success!
+			break;
+		}
+		if (msg == 5) // bitfield
+		{
+			TEST_ERROR("received bitfield from seed despite advertising support for FAST");
+			break;
+		}
+	}
+}
+
+TORRENT_TEST(outgoing_have_all)
+{
+	std::cerr << "\n === test outgoing have-all ===\n" << std::endl;
+	have_all_test(true);
+}
+
+TORRENT_TEST(incoming_have_all)
+{
+	std::cerr << "\n === test outgoing have-all ===\n" << std::endl;
+	have_all_test(false);
+}
+
 #endif // TORRENT_DISABLE_EXTENSIONS
 
 // TODO: test sending invalid requests (out of bound piece index, offsets and