-
Notifications
You must be signed in to change notification settings - Fork 37.7k
p2p: remove tor v2 support #22050
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
p2p: remove tor v2 support #22050
Conversation
A recent IRC discussion for more context. http://www.erisian.com.au/bitcoin-core-dev/log-2021-05-20.html#l-401 |
Concept ACK |
583f202
to
51dd481
Compare
Concept ACK |
Concept ACK. (Thank you for your service, tor v2) |
Concept ACK |
Concept ACK, adding 22.0 label |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
51dd481
to
7cfec99
Compare
7cfec99
to
b1c825b
Compare
as the changes that follow are incompatible with the inputs.
b1c825b
to
5715134
Compare
This seems to be ready. Updated the PR description. Will propose release documentation in a follow-up. |
Light tested ACK, rebased on top of master @ 123b401 Used Tor version 0.4.5.7. with manually configured hidden service and a bitcoin.conf with
|
5715134
to
ef34bda
Compare
Changed
|
utACK ef34bda |
cc @laanwj from #22050 (comment) |
ef34bda
to
f924515
Compare
Changed the torv2-in-ipv6 case to deserialize explicitly to an invalid address which will be ignored, changed the ignore value in generate-seeds.py from
|
f924515
to
5d82a57
Compare
Changes look good to me now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
case BIP155Network::TORV2: | ||
if (address_size == ADDR_TORV2_SIZE) { | ||
m_net = NET_ONION; | ||
return true; | ||
} | ||
throw std::ios_base::failure( | ||
strprintf("BIP155 TORv2 address with length %u (should be %u)", address_size, | ||
ADDR_TORV2_SIZE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(can be ignored)
This change in CNetAddr::SetNetFromBIP155Network()
will now accept and ignore a Tor v2 addresses with bogus length, e.g. 13. whereas we know it should be exactly 10. The comment near the bottom of this function: // Don't throw on addresses with unknown network ids (maybe from the future).
is somewhat incorrect now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll have a look.
…2 support 2ad034a doc: update release notes with removal of tor v2 support (Jon Atack) 49938ee doc: update tor.md with removal of tor v2 support (Jon Atack) Pull request description: Follow-up documentation to #22050 that removed support for Tor version 2 hidden services from Bitcoin Core. ACKs for top commit: laanwj: ACK 2ad034a Tree-SHA512: 0f13f9d1db7e11f1e3d9967b6d17b8dc3144b3ab3a258c706464c5e6ac5cbcf2ce2db4ea54be9939f05a82ebd1e7f325f50b435f9822c08b4f21ed4ac58de0af
…etworkName() 49d503a doc: update -addrinfo in release-notes.md and tor.md (Jon Atack) 75ea9ec cli -addrinfo: drop torv2, torv3 becomes onion per GetNetworkName() (Jon Atack) Pull request description: #22050 removed torv2 support from 22.0. For 23.0 and subsequent releases, we can probably remove torv2 from -addrinfo. before ``` "addresses_known": { "ipv4": 58305, "ipv6": 5138, "torv2": 0, "torv3": 5441, "i2p": 14, "total": 68898 } ``` after ``` "addresses_known": { "ipv4": 58305, "ipv6": 5138, "onion": 5441, "i2p": 14, "total": 68898 } ``` Per the naming of `netbase.{h, cpp}::GetNetworkName()`, torv3 becomes onion, which is what is printed in the output of getpeerinfo, getnetworkinfo and getnodeaddresses. ACKs for top commit: practicalswift: cr ACK 49d503a Zero-1729: tACK 49d503a 🧉 klementtan: Code review and tested ACK 49d503a Tree-SHA512: bca52520d8b12c26f1c329d661b9e22c567954ed2af7d2a16d7669eae1a221eada20944f8b2f4e78e31a7190d5f3d3fbfd37509e5edf2d9a3747a0a8f4e375bb
This patch removes support in Bitcoin Core for Tor v2 onions, which are already removed from the release of Tor 0.4.6.
Tested with tor-0.4.6.1-alpha (no v2 support) and 0.4.5.7 (v2 support). With the latest Tor (no v2 support), this removes all the warnings like those reported with current master in #21351
and the addrman no longer has Tor v2 addresses on launching bitcoind.
After recompiling back to current master and restarting with either of the two Tor versions (0.4.5.7 or 0.4.6.1), -addrinfo initially returns 0 Tor v2 addresses and then begins finding them again.
Ran nodes on this patch over the past week on mainnet/testnet/signet/regtest after building with DEBUG_ADDRMAN.
Verified that this patch bootstraps an onlynet=onion node from the Tor v3 hardcoded fixed seeds on mainnet and testnet and connects to blocks and v3 onion peers:
rm ~/.bitcoin/testnet3/peers.dat ; ./src/bitcoind -testnet -dnsseed=0 -onlynet=onion
Tested using
addnode
,getaddednodeinfo
,addpeeraddress
,disconnectnode
and-addrinfo
that a currently valid, connectable Tor v2 peer can no longer be added:Thanks to Vasil Dimov, Carl Dong, and Wladimir J. van der Laan for their work on BIP155 and Tor v3 that got us here.