Torv2 removal followups #22179
Torv2 removal followups #22179
Conversation
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`.
If an address classifies as `IsRFC4193()`, then it cannot be `NET_ONION` (Tor v3), thus remove that condition from the assert.
|
cc @jonatack |
|
Thanks! Will have a look after tomorrow (Chaincode seminar, Optech writing and work interviews today and tomorrow). |
|
Concept ACK |
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.
vasild
force-pushed
the
torv2_removal_followup
branch
from
26227ae
to
00b875b
Compare
|
|
|
Concept ACK
Good catch. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
ACK 00b875b. Reviewed the code, and tested with -DDEBUG_ADDRMAN (unit tests + mainnet run with peers.dat that contained v2 onions). |
There was a problem hiding this comment.
ACK 00b875b 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]"
|
Concept ACK. |
|
Concept ACK |
There was a problem hiding this comment.
ACK 00b875b
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
- a164cd3
- checking for
!isTor()is in fact redundant asIsRFC4193callsIsIPV6. Since we've already determined thatm_netisNET_IPV6, it is redundant to then check that it is notNET_ONION
- checking for
- bdb6209
- makes sense to remove this here 🥃
- code review ack only, did not fuzz
- 00b875b
- Code Review ACK
- copied and pasted the test added here in
addrman_tests.cpponto 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]
-
|
Code review and lightly tested ACK 00b875b |
65332b1 [addrman] Remove RemoveInvalid() (John Newbery) Pull request description: 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()`. ACKs for top commit: sipa: utACK 65332b1. I tried to reason through scenarios that could introduce inconsistencies with this code, but can't find any. fanquake: ACK 65332b1 - 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. Tree-SHA512: 023113764cb475572f15da7bf9824b62b79e10a7e359af2eee59017df354348d2aeed88de0fd4ad7a9f89a0dad10827f99d70af6f1cb20abb0eca2714689c8d7
CNetAddr::IsRFC4193()andCNetAddr::IsTor()cannot betrueat the same time.peers.dat- they would have been loaded as dummy-all-zeros IPv6 addresses and linger in addrman, wasting space.