Torv2 removal followups #22179

pull vasild wants to merge 3 commits into bitcoin:master from vasild:torv2_removal_followup changing 5 files +79 −2

vasild commented at 11:07 AM on June 7, 2021: member
- Simplify some code, now that we know CNetAddr::IsRFC4193() and CNetAddr::IsTor() cannot be true at the same time.
- Drop Tor v2 addresses when loading addrman from peers.dat - they would have been loaded as dummy-all-zeros IPv6 addresses and linger in addrman, wasting space.

net: simplify CNetAddr::IsRoutable()

Reduce the condition `IsRFC4193() && !IsTor()` to `IsRFC4193()`. We know
that if `IsRFC4193()` is `true` then, for sure, the address is not Tor,
so `!IsTor()` is also `true`.

a164cd3ba6

fuzz: reduce possible networks check

If an address classifies as `IsRFC4193()`, then it cannot be
`NET_ONION` (Tor v3), thus remove that condition from the assert.

bdb62096f0

vasild commented at 11:09 AM on June 7, 2021: member

cc @jonatack
fanquake added the label P2P on Jun 7, 2021
jonatack commented at 11:18 AM on June 7, 2021: member

Thanks! Will have a look after tomorrow (Chaincode seminar, Optech writing and work interviews today and tomorrow).
jonatack commented at 11:21 AM on June 7, 2021: member

Concept ACK

addrman: remove invalid addresses when unserializing

The Tor v2 addresses, left over from when Tor v2 was supported will be
unserialized as a dummy, invalid `::` (all zeros) IPv6 address. Remove
them so that they do not take up space in addrman.

00b875ba94

vasild force-pushed on Jun 7, 2021
vasild commented at 12:45 PM on June 7, 2021: member

26227ae770...00b875ba94: fix test mishap
laanwj commented at 1:54 PM on June 7, 2021: member

Concept ACK

they would have been loaded as dummy-all-zeros IPv6 addresses and linger in addrman, wasting space.

Good catch.
DrahtBot commented at 2:47 PM on June 7, 2021: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #22112 by vasild
- #19985 by n-thumann
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
sipa commented at 2:31 AM on June 8, 2021: member

ACK 00b875ba9414463d0041da6924fd9b54d6a06dee. Reviewed the code, and tested with -DDEBUG_ADDRMAN (unit tests + mainnet run with peers.dat that contained v2 onions).
jonatack commented at 3:31 PM on June 9, 2021: member

ACK 00b875ba9414463d0041da6924fd9b54d6a06dee reviewed, debug-built with -DEBUG_ADDRMAN rebased to current master, restarted node on mainnet/signet/testnet and verified that on each chain -addrinfo shows no change in address counts (as expected). Added some sanity check asserts, rebuilt/re-ran test. Checked that the new test fails on master with "test/addrman_tests.cpp(824): error: in "addrman_tests/remove_invalid": check addrman.size() == 2 has failed [4 != 2]"
naumenkogs commented at 9:45 AM on June 18, 2021: member

Concept ACK.
0xB10C commented at 2:26 PM on June 22, 2021: member

Concept ACK
jarolrod commented at 12:55 AM on June 23, 2021: member
ACK 00b875ba9414463d0041da6924fd9b54d6a06dee

Cherry-picked each commit on top of master, and test compile + run unit tests. Tested on Linux and macOS connected through tor.

Notes on commits
- a164cd3ba694ffeba03b2887a411b7f82f6c087e
  
  checking for !isTor() is in fact redundant as IsRFC4193 calls IsIPV6. Since we've already determined that m_net is NET_IPV6, it is redundant to then check that it is not NET_ONION
- bdb62096f0109b2ec76849d33d6cf7187dea299f
  
  makes sense to remove this here 🥃
  
  code review ack only, did not fuzz
- 00b875ba9414463d0041da6924fd9b54d6a06dee
  
  Code Review ACK
  
  copied and pasted the test added here in addrman_tests.cpp onto master and confirm that the test would fail
  
  test/addrman_tests.cpp(824): error: in "addrman_tests/remove_invalid": check addrman.size() == 2 has failed [4 != 2]
laanwj added this to the milestone 22.0 on Jul 8, 2021
laanwj commented at 3:19 PM on July 8, 2021: member

Code review and lightly tested ACK 00b875ba9414463d0041da6924fd9b54d6a06dee
laanwj merged this on Jul 8, 2021
laanwj closed this on Jul 8, 2021
vasild deleted the branch on Jul 8, 2021
sidhujag referenced this in commit cd1415ff63 on Jul 10, 2021
fanquake referenced this in commit 5cf28d5203 on Aug 3, 2021
gwillen referenced this in commit 304587c1e2 on Jun 1, 2022
DrahtBot locked this on Aug 16, 2022