Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/qt/guiutil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,10 +766,10 @@ QString NetworkToQString(Network net)
assert(false);
}

QString ConnectionTypeToQString(ConnectionType conn_type, bool relay_txes)
QString ConnectionTypeToQString(ConnectionType conn_type, bool relay_txes, bool relevant_services)
{
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");
Copy link
Contributor

@maflcko maflcko Jan 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jonatack jonatack Jan 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

case ConnectionType::OUTBOUND_FULL_RELAY: return QObject::tr("Outbound Full Relay");
case ConnectionType::BLOCK_RELAY: return QObject::tr("Outbound Block Relay");
case ConnectionType::MANUAL: return QObject::tr("Outbound Manual");
Expand Down
3 changes: 2 additions & 1 deletion src/qt/guiutil.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <fs.h>
#include <net.h>
#include <netaddress.h>
#include <protocol.h>

#include <QEvent>
#include <QHeaderView>
Expand Down Expand Up @@ -233,7 +234,7 @@ namespace GUIUtil
QString NetworkToQString(Network net);

/** Convert enum ConnectionType to QString */
QString ConnectionTypeToQString(ConnectionType conn_type, bool relay_txes);
QString ConnectionTypeToQString(ConnectionType conn_type, bool relay_txes, bool relevant_services);

/** Convert seconds into a QString with days, hours, mins, secs */
QString formatDurationStr(int secs);
Expand Down
4 changes: 2 additions & 2 deletions src/qt/rpcconsole.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty

constexpr QChar nonbreaking_hyphen(8209);
const std::vector<QString> CONNECTION_TYPE_DOC{
tr("Inbound Full/Block Relay: initiated by peer"),
tr("Inbound/Inbound Block Relay: initiated by peer"),
tr("Outbound Full Relay: default"),
tr("Outbound Block Relay: does not relay transactions or addresses"),
tr("Outbound Manual: added using RPC %1 or %2/%3 configuration options")
Expand Down Expand Up @@ -1120,7 +1120,7 @@ void RPCConsole::updateDetailWidget()
ui->timeoffset->setText(GUIUtil::formatTimeOffset(stats->nodeStats.nTimeOffset));
ui->peerVersion->setText(QString::number(stats->nodeStats.nVersion));
ui->peerSubversion->setText(QString::fromStdString(stats->nodeStats.cleanSubVer));
ui->peerConnectionType->setText(GUIUtil::ConnectionTypeToQString(stats->nodeStats.m_conn_type, stats->nodeStats.fRelayTxes));
ui->peerConnectionType->setText(GUIUtil::ConnectionTypeToQString(stats->nodeStats.m_conn_type, stats->nodeStats.fRelayTxes, HasAllDesirableServiceFlags(stats->nodeStats.nServices)));
ui->peerNetwork->setText(GUIUtil::NetworkToQString(stats->nodeStats.m_network));
if (stats->nodeStats.m_permissionFlags == PF_NONE) {
ui->peerPermissions->setText(tr("N/A"));
Expand Down