Add Tor icon #86

hebasto · 2020-09-10T08:30:08Z

New behavior:

the Tor icon is showed in the status bar iif all peer connections are made via Tor (credits to Sjors)
a click on the Tor icon opens the "Network" tab of the "Options" dialog

Designer: Bosch-0
Icon PNG file is optimized with optimize-pngs.py

Fix bitcoin/bitcoin#7734
Fix #58

Screenshots:

Based on bitcoin/bitcoin#20172 (the first five commits).

hebasto · 2020-09-10T08:31:03Z

Moved from bitcoin/bitcoin#19926 as requested by @fanquake.

maflcko · 2020-09-10T11:17:24Z

src/interfaces/node.cpp

@@ -87,6 +87,12 @@ class NodeImpl : public Node
        }
    }
    bool getProxy(Network net, proxyType& proxy_info) override { return GetProxy(net, proxy_info); }
+
+    bool hasTorOnlyConnections() override


would it make sense to expose this bool over rpc as well?

See bitcoin/bitcoin#20172.

hebasto · 2020-09-10T12:07:04Z

Updated 4930c92 -> 082a680 (pr86.01 -> pr86.02, diff):

addressed @MarcoFalke's comment:

would it make sense to expose this bool over rpc as well?

jonasschnelli · 2020-09-10T12:11:24Z

Concept ACK.

If one adds a non-TOR node via addnode, will the icon change (I haven't looked through the code yet)?

hebasto · 2020-09-10T12:14:19Z

@jonasschnelli

If one adds a non-TOR node via addnode, will the icon change (I haven't looked through the code yet)?

No. updateTorIcon() is called only once in BitcoinGUI::setClientModel(). Going to address this issue.

UPDATE: changed in #86 (comment)

jonasschnelli · 2020-09-10T13:10:18Z

No. updateTorIcon() is called only once in BitcoinGUI::setClientModel(). Going to address this issue.

Thanks!

I think for TOR only,... it's very important that users can't end up with the icon "confirming" tor-only when the actually have non-tor connections.

hebasto · 2020-09-11T09:31:38Z

Updated 082a680 -> e61b120 (pr86.02 -> pr86.03, diff):

addressed @jonasschnelli's comment:

I think for TOR only,... it's very important that users can't end up with the icon "confirming" tor-only when the actually have non-tor connections.

hebasto · 2020-09-11T09:34:02Z

@fanquake @MarcoFalke Is this PR in its current state (with net part refactoring) placed in this repo correctly?

Fichte42 · 2020-09-11T12:19:38Z

Concept ACK.

Not tested, code looks pretty straight forward. I would maybe rename tor_connected to tor_only_connections or something like that to reflect that all connections are via tor.

jnewbery

Concept ACK and code review ACK of the net code changes. I've left a bunch of style comments, which you can feel free to ignore, but might be good to address if you have to touch the branch again.

I'm not familiar with the qt code, but I've skimmed the changes and they seem fine.

src/net.cpp

src/net.h

src/rpc/mining.cpp

hebasto · 2020-09-11T17:18:46Z

Updated 01fb75e -> 6549abc (pr86.04 -> pr86.05, diff):

addressed almost all @jnewbery's comments.

@Fichte42

I would maybe rename tor_connected to tor_only_connections or something like that to reflect that all connections are via tor.

The icon name is used only once, and there is the only Tor icon in the repo. I'd keep the more general name for now.

Rspigler · 2020-09-12T20:43:26Z

these conditions could be achieved with -onlynet=onion, -listen=0, and -listenonion=1 options)

onlynet=onion makes it so that Core only connects out to Hidden Services. What about also detecting situations like:

proxy=127.0.0.1:9050 bind=127.0.0.1

This disables listening, so could be with OR without:

listen=1 AND
the ephemeral hidden services supported since Tor version 0.2.7.1 and Core 0.12.0

hebasto · 2020-09-12T21:20:25Z

@Rspigler

these conditions could be achieved with -onlynet=onion, -listen=0, and -listenonion=1 options)

onlynet=onion makes it so that Core only connects out to Hidden Services. What about also detecting situations like:

proxy=127.0.0.1:9050 bind=127.0.0.1

This disables listening, so could be with OR without:

listen=1 AND
the ephemeral hidden services supported since Tor version 0.2.7.1 and Core 0.12.0

Currently, only the CNetAddr::IsTor() function is used for checking. Command line options in the OP are listed for testing purposes.

Bosch-0 · 2020-09-14T03:53:44Z

ACK, looks good on windows 10. Though I did have to add the below to my .config file - it would be ideal if the user did not have to do this.

onlynet=onion listen=0 listenonion=1

A quick note, this was the first PR I have reviewed and as non super technical designer, I did not find the process too difficult - I will be making a concerted effort with the Bitcoin Design community to get more reviewers looking at these kind of PRs.

jnewbery · 2020-09-14T08:29:40Z

Code review ACK 6549abc (node changes only. Did not fully review qt changes)

jonatack · 2020-09-14T09:04:47Z

ACK, looks good on windows 10. Though I did have to add the below to my .config file - it would be ideal if the user did not have to do this.

onlynet=onion listen=0 listenonion=1

@Bosch-0 if you have tor configured and running, running the default configuration of Bitcoin Core should automatically start an onion service. See "3. Automatically listen on Tor" in doc/tor.md.

hebasto · 2020-09-14T09:09:29Z

@jonatack

@Bosch-0 if you have tor configured and running, running the default configuration of Bitcoin Core should automatically start an onion service. See "3. Automatically listen on Tor" in doc/tor.md.

Correct. But the default setup does not prevent non-onion connections. Therefore, the Tor icon will not be shown to a user.

jonatack · 2020-09-14T09:10:09Z

The additional configuration is if you only want onion connections. onlynet=onion isn't necessarily recommended, IIUC. Though I can understand why we'd want to indicate when we're running on onion-only.

hebasto · 2020-09-14T09:12:45Z

The additional configuration is if you only want onion connections. onlynet=onion isn't necessarily recommended, IIUC.

Correct. onlynet=onion is for outgoing connection. That (the making outgoing connections Tor-only) is optional, ofc.

Bosch-0 · 2020-09-14T13:42:04Z

The additional configuration is if you only want onion connections. onlynet=onion isn't necessarily recommended, IIUC

Why is this not recommended?

jonatack · 2020-09-14T15:01:36Z

The additional configuration is if you only want onion connections. onlynet=onion isn't necessarily recommended, IIUC

Why is this not recommended?

https://en.bitcoin.it/wiki/Setting_up_a_Tor_hidden_service:

"If you additionally want Bitcoin Core to only connect out to Tor hidden services, also add this line (not particularly recommended):"
```
onlynet=onion
```
"Doing so will make your specific bitcoind node arguably more secure because it will never have an unencrypted connection to another node, but if everyone used onlynet=onion nobody on the onion bitcoin chain would be able to communicate with the clearnet chain. It is essential that some nodes access both clearnet and Tor. If you need to submit bitcoin transactions to the network with the highest level of obscurity, use onlynet=onion. If you only wish to give access to your node to other Tor users, do not use it."

Also, if you're interested, google "Bitcoin over Tor isn’t a good idea" (from 2015). Possible countermeasures include BIP324 (in the future) or using addnode to ensure connection to trusted "good" onion peers.
Finally, Tor v2 seems increasingly compromised and begins its deprecation period tomorrow (September 15, 2020), which is why implementation of BIP155 is important to move to Tor v3, I2P, CJDNS, etc.

Edit: don't let this hijack the PR! Concept ACK.

Rspigler · 2020-09-15T04:45:17Z

Though I can understand why we'd want to indicate when we're running on onion-only.

I think for the very reason that it risks partitioning the network, and less educated users may think that there is a greater benefit than there really is, we shouldn't have an icon to indicate an onion only mode (this PR doesn't do that).

That (the making outgoing connections) is optional, ofc.

Outgoing connections is default, incoming connections are optional. For outgoing connections, you can set a proxy command in the conf file to put all connections behind tor, or set onlynet=onion to connect out only to HS (not recommended).

For incoming connections, since Tor version 0.2.7.1 and Core 0.12.0, Core can create and destroy ephemeral hidden services programmatically.

Sjors

tACK 6549abc with notes for followup

We can decide in another PR whether to encourage Tor more, as well as make it less painful to correctly configure.

Tested on macOS 10.15.6 with the Tor proxy configured in the GUI (using port 9150 which the Tor browser offers), -onlynet=onion and -listen=0.

Note that the Tor icon won't appear until at least one peer is connected. This is the same behavior as connections_tor_only in getnetworkinfo (uses the same code).

I didn't try launching a hidden service; someone running Linux should.

In light of the 727af0f refactor, I also tested that setnetworkactive still works with bitcoin-cli and from the GUI console.

In the event this PR doesn't make it, there's some useful refactoring commits that should be salvaged.

Sjors · 2020-09-16T12:05:00Z

src/qt/rpcconsole.cpp

@@ -854,19 +852,6 @@ void RPCConsole::updateNetworkState()
    ui->numberOfConnections->setText(connections);
 }

-void RPCConsole::setNumConnections()
-{
-    if (!clientModel)


Was this check already superfluous?

Sjors · 2020-09-16T12:10:17Z

src/net.cpp

@@ -2598,13 +2598,16 @@ ConnCounts CConnman::ConnectionCounts()
 {
    int num_in{0};
    int num_out{0};
+    bool tor_only{false};


Maybe this should be an Optional, but I don't care deeply.

Sjors · 2020-09-16T12:10:28Z

src/rpc/net.cpp

@@ -544,6 +544,7 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request)
        obj.pushKV("connections", conn_counts.all);
        obj.pushKV("connections_in", conn_counts.in);
        obj.pushKV("connections_out", conn_counts.out);
+        obj.pushKV("connections_tor_only", conn_counts.tor_only);


connections_tor_only should be documented in the RPCResult above

Also, the RPC and net changes are not gui related, so should go into the main repo

Done in bitcoin/bitcoin#20172.

Sjors · 2020-09-16T12:12:58Z

src/net.cpp

        for (const auto& pnode : vNodes) {
            pnode->IsInboundConn() ? ++num_in : ++num_out;
+            if (!pnode->addr.IsTor()) tor_only = false;


I didn't test if IsTor() behaves correctly when you have an inbound connection via a hidden service.

I didn't test if IsTor() behaves correctly when you have an inbound connection via a hidden service.

No, it doesn't. Going to rework his code.

bitcoin/bitcoin#8973

If it's too tedious, I'm fine with the current change and just leaving correct handling of hidden service as a (well documented) TODO. At least this PR shows the Tor icon in a subset of acceptable circumstances.

The solution suggested by @laanwj submitted in bitcoin/bitcoin#19991.

Rspigler · 2020-10-24T00:11:56Z

src/qt/bitcoingui.cpp

-    updateNetworkState();
+    if (conn_counts.onion_only) {
+        m_tor_icon->setPixmap(platformStyle->SingleColorIcon(":/icons/tor_connected").pixmap(STATUSBAR_ICONSIZE, STATUSBAR_ICONSIZE));
+        m_tor_icon->setToolTip(tr("All connections are <b>via Tor</b> only"));


How about "All current connections are via Tor only"?

luke-jr

I don't think the logic makes sense as-is.

I see two possibilities for a Tor icon:

Tor is working and usable (reachability).
All connections, even hypothetically, will be only via Tor (network privacy).

Right now, it's "all connections at the moment happen to be via Tor", which seems relatively useless - it doesn't guarantee network privacy, nor reachability. What good is it?

IMO the icon should probably be tri-state: "off", or either scenario listed above.

jonatack · 2020-10-24T20:44:03Z

Agree with @luke-jr here. I'd suggest the first one, "Tor is working and usable on this node", and not focus on the onion-only aspect for the reasons described above in #86 (comment).

We may also want to distinguish between outbound (onlynet=onion) and inbound onion connections.

The tooltip needs to be precise and clear about what the icon states signify.

Rspigler · 2020-10-24T21:09:32Z

I was hesitant to make an argument because I don't review code, but as my comment (#86 (comment)) indicates above, I agree with @luke-jr and @jonatack as well. I would need someone else to analyze the partitioning risks of many nodes possibly going onion only, I don't know enough to do so myself. But I prefer the 'tri-state' option to Jonatack's dual-state because it would be nice for user's to know if they (1) have configured their HS properly and/or (2) have Tor only settings achieved.

vasild · 2020-10-26T10:15:27Z

All connections, even hypothetically, will be only via Tor (network privacy).

Currently we can't "guarantee" this because we always have a listening port for clearnet. bitcoin/bitcoin#20234 would make it possible. With that PR, if -bind=127.0.0.1:8334=onion is specified we would not bind on any other port and we can assume the node is running in Tor-only mode (for incoming connections, I guess -onlynet=onion would control the outgoing ones).

Also, something to consider - once we have I2P connectivity, if "all connections, even hypothetically will be only via Tor or I2P", should we then display both Tor or I2P icons or none of them (wrt the 3rd state "network privacy")?

Sjors · 2020-10-26T11:23:07Z

The combination ""All connections, at the moment happen to be via Tor" && "All connections, even hypothetically, will be only via Tor" makes sense as the boolean.

A tristate works for me too, if there's actually a way to verify reachability.

Rspigler · 2020-10-26T17:26:35Z

Currently we can't "guarantee" this because we always have a listening port for clearnet.

What about a HS with onlynet=onion listen=0 listenonion=1 ?

once we have I2P connectivity, if "all connections, even hypothetically will be only via Tor or I2P", should we then display both Tor or I2P icons or none of them (wrt the 3rd state "network privacy")?

Probably should be both, but maybe re-word the tooltip text from "All connections will be only via Tor" to something like "Clearnet connections are disabled for all connections. Tor is enabled) so there's no error in logic when one or both is enabled.

vasild · 2020-10-27T08:15:14Z

Currently we can't "guarantee" this because we always have a listening port for clearnet.

What about a HS with onlynet=onion listen=0 listenonion=1 ?

listen=0 will cause us not to listen on any port, so we would not accept any incoming connections, regardless of whether clearnet or Tor. listenonion=1 will be automatically changed to listenonion=0 due to listen=0.

But yes, just onlynet=onion listen=0 can be considered as Tor-only mode.

Sjors · 2020-10-27T08:53:43Z

We could expand the behavior of onlynet=onion to prevent listening on the non-onion port.

Currently:

  -onlynet=<net>
       Make outgoing connections only through network <net> (ipv4, ipv6 or
       onion). Incoming connections are not affected by this option.
       This option can be specified multiple times to allow multiple
       networks.

Rspigler · 2020-10-28T03:44:38Z

listenonion=1 will be automatically changed to listenonion=0 due to listen=0.

I don't think this is true, because when I set onlynet=onion listen=0 and listenonion=1 my HS is enabled. But when I have onlynet=onion listen=0 and listenonion=0 in my config, my HS is disabled. So I would assume it would allow incoming onion connections

We could expand the behavior of onlynet=onion to prevent listening on the non-onion port.

I believe this was discussed here: bitcoin/bitcoin#13436

vasild · 2020-10-28T08:51:16Z

Alright, I was halfway wrong.

listenonion=1 will be automatically changed to listenonion=0 due to listen=0

^ this is not true, as you have observed. However the following is true:

listen=0 will cause us not to listen on any port, so we would not accept any incoming connections, regardless of whether clearnet or Tor.

$ bitcoind
...
2020-10-28T08:41:57Z Config file arg: listen="0"
2020-10-28T08:41:57Z Config file arg: listenonion="1"
2020-10-28T08:41:57Z Config file arg: rpcport="8332"
...
2020-10-28T08:42:19Z tor: Got service ID foo, advertising service foo.onion:8333
2020-10-28T08:42:19Z AddLocal(foo.onion:8333,4)

$ netstat
tcp4       0      0 127.0.0.1.8332         *.*                    LISTEN     
(bitcoind is only listening on the RPC port)

I guess this can be considered as a bug because it will (I guess) advertise foo.onion:8333 where nobody is listening.

Sjors · 2020-10-30T15:02:24Z

I believe this was discussed here: bitcoin/bitcoin#13436

That discussion was before we had a separate Tor port (binding to 127.0.0.1).

Rspigler · 2020-10-30T18:46:48Z

That discussion was before we had a separate Tor port (binding to 127.0.0.1).

Ah, ok. IMO, that seems like it would be the easiest for the end user and also for this PR. Not sure how changing it would interact with other current network setting configurations we have.

DrahtBot · 2020-12-01T09:39:45Z

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

Rspigler · 2020-12-14T21:38:19Z

I think @luke-jr's Tri-state should be combined w/ @jonatack's inbound/outbound distinction. Discussed more here: #110 (comment)

Is @vasild's bitcoin/bitcoin#20234 still a blocker for this? How does that work with/without expanding the behavior of onlynet=onion @Sjors ?

Rspigler · 2020-12-14T23:42:54Z

Edit: I feel less strongly about the concept of a Tor icon in general. Definitely supportive of switches in the GUI to help end users set up permanent configurations of connection types (#110 (comment)). But, if the user has done that, they should know what their connections are like.

The icon possibility set is just too much. Tor incoming, both proxy and tor outgoing...etc. We would either need far too many icons, or the icons would only apply in rare scenarios (Would you give someone a Tor icon if they only have Tor outgoing and Incoming disabled? What about Tor outgoing and Proxy incoming? Or Tor outgoing and Tor incoming?)...

This would get more complicated when I2P support is added. Maybe just have an icon for "My connections are only using privacy networks" as suggested by @kristapsk (bitcoin/bitcoin#20172 (comment))

jarolrod

Concept ACK, Tested bd8df33 on macOS 11.1 with Qt 5.15.2

Tor Only: Tor Icon Shows

Tor and IPv4 Connection: Tor Icon Does not Show (intended)

Thoughts on the discussion had so far:
This is part of a bigger discussion, but it would be a good feature to represent the connection type in the form of an Icon, for all connection types. The Tor icon is one step towards this. While outside the scope of this PR, I would propose an Icon for each network type to show.

Specifically concerning the Tor Icon:
I think that the Tor Icon should show ONLY when ALL of our connections are Tor (Like this PR implements). Perhaps we show a (to be designed) Fusion icon when you have a combination of Tor and another network type like IPv4.

rebroad · 2021-04-19T18:59:51Z

@hebasto What's a use case for this?

hebasto · 2021-05-02T13:40:56Z

@rebroad

@hebasto What's a use case for this?

The initial idea was to give to a user some representation about her node privacy level.

hebasto · 2021-05-02T13:41:15Z

Closing. Leaving it up for grabs.

This was referenced Sep 10, 2020

gui: Add Tor icon bitcoin/bitcoin#19926

Closed

Tor status indicator icon in bottom right #58

Open

maflcko reviewed Sep 10, 2020

View reviewed changes

hebasto force-pushed the 200902-tor branch from 4930c92 to 082a680 Compare September 10, 2020 12:03

hebasto force-pushed the 200902-tor branch from 082a680 to e61b120 Compare September 11, 2020 09:29

hebasto mentioned this pull request Sep 11, 2020

qt: Deduplicate NumConnections enum bitcoin/bitcoin#19193

Closed

hebasto force-pushed the 200902-tor branch from e61b120 to 01fb75e Compare September 11, 2020 11:31

jnewbery reviewed Sep 11, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

src/rpc/mining.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 200902-tor branch from 01fb75e to 6549abc Compare September 11, 2020 16:45

Sjors approved these changes Sep 16, 2020

View reviewed changes

Rspigler reviewed Oct 24, 2020

View reviewed changes

This was referenced Oct 24, 2020

Easier Configuration for Permanent Proxy and Tor only Connections #110

Open

Proxy Icon Tooltip Text #111

Closed

luke-jr suggested changes Oct 24, 2020

View reviewed changes

DrahtBot added the Needs rebase label Dec 1, 2020

Rspigler mentioned this pull request Dec 15, 2020

Advertised address where nobody is listening bitcoin/bitcoin#20657

Closed

hebasto marked this pull request as draft December 23, 2020 20:26

jarolrod reviewed Jan 19, 2021

View reviewed changes

hebasto closed this May 2, 2021

hebasto added Feature Up for grabs and removed Needs rebase labels May 2, 2021

bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022

Add Tor icon #86

Add Tor icon #86

Conversation

hebasto commented Sep 10, 2020 • edited

hebasto commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hebasto commented Sep 10, 2020

jonasschnelli commented Sep 10, 2020

hebasto commented Sep 10, 2020 • edited

jonasschnelli commented Sep 10, 2020

hebasto commented Sep 11, 2020

hebasto commented Sep 11, 2020

Fichte42 commented Sep 11, 2020

jnewbery left a comment

Choose a reason for hiding this comment

hebasto commented Sep 11, 2020

Rspigler commented Sep 12, 2020

hebasto commented Sep 12, 2020

Bosch-0 commented Sep 14, 2020 • edited

jnewbery commented Sep 14, 2020

jonatack commented Sep 14, 2020

hebasto commented Sep 14, 2020

jonatack commented Sep 14, 2020 • edited

hebasto commented Sep 14, 2020 • edited

Bosch-0 commented Sep 14, 2020

jonatack commented Sep 14, 2020 • edited

Rspigler commented Sep 15, 2020

Sjors left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors Sep 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luke-jr left a comment

Choose a reason for hiding this comment

jonatack commented Oct 24, 2020 • edited

Rspigler commented Oct 24, 2020

vasild commented Oct 26, 2020 • edited

Sjors commented Oct 26, 2020

Rspigler commented Oct 26, 2020

vasild commented Oct 27, 2020

Sjors commented Oct 27, 2020

Rspigler commented Oct 28, 2020 • edited

vasild commented Oct 28, 2020

Sjors commented Oct 30, 2020

Rspigler commented Oct 30, 2020

DrahtBot commented Dec 1, 2020

Rspigler commented Dec 14, 2020

Rspigler commented Dec 14, 2020

jarolrod left a comment

Choose a reason for hiding this comment

rebroad commented Apr 19, 2021

hebasto commented May 2, 2021

hebasto commented May 2, 2021

hebasto commented Sep 10, 2020 •

edited

hebasto commented Sep 10, 2020 •

edited

Bosch-0 commented Sep 14, 2020 •

edited

jonatack commented Sep 14, 2020 •

edited

hebasto commented Sep 14, 2020 •

edited

jonatack commented Sep 14, 2020 •

edited

Sjors left a comment •

edited

Sjors Sep 17, 2020 •

edited

jonatack commented Oct 24, 2020 •

edited

vasild commented Oct 26, 2020 •

edited

Rspigler commented Oct 28, 2020 •

edited