Torv2 removal followups #22179

1. 
   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.
2. 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
3. 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
4. 
   vasild commented at 11:09 am on June 7, 2021: member

   cc @jonatack
5. fanquake added the label P2P on Jun 7, 2021
6. 
   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).
7. 
   jonatack commented at 11:21 am on June 7, 2021: member

   Concept ACK
8. 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
9. vasild force-pushed on Jun 7, 2021
10. 
    vasild commented at 12:45 pm on June 7, 2021: member

    26227ae770...00b875ba94: fix test mishap
11. 
    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.

12. 
    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.

13. 
    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).
14. 
    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]”
15. 
    naumenkogs commented at 9:45 am on June 18, 2021: member

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

    Concept ACK
17. 
    jarolrod commented at 0: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

        • 0 test/addrman_tests.cpp(824): error: in "addrman_tests/remove_invalid": check addrman.size() == 2 has failed [4 != 2]
          

18. laanwj added this to the milestone 22.0 on Jul 8, 2021
19. 
    laanwj commented at 3:19 pm on July 8, 2021: member

    Code review and lightly tested ACK 00b875ba9414463d0041da6924fd9b54d6a06dee
20. laanwj merged this on Jul 8, 2021
21. laanwj closed this on Jul 8, 2021

    ---

22. vasild deleted the branch on Jul 8, 2021
23. sidhujag referenced this in commit cd1415ff63 on Jul 10, 2021
24. fanquake referenced this in commit 5cf28d5203 on Aug 3, 2021
25. gwillen referenced this in commit 304587c1e2 on Jun 1, 2022
26. DrahtBot locked this on Aug 16, 2022

Contributors
vasild jonatack laanwj DrahtBot sipa naumenkogs 0xB10C jarolrod

Labels
P2P

Milestone
22.0