gui: ensure inbound block relay peers have relevant services

jonatack commented at 11:36 pm on January 28, 2021: contributor

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

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

in src/qt/guiutil.cpp:772 in fd0f17d9b4 outdated

765@@ -766,10 +766,10 @@ QString NetworkToQString(Network net)
766     assert(false);
767 }
768 
769-QString ConnectionTypeToQString(ConnectionType conn_type, bool relay_txes)
770+QString ConnectionTypeToQString(ConnectionType conn_type, bool relay_txes, bool relevant_services)
771 {
772     switch (conn_type) {
773-    case ConnectionType::INBOUND: return relay_txes ? QObject::tr("Inbound Full Relay") : QObject::tr("Inbound Block Relay");
774+    case ConnectionType::INBOUND: return !relay_txes && relevant_services ? QObject::tr("Inbound Block Relay") : QObject::tr("Inbound Full Relay");

MarcoFalke commented at 6:21 am on January 29, 2021:

spv nodes aren’t full relay peers either if they have tx relay disabled

jonatack commented at 10:46 pm on January 29, 2021:

thanks for having a look…simplified to “Inbound” and “Inbound Block Relay”, open to other suggestions you have

jonatack force-pushed on Jan 29, 2021

in src/qt/guiutil.cpp:772 in 89c20ee420

765@@ -766,10 +766,10 @@ QString NetworkToQString(Network net)
766     assert(false);
767 }
768 
769-QString ConnectionTypeToQString(ConnectionType conn_type, bool relay_txes)
770+QString ConnectionTypeToQString(ConnectionType conn_type, bool relay_txes, bool relevant_services)
771 {
772     switch (conn_type) {
773-    case ConnectionType::INBOUND: return relay_txes ? QObject::tr("Inbound Full Relay") : QObject::tr("Inbound Block Relay");
774+    case ConnectionType::INBOUND: return !relay_txes && relevant_services ? QObject::tr("Inbound Block Relay") : QObject::tr("Inbound");

MarcoFalke commented at 7:49 am on January 30, 2021:

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 https://github.com/bitcoin/bips/pull/1052 wouldn’t be needed.

jonatack commented at 8:29 am on January 30, 2021:

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.

MarcoFalke commented at 8:47 am on January 30, 2021:

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).

jonatack commented at 3:21 pm on January 30, 2021:

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 https://github.com/bitcoin/bitcoin/pull/20726#issuecomment-754802089:

#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.

MarcoFalke commented at 4:40 pm on January 30, 2021:

Even with BIP https://github.com/bitcoin/bips/pull/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.

jonatack commented at 6:27 pm on January 30, 2021:

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.

MarcoFalke commented at 6:39 pm on January 30, 2021:

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

jonatack commented at 6:43 pm on January 30, 2021:

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

hebasto commented at 8:12 pm on January 30, 2021:

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?

jonatack closed this on Jan 30, 2021

hebasto commented at 7:55 pm on January 30, 2021: member

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 commented at 8:00 pm on January 30, 2021: contributor

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

jonatack reopened this on Jan 30, 2021

hebasto approved

hebasto commented at 8:12 pm on January 30, 2021: member

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

MarcoFalke commented at 9:17 am on February 1, 2021: contributor

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:

Screenshot from 2021-02-01 10-11-44

jonatack commented at 4:39 pm on February 5, 2021: contributor

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

jonatack closed this on Feb 5, 2021

jonasschnelli referenced this in commit 6c6140846f on Feb 5, 2021

bitcoin-core locked this on Aug 16, 2022

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