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.

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

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

Conversation

jonatack
Copy link
Member

  • 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
Copy link
Member Author

jonatack commented Feb 24, 2021

Screenshots 🍰

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

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK c275898

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

@jonatack
Copy link
Member Author

jonatack commented Feb 24, 2021

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
Copy link
Member

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

@hebasto
Copy link
Member

hebasto commented Feb 24, 2021

Concept ACK. Will review in the morning :)

@jonatack jonatack force-pushed the add-last-block-and-last-transaction-to-peer-details branch from c275898 to a5b349a Compare February 25, 2021 10:36
@jonatack
Copy link
Member Author

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

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK a5b349a, 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>

@jonatack jonatack force-pushed the add-last-block-and-last-transaction-to-peer-details branch from a5b349a to 70d3c5d Compare February 25, 2021 11:25
@jonatack
Copy link
Member Author

Thanks @hebasto, done.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

re-ACK 70d3c5d

Thank you @jonatack !

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

re-ack 70d3c5d

@maflcko maflcko merged commit e491174 into bitcoin-core:master Feb 25, 2021
@jonatack jonatack deleted the add-last-block-and-last-transaction-to-peer-details branch February 25, 2021 14:18
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 25, 2021
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 3, 2022
Summary:
> qt: add RPCConsole::TimeDurationField helper, call systime only once

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

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

This is a backport of [[bitcoin-core/gui#226 | core-gui#226]]
Depends on D10963

Test Plan:
`ninja && src/qt/bitcoin-qt`

Select a peer in the "peers" view, check the new "Last Block/Tx" fields. Wait for a while to see it change from "Never" to a duration.

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D10964
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 28, 2022
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants