Peer details: replace Direction with Connection Type

jonatack commented at 12:27 pm on December 24, 2020: contributor

Screenshot from 2021-01-09 11-23-17

Closes #159.

MarcoFalke commented at 12:34 pm on December 24, 2020: contributor

Nice. Concept ACK

in src/qt/guiutil.cpp:763 in c093dedbc1 outdated

758+    if (conn_type == "feeler") return "Outbound feeler";
759+    if (conn_type == "addr-fetch") return "Outbound address fetch";
760+    // Inbound peer connections
761+    if (conn_type == "inbound") return "Inbound full relay";
762+    if (conn_type == "inbound-block-relay") return "Inbound block relay";
763+    return "";

MarcoFalke commented at 12:37 pm on December 24, 2020:

nit: Would be nice if this default fallback was replaced with an assert(false) and the above ifs with an enum-switch-case

jonatack commented at 12:38 pm on December 24, 2020:

Agree, but ISTM that only works for integer types.

MarcoFalke commented at 12:40 pm on December 24, 2020:

ConnectionType is an enum class (implementation defined integer)

jonatack commented at 3:23 pm on December 24, 2020:

Right, I wanted to do that but the ConnectionType enum wasn’t exposed in CNodeStats. Ok, done.

jonatack force-pushed on Dec 24, 2020

hebasto commented at 10:58 am on December 25, 2020: member

Concept ACK.

in src/qt/guiutil.h:228 in bf8135db2a outdated

224@@ -224,6 +225,9 @@ namespace GUIUtil
225     /* Convert OS specific boost path to QString through UTF-8 */
226     QString boostPathToQString(const fs::path &path);
227 
228+    /* Convert ConnectionType enum to QString */

hebasto commented at 1:29 pm on December 25, 2020:

571b0469fa952be98874d1afd772a7fa708dd83f

This comment is not Doxygen-compatible.

jonatack commented at 5:10 pm on December 25, 2020:

thanks! fixed

in src/qt/guiutil.cpp:779 in bf8135db2a outdated

759+    case ConnectionType::FEELER: return "Outbound feeler";
760+    case ConnectionType::ADDR_FETCH: return "Outbound address fetch";
761+    } // no default case, so the compiler can warn about missing cases
762+    assert(false);
763+}
764+

hebasto commented at 1:33 pm on December 25, 2020:

571b0469fa952be98874d1afd772a7fa708dd83f

The returned values should be translated.

UX note: I have a personal preference do not capitalize the first letter of returned values. Feel free to ignore it.

jonatack commented at 2:20 pm on December 25, 2020:

I also prefer all lowercase (e.g. rpc/cli) but the GUI UI uses capitalization (I hesitated to use titlecase here).

hebasto commented at 2:44 pm on December 25, 2020:

I perceive it like “Connection type: Outbound full relay”. That’s why I prefer the lowercased variant.

If the capitalized variant will be required in some other places in the GUI, it is easy to provide it with a generic function.

I’m not a design expert, then I’m fine with any variant.

jonatack commented at 5:11 pm on December 25, 2020:

Thanks–translated and changed to titlecase for coherence with the general GUI UI.

in src/qt/guiutil.cpp:755 in bf8135db2a outdated

748@@ -749,6 +749,19 @@ QString boostPathToQString(const fs::path &path)
749     return QString::fromStdString(path.string());
750 }
751 
752+QString ConnectionTypeToQString(ConnectionType conn_type)
753+{
754+    switch (conn_type) {
755+    case ConnectionType::INBOUND: return "Inbound full relay";

hebasto commented at 1:35 pm on December 25, 2020:

571b0469fa952be98874d1afd772a7fa708dd83f

It seems not always true :)

IIUC, you recent changes in bitcoin-cli distinguish “block relay” for inbound connections as well.

jonatack commented at 2:37 pm on December 25, 2020:

IIUC, you recent changes in bitcoin-cli distinguish “block relay” for inbound connections as well.

Yes, I’m anticipating inbound full relay and inbound block relay connection types (https://github.com/bitcoin/bitcoin/pull/20726 and https://github.com/bitcoin/bitcoin/pull/20729) but I guess it’s better to revert to just inbound for now and update it later.

jonatack commented at 5:11 pm on December 25, 2020:

Now “Inbound”

in src/qt/forms/debugwindow.ui:1083 in bf8135db2a outdated

1076@@ -1077,14 +1077,17 @@
1077                 </widget>
1078                </item>
1079                <item row="1" column="0">
1080-                <widget class="QLabel" name="label_23">
1081+                <widget class="QLabel" name="peerConnectionTypeLabel">
1082+                 <property name="toolTip">
1083+                  <string>The connection type of this peer.</string>
1084+                 </property>

hebasto commented at 1:43 pm on December 25, 2020:

bf8135db2a658aed3129ec82e237f8030a8ec7dd

I’ve noted your comment, but here the tooltip provides no additional info. I don’t think that adding such a tooltip for the sake itself is a good approach for the GUI:

jonatack commented at 2:31 pm on December 25, 2020:

True. Will improve it with more useful info. Perhaps it should link to more information about the connection types (like we have in the codebase), maybe in a markdown file in /doc.

jonatack commented at 2:39 pm on December 25, 2020:

Maybe I should add more useful info, like in the RPC help (getpeerinfo)

hebasto commented at 2:50 pm on December 25, 2020:

Anyway, please keep it useful and succinct, or else do not add it at all :)

jonatack commented at 5:12 pm on December 25, 2020:

Added info similar to the getpeerinfo help, see updated screenshot in the PR description.

hebasto changes_requested

hebasto commented at 1:43 pm on December 25, 2020: member

Concept ACK.

I have reviewed the code (bf8135db2a658aed3129ec82e237f8030a8ec7dd), and tested it.

jonatack force-pushed on Dec 25, 2020

jonatack commented at 5:14 pm on December 25, 2020: contributor

Thanks for the excellent feedback @hebasto! Updated.

hebasto commented at 9:29 pm on December 25, 2020: member

I have reviewed the code (8e7f82a6a84c7b2553f88767dd356f480d1ae4b7), and tested it.

The following patch:

 0--- a/src/qt/forms/debugwindow.ui
 1+++ b/src/qt/forms/debugwindow.ui
 2@@ -1079,7 +1079,7 @@
 3                <item row="1" column="0">
 4                 <widget class="QLabel" name="peerConnectionTypeLabel">
 5                  <property name="toolTip">
 6-                  <string>The type of peer connection: Outbound Full Relay (default), Outbound Block Relay (does not relay transactions or addresses), Inbound (initiated by peer), Outbound Manual (added using RPC addnode or -addnode/-connect config options), Outbound Feeler (short-lived, for testing addresses), or Outbound Address Fetch (short-lived, for soliciting addresses).</string>
 7+                  <string>The type of peer connection: Outbound Full Relay (default), Outbound Block Relay (does not relay transactions or addresses), Inbound (initiated by peer), Outbound Manual (added using RPC %1 or %2/%3 config options), Outbound Feeler (short-lived, for testing addresses), or Outbound Address Fetch (short-lived, for soliciting addresses).</string>
 8                  </property>
 9                  <property name="text">
10                   <string>Connection Type</string>
11--- a/src/qt/rpcconsole.cpp
12+++ b/src/qt/rpcconsole.cpp
13@@ -496,6 +496,8 @@ RPCConsole::RPCConsole(interfaces::Node& node, const PlatformStyle *_platformSty
14     clear();
15 
16     GUIUtil::handleCloseWindowShortcut(this);
17+
18+    ui->peerConnectionTypeLabel->setToolTip(ui->peerConnectionTypeLabel->toolTip().arg("addnode").arg(QString(nonbreaking_hyphen) + "addnode").arg(QString(nonbreaking_hyphen) + "connect"));
19 }
20 
21 RPCConsole::~RPCConsole()

removes RPC and option names from the translatable string
prevents effects like this

jonatack commented at 9:46 pm on December 25, 2020: contributor

Thanks @hebasto, tested and added your suggestion, updated the screenshot in the PR description.

hebasto approved

hebasto commented at 9:55 pm on December 25, 2020: member

ACK 6cd242c50f4a8d3f3c509937d80962fe58096ead, tested on Linux Mint 20 (x86_64) + Qt 5.12.8.

Looks great: Screenshot from 2020-12-25 23-52-27

in src/net.h:729 in a60f67b2fb outdated

724@@ -725,6 +725,7 @@ class CNodeStats
725     // Name of the network the peer connected through
726     std::string m_network;
727     uint32_t m_mapped_as;
728+    ConnectionType m_conn_type;

promag commented at 6:55 pm on December 26, 2020:

a60f67b2fb0eede205db8cce11ad0e6f42dbc90e

This was suggested here https://github.com/bitcoin/bitcoin/pull/19725#discussion_r473361238 and now makes m_conn_type_string redundant, doesn’t makes sense to keep both TBH. Since m_conn_type_string is only used in getpeerinfo then drop it and refactor ConnectionTypeAsString to take conn_type instead?

MarcoFalke commented at 8:45 pm on December 26, 2020:

Concept ACK, but RPC changes (refactoring) can only be made in the main repo

jonatack commented at 9:19 pm on December 26, 2020:

Done, I think. Should I close this PR and re-open in the main repo?

hebasto commented at 9:24 pm on December 26, 2020:

Maybe just open a pr in the main repo with the first and last commits?

promag commented at 9:26 pm on December 26, 2020:

IMO this isn’t a RPC change, it’s just a member refactor in a net struct, and the refactor is motivated by this GUI change. Otherwise this can go forward in 2 ways I think, either leave this as it was, and then in main repo drop the redundant member, or 1st refactor and make this dependant of that PR.

jonatack commented at 10:29 pm on December 26, 2020:

Opened the same pull in https://github.com/bitcoin/bitcoin/pull/20778 and will let the maintainers decide, closing this one for now to not maintain multiple pulls.

promag commented at 12:24 pm on December 28, 2020:

Just noting that redundancy is not a blocker, but could be avoided.

jonatack force-pushed on Dec 26, 2020

jonatack closed this on Dec 28, 2020

jonatack commented at 9:45 pm on December 28, 2020: contributor

It was suggested to re-open here.

jonatack reopened this on Dec 28, 2020

MarcoFalke commented at 9:50 pm on December 28, 2020: contributor

Concept re-ACK. Thanks for re-opening.

DrahtBot added the label Needs rebase on Dec 28, 2020

jonatack force-pushed on Dec 28, 2020

jonatack commented at 11:43 pm on December 28, 2020: contributor

Rebased.

DrahtBot removed the label Needs rebase on Dec 29, 2020

hebasto approved

hebasto commented at 4:57 pm on December 30, 2020: member

re-ACK aef969363a7685fc805833a3ec6863c27500d02f, since my previous review only rebased and the “p2p, rpc, refactor: remove CNodeStats::m_conn_type_string” commit added.

DrahtBot added the label Needs rebase on Jan 2, 2021

jonatack force-pushed on Jan 2, 2021

jonatack commented at 9:14 am on January 2, 2021: contributor

Rebased git range-diff ae8f797 aef9693 7098509

DrahtBot removed the label Needs rebase on Jan 2, 2021

in src/rpc/net.cpp:252 in 70985097c0 outdated

248@@ -249,7 +249,7 @@ static RPCHelpMan getpeerinfo()
249                 recvPerMsgCmd.pushKV(i.first, i.second);
250         }
251         obj.pushKV("bytesrecv_per_msg", recvPerMsgCmd);
252-        obj.pushKV("connection_type", stats.m_conn_type_string);
253+        obj.pushKV("connection_type", ConnectionTypeAsString(stats.m_conn_type));

hebasto commented at 9:35 am on January 2, 2021:

70985097c00827de94a7922620b63e1ee10dd53d

Mind fixing https://github.com/bitcoin/bitcoin/pull/19725/commits/395acfa83a5436790c1a722a5609ac9d48df235f (https://github.com/bitcoin/bitcoin/pull/19725) by aligning the order of the “connection_type” field both in RPC help and in RPC output?

jonatack commented at 9:39 pm on January 2, 2021:

thanks; done

hebasto approved

hebasto commented at 9:35 am on January 2, 2021: member

re-ACK 70985097c00827de94a7922620b63e1ee10dd53d, only rebased since my previous review.

jonatack force-pushed on Jan 2, 2021

jonatack commented at 9:43 pm on January 2, 2021: contributor

Thanks @hebasto, updated the last commit to move the getpeerinfo connection_type help to the correct place per review feedback in #163 (review) and updated the commit message.

hebasto approved

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

re-ACK be8294ff428da7d13043b3f9a27d7ace1c2dadf4

DrahtBot added the label Needs rebase on Jan 7, 2021

laanwj commented at 3:05 pm on January 7, 2021: member

Concept ACK

gui: create GUIUtil::ConnectionTypeToQString utility function 7e2beab2d2

jonatack force-pushed on Jan 8, 2021

jonatack commented at 2:39 pm on January 8, 2021: contributor

Rebased after the merge of https://github.com/bitcoin/bitcoin/pull/20786.

hebasto approved

hebasto commented at 3:20 pm on January 8, 2021: member

re-ACK 20fa97560b2d27f94f556a570f3556356877b79f, only rebased since my previous review due to the conflict with https://github.com/bitcoin/bitcoin/pull/20786, verified with

0$ git range-diff master be8294ff428da7d13043b3f9a27d7ace1c2dadf4 20fa97560b2d27f94f556a570f3556356877b79f

DrahtBot removed the label Needs rebase on Jan 8, 2021

jarolrod commented at 4:30 pm on January 8, 2021: member

ACK 20fa97560b2d27f94f556a570f3556356877b79f

Tested on macOS Big Sur 11.1 with qt 5.15.2. Looks great!

Small NIT: There’s a lot of text in the tooltip. It would be nice if all the possible values were put into a list instead of separated by commas. It would look something like this, which to me would be an improvement.

See: Supported HTML Subset,

jonatack commented at 11:07 pm on January 8, 2021: contributor

Better? If yes, bold or no?

Also, any opinions on Inbound being first in the list vs where it is currently.

Screenshot from 2021-01-09 00-04-04 Screenshot from 2021-01-08 23-57-49

jarolrod commented at 11:49 pm on January 8, 2021: member

@jonatack the list looks great! I think not-bold is better. I think that inbound can stay where it is. If i may, I would suggest one more thing:

Write the descriptions like this: Outbound Block Relay: does not relay transactions or addresses

instead of: Outbound Block Relay (does not relay transactions or addresses)

jonatack commented at 0:13 am on January 9, 2021: contributor

@jarolrod test with inbound first and your colon suggestion (agree, it’s more readable)

Screenshot from 2021-01-09 01-10-03

jarolrod commented at 0:18 am on January 9, 2021: member

@jonatack looks awesome 👍

jonatack force-pushed on Jan 9, 2021

jonatack commented at 10:34 am on January 9, 2021: contributor

Thanks @jarolrod. Updated in latest push, if you’d like to review/test. @hebasto, can you verify the changed tooltip code?

hebasto commented at 12:09 pm on January 9, 2021: member

@hebasto, can you verify the changed tooltip code?

The used HTML code is valid.

But I’d prefer to keep the tooltip content in the form *.ui file. It makes usage of design tools (e.g., Qt Designer) much easier. In *.cpp file should be text substitution code only.

jonatack force-pushed on Jan 9, 2021

jonatack commented at 12:47 pm on January 9, 2021: contributor

@hebasto thanks–done.

gui: replace Direction with Connection Type in peer details 2c19ba2e1d

gui: improve connection type tooltip

- remove RPC and option names from the translatable string
- use non-breaking hyphens

c95fe6e38f

rpc: move getpeerinfo connection_type help to correct place

per review feedback

06ba9b3008

jonatack force-pushed on Jan 9, 2021

hebasto approved

hebasto commented at 12:59 pm on January 9, 2021: member

re-ACK 06ba9b300866f33e21512af9d7d2891ee1501bf4, the tooltip content is organized as unordered list.

jarolrod commented at 3:36 pm on January 9, 2021: member

re-ACK 06ba9b300866f33e21512af9d7d2891ee1501bf4

MarcoFalke merged this on Jan 10, 2021

MarcoFalke closed this on Jan 10, 2021

in src/qt/forms/debugwindow.ui:1082 in 06ba9b3008

1076@@ -1077,14 +1077,17 @@
1077                 </widget>
1078                </item>
1079                <item row="1" column="0">
1080-                <widget class="QLabel" name="label_23">
1081+                <widget class="QLabel" name="peerConnectionTypeLabel">
1082+                 <property name="toolTip">
1083+                   <string>The type of peer connection:&lt;ul&gt;&lt;li&gt;Inbound: initiated by peer&lt;/li&gt;&lt;li&gt;Outbound Full Relay: default&lt;/li&gt;&lt;li&gt;Outbound Block Relay: does not relay transactions or addresses&lt;/li&gt;&lt;li&gt;Outbound Manual: added using RPC %1 or %2/%3 configuration options&lt;/li&gt;&lt;li&gt;Outbound Feeler: short-lived, for testing addresses&lt;/li&gt;&lt;li&gt;Outbound Address Fetch: short-lived, for soliciting addresses&lt;/li&gt;&lt;/ul&gt;</string>

MarcoFalke commented at 9:45 am on January 10, 2021:

translation nit: How are translators supposed to translate :<ul><li>? It might be better to say “Can be one of %s” and then use Join(translated_strings, ", ") or simply “Refer to the getpeerinfo RPC help for details on the connection types”?

jonatack commented at 10:08 am on January 10, 2021:

@hebasto, if we replace the escaped markup with placeholders (as done here for the nonbreaking hyphen), is that still QT Designer-friendly? Can do it in #179.

hebasto commented at 10:22 am on January 10, 2021:

translation nit: How are translators supposed to translate :<ul><li>?

There was a discussion in private IRC with @jonatack:

<hebasto> 14:24:52 but I expect the translators’ burden :) 14:25:39 hope that translators are aware of escaping :)

I’m not an expert in translation but I believe that :<ul><li> is not a subject to translate as %1 or %2.

It might be better to say “Can be one of %s” and then use Join(translated_strings, ", ") or simply “Refer to the getpeerinfo RPC help for details on the connection types”?

Either of the suggested variants LGTM.

if we replace the escaped markup with placeholders (as done here for the nonbreaking hyphen), is that still QT Designer-friendly?

I think it is. But, tbh, I’d prefer more succinct @MarcoFalke’s suggestions.

jonatack commented at 10:39 am on January 10, 2021:

Thanks. Helpfulness and readability for users were the reasons for the iterations above and it seems best to not regress on those improvements with referring GUI users to the command line or dropping the readable list layout. I’ll propose replacing the escaped markup with placeholders.

hebasto commented at 10:47 am on January 10, 2021:

Why not a single placeholder for the whole unordered list? We still have a (shorter) tooltip in the form file, and make translators’ life easier :)

jonatack commented at 11:01 am on January 10, 2021:

SGTM too.

jonatack commented at 1:56 pm on January 10, 2021:

Done in #180

jonatack deleted the branch on Jan 10, 2021

sidhujag referenced this in commit dc579d1895 on Jan 10, 2021

laanwj referenced this in commit d1ddead09a on Jan 27, 2021

jonatack referenced this in commit ff8afeb222 on Jan 30, 2021

bitcoin-core locked this on Aug 16, 2022

Peer details: replace Direction with Connection Type #163