From d4b67c8ebc2bb7488bcaaccc3a801cdef1cf1678 Mon Sep 17 00:00:00 2001 From: Vasil Dimov Date: Mon, 19 Jul 2021 14:13:35 +0200 Subject: scripted-diff: remove ResetI2PPorts() (revert e0a2b390c14) `CAddrMan::ResetI2PPorts()` was temporary. Remove it: * it has partially achieved its goal: probably ran on about half of the I2P nodes * it is hackish, deemed risky and two bugs where found in it https://github.com/bitcoin/bitcoin/issues/22467 https://github.com/bitcoin/bitcoin/issues/22470 -BEGIN VERIFY SCRIPT- git show e0a2b390c144e123e2fc8a289fdff36815476964 |git apply -R -END VERIFY SCRIPT- Fixes https://github.com/bitcoin/bitcoin/issues/22467 Fixes https://github.com/bitcoin/bitcoin/issues/22470 --- src/addrman.cpp | 98 ------------------------------------- src/addrman.h | 10 ---- src/test/addrman_tests.cpp | 117 --------------------------------------------- 3 files changed, 225 deletions(-) diff --git a/src/addrman.cpp b/src/addrman.cpp index 389d106164..8192b4eba6 100644 --- a/src/addrman.cpp +++ b/src/addrman.cpp @@ -6,7 +6,6 @@ #include #include -#include #include #include #include @@ -732,100 +731,3 @@ std::vector CAddrMan::DecodeAsmap(fs::path path) } return bits; } - -void CAddrMan::ResetI2PPorts() -{ - for (int bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; ++bucket) { - for (int i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { - const auto id = vvNew[bucket][i]; - if (id == -1) { - continue; - } - auto it = mapInfo.find(id); - if (it == mapInfo.end()) { - return; - } - auto& addr_info = it->second; - if (!addr_info.IsI2P() || addr_info.GetPort() == I2P_SAM31_PORT) { - continue; - } - - auto addr_info_newport = addr_info; - // The below changes addr_info_newport.GetKey(), which is used in finding a - // bucket and a position within that bucket. So a re-bucketing may be necessary. - addr_info_newport.port = I2P_SAM31_PORT; - - // Reposition entries of vvNew within the same bucket because we don't know the source - // address which led to the decision to store the entry in vvNew[bucket] so we can't - // re-evaluate that decision, but even if we could, CAddrInfo::GetNewBucket() does not - // use CAddrInfo::GetKey() so it would end up in the same bucket as before the port - // change. - const auto i_target = addr_info_newport.GetBucketPosition(nKey, true, bucket); - - if (i_target == i) { // No need to re-position. - addr_info = addr_info_newport; - continue; - } - - // Reposition from i to i_target, removing the entry from i_target (if any). - ClearNew(bucket, i_target); - vvNew[bucket][i_target] = id; - vvNew[bucket][i] = -1; - addr_info = addr_info_newport; - } - } - - for (int bucket = 0; bucket < ADDRMAN_TRIED_BUCKET_COUNT; ++bucket) { - for (int i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) { - const auto id = vvTried[bucket][i]; - if (id == -1) { - continue; - } - auto it = mapInfo.find(id); - if (it == mapInfo.end()) { - return; - } - auto& addr_info = it->second; - if (!addr_info.IsI2P() || addr_info.GetPort() == I2P_SAM31_PORT) { - continue; - } - - auto addr_info_newport = addr_info; - // The below changes addr_info_newport.GetKey(), which is used in finding a - // bucket and a position within that bucket. So a re-bucketing may be necessary. - addr_info_newport.port = I2P_SAM31_PORT; - - const auto bucket_target = addr_info_newport.GetTriedBucket(nKey, m_asmap); - const auto i_target = addr_info_newport.GetBucketPosition(nKey, false, bucket_target); - - if (bucket_target == bucket && i_target == i) { // No need to re-position. - addr_info = addr_info_newport; - continue; - } - - // Reposition from (bucket, i) to (bucket_target, i_target). If the latter is - // occupied, then move the entry from there to vvNew. - - const auto old_target_id = vvTried[bucket_target][i_target]; - if (old_target_id != -1) { - CAddrInfo& old_target_info = mapInfo[old_target_id]; - - old_target_info.fInTried = false; - vvTried[bucket_target][i_target] = -1; - --nTried; - - const auto new_bucket = old_target_info.GetNewBucket(nKey, m_asmap); - const auto new_bucket_i = old_target_info.GetBucketPosition(nKey, true, new_bucket); - ClearNew(new_bucket, new_bucket_i); - - old_target_info.nRefCount = 1; - vvNew[new_bucket][new_bucket_i] = old_target_id; - ++nNew; - } - - vvTried[bucket_target][i_target] = id; - vvTried[bucket][i] = -1; - addr_info = addr_info_newport; - } - } -} diff --git a/src/addrman.h b/src/addrman.h index 2a5c6c06b4..c2f425f2fa 100644 --- a/src/addrman.h +++ b/src/addrman.h @@ -452,8 +452,6 @@ public: RemoveInvalid(); - ResetI2PPorts(); - Check(); } @@ -769,14 +767,6 @@ private: //! Remove invalid addresses. void RemoveInvalid() EXCLUSIVE_LOCKS_REQUIRED(cs); - /** - * Reset the ports of I2P peers to 0. - * This is needed as a temporary measure because now we enforce port 0 and - * only connect to I2P hosts if the port is 0, but in the early days some - * I2P addresses with port 8333 were rumoured and persisted into addrmans. - */ - void ResetI2PPorts() EXCLUSIVE_LOCKS_REQUIRED(cs); - friend class CAddrManTest; }; diff --git a/src/test/addrman_tests.cpp b/src/test/addrman_tests.cpp index f2eed956cd..1103292c1a 100644 --- a/src/test/addrman_tests.cpp +++ b/src/test/addrman_tests.cpp @@ -2,7 +2,6 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include -#include #include #include #include @@ -967,121 +966,5 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks) BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0"); } -BOOST_AUTO_TEST_CASE(reset_i2p_ports) -{ - CAddrManTest addrman1; - CAddrManTest addrman2; - const uint32_t good_time{static_cast(GetAdjustedTime())}; - constexpr uint16_t port = 8333; - - // Has its port changed, will be re-positioned within the same bucket in vvNew. - const CAddress i2p_new1{ - ResolveService("72l3ucjkuscrbiiepoehuwqgknyzgo7zuix5ty4puwrkyhtmnsga.b32.i2p", port), - NODE_NONE, - good_time}; - - // Has its port changed, will not be re-positioned in vvNew because ports 0 and 10075 result in - // the same bucket position. - const CAddress i2p_new2{ - ResolveService("gehtac45oaghz54ypyopim64mql7oad2bqclla74l6tfeolzmodq.b32.i2p", 10075), - NODE_NONE, - good_time}; - - // Remains unchanged, port is already as it should be. - const CAddress i2p_new3{ - ResolveService("c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p", - I2P_SAM31_PORT), - NODE_NONE, - good_time}; - - // Has its port changed, re-positioning in vvNew will cause i2p_new3 to be evicted. - const CAddress i2p_new4{ - ResolveService("c4cbbkn46qxftwja53pxiykntegfyfjqtnzbm6iv6r5mungmqgmq.b32.i2p", port), - NODE_NONE, - good_time}; - - // Remains unchanged. - const CAddress ipv4_new{ResolveService("1.2.3.4", port), NODE_NONE, good_time}; - - // Has its port changed, will be re-positioned in vvTried. - const CAddress i2p_tried1{ - ResolveService("h3r6bkn46qxftwja53pxiykntegfyfjqtnzbm6iv6r5mungmqgmq.b32.i2p", port), - NODE_NONE, - good_time}; - - // Has its port changed, will not be re-positioned in vvTried because ports 0 and 10537 - // result in the same position (bucket, i). - const CAddress i2p_tried2{ - ResolveService("pjs7or2ctvteeo5tu4bwyrtydeuhqhvdprtujn4daxr75jpebjxa.b32.i2p", 10537), - NODE_NONE, - good_time}; - - // Remains unchanged, port is already as it should be. - const CAddress i2p_tried3{ - ResolveService("hnbbyjpxx54623l555sta7pocy3se4sdgmuebi5k6reesz5rjp6q.b32.i2p", - I2P_SAM31_PORT), - NODE_NONE, - good_time}; - - // Has its port changed, re-positioning in vvTried will cause i2p_tried3 to be moved to vvNew. - const CAddress i2p_tried4{ - ResolveService("hna37nqr3ahkqv62cuqfwgtneekvvpnuc4i4f6yo7tpoqjswvcwa.b32.i2p", port), - NODE_NONE, - good_time}; - - // Remains unchanged. - const CAddress ipv4_tried{ResolveService("2.3.4.5", port), NODE_NONE, good_time}; - - const CNetAddr source; - - CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); - - addrman1.Add(i2p_new1, source); - addrman1.Add(i2p_new2, source); - addrman1.Add(i2p_new3, source); - addrman1.Add(i2p_new4, source); - addrman1.Add(ipv4_new, source); - - addrman1.Add(i2p_tried1, source); - addrman1.Good(i2p_tried1); - addrman1.Add(i2p_tried2, source); - addrman1.Good(i2p_tried2); - addrman1.Add(i2p_tried3, source); - addrman1.Good(i2p_tried3); - addrman1.Add(i2p_tried4, source); - addrman1.Good(i2p_tried4); - addrman1.Add(ipv4_tried, source); - addrman1.Good(ipv4_tried); - - stream << addrman1; - stream >> addrman2; - - const size_t max_addresses{0}; - const size_t max_pct{0}; - - auto addresses = addrman2.GetAddr(max_addresses, max_pct, NET_I2P); - BOOST_REQUIRE_EQUAL(addresses.size(), 7UL); - std::sort(addresses.begin(), addresses.end()); // Just some deterministic order. - BOOST_CHECK_EQUAL(addresses[0].ToStringIP(), i2p_new4.ToStringIP()); - BOOST_CHECK_EQUAL(addresses[0].GetPort(), I2P_SAM31_PORT); - BOOST_CHECK_EQUAL(addresses[1].ToStringIP(), i2p_new2.ToStringIP()); - BOOST_CHECK_EQUAL(addresses[1].GetPort(), I2P_SAM31_PORT); - BOOST_CHECK_EQUAL(addresses[2].ToStringIP(), i2p_tried4.ToStringIP()); - BOOST_CHECK_EQUAL(addresses[2].GetPort(), I2P_SAM31_PORT); - BOOST_CHECK_EQUAL(addresses[3].ToStringIP(), i2p_tried3.ToStringIP()); - BOOST_CHECK_EQUAL(addresses[3].GetPort(), I2P_SAM31_PORT); - BOOST_CHECK_EQUAL(addresses[4].ToStringIP(), i2p_tried1.ToStringIP()); - BOOST_CHECK_EQUAL(addresses[4].GetPort(), I2P_SAM31_PORT); - BOOST_CHECK_EQUAL(addresses[5].ToStringIP(), i2p_tried2.ToStringIP()); - BOOST_CHECK_EQUAL(addresses[5].GetPort(), I2P_SAM31_PORT); - BOOST_CHECK_EQUAL(addresses[6].ToStringIP(), i2p_new1.ToStringIP()); - BOOST_CHECK_EQUAL(addresses[6].GetPort(), I2P_SAM31_PORT); - - addresses = addrman2.GetAddr(max_addresses, max_pct, NET_IPV4); - BOOST_REQUIRE_EQUAL(addresses.size(), 2UL); - std::sort(addresses.begin(), addresses.end()); // Just some deterministic order. - BOOST_CHECK_EQUAL(addresses[0].ToStringIPPort(), ipv4_new.ToStringIPPort()); - BOOST_CHECK_EQUAL(addresses[1].ToStringIPPort(), ipv4_tried.ToStringIPPort()); -} BOOST_AUTO_TEST_SUITE_END() -- cgit v1.2.3