gui: ensure inbound block relay peers have relevant services #201

jonatack · 2021-01-28T23:36:01Z

Responding to #180 (comment), this ensures that inbound block relay peers have relevant services and returns all others as inbound.

src/qt/guiutil.cpp

maflcko · 2021-01-30T07:49:59Z

src/qt/guiutil.cpp

 {
    switch (conn_type) {
-    case ConnectionType::INBOUND: return relay_txes ? QObject::tr("Inbound Full Relay") : QObject::tr("Inbound Block Relay");
+    case ConnectionType::INBOUND: return !relay_txes && relevant_services ? QObject::tr("Inbound Block Relay") : QObject::tr("Inbound");


This heuristic seems to be working fine. At least I can't find a counter example right now that can be observed on today's network (Edit: counter example: #201 (comment)). Though, it still seems fragile to assume that the type of the inbound connection can be deduced based on fuzzy logic.

If it was fine to assume so, then BIP bitcoin/bips#1052 wouldn't be needed.

If helpful, the logic is from https://github.com/bitcoin/bitcoin/blob/16b784d953365bb2d7ae65acd2b20a79ef8ba7b6/src/net.cpp#L872 in our inbound eviction protection logic, so that's why I was interested in seeing these peers. Otherwise we could just display all of them as inbound.

I think plain "Inbound" makes most sense because it is neither possible to enumerate all inbound connection types, nor is it possible to determine them (if they were enumerable).

Yes, it seems we will need to deduce some types of connections from other properties, as currently done to obtain the inbound block relay peers for eviction protection. Take for instance this bitcoin/bitcoin#20726 (comment):

#19670 introduced a workaround to deduce inbound block-relay-only peers and protect them from eviction. After this PR, do you think we could revisit to make it more direct?

I think we would wait until this has been deployed for a while and then (potentially) change the eviction logic. Two things occur to me on this point:

Older software will continue to use the old way of (not) communicating their intentions, via the fRelayTxes flag. So to accommodate older software we may not want to switch to keying off this p2p message for a little while at least.

It seems most of the feedback has been to shy away from encoding overly broad semantics into a single p2p message; while I think that is fine it also means that from our peer's point of view, they will have to infer the connection "type" from the properties of the peer, rather than from the peer declaring its full intentions in a single message. I think for now we can treat "will never relay txes" as essentially synonymous with these block-relay connections, but perhaps in the future there will be some distinction, and all a peer can do is rely on some set of flags that it might use to preference some peers over others -- which might put us exactly back to where we are today, with us just keying off of whether fRelayTxes is false, anyway.

So while I'd like to see this type of peers easily, yes for now it may be more prudent to just use plain "Inbound" and display other properties useful for inferring the connection status, like fRelayTxes, separately.

Even with BIP bitcoin/bips#1052 you couldn't enumerate all possible inbound connection types. This is simply impossible.

Using properties of the connection like service bits or relay flags (and maybe later a disabletx message) or other statistics like last sent tx/block to aid eviction can be done without exactly enumerating the connection types.

I don't disagree, though as a user I'd like to see this information...either directly as proposed here, or indirectly by manually looking at the inbound + relaytxes + services/servicesnames getpeerinfo boolean fields. Anyway, closing for now, replaced by #203.

Services and inbound status should be shown in the gui. I don't recall whether version.fRelay is shown

Yes, inbound and services are. Proposing now to add frelaytxes and bip152 high bandwidth.

hebasto · 2021-01-30T19:55:17Z

Sorry for being late...

It's unfortunate that it is closed for the following reasons:

no need to enumerate all possible inbound connection types, only block-relay-only are important to highlight as they form a subnetwork with a node graph that is untraceable for spies; other inbound connection types differ between each other only by requested resources;
using heuristic to detect "potential block-relay only peers" seems fine to me

Concept ACK (and ready to test if it'll be reopened).

Nevertheless, will review #203.

jonatack · 2021-01-30T20:00:05Z

@hebasto Thanks--I'm happy to re-open if desired.

hebasto

ACK 89c20ee, tested on Linux Mint 20.1 (Qt 5.12.8).

hebasto · 2021-01-30T20:12:03Z

src/qt/guiutil.cpp

 {
    switch (conn_type) {
-    case ConnectionType::INBOUND: return relay_txes ? QObject::tr("Inbound Full Relay") : QObject::tr("Inbound Block Relay");
+    case ConnectionType::INBOUND: return !relay_txes && relevant_services ? QObject::tr("Inbound Block Relay") : QObject::tr("Inbound");


pico-nit:

If helpful, the logic is from https://github.com/bitcoin/bitcoin/blob/16b784d953365bb2d7ae65acd2b20a79ef8ba7b6/src/net.cpp#L872 in our inbound eviction protection logic...

Maybe add a relevant comment here?

maflcko · 2021-02-01T09:17:05Z

only block-relay-only are important to highlight as they form a subnetwork with a node graph that is untraceable for spies; [...] using heuristic to detect "potential block-relay only peers" seems fine to me

My point is that what is displayed in the gui is not the "untraceable subnetwork". Whether a node asked you to send txs has nothing to do of their behaviour to send you txs (or other peers they have sent a version.fRalay=false), so I think incorrectly classifying them as "block-relay-only" is confusing at best. It would be good to describe your use case, but if your use case is to determine the "untraceable subnetwork", this heuristic is neither accurate nor precise. https://en.wikipedia.org/wiki/Accuracy_and_precision

Even when ignoring all different p2p clients on the network and only considering vanilla Bitcoin Core nodes (at the commit of this pull) wont give precise results for -blocksonly peers:

jonatack · 2021-02-05T16:39:37Z

Thanks for the discussions @MarcoFalke and @hebasto. I agree this can be deferred for now and have aligned -netinfo (in merged pull bitcoin/bitcoin#20764) in the same direction as #203.

506e658 gui: display plain "Inbound" in peer details (Jon Atack) Pull request description: Alternative version to #201. ACKs for top commit: MarcoFalke: ACK 506e658 jonasschnelli: utACK 506e658 Tree-SHA512: 88d141b14684c1dcdff47f7ba241e5a7c42c14da3d9aaa89f1649235a64fd26bc5a6055707dc07992cd9d8c05d143754f6dd51ccee69fd4309336dd07c52e61c

jonatack mentioned this pull request Jan 28, 2021

Peer details: connection type follow-ups #180

Merged

maflcko reviewed Jan 29, 2021

View reviewed changes

src/qt/guiutil.cpp Outdated Show resolved Hide resolved

jonatack mentioned this pull request Jan 29, 2021

Add Type column to peers window, update peer details name/tooltip #179

Merged

gui: ensure inbound-block-relay peers have relevant services

89c20ee

jonatack force-pushed the inbound-block-relay branch from fd0f17d to 89c20ee Compare January 29, 2021 22:41

maflcko reviewed Jan 30, 2021

View reviewed changes

jonatack closed this Jan 30, 2021

jonatack mentioned this pull request Jan 30, 2021

Display plain "Inbound" in peer details #203

Merged

jonatack reopened this Jan 30, 2021

hebasto approved these changes Jan 30, 2021

View reviewed changes

jonatack closed this Feb 5, 2021

jonatack mentioned this pull request Apr 22, 2021

gui: Node appears as Unroutable in peers list bitcoin/bitcoin#21747

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gui: ensure inbound block relay peers have relevant services #201

gui: ensure inbound block relay peers have relevant services #201

jonatack commented Jan 28, 2021 •

edited

Loading

maflcko Jan 30, 2021 •

edited

Loading

jonatack Jan 30, 2021 •

edited

Loading

maflcko Jan 30, 2021

jonatack Jan 30, 2021

maflcko Jan 30, 2021

jonatack Jan 30, 2021

maflcko Jan 30, 2021

jonatack Jan 30, 2021

hebasto commented Jan 30, 2021

jonatack commented Jan 30, 2021

hebasto left a comment

hebasto Jan 30, 2021

maflcko commented Feb 1, 2021

jonatack commented Feb 5, 2021

gui: ensure inbound block relay peers have relevant services #201

gui: ensure inbound block relay peers have relevant services #201

Conversation

jonatack commented Jan 28, 2021 • edited Loading

maflcko Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

jonatack Jan 30, 2021 • edited Loading

Choose a reason for hiding this comment

maflcko Jan 30, 2021

Choose a reason for hiding this comment

jonatack Jan 30, 2021

Choose a reason for hiding this comment

maflcko Jan 30, 2021

Choose a reason for hiding this comment

jonatack Jan 30, 2021

Choose a reason for hiding this comment

maflcko Jan 30, 2021

Choose a reason for hiding this comment

jonatack Jan 30, 2021

Choose a reason for hiding this comment

hebasto commented Jan 30, 2021

jonatack commented Jan 30, 2021

hebasto left a comment

Choose a reason for hiding this comment

hebasto Jan 30, 2021

Choose a reason for hiding this comment

maflcko commented Feb 1, 2021

jonatack commented Feb 5, 2021

jonatack commented Jan 28, 2021 •

edited

Loading

maflcko Jan 30, 2021 •

edited

Loading

jonatack Jan 30, 2021 •

edited

Loading