doc: remove Tor link & generalize onion getnodeaddresses RPC #27719

MarnixCroes commented at 2:18 PM on May 22, 2023: contributor

remove broken link about how to properly configure tor
generalize getnodeaddresses RPC in doc

DrahtBot commented at 2:18 PM on May 22, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fanquake
Concept ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Docs on May 22, 2023

in doc/tor.md:5 in ea033133c7 outdated

   1 | @@ -2,7 +2,7 @@
   2 |  
   3 |  It is possible to run Bitcoin Core as a Tor onion service, and connect to such services.
   4 |  
   5 | -The following directions assume you have a Tor proxy running on port 9050. Many distributions default to having a SOCKS proxy listening on port 9050, but others may not. In particular, the Tor Browser Bundle defaults to listening on port 9150. See [Tor Project FAQ:TBBSocksPort](https://www.torproject.org/docs/faq.html.en#TBBSocksPort) for how to properly
   6 | +The following directions assume you have a Tor proxy running on port 9050. Many distributions default to having a SOCKS proxy listening on port 9050, but others may not. In particular, the Tor Browser Bundle defaults to listening on port 9150. See [Tor Project: Set Up Your Onion Service](https://community.torproject.org/onion-services/setup/) for how to properly

jonatack commented at 11:28 AM on May 31, 2023:

Good catch on the obsolete link. The proposed new link doesn't seem relevant, however. I didn't immediately see a good replacement. For less future maintenance, maybe just drop this "See..." sentence from the doc.

jonatack commented at 12:23 PM on June 1, 2023:

Now that the link has been removed, please update the PR title and description from "fix" to "remove" (and remove "onion")

in doc/tor.md:31 in ea033133c7 outdated

  26 | @@ -27,8 +27,8 @@ CLI `-addrinfo` returns the number of addresses known to your node per
  27 |  network. This can be useful to see how many onion peers your node knows,
  28 |  e.g. for `-onlynet=onion`.
  29 |  
  30 | -To fetch a number of onion addresses that your node knows, for example seven
  31 | -addresses, use the `getnodeaddresses 7 onion` RPC.
  32 | +To see which onion addresses your node knows, use the `getnodeaddresses 0 onion`
  33 | +RPC.

jonatack commented at 11:44 AM on May 31, 2023:

This was done because there are many peers returned by ./src/bitcoin-cli getnodeaddresses 0 onion, and at the time, only a few hundred I2P ones and a handful of CJDNS ones.

Perhaps it would be best to make the I2P doc like the Tor doc here, or combine both approaches for all three docs (Tor, I2P, CJDNS), i.e. something like:

To fetch a number of onion peers that your node knows, for example 7 addresses, run
`bitcoin-cli getnodeaddresses 7 onion`. You may also run
`bitcoin-cli getnodeaddresses 0 onion` to return all of them. These results, as well
as those of `-addrinfo`, are filtered for quality and recency.

jonatack commented at 11:48 AM on May 31, 2023: contributor

Concept ACK

MarnixCroes force-pushed on Jun 1, 2023

MarnixCroes commented at 9:24 AM on June 1, 2023: contributor

@jonatack thank you! I've applied your suggestions

in doc/cjdns.md:117 in 79cd3dcc26 outdated

 111 | @@ -112,5 +112,7 @@ There are several ways to see your CJDNS address in Bitcoin Core:
 112 |  To see which CJDNS peers your node is connected to, use `bitcoin-cli -netinfo 4`
 113 |  or the `getpeerinfo` RPC (i.e. `bitcoin-cli getpeerinfo`).
 114 |  
 115 | -To see which CJDNS addresses your node knows, use the `getnodeaddresses 0 cjdns`
 116 | -RPC.
 117 | +To fetch a number of CJDNS peers that your node knows, for example 7 addresses, run
 118 | +`bitcoin-cli getnodeaddresses 7 cjdns`. You may also run
 119 | +`bitcoin-cli getnodeaddresses 0 cjdns` to return all of them. These results, as well

fanquake commented at 9:50 AM on June 1, 2023:

These results, as well as those of -addrinfo, are filtered for quality and recency. - can you drop this. It's not clear why we need to mention an implemention detail of -addrinfo here.

MarnixCroes commented at 11:13 AM on June 1, 2023:

done

jonatack commented at 12:18 PM on June 1, 2023:

It's not clear why we need to mention an implemention detail of -addrinfo here.

That is how RPC getnodeaddresses works as well -- see its help.

Would suggest either reverting to the previous push, or replacing this paragraph in tor/i2p/cjdns.md with something like the following (perhaps better, as it is shorter and easier to maintain):

You can use the getnodeaddresses RPC to fetch a number of onion peers known to your node; run bitcoin-cli help getnodeaddresses for details.

fanquake commented at 1:08 PM on June 1, 2023:

You can use the getnodeaddresses RPC to fetch a number of onion peers known to your node; run bitcoin-cli help getnodeaddresses for details.

If anything, this seems better than reverting to the previous text.

jonatack commented at 1:09 PM on June 1, 2023:

Agree.

MarnixCroes force-pushed on Jun 1, 2023

doc: update getnodeaddresses for CJDNS, I2P and Tor and rm link

- unify bitcoin-cli getnodeaddresses for CJDNS, I2P and Tor
- remove outdated link to Tor project

6fce5ddc17

MarnixCroes force-pushed on Jun 1, 2023

fanquake renamed this:
~~doc: Tor: fix link & generalize onion getnodeaddresses RPC~~
doc: remove Tor link & generalize onion getnodeaddresses RPC
on Jun 1, 2023

fanquake approved

fanquake commented at 2:13 PM on June 1, 2023: member

ACK 6fce5ddc17ac9d1e07849f92088ea3f7cfcafe26

fanquake merged this on Jun 1, 2023

fanquake closed this on Jun 1, 2023

MarnixCroes deleted the branch on Jun 1, 2023

sidhujag referenced this in commit 30642b27bb on Jun 1, 2023

bitcoin locked this on May 31, 2024