diff --git a/ChangeLog b/ChangeLog index 070be252f..47fde55b5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -70,6 +70,7 @@ * almost completely changed the storage interface (for custom storage) * added support for hashing pieces in multiple threads + * improve ip_voter to avoid flapping * fixed bug when max_peerlist_size was set to 0 * fix issues with missing exported symbols when building dll * fix division by zero bug in edge case while connecting peers diff --git a/include/libtorrent/ip_voter.hpp b/include/libtorrent/ip_voter.hpp index f867abb48..72584ed72 100644 --- a/include/libtorrent/ip_voter.hpp +++ b/include/libtorrent/ip_voter.hpp @@ -61,11 +61,13 @@ namespace libtorrent external_ip_t(): sources(0), num_votes(0) {} bool add_vote(sha1_hash const& k, int type); + + // we want to sort decending bool operator<(external_ip_t const& rhs) const { - if (num_votes < rhs.num_votes) return true; - if (num_votes > rhs.num_votes) return false; - return sources < rhs.sources; + if (num_votes > rhs.num_votes) return true; + if (num_votes < rhs.num_votes) return false; + return sources > rhs.sources; } // this is a bloom filter of the IPs that have diff --git a/src/ip_voter.cpp b/src/ip_voter.cpp index be38a6f97..4526fa8b4 100644 --- a/src/ip_voter.cpp +++ b/src/ip_voter.cpp @@ -58,17 +58,35 @@ namespace libtorrent // this is the inverse condition, since this is the case // were we exit, without rotating if (m_total_votes < 50 - && (now - m_last_rotate < minutes(5) || m_total_votes == 0)) + && (now - m_last_rotate < minutes(5) || m_total_votes == 0) + && m_valid_external) return false; // this shouldn't really happen if we have at least one // vote. if (m_external_addresses.empty()) return false; - // rotate - std::vector::iterator i = std::max_element( - m_external_addresses.begin(), m_external_addresses.end()); - TORRENT_ASSERT(i != m_external_addresses.end()); + // if there's just one vote, go with that + std::vector::iterator i; + if (m_external_addresses.size() == 1) + { + // avoid flapping. We need more votes to change our mind on the + // external IP + if (m_external_addresses[0].num_votes < 2) return false; + } + else + { + // find the top two votes. + std::partial_sort(m_external_addresses.begin() + , m_external_addresses.begin() + 2, m_external_addresses.end()); + + // if we don't have enough of a majority voting for the winning + // IP, don't rotate. This avoids flapping + if (m_external_addresses[0].num_votes * 2 / 3 <= m_external_addresses[1].num_votes) + return false; + } + + i = m_external_addresses.begin(); bool ret = m_external_address != i->addr; m_external_address = i->addr; @@ -119,12 +137,9 @@ namespace libtorrent // is a sort of weighted LRU. std::stable_sort(m_external_addresses.begin(), m_external_addresses.end()); - // erase the first element, since this is the - // oldest entry and the one with lowst number - // of votes. This makes sense because the oldest - // entry has had the longest time to receive more - // votes to be bumped up - m_external_addresses.erase(m_external_addresses.begin()); + // erase the last element, since it is one of the + // ones with the fewest votes + m_external_addresses.erase(m_external_addresses.end() - 1); } m_external_addresses.push_back(external_ip_t()); i = m_external_addresses.end() - 1; @@ -136,11 +151,18 @@ namespace libtorrent if (m_valid_external) return maybe_rotate(); - i = std::max_element(m_external_addresses.begin(), m_external_addresses.end()); + i = std::min_element(m_external_addresses.begin(), m_external_addresses.end()); TORRENT_ASSERT(i != m_external_addresses.end()); if (i->addr == m_external_address) return maybe_rotate(); + if (m_external_address != address_v4()) + { + // we have a temporary external address. As soon as we have + // more than 25 votes, consider deciding which one to settle for + return (m_total_votes >= 25) ? maybe_rotate() : false; + } + m_external_address = i->addr; return true; diff --git a/test/Jamfile b/test/Jamfile index cb91ccd31..ce0705356 100644 --- a/test/Jamfile +++ b/test/Jamfile @@ -102,6 +102,7 @@ test-suite libtorrent : [ run test_resolve_links.cpp ] [ run test_crc32.cpp ] [ run test_heterogeneous_queue.cpp ] + [ run test_ip_voter.cpp ] [ run test_resume.cpp ] [ run test_sliding_average.cpp ] [ run test_socket_io.cpp ] diff --git a/test/Makefile.am b/test/Makefile.am index 422e8b058..10d10fc4a 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -23,6 +23,7 @@ test_programs = \ test_heterogeneous_queue \ test_http_connection \ test_ip_filter \ + test_ip_voter \ test_dht \ test_lsd \ test_metadata_extension \ @@ -164,6 +165,7 @@ test_hasher_SOURCES = test_hasher.cpp test_heterogeneous_queue_SOURCES = test_heterogeneous_queue.cpp test_http_connection_SOURCES = test_http_connection.cpp test_ip_filter_SOURCES = test_ip_filter.cpp +test_ip_voter_SOURCES = test_ip_voter.cpp test_lsd_SOURCES = test_lsd.cpp test_metadata_extension_SOURCES = test_metadata_extension.cpp test_peer_priority_SOURCES = test_peer_priority.cpp diff --git a/test/test_ip_voter.cpp b/test/test_ip_voter.cpp new file mode 100644 index 000000000..165414efc --- /dev/null +++ b/test/test_ip_voter.cpp @@ -0,0 +1,120 @@ +/* + +Copyright (c) 2015, Arvid Norberg +All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions +are met: + + * Redistributions of source code must retain the above copyright + notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above copyright + notice, this list of conditions and the following disclaimer in + the documentation and/or other materials provided with the distribution. + * Neither the name of the author nor the names of its + contributors may be used to endorse or promote products derived + from this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE +LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR +CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF +SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS +INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN +CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) +ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE +POSSIBILITY OF SUCH DAMAGE. + +*/ + +#include "test.hpp" +#include "libtorrent/ip_voter.hpp" +#include "libtorrent/address.hpp" +#include "libtorrent/socket.hpp" +#include "libtorrent/random.hpp" +#include "setup_transfer.hpp" // for rand_v4 + +using namespace libtorrent; + +// test the case where every time we get a new IP. Make sure +// we don't flap +void test_random() +{ + ip_voter ipv; + + bool new_ip = ipv.cast_vote(rand_v4(), 1, rand_v4()); + TEST_CHECK(new_ip); + for (int i = 0; i < 1000; ++i) + { + new_ip = ipv.cast_vote(rand_v4(), 1, rand_v4()); + TEST_CHECK(!new_ip); + } +} + +void test_two_ips() +{ + ip_voter ipv; + + address_v4 addr1(address_v4::from_string("51.1.1.1")); + address_v4 addr2(address_v4::from_string("53.3.3.3")); + + // addr1 is the first address we see, which is the one we pick. Even though + // we'll have as many votes for addr2, we shouldn't flap, since addr2 never + // gets an overwhelming majority. + bool new_ip = ipv.cast_vote(addr1, 1, rand_v4()); + TEST_CHECK(new_ip); + for (int i = 0; i < 1000; ++i) + { + fprintf(stderr, "%d\n", i); + new_ip = ipv.cast_vote(addr2, 1, rand_v4()); + TEST_CHECK(!new_ip); + new_ip = ipv.cast_vote(rand_v4(), 1, rand_v4()); + TEST_CHECK(!new_ip); + new_ip = ipv.cast_vote(addr1, 1, rand_v4()); + TEST_CHECK(!new_ip); + + TEST_CHECK(ipv.external_address() == addr1); + } +} + +void test_one_ip() +{ + ip_voter ipv; + + address_v4 addr1(address_v4::from_string("51.1.1.1")); + address_v4 addr2(address_v4::from_string("53.3.3.3")); + + bool new_ip = ipv.cast_vote(rand_v4(), 1, rand_v4()); + TEST_CHECK(new_ip); + bool switched_ip = false; + for (int i = 0; i < 1000; ++i) + { + new_ip = ipv.cast_vote(addr2, 1, rand_v4()); + TEST_CHECK(!new_ip); + new_ip = ipv.cast_vote(rand_v4(), 1, rand_v4()); + TEST_CHECK(!new_ip); + new_ip = ipv.cast_vote(addr1, 1, rand_v4()); + if (new_ip) switched_ip = true; + new_ip = ipv.cast_vote(addr1, 1, rand_v4()); + if (new_ip) switched_ip = true; + + if (switched_ip) + { + TEST_CHECK(ipv.external_address() == addr1); + } + } + TEST_CHECK(switched_ip); + TEST_CHECK(ipv.external_address() == addr1); +} + +int test_main() +{ + test_random(); + test_two_ips(); + test_one_ip(); + return 0; +} +