Skip to content

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

Merged
laanwj merged 2 commits intobitcoin:masterfrom
jonatack:rm-torv2-from-addrinfo
Sep 16, 2021
Merged

cli -addrinfo: drop torv2; torv3 becomes onion per GetNetworkName()#22544
laanwj merged 2 commits intobitcoin:masterfrom
jonatack:rm-torv2-from-addrinfo

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Jul 25, 2021

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

@practicalswift
Copy link
Contributor

practicalswift commented Jul 25, 2021

Concept ACK

ghost
ghost reviewed Jul 25, 2021
View reviewed changes
@sipa
Copy link
Member

sipa commented Jul 26, 2021

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

@jonatack
Copy link
Member Author

jonatack commented Jul 26, 2021 •
edited
Loading

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
Copy link
Member

sipa commented Jul 26, 2021

@jonatack Ah, being consistent with existing RPCs is a good reason.

@jonatack
Copy link
Member Author

jonatack commented Jul 26, 2021 •
edited
Loading

(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
Copy link
Member

laanwj commented Jul 27, 2021 •
edited
Loading

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
Copy link
Member Author

jonatack commented Jul 27, 2021

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
Copy link
Contributor

Zero-1729 commented Jul 28, 2021

Concept ACK

@jonatack
Copy link
Member Author

jonatack commented Jul 29, 2021

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

@tryphe
Copy link
Contributor

tryphe commented Jul 30, 2021

Concept ACK

"tor" seems maybe slightly more descriptive than "onion" but "onion" exists in the code more 🤷‍♂️

@laanwj
Copy link
Member

laanwj commented Aug 2, 2021 •
edited
Loading

"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
Copy link
Member Author

jonatack commented Aug 2, 2021

Yes, I agree. Sorry for the mini-kerfuffle, I think "onion" is fine.

@tryphe
Copy link
Contributor

tryphe commented Aug 3, 2021

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.

@jonatack
Copy link
Member Author

jonatack commented Aug 3, 2021

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

Done. This should be ready now.

Zero-1729
Zero-1729 approved these changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@Zero-1729 Zero-1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 49d503a 🧉

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
klementtan approved these changes Aug 30, 2021
View reviewed changes
Copy link
Contributor

@klementtan klementtan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review and tested ACK 49d503a

{
  "addresses_known": {
    "ipv4": 83,
    "ipv6": 37,
    "onion": 30,
    "i2p": 0,
    "total": 150
  }
}

@practicalswift
Copy link
Contributor

practicalswift commented Sep 4, 2021

cr ACK 49d503a

@laanwj laanwj merged commit 0de84b7 into bitcoin:master Sep 16, 2021
@jonatack jonatack deleted the rm-torv2-from-addrinfo branch September 16, 2021 16:48
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2021
@laanwj
Merge bitcoin#22544: cli -addrinfo: drop torv2; torv3 becomes onion p…
13bcb98 
…er GetN…
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@Zero-1729 Zero-1729 Zero-1729 approved these changes

@klementtan klementtan klementtan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Utils/log/libs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jonatack @practicalswift @sipa @laanwj @Zero-1729 @tryphe @klementtan @DrahtBot