net: Add CNode::ConnectedThroughNetwork member function #19998

hebasto · 2020-09-22T21:17:34Z

This PR:

adds CNode::ConnectedThroughNetwork member function
is based on net: Use alternative port for incoming Tor connections #19991, and only last two commits belong to it
is required for Add Tor icon bitcoin-core/gui#86 and net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo #20002

DrahtBot · 2020-09-23T05:18:38Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo #20002 (net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo by jonatack)
net: guard vRecvGetData with cs_vRecv and orphan_work_set with g_cs_orphans #19911 (net: guard vRecvGetData with cs_vRecv and orphan_work_set with g_cs_orphans by narula)
net: Add NAT-PMP port forwarding support #18077 (net: Add NAT-PMP port forwarding support by hebasto)

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.

practicalswift · 2020-09-23T05:35:06Z

Concept ACK

Nice! :)

laanwj · 2020-09-23T11:05:19Z

Concept ACK

What about a multiple-choice option that more generally specifies the network that the connection (comes in/goes out) through, instead of specializing it for Tor? Might want to have the same for I2P later, for example.

Also this should be 'onion' to be consistent with our other uses.

hebasto · 2020-09-23T11:21:41Z

@laanwj

What about a multiple-choice option that more generally specifies the network that the connection (comes in/goes out) through, instead of specializing it for Tor? Might want to have the same for I2P later, for example.

Do I understand you correctly that smth following is expected:

$ src/bitcoin-cli help getpeerinfo | grep network_type
    "network_type" : clearnet|onion|I2P,        Type of the network the peer has connected via

?

naumenkogs · 2020-09-23T14:12:48Z

Concept ACK.
I agree with wladimir's suggestions.
@hebasto I think your understanding is correct.

jonatack · 2020-09-23T14:21:03Z

Concept ACK

What about a multiple-choice option that more generally specifies the network that the connection (comes in/goes out) through, instead of specializing it for Tor? Might want to have the same for I2P later, for example.

Also this should be 'onion' to be consistent with our other uses.

This was the goal in #20002, which exposes the GetNetClass() index (allowing for a significant code simplification in -netinfo). Feedback welcome if it should also expose the network class name.

hebasto · 2020-09-23T14:33:26Z

@jonatack

This was the goal in #20002, which exposes the GetNetClass() index (allowing for a significant code simplification in -netinfo). Feedback welcome if it should also expose the network class name.

What about limiting this PR to introducing CNode::GetNetClass() in addition to CNetAddr::GetNetClass() as the latter does not work for inbound connections via Tor?

Or just drop the last commit in favor of #20002 (if CNode::ConnectedViaTor() logic is sufficient)?

jonatack · 2020-09-23T16:03:29Z

@hebasto after chatting offline, that makes sense, will look at building on your commit "net: Add CNode::ConnectedViaTor() member function".

Concept ACK in any case, will test your inbound onions detection.

hebasto · 2020-09-23T16:09:33Z

Updated fcd938e -> 911a3f9 (pr19998.02 -> pr19998.03, diff):

dropped the last commit "rpc: Add via_tor to getpeerinfo output" in favor of more consistent approach in net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo #20002

@laanwj @naumenkogs RPC part has been moved to #20002 now.

src/net.cpp

Sjors · 2020-10-01T18:25:10Z

utACK c972792

This will be easier to test after #20002 is refreshed (don't forget to mention it in the description)

The asmap test failure on macOS Travis seems spurious (it passes on my machine)

hebasto · 2020-10-02T11:43:34Z

Updated c972792 -> ac0f497 (pr19998.07 -> pr19998.08) due to the conflict with #19991.

jonatack · 2020-10-02T12:05:49Z

Do you plan to add test coverage for the changes here (and in #19991?)

hebasto · 2020-10-02T12:10:25Z

@jonatack

Do you plan to add test coverage for the changes here (and in #19991?)

I thought your tests could cover all aspects, right?

jonatack · 2020-10-02T12:39:49Z

@hebasto I don't plan to open a PR just for follow-up tests. I think it's easiest when the coverage accompanies the changes.

jonatack · 2020-10-02T17:04:26Z

Starting to see inbound onions while testing this on #20002 (Edit: WIP update now pushed if anyone wants to use it for testing).

<-> relay   net mping   ping send recv  txn  blk uptime  asmap  id address
 in  full onion   358    469    0   90               49         35 127.0.0.1:43064
 in block onion   450    722   29  104               35         66 127.0.0.1:45118

The updated function here is easy to use, too.

stats.m_network = GetNetworkName(ConnectedThroughNetwork());

hebasto · 2020-10-03T09:45:11Z

Added test coverage.

jonatack

ACK, I've been testing this with #20002 and also rebased on #19954 (it's thankfully a clean merge) and it seems to be working. A few optional comments below. It may be good to update fuzz/netaddress.cpp (or other fuzzers, I didn't look further) with ConnectedThroughNetwork() and add unit or functional tests for the new -bind functionality. I did add some testing in #20002 for the values returned by ConnectedThroughNetwork(). Edit: reviewing the new push!

src/net.h

src/net.cpp

src/net.h

jonatack

ACK. The new unit tests are green for me locally. Not sure what's up with the travis linter.

src/test/net_tests.cpp

hebasto · 2020-10-03T11:40:27Z

Updated 5fc8f15 -> 3989fcf (pr19998.10 -> pr19998.11):

addressed all of the recent @jonatack's comments
rebased to avoid a weird CI error message

jonatack

ACK 3989fcf

src/net.h

hebasto · 2020-10-03T12:41:51Z

Updated 3989fcf -> 3984b78 (pr19998.11 -> pr19998.12, diff):

addressed @jonatack's comment

jonatack · 2020-10-03T13:12:42Z

Thanks for your patience with the suggestions.

re-ACK 3984b78 per git diff 3989fcf 3984b78c

Edit: testing with #20002 (inbound onions detected server-side) vs on master (inbound onions detected client-side), I'm seeing the same ones detected.

laanwj · 2020-10-12T16:37:27Z

Code review ACK 3984b78
Thanks for addressing my suggestion. And making CNode::ConnectedThroughNetwork() return a Network enum directly is a nice touch.

…unction 3984b78 test: Add tests for CNode::ConnectedThroughNetwork (Hennadii Stepanov) 49fba9c net: Add CNode::ConnectedThroughNetwork member function (Hennadii Stepanov) d4dde24 net: Add CNode::m_inbound_onion data member (Hennadii Stepanov) Pull request description: This PR: - adds `CNode::ConnectedThroughNetwork` member function - is based on bitcoin#19991, and only last two commits belong to it - is required for bitcoin-core/gui#86 and bitcoin#20002 ACKs for top commit: jonatack: re-ACK 3984b78 per `git diff 3989fcf 3984b78` laanwj: Code review ACK 3984b78 Tree-SHA512: 23a9c8bca8dca75113b5505fe443b294f2d42d03c98c7e34919da12d8396beb8d0ada3a58ae16e3da04b7044395f72cf9c216625afc078256cd6c897ac42bf3d

…lify/improve -netinfo 6272604 refactor: enable -netinfo to add future networks (i2p, cjdns) (Jon Atack) 82fd402 refactor: promote some -netinfo localvars to class members (Jon Atack) 5133fab cli: simplify -netinfo using getpeerinfo network field (Jon Atack) 4938a10 rpc, test: expose CNodeStats network in RPC getpeerinfo (Jon Atack) 6df7882 net: add peer network to CNodeStats (Jon Atack) Pull request description: This PR: - builds on #19991 and #19998 - exposes peer networks via a new getpeerinfo `network` field ("ipv4", "ipv6", or "onion"), and adds functional tests - updates -netinfo to use getpeerinfo `network` rather than detecting the peer networks client-side - refactors -netinfo to easily add future networks ACKs for top commit: laanwj: ACK 6272604 Tree-SHA512: 28883487585135ceaaf84ce09131f2336e3193407f2e3df0960e3f4ac340f500ab94ffecb9d06a4c49bc05e3cca4f914ea4379860bea0bd5df2f834f74616015

…o; simplify/improve -netinfo 6272604 refactor: enable -netinfo to add future networks (i2p, cjdns) (Jon Atack) 82fd402 refactor: promote some -netinfo localvars to class members (Jon Atack) 5133fab cli: simplify -netinfo using getpeerinfo network field (Jon Atack) 4938a10 rpc, test: expose CNodeStats network in RPC getpeerinfo (Jon Atack) 6df7882 net: add peer network to CNodeStats (Jon Atack) Pull request description: This PR: - builds on bitcoin#19991 and bitcoin#19998 - exposes peer networks via a new getpeerinfo `network` field ("ipv4", "ipv6", or "onion"), and adds functional tests - updates -netinfo to use getpeerinfo `network` rather than detecting the peer networks client-side - refactors -netinfo to easily add future networks ACKs for top commit: laanwj: ACK 6272604 Tree-SHA512: 28883487585135ceaaf84ce09131f2336e3193407f2e3df0960e3f4ac340f500ab94ffecb9d06a4c49bc05e3cca4f914ea4379860bea0bd5df2f834f74616015

Summary: This is a backport of [[bitcoin/bitcoin#19998 | core#19998]] [1/3] bitcoin/bitcoin@d4dde24 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10456

Summary: This is a backport of [[bitcoin/bitcoin#19998 | core#19998]] [2/3] bitcoin/bitcoin@49fba9c Depends on D10456 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D10457

Summary: Note: I didn't backport the inline comments for argument names, because I don't think it is an improvement (my IDE already show the parameter names for literal parameters) and it will not help avoiding future merge conflicts (coding style incompatible with our linter and different signature for the `CNode` constructor) This is a backport of [[bitcoin/bitcoin#19998 | core#19998]] [3/3] bitcoin/bitcoin@3984b78 Depends on D10457 Test Plan: `ninja check` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D10458

DrahtBot added P2P RPC/REST/ZMQ Validation labels Sep 22, 2020

This was referenced Sep 23, 2020

net: guard vRecvGetData with cs_vRecv and orphan_work_set with g_cs_orphans #19911

Merged

net, rpc: expose connection type in getpeerinfo #19883

Closed

hebasto force-pushed the 200922-istor branch from 9efd992 to fcd938e Compare September 23, 2020 07:39

This was referenced Sep 23, 2020

[RPC] Add connection type to getpeerinfo, improve logs #19725

Merged

p2p: Move all header verification into the network layer, extend logging #19107

Merged

refactor: Cleanup thread ctor calls #19064

Merged

torcontrol: add -tortarget config #19043

Closed

DrahtBot mentioned this pull request Sep 23, 2020

Allow whitelisting outgoing connections #17167

Closed

hebasto force-pushed the 200922-istor branch from fcd938e to 911a3f9 Compare September 23, 2020 16:05

hebasto changed the title ~~rpc: Add via_tor to getpeerinfo output~~ Net: Add CNode::ConnectedViaTor() Sep 23, 2020

jonatack reviewed Sep 23, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

This was referenced Sep 24, 2020

net, rpc, cli: expose peer network in getpeerinfo; simplify/improve -netinfo #20002

Merged

net: Use alternative port for incoming Tor connections #19991

Merged

DrahtBot added the Needs rebase label Sep 29, 2020

hebasto force-pushed the 200922-istor branch from c972792 to ac0f497 Compare October 2, 2020 11:42

jonatack reviewed Oct 3, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Show resolved Hide resolved

hebasto force-pushed the 200922-istor branch from 8dcd600 to 5fc8f15 Compare October 3, 2020 09:56

jonatack reviewed Oct 3, 2020

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

net: Add CNode::m_inbound_onion data member

d4dde24

hebasto force-pushed the 200922-istor branch from 5fc8f15 to 3989fcf Compare October 3, 2020 11:27

jonatack approved these changes Oct 3, 2020

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

hebasto added 2 commits October 3, 2020 15:38

net: Add CNode::ConnectedThroughNetwork member function

49fba9c

test: Add tests for CNode::ConnectedThroughNetwork

3984b78

hebasto force-pushed the 200922-istor branch from 3989fcf to 3984b78 Compare October 3, 2020 12:39

laanwj merged commit f79a4a8 into bitcoin:master Oct 12, 2020

hebasto deleted the 200922-istor branch October 12, 2020 16:45

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

net: Add CNode::ConnectedThroughNetwork member function #19998

net: Add CNode::ConnectedThroughNetwork member function #19998

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

practicalswift commented Sep 23, 2020

Uh oh!

laanwj commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Sep 23, 2020

Uh oh!

naumenkogs commented Sep 23, 2020

Uh oh!

jonatack commented Sep 23, 2020

Uh oh!

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

Uh oh!

Uh oh!

jonatack commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Sjors commented Oct 1, 2020

Uh oh!

hebasto commented Oct 2, 2020

Uh oh!

jonatack commented Oct 2, 2020

Uh oh!

hebasto commented Oct 2, 2020

Uh oh!

jonatack commented Oct 2, 2020

Uh oh!

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

Uh oh!

Uh oh!

hebasto commented Oct 3, 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!

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!

hebasto commented Oct 3, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Oct 3, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

hebasto commented Sep 22, 2020 •

edited

Loading

DrahtBot commented Sep 23, 2020 •

edited

Loading

laanwj commented Sep 23, 2020 •

edited

Loading

hebasto commented Sep 23, 2020 •

edited

Loading

jonatack commented Sep 23, 2020 •

edited

Loading

hebasto commented Sep 23, 2020 •

edited

Loading

jonatack commented Oct 2, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack left a comment •

edited

Loading

jonatack commented Oct 3, 2020 •

edited

Loading

laanwj commented Oct 12, 2020 •

edited

Loading