cli -addrinfo: drop torv2; torv3 becomes onion per GetNetworkName() #22544

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:rm-torv2-from-addrinfo changing 3 files +9 −9

jonatack commented at 11:07 AM on July 25, 2021: member
#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.
cli -addrinfo: drop torv2, torv3 becomes onion per GetNetworkName() 75ea9ecf11
DrahtBot added the label Utils/log/libs on Jul 25, 2021
practicalswift commented at 8:30 PM on July 25, 2021: contributor

Concept ACK
in src/bitcoin-cli.cpp:248 in 75ea9ecf11 outdated
```
 244 | @@ -245,7 +245,7 @@ class BaseRequestHandler
 245 |  class AddrinfoRequestHandler : public BaseRequestHandler
 246 |  {
 247 |  private:
 248 | -    static constexpr std::array m_networks{"ipv4", "ipv6", "torv2", "torv3", "i2p"};
 249 | +    static constexpr std::array m_networks{"ipv4", "ipv6", "onion", "i2p"};
```
unknown commented at 8:39 PM on July 25, 2021:

nit: maybe we can write torv3/v3 in bracket for few months

static constexpr std::array m_networks{"ipv4", "ipv6", "onion(v3)", "i2p"};
jonatack commented at 12:15 PM on July 27, 2021:

That wouldn't work (but could be done by keeping and modifying the conditional that this patch removes). By the time 23.0 is released in 2022, my thought was that it should be widely known that only v3 onions are supported by Bitcoin Core and current releases of Tor, and "onion" is both simpler to implement and the same as the other RPCs.
sipa commented at 5:57 AM on July 26, 2021: member

Do you think "onion" is clearer than "tor"?
jonatack commented at 7:31 AM on July 26, 2021: member

I don't have a strong opinion, but in the absence of a need to distinguish between tor/onion v2 and v3, using the default naming of GetNetworkName(), getnetworkinfo, getpeerinfo, and getnodeaddresses seemed to make sense and is the simplest to implement.
sipa commented at 8:01 AM on July 26, 2021: member

@jonatack Ah, being consistent with existing RPCs is a good reason.
jonatack commented at 8:06 AM on July 26, 2021: member

(Given that "onion" is used by our Lightning Network colleagues as a more generic term, it may make sense to globally revert our language back to "tor" at some point.)
laanwj commented at 11:43 AM on July 27, 2021: member

Do you think "onion" is clearer than "tor"?

As Tor allows connectivity to IPv4 and IPv6 as well, it's less specific. This was always the reasoning to use "onion" specifically when dealing with hidden services. I don't think lightning also using the term is a good reason to change this.

(So concept ACK anyway)
jonatack commented at 12:09 PM on July 27, 2021: member

As Tor allows connectivity to IPv4 and IPv6 as well, it's less specific. This was always the reasoning to use "onion" specifically when dealing with hidden services. I don't think lightning also using the term is a good reason to change this.

(So concept ACK anyway)

Ah, thanks for the reminder why we did this.
Zero-1729 commented at 7:28 PM on July 28, 2021: contributor

Concept ACK
jonatack commented at 8:58 AM on July 29, 2021: member

(Note to self, this needs an update to doc/tor.md and the release notes.)
tryphe commented at 9:54 AM on July 30, 2021: contributor

Concept ACK

"tor" seems maybe slightly more descriptive than "onion" but "onion" exists in the code more :man_shrugging:
laanwj commented at 10:52 AM on August 2, 2021: member

"tor" seems maybe slightly more descriptive than "onion" but "onion" exists in the code more man_shrugging

The addresses are literally .onion addresses I don't understand why people suddenly don't find that descriptive enough anymore and want to change this around. "Tor hidden service" would work as welll if you really want to avoid that word, but just "Tor" doesn't mean anything as Tor also has exit nodes for plainnet and that's the most common use of Tor.

I guess a compromise could be torhs. But I had the impression that Tor themselves was also moving away from the "hidden service" naming e.g. https://community.torproject.org/onion-services/ doesn't even mention it. tor onion could work, but that might be unnecessary specicifc unless we plan on supporting some other onions, I don't think any other make sense in this context.
jonatack commented at 11:30 AM on August 2, 2021: member

Yes, I agree. Sorry for the mini-kerfuffle, I think "onion" is fine.
tryphe commented at 4:12 AM on August 3, 2021: contributor

The addresses are literally .onion addresses I don't understand why people suddenly don't find that descriptive enough anymore and want to change this around.

I wasn't saying I preferred one or the other, but rather I was neutral to the conversation above and didn't really mind either one.
doc: update -addrinfo in release-notes.md and tor.md 49d503aefa
jonatack commented at 10:14 AM on August 3, 2021: member

(Note to self, this needs an update to doc/tor.md and the release notes.)

Done. This should be ready now.
Zero-1729 approved
Zero-1729 commented at 2:59 PM on August 3, 2021: contributor
tACK 49d503aefa74f11e5d93432987fa3775ed82c979 🧉

LGTM, patch works as described.

Updated Tor docs and release notes are clear and straightforward.
```
$ bitcoin-cli -addrinfo
{
  "addresses_known": {
    "ipv4": 39192,
    "ipv6": 3219,
    "onion": 11,
    "i2p": 0,
    "total": 42422
  }
}
```
klementtan approved
klementtan commented at 6:04 PM on August 30, 2021: contributor
Code review and tested ACK 49d503aefa74f11e5d93432987fa3775ed82c979
```
{
  "addresses_known": {
    "ipv4": 83,
    "ipv6": 37,
    "onion": 30,
    "i2p": 0,
    "total": 150
  }
}
```
practicalswift commented at 1:37 PM on September 4, 2021: contributor

cr ACK 49d503aefa74f11e5d93432987fa3775ed82c979
laanwj merged this on Sep 16, 2021
laanwj closed this on Sep 16, 2021
jonatack deleted the branch on Sep 16, 2021
sidhujag referenced this in commit 13bcb988d7 on Sep 19, 2021
DrahtBot locked this on Oct 30, 2022

Contributors

Labels

Linked (view graph)

#22050 p2p: remove tor v2 support