Skip to content

Commit 624a193

Browse files
fanquakefanquake
committed
Merge bitcoin/bitcoin#22497: scripted-diff: remove ResetI2PPorts() (revert e0a2b39)
d4b67c8 scripted-diff: remove ResetI2PPorts() (revert e0a2b39) (Vasil Dimov) Pull request description: `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: bitcoin/bitcoin#22467 bitcoin/bitcoin#22470 -BEGIN VERIFY SCRIPT- git show e0a2b39 |git apply -R -END VERIFY SCRIPT- Fixes bitcoin/bitcoin#22467 Fixes bitcoin/bitcoin#22470 ACKs for top commit: laanwj: ACK d4b67c8 MarcoFalke: review ACK d4b67c8 😲 jonatack: ACK d4b67c8 per IRC discussions https://www.erisian.com.au/bitcoin-core-dev/log-2021-07-16.html#l-212 and https://www.erisian.com.au/bitcoin-core-dev/log-2021-07-19.html#l-210 Tree-SHA512: 60d8f0ea0f66a8fcedfcb9c8944a419b974b15509b54ddfeec58db49ae9418e6916df712bba3fbd6b29497d85f7951fb9aa2e48eb9c59f88d09435685bd00b4c
2 parents 54e3174 + d4b67c8 commit 624a193

File tree

3 files changed

+0
-225
lines changed

3 files changed

+0
-225
lines changed

‎src/addrman.cpp

Lines changed: 0 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
#include <addrman.h>
77

88
#include <hash.h>
9-
#include <i2p.h>
109
#include <logging.h>
1110
#include <netaddress.h>
1211
#include <serialize.h>
@@ -732,100 +731,3 @@ std::vector<bool> CAddrMan::DecodeAsmap(fs::path path)
732731
}
733732
return bits;
734733
}
735-
736-
void CAddrMan::ResetI2PPorts()
737-
{
738-
for (int bucket = 0; bucket < ADDRMAN_NEW_BUCKET_COUNT; ++bucket) {
739-
for (int i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
740-
const auto id = vvNew[bucket][i];
741-
if (id == -1) {
742-
continue;
743-
}
744-
auto it = mapInfo.find(id);
745-
if (it == mapInfo.end()) {
746-
return;
747-
}
748-
auto& addr_info = it->second;
749-
if (!addr_info.IsI2P() || addr_info.GetPort() == I2P_SAM31_PORT) {
750-
continue;
751-
}
752-
753-
auto addr_info_newport = addr_info;
754-
// The below changes addr_info_newport.GetKey(), which is used in finding a
755-
// bucket and a position within that bucket. So a re-bucketing may be necessary.
756-
addr_info_newport.port = I2P_SAM31_PORT;
757-
758-
// Reposition entries of vvNew within the same bucket because we don't know the source
759-
// address which led to the decision to store the entry in vvNew[bucket] so we can't
760-
// re-evaluate that decision, but even if we could, CAddrInfo::GetNewBucket() does not
761-
// use CAddrInfo::GetKey() so it would end up in the same bucket as before the port
762-
// change.
763-
const auto i_target = addr_info_newport.GetBucketPosition(nKey, true, bucket);
764-
765-
if (i_target == i) { // No need to re-position.
766-
addr_info = addr_info_newport;
767-
continue;
768-
}
769-
770-
// Reposition from i to i_target, removing the entry from i_target (if any).
771-
ClearNew(bucket, i_target);
772-
vvNew[bucket][i_target] = id;
773-
vvNew[bucket][i] = -1;
774-
addr_info = addr_info_newport;
775-
}
776-
}
777-
778-
for (int bucket = 0; bucket < ADDRMAN_TRIED_BUCKET_COUNT; ++bucket) {
779-
for (int i = 0; i < ADDRMAN_BUCKET_SIZE; ++i) {
780-
const auto id = vvTried[bucket][i];
781-
if (id == -1) {
782-
continue;
783-
}
784-
auto it = mapInfo.find(id);
785-
if (it == mapInfo.end()) {
786-
return;
787-
}
788-
auto& addr_info = it->second;
789-
if (!addr_info.IsI2P() || addr_info.GetPort() == I2P_SAM31_PORT) {
790-
continue;
791-
}
792-
793-
auto addr_info_newport = addr_info;
794-
// The below changes addr_info_newport.GetKey(), which is used in finding a
795-
// bucket and a position within that bucket. So a re-bucketing may be necessary.
796-
addr_info_newport.port = I2P_SAM31_PORT;
797-
798-
const auto bucket_target = addr_info_newport.GetTriedBucket(nKey, m_asmap);
799-
const auto i_target = addr_info_newport.GetBucketPosition(nKey, false, bucket_target);
800-
801-
if (bucket_target == bucket && i_target == i) { // No need to re-position.
802-
addr_info = addr_info_newport;
803-
continue;
804-
}
805-
806-
// Reposition from (bucket, i) to (bucket_target, i_target). If the latter is
807-
// occupied, then move the entry from there to vvNew.
808-
809-
const auto old_target_id = vvTried[bucket_target][i_target];
810-
if (old_target_id != -1) {
811-
CAddrInfo& old_target_info = mapInfo[old_target_id];
812-
813-
old_target_info.fInTried = false;
814-
vvTried[bucket_target][i_target] = -1;
815-
--nTried;
816-
817-
const auto new_bucket = old_target_info.GetNewBucket(nKey, m_asmap);
818-
const auto new_bucket_i = old_target_info.GetBucketPosition(nKey, true, new_bucket);
819-
ClearNew(new_bucket, new_bucket_i);
820-
821-
old_target_info.nRefCount = 1;
822-
vvNew[new_bucket][new_bucket_i] = old_target_id;
823-
++nNew;
824-
}
825-
826-
vvTried[bucket_target][i_target] = id;
827-
vvTried[bucket][i] = -1;
828-
addr_info = addr_info_newport;
829-
}
830-
}
831-
}

‎src/addrman.h

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -458,8 +458,6 @@ class CAddrMan
458458

459459
RemoveInvalid();
460460

461-
ResetI2PPorts();
462-
463461
Check();
464462
}
465463

@@ -775,14 +773,6 @@ class CAddrMan
775773
//! Remove invalid addresses.
776774
void RemoveInvalid() EXCLUSIVE_LOCKS_REQUIRED(cs);
777775

778-
/**
779-
* Reset the ports of I2P peers to 0.
780-
* This is needed as a temporary measure because now we enforce port 0 and
781-
* only connect to I2P hosts if the port is 0, but in the early days some
782-
* I2P addresses with port 8333 were rumoured and persisted into addrmans.
783-
*/
784-
void ResetI2PPorts() EXCLUSIVE_LOCKS_REQUIRED(cs);
785-
786776
friend class CAddrManTest;
787777
};
788778

‎src/test/addrman_tests.cpp

Lines changed: 0 additions & 117 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// Distributed under the MIT software license, see the accompanying
33
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
44
#include <addrman.h>
5-
#include <i2p.h>
65
#include <test/data/asmap.raw.h>
76
#include <test/util/setup_common.h>
87
#include <util/asmap.h>
@@ -967,121 +966,5 @@ BOOST_AUTO_TEST_CASE(addrman_evictionworks)
967966
BOOST_CHECK(addrman.SelectTriedCollision().ToString() == "[::]:0");
968967
}
969968

970-
BOOST_AUTO_TEST_CASE(reset_i2p_ports)
971-
{
972-
CAddrManTest addrman1;
973-
CAddrManTest addrman2;
974-
const uint32_t good_time{static_cast<uint32_t>(GetAdjustedTime())};
975-
constexpr uint16_t port = 8333;
976-
977-
// Has its port changed, will be re-positioned within the same bucket in vvNew.
978-
const CAddress i2p_new1{
979-
ResolveService("72l3ucjkuscrbiiepoehuwqgknyzgo7zuix5ty4puwrkyhtmnsga.b32.i2p", port),
980-
NODE_NONE,
981-
good_time};
982-
983-
// Has its port changed, will not be re-positioned in vvNew because ports 0 and 10075 result in
984-
// the same bucket position.
985-
const CAddress i2p_new2{
986-
ResolveService("gehtac45oaghz54ypyopim64mql7oad2bqclla74l6tfeolzmodq.b32.i2p", 10075),
987-
NODE_NONE,
988-
good_time};
989-
990-
// Remains unchanged, port is already as it should be.
991-
const CAddress i2p_new3{
992-
ResolveService("c4gfnttsuwqomiygupdqqqyy5y5emnk5c73hrfvatri67prd7vyq.b32.i2p",
993-
I2P_SAM31_PORT),
994-
NODE_NONE,
995-
good_time};
996-
997-
// Has its port changed, re-positioning in vvNew will cause i2p_new3 to be evicted.
998-
const CAddress i2p_new4{
999-
ResolveService("c4cbbkn46qxftwja53pxiykntegfyfjqtnzbm6iv6r5mungmqgmq.b32.i2p", port),
1000-
NODE_NONE,
1001-
good_time};
1002-
1003-
// Remains unchanged.
1004-
const CAddress ipv4_new{ResolveService("1.2.3.4", port), NODE_NONE, good_time};
1005-
1006-
// Has its port changed, will be re-positioned in vvTried.
1007-
const CAddress i2p_tried1{
1008-
ResolveService("h3r6bkn46qxftwja53pxiykntegfyfjqtnzbm6iv6r5mungmqgmq.b32.i2p", port),
1009-
NODE_NONE,
1010-
good_time};
1011-
1012-
// Has its port changed, will not be re-positioned in vvTried because ports 0 and 10537
1013-
// result in the same position (bucket, i).
1014-
const CAddress i2p_tried2{
1015-
ResolveService("pjs7or2ctvteeo5tu4bwyrtydeuhqhvdprtujn4daxr75jpebjxa.b32.i2p", 10537),
1016-
NODE_NONE,
1017-
good_time};
1018-
1019-
// Remains unchanged, port is already as it should be.
1020-
const CAddress i2p_tried3{
1021-
ResolveService("hnbbyjpxx54623l555sta7pocy3se4sdgmuebi5k6reesz5rjp6q.b32.i2p",
1022-
I2P_SAM31_PORT),
1023-
NODE_NONE,
1024-
good_time};
1025-
1026-
// Has its port changed, re-positioning in vvTried will cause i2p_tried3 to be moved to vvNew.
1027-
const CAddress i2p_tried4{
1028-
ResolveService("hna37nqr3ahkqv62cuqfwgtneekvvpnuc4i4f6yo7tpoqjswvcwa.b32.i2p", port),
1029-
NODE_NONE,
1030-
good_time};
1031-
1032-
// Remains unchanged.
1033-
const CAddress ipv4_tried{ResolveService("2.3.4.5", port), NODE_NONE, good_time};
1034-
1035-
const CNetAddr source;
1036-
1037-
CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
1038-
1039-
addrman1.Add(i2p_new1, source);
1040-
addrman1.Add(i2p_new2, source);
1041-
addrman1.Add(i2p_new3, source);
1042-
addrman1.Add(i2p_new4, source);
1043-
addrman1.Add(ipv4_new, source);
1044-
1045-
addrman1.Add(i2p_tried1, source);
1046-
addrman1.Good(i2p_tried1);
1047-
addrman1.Add(i2p_tried2, source);
1048-
addrman1.Good(i2p_tried2);
1049-
addrman1.Add(i2p_tried3, source);
1050-
addrman1.Good(i2p_tried3);
1051-
addrman1.Add(i2p_tried4, source);
1052-
addrman1.Good(i2p_tried4);
1053-
addrman1.Add(ipv4_tried, source);
1054-
addrman1.Good(ipv4_tried);
1055-
1056-
stream << addrman1;
1057-
stream >> addrman2;
1058-
1059-
const size_t max_addresses{0};
1060-
const size_t max_pct{0};
1061-
1062-
auto addresses = addrman2.GetAddr(max_addresses, max_pct, NET_I2P);
1063-
BOOST_REQUIRE_EQUAL(addresses.size(), 7UL);
1064-
std::sort(addresses.begin(), addresses.end()); // Just some deterministic order.
1065-
BOOST_CHECK_EQUAL(addresses[0].ToStringIP(), i2p_new4.ToStringIP());
1066-
BOOST_CHECK_EQUAL(addresses[0].GetPort(), I2P_SAM31_PORT);
1067-
BOOST_CHECK_EQUAL(addresses[1].ToStringIP(), i2p_new2.ToStringIP());
1068-
BOOST_CHECK_EQUAL(addresses[1].GetPort(), I2P_SAM31_PORT);
1069-
BOOST_CHECK_EQUAL(addresses[2].ToStringIP(), i2p_tried4.ToStringIP());
1070-
BOOST_CHECK_EQUAL(addresses[2].GetPort(), I2P_SAM31_PORT);
1071-
BOOST_CHECK_EQUAL(addresses[3].ToStringIP(), i2p_tried3.ToStringIP());
1072-
BOOST_CHECK_EQUAL(addresses[3].GetPort(), I2P_SAM31_PORT);
1073-
BOOST_CHECK_EQUAL(addresses[4].ToStringIP(), i2p_tried1.ToStringIP());
1074-
BOOST_CHECK_EQUAL(addresses[4].GetPort(), I2P_SAM31_PORT);
1075-
BOOST_CHECK_EQUAL(addresses[5].ToStringIP(), i2p_tried2.ToStringIP());
1076-
BOOST_CHECK_EQUAL(addresses[5].GetPort(), I2P_SAM31_PORT);
1077-
BOOST_CHECK_EQUAL(addresses[6].ToStringIP(), i2p_new1.ToStringIP());
1078-
BOOST_CHECK_EQUAL(addresses[6].GetPort(), I2P_SAM31_PORT);
1079-
1080-
addresses = addrman2.GetAddr(max_addresses, max_pct, NET_IPV4);
1081-
BOOST_REQUIRE_EQUAL(addresses.size(), 2UL);
1082-
std::sort(addresses.begin(), addresses.end()); // Just some deterministic order.
1083-
BOOST_CHECK_EQUAL(addresses[0].ToStringIPPort(), ipv4_new.ToStringIPPort());
1084-
BOOST_CHECK_EQUAL(addresses[1].ToStringIPPort(), ipv4_tried.ToStringIPPort());
1085-
}
1086969

1087970
BOOST_AUTO_TEST_SUITE_END()

0 commit comments

Comments
 (0)