Add "Last Block" and "Last Tx" rows to peer details area #226

jonatack commented at 4:19 PM on February 24, 2021: contributor

add RPCConsole::TimeDurationField helper to replace repeated code and call system time only once in RPCConsole::updateDetailWidget
add "Last Tx" (CNodeStats::nLastTXTime) field to peer details
add "Last Block" (CNodeStats::nLastBlockTime) field to peer details

jonatack commented at 5:09 PM on February 24, 2021: contributor

Screenshots :cake:

Screenshot from 2021-02-24 18-04-13

Screenshot from 2021-02-24 18-04-17

Screenshot from 2021-02-24 17-02-58

Screenshot from 2021-02-24 17-03-13

in src/qt/forms/debugwindow.ui:1347 in c2758983f2 outdated

1341 | @@ -1342,13 +1342,65 @@
1342 |                  </widget>
1343 |                 </item>
1344 |                 <item row="12" column="0">
1345 | +                <widget class="QLabel" name="peerLastBlockLabel">
1346 | +                 <property name="toolTip">
1347 | +                  <string>Elapsed time since a novel block passing initial validity checks was received from this peer.</string>

jarolrod commented at 5:46 PM on February 24, 2021:

                  <string>Elapsed time since a novel block was received from this peer.</string>

in src/qt/forms/debugwindow.ui:1373 in c2758983f2 outdated

1368 | +                </widget>
1369 | +               </item>
1370 | +               <item row="13" column="0">
1371 | +                <widget class="QLabel" name="peerLastTxLabel">
1372 | +                 <property name="toolTip">
1373 | +                  <string>Elapsed time since a novel transaction accepted into our mempool was received from this peer.</string>

jarolrod commented at 5:47 PM on February 24, 2021:

                  <string>Elapsed time since a novel transaction was received from this peer.</string>

jarolrod commented at 5:49 PM on February 24, 2021:

alternative

                  <string>Elapsed time since a novel transaction, accepted into our mempool, was received from this peer.</string>

jarolrod commented at 5:50 PM on February 24, 2021: member

ACK c2758983f21415adde73f6948009ff3ef7f9bdb2

Small nits, maybe the extra explanation is not needed. Feel free to ignore.

jonatack commented at 6:09 PM on February 24, 2021: contributor

Thanks for having a look, @jarolrod! Much appreciated.

The docs echo the ones I added in the -netinfo help and are a compact version of those I added to src/net.h:

Time since last novel transaction received from the peer and accepted into our mempool, in minutes
Time since last novel block passing initial validity checks received from the peer, in minutes

    /** UNIX epoch time of the last block received from this peer that we had
     * not yet seen (e.g. not already received from another peer), that passed
     * preliminary validity checks and was saved to disk, even if we don't
     * connect the block or it eventually fails connection. Used as an inbound
     * peer eviction criterium in CConnman::AttemptToEvictConnection. */

    /** UNIX epoch time of the last transaction received from this peer that we
     * had not yet seen (e.g. not already received from another peer) and that
     * was accepted into our mempool. Used as an inbound peer eviction criterium
     * in CConnman::AttemptToEvictConnection. */

which I think are a bit more useful to users than the equivalent ones I added in the getpeerinfo rpc help (I've been adding these docs everywhere, hehe). For example, if we receive a novel block that does not pass initial validity checks, or a novel transaction that isn't accepted into our mempool, it won't appear--and the simplified docs would be incorrect. A user might wonder why, but the answers are difficult for casual users to glean from reading the code, and while writing the docs it took some review discussion with others to get this info cleared up. So it seems worth documenting when we have the space to do so.

jarolrod commented at 6:12 PM on February 24, 2021: member

@jonatack thanks for the context, looks good to me then 👍

hebasto commented at 9:53 PM on February 24, 2021: member

Concept ACK. Will review in the morning :)

qt: add RPCConsole::TimeDurationField helper, call systime only once 4dc2fd6c37

gui: add "Last Tx" (CNodeStats::nLastTXTime) to peer details a21be7c401

in src/qt/rpcconsole.cpp:1124 in ca928905d0 outdated

1119 | @@ -1120,11 +1120,12 @@ void RPCConsole::updateDetailWidget()
1120 |      if (stats->nodeStats.m_bip152_highbandwidth_from) bip152_hb_settings += (bip152_hb_settings == "" ? "From" : "/From");
1121 |      if (bip152_hb_settings == "") bip152_hb_settings = "No";
1122 |      ui->peerHighBandwidth->setText(bip152_hb_settings);
1123 | -    ui->peerLastSend->setText(stats->nodeStats.nLastSend ? GUIUtil::formatDurationStr(GetSystemTimeInSeconds() - stats->nodeStats.nLastSend) : tr("never"));
1124 | -    ui->peerLastRecv->setText(stats->nodeStats.nLastRecv ? GUIUtil::formatDurationStr(GetSystemTimeInSeconds() - stats->nodeStats.nLastRecv) : tr("never"));
1125 | +    const int64_t time_now{GetSystemTimeInSeconds()};
1126 | +    ui->peerConnTime->setText(TimeDurationField(stats->nodeStats.nTimeConnected, time_now));

hebasto commented at 10:19 AM on February 25, 2021:

ca928905d0d2cf991c0ea1532afb0bfc313a1899 This refactoring changes the logic of the peerConnTime value setting, as now "Never" is a possible option. Does it make sense? Is it guaranteed to be safe?

jonatack commented at 10:38 AM on February 25, 2021:

Good point. I was seeing output of 0s and not "Never" but no reason to change the logic. Changed.

jonatack force-pushed on Feb 25, 2021

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

Updated per git diff c275898 a5b349 to not use the helper for peerConnTime (and inverted the order of the helper arguments).

hebasto approved

hebasto commented at 11:22 AM on February 25, 2021: member

ACK a5b349af5878c8127e89abb21d96863a8debbc40, tested on Linux Mint 20.1 (Qt 5.12.8):

Screenshot from 2021-02-25 13-15-21

Screenshot from 2021-02-25 13-14-58

piconit: while here why not to fix indentation from #179:

--- a/src/qt/forms/debugwindow.ui
+++ b/src/qt/forms/debugwindow.ui
@@ -1079,7 +1079,7 @@
                <item row="1" column="0">
                 <widget class="QLabel" name="peerConnectionTypeLabel">
                  <property name="toolTip">
-                   <string>The direction and type of peer connection: %1</string>
+                  <string>The direction and type of peer connection: %1</string>
                  </property>
                  <property name="text">
                   <string>Direction/Type</string>

gui: add "Last Block" (CNodeStats::nLastBlockTime) to peer details 70d3c5d0b9

jonatack force-pushed on Feb 25, 2021

jonatack commented at 11:26 AM on February 25, 2021: contributor

Thanks @hebasto, done.

hebasto approved

hebasto commented at 11:27 AM on February 25, 2021: member

re-ACK 70d3c5d0b987411873d557440607aa4f3f65b1ec

Thank you @jonatack !

jarolrod commented at 1:47 PM on February 25, 2021: member

re-ack 70d3c5d0b987411873d557440607aa4f3f65b1ec

MarcoFalke merged this on Feb 25, 2021

MarcoFalke closed this on Feb 25, 2021

jonatack deleted the branch on Feb 25, 2021

sidhujag referenced this in commit e47253ab53 on Feb 25, 2021

Fabcien referenced this in commit 85e5bdc9a1 on Feb 3, 2022

gwillen referenced this in commit 59c8718a07 on Jun 28, 2022

bitcoin-core locked this on Aug 16, 2022