Skip to content

Commit

Permalink
scripted-diff: remove ResetI2PPorts() (revert e0a2b39)
Browse files Browse the repository at this point in the history 
`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#22467
  bitcoin#22470

-BEGIN VERIFY SCRIPT-
git show e0a2b39 |git apply -R
-END VERIFY SCRIPT-

Fixes bitcoin#22467
Fixes bitcoin#22470
  • Loading branch information
@vasild
vasild committed Jul 19, 2021
1 parent d3474b8 commit d4b67c8
Show file tree
Hide file tree
Showing 3 changed files with 0 additions and 225 deletions.
98 changes: 0 additions & 98 deletions src/addrman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
#include <addrman.h>

#include <hash.h>
#include <i2p.h>
#include <logging.h>
#include <netaddress.h>
#include <serialize.h>
Expand Down Expand Up @@ -732,100 +731,3 @@ std::vector<bool> 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;
}
}
}
10 changes: 0 additions & 10 deletions src/addrman.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,8 +452,6 @@ class CAddrMan

RemoveInvalid();

ResetI2PPorts();

Check();
}

Expand Down Expand Up @@ -769,14 +767,6 @@ class CAddrMan
//! 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;
};

Expand Down
117 changes: 0 additions & 117 deletions src/test/addrman_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 <addrman.h>
#include <i2p.h>
#include <test/data/asmap.raw.h>
#include <test/util/setup_common.h>
#include <util/asmap.h>
Expand Down Expand Up @@ -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<uint32_t>(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()

0 comments on commit d4b67c8

Please sign in to comment.