rpc, net: Expose connections_onion_only in getnetworkinfo RPC output #20172

hebasto · 2020-10-16T20:56:02Z

This is a non-gui part of adding Tor icon to the GUI.

Split from bitcoin-core/gui#86 as requested:

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

The first approach was in #19926.

hebasto · 2020-10-16T22:03:22Z

The commits moved from bitcoin-core/gui#86 already have two ACKs:

from @jnewbery Add Tor icon bitcoin-core/gui#86 (comment)

Code review ACK bitcoin-core/gui@6549abc (node changes only. Did not fully review qt changes)

from @Sjors Add Tor icon bitcoin-core/gui#86 (review)

tACK bitcoin-core/gui@6549abc with notes for followup

jonatack

Approach ACK, only skimmed the code so far but the changes look good. Needs test coverage for the new getnetworkinfo "connections_onion_only" field. Debug build is clean and I did some quick manual testing. The counts fields contain the correct values. With mixed peers:

  "connections_onion_only": false,

With only onion peers:

  "connections_onion_only": true,

RPC help:

  "connections_onion_only" : true|false,             (boolean) whether all connection are through the onion network

src/rpc/net.cpp

src/net.h

DrahtBot · 2020-10-17T02:57:15Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Move node and wallet code out of src/interfaces #20494 (refactor: Move node and wallet code out of src/interfaces by ryanofsky)
Disable and fix tests for when BDB is not compiled #20267 (Disable and fix tests for when BDB is not compiled by achow101)
signet mining utility #19937 (signet mining utility by ajtowns)
net: Replace enum CConnMan::NumConnections with enum class ConnectionDirection #19771 (net: Replace enum CConnMan::NumConnections with enum class ConnectionDirection by luke-jr)
multiprocess: Add bitcoin-gui -ipcconnect option #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
multiprocess: Add bitcoin-wallet -ipcconnect option #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
[tests] Allow outbound & block-relay-only connections in functional tests. #19315 ([tests] Allow outbound & block-relay-only connections in functional tests. by amitiuttarwar)
multiprocess: Add basic spawn and IPC support #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
Allow whitelisting outgoing connections #17167 (Allow whitelisting outgoing connections by luke-jr)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors · 2020-10-17T07:52:12Z

Concept ACK.

Agree that a functional test for connections_onion_only would be good, but not sure how easy it is. You could spin up two test nodes, 0 with default settings, 1 with -onlynet=onion and -bind=...=onion, and then connect node 0 to node 1 via it's "onion" port. In that case node 1 should have connections_onion_only=true.

hebasto · 2020-10-17T12:25:30Z

Updated 174848f -> 9371757 (pr20172.01 -> pr20172.02, diff):

addressed all of @jonatack's and @Sjors' comments
added a functional test

hebasto · 2020-10-17T21:18:55Z

~~Hmm, the current pr20172.03 branch does not pass macOS 10.14 build on Travis.~~

~~The alternative pr20172.04 branch pass locally on macOS 10.15 and Linux Mint 20 (Focal codebase), but it does not pass on Travis at all.~~

~~What is the correct portable way to -bind=localhost and -bind=localhost:port=onion simultaneously?~~

UPDATE: it was such a silly error, and I've caught it after sleep 😃

hebasto · 2020-10-18T12:02:23Z

Updated 9371757 -> d301cc5 (pr20172.02 -> pr20172.06, diff):

fixed functional test

Travis is green now!

jnewbery

One question and one small naming suggestion.

This PR contains qt-only changes. It shouldn't be too difficult to separate the changes so that PRs to the main repo only contain changes to the node code, and PRs to the GUI repo only contain changes to qt code.

src/net.h

test/functional/feature_onion.py

hebasto · 2020-10-19T09:20:40Z

@jnewbery

This PR contains qt-only changes. It shouldn't be too difficult to separate the changes so that PRs to the main repo only contain changes to the node code, and PRs to the GUI repo only contain changes to qt code.

Clear qt-only changes are already leaved in the GUI repo. This pull contains two commits that modify signal signatures. As these signals propagate through the qt code, I do not want to break those commits in "non-qt" and "qt" parts.

test/functional/feature_onion.py

This change prevents repetitive traversing vNodes.

hebasto · 2020-10-19T22:35:12Z

Updated d301cc5 -> caebabf (pr20172.06 -> pr20172.09, diff):

reworked functional test
addressed @jnewbery's comment:

This repetitious code could potentially be refactored into a small helper function get_connection_info().

test/functional/feature_onion.py

hebasto · 2020-10-20T10:47:05Z

Updated 85ff09e -> da8b109 (pr20172.10 -> pr20172.11, diff):

addressed @jnewbery's comment:

Stop-starting the nodes 11 times just to test new connections seems very inefficient. Is there a reason you need to do this stop-start?

improved robustness

Sjors · 2020-10-20T12:39:29Z

tACK da8b109

luke-jr

Concept NACK per bitcoin-core/gui#86 (review)

kristapsk · 2020-11-05T15:30:55Z

I'm thinking about this in a context of if / when other privacy network support is added to Bitcoin Core (like I2P, #20254). What people might want then is not "are all of my connections Tor only?", but "are all of my connections using privacy networks?" and "which privacy networks are they using?".

DrahtBot · 2020-12-01T09:43:05Z

🐙 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".}

hebasto · 2021-05-03T19:55:55Z

Closing. Leaving it up for grabs.

hebasto added 3 commits October 16, 2020 22:46

net: Replace uiInterface global with CConnman::clientInterface member

2596322

Drop unused argument in NetworkActiveChanged signal

7eccce5

Drop unused argument in NumConnectionsChanged signal

f4cbe6b

This was referenced Oct 16, 2020

qt: Deduplicate NumConnections enum #19193

Closed

Add Tor icon bitcoin-core/gui#86

Closed

DrahtBot added GUI Mining P2P RPC/REST/ZMQ labels Oct 16, 2020

jonatack reviewed Oct 16, 2020

View reviewed changes

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

src/net.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Oct 17, 2020

signet mining utility #19937

Merged

DrahtBot mentioned this pull request Oct 17, 2020

interfaces: Expose settings.json methods to GUI #15936

Merged

hebasto force-pushed the 201016-tor branch from 174848f to 9371757 Compare October 17, 2020 12:20

hebasto force-pushed the 201016-tor branch 3 times, most recently from 3dc8e15 to b9fc611 Compare October 17, 2020 21:08

hebasto force-pushed the 201016-tor branch 2 times, most recently from 8d66f6c to d301cc5 Compare October 18, 2020 11:25

jnewbery reviewed Oct 19, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

test/functional/feature_onion.py Outdated Show resolved Hide resolved

jnewbery reviewed Oct 19, 2020

View reviewed changes

test/functional/feature_onion.py Outdated Show resolved Hide resolved

test/functional/feature_onion.py Outdated Show resolved Hide resolved

hebasto added 2 commits October 19, 2020 21:10

Split CConnman::GetNodeCount() in two functions

72632da

This change prevents repetitive traversing vNodes.

rpc: Add connections_onion_only to getnetworkinfo output

a855538

hebasto force-pushed the 201016-tor branch from d301cc5 to 8d077c1 Compare October 19, 2020 18:16

hebasto force-pushed the 201016-tor branch 2 times, most recently from ddced9f to caebabf Compare October 19, 2020 22:26

hebasto force-pushed the 201016-tor branch from caebabf to 85ff09e Compare October 19, 2020 23:24

jnewbery reviewed Oct 20, 2020

View reviewed changes

test/functional/feature_onion.py Outdated Show resolved Hide resolved

qa: Add feature_onion.py

da8b109

hebasto force-pushed the 201016-tor branch from 85ff09e to da8b109 Compare October 20, 2020 10:43

hebasto mentioned this pull request Oct 20, 2020

p2p: improve onion detection in AttemptToEvictConnection #20193

Closed

jonatack mentioned this pull request Oct 20, 2020

p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

Merged

luke-jr suggested changes Oct 24, 2020

View reviewed changes

This was referenced Oct 28, 2020

[tests] Allow outbound & block-relay-only connections in functional tests. #19315

Merged

net: Replace enum CConnMan::NumConnections with enum class ConnectionDirection #19771

Merged

Allow whitelisting outgoing connections #17167

Closed

DrahtBot mentioned this pull request Nov 7, 2020

Disable and fix tests for when BDB is not compiled #20267

Merged

DrahtBot added the Needs rebase label Dec 1, 2020

hebasto closed this May 3, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

rpc, net: Expose connections_onion_only in getnetworkinfo RPC output #20172

rpc, net: Expose connections_onion_only in getnetworkinfo RPC output #20172

Uh oh!

Conversation

hebasto commented Oct 16, 2020

Uh oh!

hebasto commented Oct 16, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors commented Oct 17, 2020

Uh oh!

hebasto commented Oct 17, 2020

Uh oh!

hebasto commented Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Oct 18, 2020

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hebasto commented Oct 19, 2020

Uh oh!

Uh oh!

Uh oh!

hebasto commented Oct 19, 2020

Uh oh!

Uh oh!

hebasto commented Oct 20, 2020

Uh oh!

Sjors commented Oct 20, 2020

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

kristapsk commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 1, 2020

Uh oh!

hebasto commented May 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jonatack left a comment •

edited

Loading

DrahtBot commented Oct 17, 2020 •

edited

Loading

hebasto commented Oct 17, 2020 •

edited

Loading

kristapsk commented Nov 5, 2020 •

edited

Loading