Display fRelayTxes and bip152_highbandwidth_{to, from} in peer details #206

jonatack commented at 10:40 AM on February 1, 2021: contributor

This pull adds two fields to the peer details, "Wants Tx Relay" (fRelayTxes) and "High Bandwidth" (bip152_highbandwidth to/from). See the added tooltips for more info.

jonatack force-pushed on Feb 8, 2021

jonatack commented at 3:17 PM on February 8, 2021: contributor

I think this is ready for review :rocket: Will add last block and last transaction fields in a separate pull to keep this change small.

Relay Transactions

Screenshot from 2021-02-07 23-26-50

High Bandwidth

Screenshot from 2021-02-07 23-26-46

jonatack marked this as ready for review on Feb 8, 2021

jarolrod commented at 6:03 PM on February 17, 2021: member

Concept ACK, just a few thoughts

The usage of text symbols to indicate something makes sense in a CLI environment, as used in the netinfo dashboard: <img width="807" alt="netinfo-cli" src="https://user-images.githubusercontent.com/23396902/108240765-27c0cd00-7119-11eb-9496-abaf309abbd1.png">

In adding these new fields, I don't think we should continue to use the same text symbols used in a CLI environment in a GUI environment.

Suggestions:

Relay Transactions

I don't think it's great UX to show a checkmark when the peer is relaying transactions and nothing when it isn't. It would be better to have a simple TRUE or FALSE for each case.
The tooltip should also be modified to have bullet points just like the high-bandwidth field's tooltip has

High Bandwidth

The . and * symbols are small. I think a TO and FROM key-word would be a better fit, given the context of the GUI environment, and provide a better UX experience.

When Not Applicable

Currently, if any of these fields are not applicable they will still show up and show up empty. Perhaps we can fill in the space with N/A or not show the field at all. Below is a screenshot showing this behavior. <img width="275" alt="empty-fields" src="https://user-images.githubusercontent.com/23396902/108247160-4c6c7300-7120-11eb-88ea-8a4ab792e11e.png">

in src/qt/forms/debugwindow.ui:1203 in caf9b87d92 outdated

1197 | @@ -1198,13 +1198,65 @@
1198 |                  </widget>
1199 |                 </item>
1200 |                 <item row="6" column="0">
1201 | +                <widget class="QLabel" name="peerRelayTxesLabel">
1202 | +                 <property name="toolTip">
1203 | +                   <string>Whether the peer relays transactions: "✔" if yes, empty if no.</string>

MarcoFalke commented at 8:25 AM on February 18, 2021:

fRelay doesn't say whether the peer relays transactions. fRelay says whether the peer asked us to relay transactions or not.

jonatack commented at 10:19 AM on February 18, 2021:

Thanks -- fixed

MarcoFalke commented at 8:25 AM on February 18, 2021: contributor

Concept ACK

gui: display fRelayTxes in peer details 9476886353

gui: display BIP152 high bandwidth relay in peer details 142807af8b

jonatack force-pushed on Feb 18, 2021

jonatack commented at 10:20 AM on February 18, 2021: contributor

Thanks @jarolrod and @MarcoFalke. Updated to "Wants Tx Relay" (Yes/No) and "High Bandwidth" (To/From/No). Updated the tooltips accordingly.

jarolrod commented at 1:02 PM on February 18, 2021: member

ACK 142807af8b82e2372a03df893c50df4f4a96aca4

The Yes/No for Relay Transactions and the To/From/No for High Bandwidth is certainly an improvement

MarcoFalke commented at 1:19 PM on February 18, 2021: contributor

review ACK 142807af8b82e2372a03df893c50df4f4a96aca4

in src/qt/rpcconsole.cpp:1117 in 142807af8b

1113 | @@ -1109,6 +1114,12 @@ void RPCConsole::updateDetailWidget()
1114 |          peerAddrDetails += "<br />" + tr("via %1").arg(QString::fromStdString(stats->nodeStats.addrLocal));
1115 |      ui->peerHeading->setText(peerAddrDetails);
1116 |      ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStats.nServices));
1117 | +    ui->peerRelayTxes->setText(stats->nodeStats.fRelayTxes ? "Yes" : "No");

Talkless commented at 7:13 PM on February 20, 2021:

Yes, No, To, From not translatable?

I've launched bitcoin-qt in my language and I see some values translated (like N/A is translated to appropriate alternative), I assume these should be translated too.

jarolrod commented at 7:17 PM on February 20, 2021:

Those are new strings being introduced in the PR. They will need translations which will be done when they are placed on Transifex.

Talkless commented at 7:21 PM on February 20, 2021:

I mean, these strings are not wrapped in tr().

jonatack commented at 5:28 PM on February 23, 2021:

Thanks @Talkless, done in #221.

in src/qt/rpcconsole.cpp:1120 in 142807af8b

1113 | @@ -1109,6 +1114,12 @@ void RPCConsole::updateDetailWidget()
1114 |          peerAddrDetails += "<br />" + tr("via %1").arg(QString::fromStdString(stats->nodeStats.addrLocal));
1115 |      ui->peerHeading->setText(peerAddrDetails);
1116 |      ui->peerServices->setText(GUIUtil::formatServicesStr(stats->nodeStats.nServices));
1117 | +    ui->peerRelayTxes->setText(stats->nodeStats.fRelayTxes ? "Yes" : "No");
1118 | +    QString bip152_hb_settings;
1119 | +    if (stats->nodeStats.m_bip152_highbandwidth_to) bip152_hb_settings += "To";
1120 | +    if (stats->nodeStats.m_bip152_highbandwidth_from) bip152_hb_settings += (bip152_hb_settings == "" ? "From" : "/From");

Talkless commented at 7:14 PM on February 20, 2021:

nit: Consider using .isEmpty() instead of == "", some times it might be costly: https://wiki.qt.io/Using_QString_Effectively

MarcoFalke commented at 7:20 AM on February 22, 2021:

Slightly prefer isEmpty as well, but this shouldn't have any impact on performance and should be only a stylistic matter

Talkless commented at 6:51 PM on February 22, 2021:

@MarcoFalke for std::string, a template class that has probably all code inlined - maybe (though == "" for std::string is also generating more code - see https://godbolt.org/z/h8r1Wq).

But QString invokes out-of-lineQString::compare_helper with size set to -1, so it will, in the runtime, call strlen to determine lenght.. while isEmpty() is just inline { return d->size == 0; }.

Using == "" to check empty string bloats code for a else trivial check.

I agree that we don't need any (premature-)optimizations here (not performance critical code at all), but not using isEmpty() (or std::string::empty() for that matter) is just a needless pessimization and (relative) code bloat.

jonatack commented at 5:29 PM on February 23, 2021:

Agree, thanks @Talkless, done in #221.

Talkless commented at 7:15 PM on February 20, 2021: none

Concept ACK.

MarcoFalke merged this on Feb 22, 2021

MarcoFalke closed this on Feb 22, 2021

jonatack deleted the branch on Feb 22, 2021

jonatack commented at 10:47 AM on February 22, 2021: contributor

Good points @Talkless, will retouch those in the next patch adding last block and last transaction fields.

sidhujag referenced this in commit c989aaabbf on Feb 22, 2021

MarcoFalke referenced this in commit 1a4a9305c2 on Mar 7, 2021

Fabcien referenced this in commit 3681ffebdd on Feb 3, 2022

gwillen referenced this in commit 8bfdfd9e77 on Jun 28, 2022

bitcoin-core locked this on Aug 16, 2022