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

ashleyholman · 2014-05-23T17:47:46Z

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 · 2014-05-23T18:29:10Z

ashleyholman · 2014-05-23T18:31:56Z

Relates to issue #4133

wtogami · 2014-05-24T05:38:04Z

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

ashleyholman · 2014-05-24T06:07:09Z

@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 · 2014-05-24T06:24:48Z

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.

Diapolo · 2014-05-24T08:32:42Z

src/qt/clientmodel.cpp

@@ -4,6 +4,7 @@

 #include "clientmodel.h"

+#include "peertablemodel.h"


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)...

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.

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

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.

Diapolo · 2014-05-24T08:55:14Z

Pretty cool addition :), nice job!

laanwj · 2014-05-24T09:27:21Z

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 · 2014-05-24T09:36:49Z

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 · 2014-05-24T09:38:14Z

@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 · 2014-05-24T10:16:51Z

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 · 2014-05-24T13:16:25Z

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

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

laanwj · 2014-05-25T16:23:47Z

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 · 2014-05-25T16:29:18Z

BIP14 repurposed strSubVer as "user agent".

ashleyholman · 2014-05-26T00:52:53Z

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 · 2014-05-26T01:08:58Z

displaying "fetching..." and periodically trying to get the information sounds like a good solution.

ashleyholman · 2014-05-26T02:05:38Z

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 · 2014-05-26T07:07:21Z

@ashleyholman seems like good choices to me

laanwj · 2014-05-28T07:51:56Z

src/qt/rpcconsole.cpp

+    ui->peerHeight->setText(QString("%1").arg(stats->nStartingHeight));
+    ui->peerSyncNode->setText(stats->fSyncNode ? tr("Yes") : tr("No"));
+
+    // if we can, display the peer's ban score


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.

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?

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

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.

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

laanwj · 2014-06-02T14:20:43Z

ACK. will merge after squash

ashleyholman · 2014-06-02T15:17:03Z

@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 · 2014-06-02T16:05:53Z

@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:

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

Then just push to github as normal.

Azulan · 2014-06-02T16:20:53Z

Tested with ipv6?

laanwj · 2014-06-02T20:07:31Z

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

BitcoinPullTester · 2014-06-03T08:44:45Z

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.

65f78a1 Qt: Add GUI view of peer information. #4133 (Ashley Holman)

Diapolo · 2014-06-03T11:25:11Z

src/qt/peertablemodel.h

+    QModelIndex index(int row, int column, const QModelIndex &parent) const;
+    Qt::ItemFlags flags(const QModelIndex &index) const;
+    void sort(int column, Qt::SortOrder order);
+    /*@}*/


What are these?

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

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

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

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

$ git grep -F '/*@}*/' src/qt/addresstablemodel.h: /*@}*/ src/qt/peertablemodel.h: /*@}*/ src/qt/recentrequeststablemodel.h: /*@}*/

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.

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

Diapolo reviewed May 24, 2014
View reviewed changes

laanwj reviewed May 28, 2014
View reviewed changes

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

65f78a1

laanwj merged commit 65f78a1 into bitcoin:master Jun 3, 2014

laanwj added a commit that referenced this pull request Jun 3, 2014

Merge pull request #4225

4c097f9

65f78a1 Qt: Add GUI view of peer information. #4133 (Ashley Holman)

Diapolo reviewed Jun 3, 2014
View reviewed changes

This was referenced Aug 8, 2021

Peer address column sorting does not honor the inbound/outbound arrow prefix bitcoin-core/gui#397

Closed

Honor inbound/outbound arrow prefix when sorting peer address column bitcoin-core/gui#400

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

		@@ -4,6 +4,7 @@

		#include "clientmodel.h"

		#include "peertablemodel.h"

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

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

Uh oh!

Conversation

ashleyholman commented May 23, 2014

Uh oh!

ashleyholman commented May 23, 2014

Uh oh!

ashleyholman commented May 23, 2014

Uh oh!

wtogami commented May 24, 2014

Uh oh!

ashleyholman commented May 24, 2014

Uh oh!

wtogami commented May 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented May 24, 2014

Uh oh!

laanwj commented May 24, 2014

Uh oh!

ashleyholman commented May 24, 2014

Uh oh!

Diapolo commented May 24, 2014

Uh oh!

sipa commented May 24, 2014

Uh oh!

ashleyholman commented May 24, 2014

Uh oh!

laanwj commented May 25, 2014

Uh oh!

sipa commented May 25, 2014

Uh oh!

ashleyholman commented May 26, 2014

Uh oh!

leofidus commented May 26, 2014

Uh oh!

ashleyholman commented May 26, 2014

Uh oh!

laanwj commented May 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 2, 2014

Uh oh!

ashleyholman commented Jun 2, 2014

Uh oh!

laanwj commented Jun 2, 2014

Uh oh!

Azulan commented Jun 2, 2014

Uh oh!

laanwj commented Jun 2, 2014

Uh oh!

BitcoinPullTester commented Jun 3, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!