Qt: Add GUI view of peer information. #4133

ashleyholman commented at 5:47 pm on May 23, 2014: contributor

Here is my code which adds a “Peers” tab to the debug window.

The peers table lists just a few columns of detail, and then when you click on a peer it shows all of the detail. It updates every second while the debug window is open (by doing a full scan of vNodes), and also handles the case where the selected peer gets disconnected.

If you have any suggestions on which columns should appear in the table, or how to format some of the values, I’m happy to implement them. I just chose to show Address, Subversion, and Height in the table for now. The advantage of including fields in the table is that they are sortable, so I guess the table should contain those properties which users are most likely to want to sort by.

There’s a fair bit of code here, so take your time with the review.

Thanks.

ashleyholman commented at 6:29 pm on May 23, 2014: contributor

ashleyholman commented at 6:31 pm on May 23, 2014: contributor

Relates to issue #4133

wtogami commented at 5:38 am on May 24, 2014: contributor

Would it be complicated to add a ‘Disconnect’ and ‘Ban’ option? Either buttons or right-click menu?

ashleyholman commented at 6:07 am on May 24, 2014: contributor

@wtogami I think I’d be able to do that. I imagine that such a function should be also added as an RPC call to keep the CLI and GUI capabilities the same?

wtogami commented at 6:24 am on May 24, 2014: contributor

RPC too sounds like a great idea. Would be helpful in debugging certain things.

bannode, unbannode, disconnectnode?

The parameter could be a domain name but in the case of banning it’s resolved and only the IP address is stored.

For Tor nodes I’m not sure how to handle it. You can’t ban but disconnect would be possible.

in src/qt/clientmodel.cpp: in 9a1da58eda outdated

3@@ -4,6 +4,7 @@
4 
5 #include "clientmodel.h"
6 
7+#include "peertablemodel.h"

Diapolo commented at 8:32 am on May 24, 2014:

Nit: Can you keep alphabetical ordering (where possible) of includes, classes etc. throughout your pull please :). Talking about just the file headers (header of .h and .cpp)…

laanwj commented at 9:28 am on May 24, 2014:

Is the part about alphabetical ordering mentioned anywhere in the coding style guides? If not, it should be, as otherwise it’s not realistic to expect people to do it.

Diapolo commented at 9:30 am on May 24, 2014:

Most likely it’s not, but I remeber a big cleanup pull by someone and we liked it :).

ashleyholman commented at 10:01 am on May 24, 2014:

This should hopefully be fixed in my new commit. Note that I have followed the format of some of the other .cpp files where they include their own .h first on a separate line, then group the others alphabetically.

in src/qt/guiutil.cpp: in 9a1da58eda outdated

749@@ -750,4 +750,24 @@ QString boostPathToQString(const boost::filesystem::path &path)
750 }
751 #endif
752 
753+QString formatDurationStr(int secs)

Diapolo commented at 8:35 am on May 24, 2014:

Isn’t there such a function somewhere already or could we use this in other places? Seems like a common task.

ashleyholman commented at 10:04 am on May 24, 2014:

RPCConsole::setTrafficGraphRange() had similar code which I replaced with this new util function. There is similar code in BitcoinGUI::setNumBlocks() but it formats the string differently and uses less precision.

in src/qt/guiutil.cpp: in 9a1da58eda outdated

757+    int hours = (secs % 86400) / 3600;
758+    int mins = (secs % 3600) / 60;
759+    int seconds = secs % 60;
760+
761+    if (days)
762+        str.append(QString(QObject::tr("%1 d ")).arg(days));

Diapolo commented at 8:37 am on May 24, 2014:

Can you remove the space after the unit, this often causes trouble during translation, if there is a space at the end.

in src/qt/rpcconsole.cpp: in 9a1da58eda outdated

 5@@ -6,7 +6,10 @@
 6 #include "ui_rpcconsole.h"
 7 
 8 #include "clientmodel.h"
 9+#include "peertablemodel.h"
10 #include "guiutil.h"
11+#include "main.h"

Diapolo commented at 8:51 am on May 24, 2014:

Nit: Add a newline after guiutil.h as main and util are core headers.

in src/qt/rpcconsole.cpp: in 9a1da58eda outdated

612+    ui->peerLastSend->setText(stats->nLastSend ?  GUIUtil::formatDurationStr(GetTime() - stats->nLastSend) : tr("never"));
613+    ui->peerLastRecv->setText(stats->nLastRecv ?  GUIUtil::formatDurationStr(GetTime() - stats->nLastRecv) : tr("never"));
614+    ui->peerBytesSent->setText(FormatBytes(stats->nSendBytes));
615+    ui->peerBytesRecv->setText(FormatBytes(stats->nRecvBytes));
616+    ui->peerConnTime->setText(GUIUtil::formatDurationStr(GetTime() - stats->nTimeConnected));
617+    ui->peerPingTime->setText(stats->dPingTime == 0 ? tr("N/A") : QString(tr("%1 secs")).arg(stats->dPingTime));

Diapolo commented at 8:53 am on May 24, 2014:

Ping time is not seconds IMHO, or?

ashleyholman commented at 10:11 am on May 24, 2014:

I wasn’t able to test this because I’m yet to come across a peer that has a non-zero dPingTime. I’m not sure why this is, as I thought most nodes should support that by now. It gets calculated like this: (((double)nPingUsecTime) / 1e6), so I interpreted that to mean its the number of seconds as a floating point.

sipa commented at 10:19 am on May 24, 2014:

dPingTime is in seconds, indeed.

in src/qt/rpcconsole.cpp: in 9a1da58eda outdated

604+    detailNodeStats.nLastSend = stats->nLastSend;
605+    detailNodeStats.nLastRecv = stats->nLastRecv;
606+    detailNodeStats.nTimeConnected = stats->nTimeConnected;
607+
608+    // update the detail ui with latest node information
609+    ui->peerHeading->setText(QString(tr("<b>Node Detail</b>")));

Diapolo commented at 8:53 am on May 24, 2014:

Nit: Can you exclude HTML tags from translations please.

Diapolo commented at 8:55 am on May 24, 2014: none

Pretty cool addition :), nice job!

laanwj commented at 9:27 am on May 24, 2014: member

Looks very good!

I think adding Disconnect/Ban functionality would be useful, but let’s not do it in this pull request. That functionality would indeed also have to be available on RPC (and the core) first, and that’s orthogonal to this nice GUI improvement. Let’s aim to merge this first.

ashleyholman commented at 9:36 am on May 24, 2014: contributor

What’s the process on pushing additional patches in response to feedback? If I push an amended commit, it’s hard for you to see what changed. But if I commit new patches on top, the history will be a bit ugly when merged.

I guess I could make new commits on top for now, and then squash it later once it’s ready for merge?

Diapolo commented at 9:38 am on May 24, 2014: none

@ashleyholman For such a “big” patch it’s nice to add commits until you got core dev ACKs and can then squash into one.

sipa commented at 10:16 am on May 24, 2014: member

Pings aren’t sent automatically (yet), so the pingtime will be zero unless you explicitly request pings (using the ping RPC command).

If you run together with #2784, you’ll pretty much always see ping times.

By the way, I think ping times (in that case) are more useful to show on the main screen table than startingheight (as that can be very outdated information).

ashleyholman commented at 1:16 pm on May 24, 2014: contributor

Here is what it’s like with #2784 merged and the Height column replaced with Ping.

guipeersping

I’ve pushed this to my gui_peers_ping branch, but won’t include it in this pull unless #2784 is getting merged.

laanwj commented at 4:23 pm on May 25, 2014: member

I did a bit of testing with this applied, works well for me; a few nits:

‘Ban Score’ shows as ‘Unavailable’ for many peers. This seems confusing. It may be better to just report ‘0’ here if the data structure for this isn’t created yet.
When enlarging the debug window vertically, the line distance between the information entries increases. This looks a bit weird, but I suppose it’s a matter of taste. I usually prefer adding a stretch at the end to avoid this.
Services: Maybe show the flags expanded as text here, so [NODE_]NETWORK instead of ‘1’. Although this leaves the question of how to show unknown service bits, maybe UNKNOWN[bit].
I think translators will have problems translating the word ‘Subversion’. Maybe ‘Sub-version’? Or just ‘Version’?

sipa commented at 4:29 pm on May 25, 2014: member

BIP14 repurposed strSubVer as “user agent”.

ashleyholman commented at 0:52 am on May 26, 2014: contributor

The ban score being “Unavailable” is because there’s an extra lock required to get that info. The rest of the stats require cs_vNodes, but getting the nMisbehaviour requires cs_main. When TRY_LOCK on cs_main fails, I show “Unavailable”. I could show it as 0, but then when it eventually succeeds on the lock, the value will change. If you click on another peer and then click back on the same peer, it would say 0 again until it can refetch the nMisbehaviour, because I’m only caching nMisbehaviour for the currently selected node.

As for how to make this meaningful to the user, perhaps I could just leave it blank if it can’t be accessed. Or, it could say “fetching…” or something like that until it gets the value?

leofidus commented at 1:08 am on May 26, 2014: none

displaying “fetching…” and periodically trying to get the information sounds like a good solution.

ashleyholman commented at 2:05 am on May 26, 2014: contributor

Pushed an update:

Change “Unavailable” to “Fetching…” when nMisbehaviour can’t be read. If you wait some seconds, you should eventually see the ban score. It’s worse when the lock is busy, such as when synchronising your chain.
Added a stretch element to fix up the vertical resize behaviour
The “Services” field is now formatted verbosely, eg. “NETWORK & UNKNOWN[2] & …” (only scans the last 8 bits for now, as its probably overkill to check all 64 when they’re not standardised yet).
Renamed “Subversion” to “User Agent”

laanwj commented at 7:07 am on May 26, 2014: member

@ashleyholman seems like good choices to me

in src/qt/rpcconsole.cpp: in 1935991193 outdated

620+    ui->peerSubversion->setText(QString(stats->cleanSubVer.c_str()));
621+    ui->peerDirection->setText(stats->fInbound ? tr("Inbound") : tr("Outbound"));
622+    ui->peerHeight->setText(QString("%1").arg(stats->nStartingHeight));
623+    ui->peerSyncNode->setText(stats->fSyncNode ? tr("Yes") : tr("No"));
624+
625+    // if we can, display the peer's ban score

laanwj commented at 7:51 am on May 28, 2014:

Is there a specific reason to collect the ban score here, instead of in PeerTableModel’s refreshPeers()? To me it seems like this would be the responsibility of the model, not the view.

ashleyholman commented at 11:03 am on May 29, 2014:

Currently the state stats, which contains the ban score, is only being retrieved for the one peer thats selected (if one is selected). The model is not aware of which one is selected, so refreshPeers() would have to get the CStateStats for every peer and store them as it does with the CNodeStats records, which I will happily do if that’s cleaner. The return type of PeerTableModel::getNodeStats() would need to change in that case, so that it returns both the CNodeStats and CNodeStateStats. How would I best define a type for that?

Diapolo commented at 11:06 am on May 29, 2014:

You could pass two references instead of using the one return type?

laanwj commented at 1:10 pm on May 30, 2014:

I think retrieving all the data is refreshPeers is cleanest. You could define a data structure for the model that contains both a CNodeStats and a CNodeStateStats or use a std::pair if lazy :)

Unless you think this has a large impact on performance, but I don’t think so, it is only a little bit extra data that gets copied.

Alternatively: add a function getNodeStateStats(id) to the PeerTableModel that retrieves this data and returns a CNodeStateStats, then just move the code and call it from here. All the code to access core data structures happens from the model then, and that’s good enough for me.

ashleyholman commented at 5:58 am on May 31, 2014:

@laanwj: my latest patch moves it into refreshPeers()

in src/qt/rpcconsole.cpp: in cf41964793 outdated

625+
626+    // if we can, display the peer's ban score
627+    CNodeStateStats statestats = combinedStats->statestats;
628+    if (statestats.nMisbehavior >= 0)
629+    {
630+        LogPrintf("UPDATING CACHE COS WE GOT A NEW BAN SCORE\n");

laanwj commented at 8:05 am on June 1, 2014:

This must be a debugging leftover? :)

ashleyholman commented at 8:11 am on June 1, 2014:

Haha, oops. Pushed a new commit to remove that. I will squash all these patches into one commit and sign it once it’s ready for merge.

laanwj commented at 2:20 pm on June 2, 2014: member

ACK. will merge after squash

ashleyholman commented at 3:17 pm on June 2, 2014: contributor

@laanwj There you go. I avoided rebasing so that my squash can be easily diff’d against the last reviewed commit. It merges cleanly to master though.

In regards to signing, should I be doing that, or is that only needed for mainline commits (ie. your merge commit)? I don’t have a PGP identity on any key servers at the moment, but it’s something I’ve been wanting to get set up with.

laanwj commented at 4:05 pm on June 2, 2014: member

@ashleyholman Thanks

Re: signing, I always sign the merge commits, but I think it’s useful if contributors sign their uppermost commit as well, so that people can check that it’s really the contributor’s code that was merged and not something manipulated by a compromised github (for example). Once you have a key set up, you can sign your last commit with:

0git commit --amend --gpg-sign[=<keyid>]

Then just push to github as normal.

Azulan commented at 4:20 pm on June 2, 2014: none

Tested with ipv6?

laanwj commented at 8:07 pm on June 2, 2014: member

@Azulan Yes, if you could test with ipv6 that’d be helpful

Qt: Add GUI view of peer information. #4133 65f78a111f

BitcoinPullTester commented at 8:44 am on June 3, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/65f78a111ff52c2212cc0a423662e7a41d1206dd for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj merged this on Jun 3, 2014

laanwj closed this on Jun 3, 2014

laanwj referenced this in commit 4c097f9669 on Jun 3, 2014

in src/qt/peertablemodel.h: in 65f78a111f

61+    QVariant data(const QModelIndex &index, int role) const;
62+    QVariant headerData(int section, Qt::Orientation orientation, int role) const;
63+    QModelIndex index(int row, int column, const QModelIndex &parent) const;
64+    Qt::ItemFlags flags(const QModelIndex &index) const;
65+    void sort(int column, Qt::SortOrder order);
66+    /*@}*/

Diapolo commented at 11:25 am on June 3, 2014:

What are these?

ashleyholman commented at 11:52 am on June 3, 2014:

@Diapolo Which lines specifically? Github is highlighting a lot there so I can’t tell which parts you mean.

Diapolo commented at 11:55 am on June 3, 2014:

Exactly that line /*@}*/ and at the beginning of the comment :).

laanwj commented at 12:04 pm on June 3, 2014:

@diapolo A doxygen grouping comment Edit: see http://www.stack.nl/~dimitri/doxygen/manual/grouping.html

ashleyholman commented at 12:04 pm on June 3, 2014:

Oh, I’m not sure what the deal is with those, but they are in the other model code that I based this on.

leofidus commented at 3:13 pm on June 3, 2014:

the @name puts a descriptive name to all the code enclosed between @{ and @} in the doxygen documentation. But I think it has to be /**@}*/ to be correctly recognized.

laanwj commented at 3:44 pm on June 3, 2014:

@leofidus It appears to work see https://dev.visucore.com/bitcoin/doxygen/class_peer_table_model.html, under Public Member Functions.

DrahtBot locked this on Sep 8, 2021

Qt: Add GUI view of peer information. #4133 #4225