addrman: Remove addrman hotfixes #22496

PRs #22179 and #22112 (EDIT: later reverted in #22497) added hotfix code to addrman to remove invalid addresses and mutate the ports of I2P entries after entering into addrman. Those hotfixes included at least two addrman data corruption bugs:

• #22467 (Assertion `nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size()' failed)
• #22470 (Changing I2P ports in addrman may wronly skip some entries from "new" buckets)

Hotfixing addrman is inherently dangerous. There are many members that have implicit assumptions on each others' state, and mutating those directly can lead to violating addrman's internal invariants.

Instead of trying to hotfix addrman, just don't insert any invalid addresses. For now, those are addresses which fail CNetAddr::IsValid().

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

Code Review ACK https://github.com/bitcoin/bitcoin/pull/22496/commits/5dc4a74bad4274caf8d98a42d64385e2ab11c327.

As I2P support hasn't even been released yet, it's much simpler and safer not to insert I2P addresses where port! = 0.

Is this pr for 22.0 release?

As I2P support hasn't even been released yet, it's much simpler and safer not to insert I2P addresses where port! = 0.

The thing is that I2P does support ports other than 0, just not in the currently used SAM proxy version (3.1). But there is no reason future versions of bitcoin core, or other clients, couldn't support I2P with ports. So I'm kind of divided on ignoring them completely, as that will give problems with such addresses not propagating later. See previous discussion here: #21514 (comment) #21514 (comment) .

• In the long term we should keep in addrman and relay I2P addresses with port != 0. If dropping I2P entries with port != 0 is deemed necessary, then it should be a temporary measure for the same reasons e0a2b390c144e123e2fc8a289fdff36815476964 was temporary. I am a little bit inclined to leave I2P ports as they are, now that ResetI2PPorts() has been removed.

• This PR seems to be complicating the already complicated Unserialize(). Complexity is a maintenance killer and a source of bugs. I find the isolated/separate method RemoveInvalid() easier to assess and test. Also it can be called from other places if needed (maybe from Check()?).

In the long term we should keep in addrman and relay I2P addresses with port != 0. If dropping I2P entries with port != 0 is deemed necessary, then it should be a temporary measure for the same reasons e0a2b39 was temporary. I am a little bit inclined to leave I2P ports as they are, now that ResetI2PPorts() has been removed.

I haven't been following the I2P work closely, but I really don't understand the motivation for adding code to addrman in #22112. There are only a handful of people using (pre-release) I2P support, so why was code added to master to patch their addrmans if the intention was to remove that code before release?

I don't understand why we'd want to keep I2P addresses in addrman before bitcoind supports them. With the original version of this PR, it would have been easy to implement that - as soon as bitcoind supports non-port-0 I2P addresses, just remove the line from IsValidForAddrman() that disallows them.

In any case, #22497 was opened shortly after this PR and quickly merged, removing all of the I2P-specific code from addrman, so I've dropped the first commit.

This PR seems to be complicating the already complicated Unserialize(). Complexity is a maintenance killer and a source of bugs. I find the isolated/separate method RemoveInvalid() easier to assess and test. Also it can be called from other places if needed (maybe from Check()?).

This PR is +7/-40 and makes the addrman code simpler rather than more complicated. Currently, addresses are deserialized from peers.dat, added into buckets, and then removed if they're invalid. It's much simpler to just not add them into those buckets if they're invalid.

Removing the v22 milestone. Now that the changes from #22112 have been reverted, this isn't urgent.

Needs OP adjusted?

Needs OP adjusted?

Done

... so why was code added to master to patch their addrmans if the intention was to remove that code before release?

Just to answer the question: the intention was to include it in 22.0, see the commit message of e0a2b390c144e123e2fc8a289fdff36815476964. The reasons why it was removed earlier are stated in the commit message of the removal: d4b67c8ebc2bb7488bcaaccc3a801cdef1cf1678.

utACK 65332b1178c75e1f83415bad24918996a1524866. I tried to reason through scenarios that could introduce inconsistencies with this code, but can't find any.

I'm also working on a POC that replaces addrman's representation with something multiindex based, which is hopefully more easily reviewable for correctness/consistencies. But I'd like the existing addrman PRs in the pipeline to settle first.

Concept ACK

ACK 65332b1178c75e1f83415bad24918996a1524866 - Skipping the addition of invalid addresses (this code was initially added for Tor addrs) rather than adding all the invalids then removing them all when finishing unserializing seems like an improvement. Especially if it can be achieved with less code.

Post-merge ACK.