New behavior:
Designer: Bosch-0
Icon PNG file is optimized with optimize-pngs.py
Fix https://github.com/bitcoin/bitcoin/issues/7734 Fix #58
Screenshots:
Based on https://github.com/bitcoin/bitcoin/pull/20172 (the first five commits).
86@@ -87,6 +87,12 @@ class NodeImpl : public Node
87 }
88 }
89 bool getProxy(Network net, proxyType& proxy_info) override { return GetProxy(net, proxy_info); }
90+
91+ bool hasTorOnlyConnections() override
Concept ACK.
If one adds a non-TOR node via addnode, will the icon change (I haven’t looked through the code yet)?
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)
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.
Updated 082a680ea8b2769ff96ae06f870adc9bf2be6819 -> e61b1203406f975891cacf410985ced88d7aac45 (pr86.02 -> pr86.03, diff):
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.
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.
2593@@ -2594,20 +2594,26 @@ bool CConnman::RemoveAddedNode(const std::string& strNode)
2594 return false;
2595 }
2596
2597-size_t CConnman::GetNodeCount(NumConnections flags)
2598+size_t CConnman::PeerCount()
WITH_LOCK, and might make sense to just move the definition to the header file.
2607- for (const auto& pnode : vNodes) {
2608- if (flags & (pnode->IsInboundConn() ? CONNECTIONS_IN : CONNECTIONS_OUT)) {
2609- nNum++;
2610+ConnCounts CConnman::ConnectionCounts()
2611+{
2612+ int num_in = 0;
167@@ -168,18 +168,28 @@ enum class ConnectionType {
168 ADDR_FETCH,
169 };
170
171+struct ConnCounts
172+{
173+ const int all = 0;
167@@ -168,18 +168,28 @@ enum class ConnectionType {
168 ADDR_FETCH,
169 };
170
171+struct ConnCounts
172+{
173+ const int all = 0;
174+ const int in = 0;
662@@ -663,7 +663,7 @@ static RPCHelpMan getblocktemplate()
663 if(!node.connman)
664 throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");
665
666- if (node.connman->GetNodeCount(CConnman::CONNECTIONS_ALL) == 0)
667+ if (node.connman->PeerCount() == 0)
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.
Updated 01fb75ea7f3921d4fb522bbfedee051fa85f1102 -> 6549abcb072d73e0ac91c7992cc5d86eb4064b70 (pr86.04 -> pr86.05, diff):
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.
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
these conditions could be achieved with -onlynet=onion, -listen=0, and -listenonion=1 options)
onlynet=onionmakes 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.1This disables listening, so could be with OR without:
listen=1AND 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.
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.
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” indoc/tor.md.
@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.
onlynet=onion isn’t necessarily recommended, IIUC. Though I can understand why we’d want to indicate when we’re running on onion-only.
The additional configuration is if you only want onion connections.
onlynet=onionisn’t necessarily recommended, IIUC.
Correct. onlynet=onion is for outgoing connection. That (the making outgoing connections Tor-only) is optional, ofc.
The additional configuration is if you only want onion connections. onlynet=onion isn’t necessarily recommended, IIUC
Why is this not recommended?
The additional configuration is if you only want onion connections. onlynet=onion isn’t necessarily recommended, IIUC
Why is this not recommended?
“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.
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.
851@@ -854,19 +852,6 @@ void RPCConsole::updateNetworkState()
852 ui->numberOfConnections->setText(connections);
853 }
854
855-void RPCConsole::setNumConnections()
856-{
857- if (!clientModel)
2597@@ -2598,13 +2598,16 @@ ConnCounts CConnman::ConnectionCounts()
2598 {
2599 int num_in{0};
2600 int num_out{0};
2601+ bool tor_only{false};
543@@ -544,6 +544,7 @@ static UniValue getnetworkinfo(const JSONRPCRequest& request)
544 obj.pushKV("connections", conn_counts.all);
545 obj.pushKV("connections_in", conn_counts.in);
546 obj.pushKV("connections_out", conn_counts.out);
547+ obj.pushKV("connections_tor_only", conn_counts.tor_only);
connections_tor_only should be documented in the RPCResult above
2602 {
2603 LOCK(cs_vNodes);
2604+ if (!vNodes.empty()) tor_only = true;
2605 for (const auto& pnode : vNodes) {
2606 pnode->IsInboundConn() ? ++num_in : ++num_out;
2607+ if (!pnode->addr.IsTor()) tor_only = false;
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.
tACK 6549abcb072d73e0ac91c7992cc5d86eb4064b70 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 727af0f0c00092f473368aa4d7cad7c321d4a4de 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.
This change prevents repetitive traversing vNodes.
Also, the RPC and net changes are not gui related, so should go into the main repo
Co-authored-by: bosch <bosch5@pm.me>
@Sjors basically said this in #58 already, but the Tor icon should only be activated when:
All outbound (default) connections are through Tor (either through the proxy, or to HS’s only); AND
Inbound (optional) connections are EITHER disabled (this is the default when using -proxy), or are all through user’s HS (automatic or manual).
Adding a non HS outbound node should remove the Tor icon.
I am testing the combinations of the above.
@Sjors basically said this in #58 already, but the Tor icon should only be activated when:
All outbound (default) connections are through Tor (either through the proxy, or to HS’s only); AND
Inbound (optional) connections are EITHER disabled (this is the default when using
-proxy), or are all through user’s HS (automatic or manual).Adding a non HS outbound node should remove the Tor icon.
I am testing the combinations of the above.
That exactly describes the current behavior :)
Some questions/notes I have from testing:
Something I found, was that the Proxy Icon appears when there is no peers, and the Tor Icon appears only when there are peers, both independent of settings that could cause unsecured settings in the future. I’m concerned about marking connections as secure (that are secured in the moment) but that could become unsecured in the future.
For example, currently if proxy is set, and listening is explicitly set as well (against defaults), the Proxy Icon still appears, even though an incoming clearnet connection could be made at any time, IIUC. If there isn’t a HS, listen should be off for the Proxy Icon to appear, IMO. (HS’s require listen=1).
Another place I saw this was with the Tor Icon. If onlynet=onion, and listen=1, the Tor Icon appears because (at the moment) there are only Tor connections. I am concerned of the use case where someone may be thinking they have set up their node for a particular scenario where they need the utmost privacy, see the icon, think they have succeeded, but then come back later and see a bunch of clearnet connections. Perhaps there should be code to check for listen=0 and onionlisten=1?
A third place I found this was if a proxy was set with a HS. If you have proxy=127.0.0.1:9050 and listen=1, you will have outgoing connections to HS’s and proxied outgoing connections to normal nodes. You will also have incoming connections to both your HS and clearnet connections to your node. However, here the Proxy Icon still appears. I believe it should only appear if you have bind=127.0.0.1?
But this definitely complicates things, so maybe there should just be something in the release notes saying that the icons only indicate current connection status, not connection settings or future connection status, and in future PRs hopefully we can have easier configuration.
Thanks for your valuable feedback!
Regarding the proxy icon behavior it seems more appropriate to open a dedicated issue that could be discussed and addressed.
Another place I saw this was with the Tor Icon. If
onlynet=onion, andlisten=1, the Tor Icon appears because (at the moment) there are only Tor connections. I am concerned of the use case where someone may be thinking they have set up their node for a particular scenario where they need the utmost privacy, see the icon, think they have succeeded, but then come back later and see a bunch of clearnet connections. Perhaps there should be code to check forlisten=0andonionlisten=1?
The Tor icon is an indicator of current node connections. It is not a switch “keep node connections Tor only”. Having such a checkbox in the GUI is an orthogonal to this PR idea, and it deserves its own submitted issue to discuss.
But this definitely complicates things, so maybe there should just be something in the release notes saying that the icons only indicate current connection status, not connection settings or future connection status, and in future PRs hopefully we can have easier configuration.
Probably, you’d want to suggest a better tooltip text to explain the Tor icon behavior to users?
Hi Hebasto,
Thank you for your comments, those are all good points, and I agree. I will create a new issue for the discussion of having a setting in the GUI to create all connections Tor only.
939-void BitcoinGUI::setNetworkActive(bool networkActive)
940-{
941- updateNetworkState();
942+ if (conn_counts.onion_only) {
943+ m_tor_icon->setPixmap(platformStyle->SingleColorIcon(":/icons/tor_connected").pixmap(STATUSBAR_ICONSIZE, STATUSBAR_ICONSIZE));
944+ m_tor_icon->setToolTip(tr("All connections are <b>via Tor</b> only"));
I don’t think the logic makes sense as-is.
I see two possibilities for a Tor icon:
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.
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.
- 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. https://github.com/bitcoin/bitcoin/pull/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”)?
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.
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.
Currently we can’t “guarantee” this because we always have a listening port for clearnet.
What about a HS with
onlynet=onionlisten=0listenonion=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.
We could expand the behavior of onlynet=onion to prevent listening on the non-onion port.
Currently:
0 -onlynet=<net>
1 Make outgoing connections only through network <net> (ipv4, ipv6 or
2 onion). Incoming connections are not affected by this option.
3 This option can be specified multiple times to allow multiple
4 networks.
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: https://github.com/bitcoin/bitcoin/issues/13436
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.
0$ bitcoind
1...
22020-10-28T08:41:57Z Config file arg: listen="0"
32020-10-28T08:41:57Z Config file arg: listenonion="1"
42020-10-28T08:41:57Z Config file arg: rpcport="8332"
5...
62020-10-28T08:42:19Z tor: Got service ID foo, advertising service foo.onion:8333
72020-10-28T08:42:19Z AddLocal(foo.onion:8333,4)
8
9$ netstat
10tcp4 0 0 127.0.0.1.8332 *.* LISTEN
11(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.
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).
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.
🐙 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”.
I think @luke-jr’s Tri-state should be combined w/ @jonatack’s inbound/outbound distinction. Discussed more here: #110 (comment)
Is @vasild’s https://github.com/bitcoin/bitcoin/pull/20234 still a blocker for this? How does that work with/without expanding the behavior of onlynet=onion @Sjors ?
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 (https://github.com/bitcoin-core/gui/issues/110#issuecomment-744758113). 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 (https://github.com/bitcoin/bitcoin/pull/20172#issuecomment-722451210)
Concept ACK, Tested bd8df33356134efd6e2ccf1acb4e12dad44cb006 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.
