Add BIP324-specific labels to peer details #754

pull hebasto wants to merge 2 commits into bitcoin-core:master from hebasto:230911-bip324-peer-details changing 2 files +121 −50

hebasto commented at 11:16 AM on September 11, 2023: member
This PR adds BIP324-specific labels to the peer details widget:
- a transport version
- a session id
See: https://github.com/bitcoin/bitcoin/pull/28331#issuecomment-1693239025.
DrahtBot commented at 11:16 AM on September 11, 2023: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK achow101, theStack, RandyMcMillan, MarnixCroes

Concept ACK jarolrod, jonatack

Stale ACK Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.
DrahtBot added the label Needs rebase on Sep 11, 2023
hebasto force-pushed on Sep 11, 2023
sipa commented at 12:09 PM on September 11, 2023: contributor

I don't think we should add a lock icon or something like that; there are significant benefits of having encrypted connections on a large scale, but users in general shouldn't assume that their specific connections are more secure for their specific purposes when they're v2.
hebasto commented at 12:22 PM on September 11, 2023: member

I don't think we should add a lock icon or something like that...

A screenshot has been added to the PR description.
DrahtBot removed the label Needs rebase on Sep 11, 2023
jarolrod commented at 5:05 AM on September 12, 2023: member

Concept ACK
Sjors commented at 7:25 PM on September 12, 2023: member

No strong feelings about the lock :-) But can you add the session id?
jonatack commented at 10:30 PM on September 13, 2023: member

Concept ACK.

Am updating -netinfo as well (adding columns to the peer details, and adding P2P_V2 to the upcoming peer services column).
DrahtBot added the label Needs rebase on Sep 14, 2023
theStack commented at 5:45 PM on September 14, 2023: contributor

Concept ACK

Will review this PR right after https://github.com/bitcoin/bitcoin/pull/28331.
hebasto force-pushed on Sep 22, 2023
hebasto renamed this:
~~Add BIP324 "Transport" label to peer details~~
Add BIP324-specific labels to peer details
on Sep 22, 2023
DrahtBot removed the label Needs rebase on Sep 22, 2023
hebasto commented at 2:14 PM on September 22, 2023: member

But can you add the session id?

Done.
DrahtBot added the label CI failed on Sep 22, 2023
Sjors commented at 11:27 AM on September 23, 2023: member

tACK 43268b0a2eaab02b8a23cddf3b4c6eae9bb36afe (tested by cherry-picking the GUI commits)

Type	Reviewers
ACK	achow101, theStack, RandyMcMillan, MarnixCroes
Concept ACK	jarolrod, jonatack
Stale ACK	Sjors

in src/qt/rpcconsole.cpp:522 in d7c8e6fb56 outdated

 519 | -    ui->peerConnectionTypeLabel->setToolTip(ui->peerConnectionTypeLabel->toolTip().arg(list));
 520 | +    const QString connection_types_list{"<ul><li>" + Join(CONNECTION_TYPE_DOC, QString("</li><li>")) + "</li></ul>"};
 521 | +    ui->peerConnectionTypeLabel->setToolTip(ui->peerConnectionTypeLabel->toolTip().arg(connection_types_list));
 522 | +    const std::vector<QString> TRANSPORT_TYPE_DOC{
 523 | +        //: Explanatory text for "detecting" transport type.
 524 | +        tr("detecting: incoming connection; peer could be v1 or v2"),

theStack commented at 10:07 PM on September 23, 2023:

In the latest version of https://github.com/bitcoin/bitcoin/pull/28331 (since the change in https://github.com/bitcoin/bitcoin/pull/28331#issuecomment-1721167073 was applied), the "detecting" state can appear in both directions, i.e. the description should be adapted accordingly:

        tr("detecting: peer could be v1 or v2"),

hebasto commented at 4:30 PM on September 24, 2023:

Thanks! Updated.

theStack commented at 10:09 PM on September 23, 2023: contributor

Tested this PR with both in- and outbound connections, and it seems to work just fine :ok_hand:

It might be worthwhile to show the transport type also as extra column directly in the table rather than only in the details (useful for e.g. quickly identifying all v2 peers by sorting by type), but that's not urgent and can probably wait until the first release that has set -v2transport to default-on.
hebasto force-pushed on Sep 24, 2023
hebasto commented at 4:30 PM on September 24, 2023: member
Updated 43268b0a2eaab02b8a23cddf3b4c6eae9bb36afe -> f73f0411cf8a76e4272bd7e5c4583e9110d97d5a (pr754.03 -> pr754.04, diff):
- addressed @theStack's comment
hebasto commented at 4:33 PM on September 24, 2023: member

It might be worthwhile to show the transport type also as extra column directly in the table rather than only in the details (useful for e.g. quickly identifying all v2 peers by sorting by type), but that's not urgent and can probably wait until the first release that has set -v2transport to default-on.

Considering the total number of columns, I believe we should provide users with the ability to hide selected ones.
qt: Add "Transport" label to peer details f08adec886
hebasto force-pushed on Oct 3, 2023
hebasto marked this as ready for review on Oct 3, 2023
hebasto commented at 9:21 AM on October 3, 2023: member

Rebased on top of just merged https://github.com/bitcoin/bitcoin/pull/28331 and undrafted.

cc @Sjors @theStack @jarolrod @sipa
hebasto added this to the milestone 26.0 on Oct 3, 2023
Sjors commented at 9:35 AM on October 3, 2023: member

ACK 805273d043055d8c93fa7571fd4845a4b026f803

Tested on macOS Ventura 13.6 and lightly reviewed the code.
DrahtBot removed the label CI failed on Oct 3, 2023
theStack approved
theStack commented at 8:53 PM on October 3, 2023: contributor

Tested ACK 805273d043055d8c93fa7571fd4845a4b026f803

Ran this branch on OpenBSD 7.3 with Xfce Window Manager, using Qt version 5.15.8. Checked that the new BIP324 labels are correct with two v2 connections (1 inbound + 1 outbound). :ok:
MarnixCroes commented at 7:48 AM on October 4, 2023: contributor

maybe only display Session id property when Transport is v2?

nit, everywhere in the BIP they use Session ID (capital ID), maybe use the same
qt: Add "Session id" label to peer details d9c4e344d7
hebasto force-pushed on Oct 4, 2023
hebasto commented at 11:10 AM on October 4, 2023: member

maybe only display Session id property when Transport is v2?

nit, everywhere in the BIP they use Session ID (capital ID), maybe use the same

Thanks! Done.
achow101 commented at 8:36 PM on October 4, 2023: member

ACK d9c4e344d70bbf31ccb7162d83d4bd25762bc678
DrahtBot requested review from theStack on Oct 4, 2023
DrahtBot requested review from Sjors on Oct 4, 2023
theStack approved
theStack commented at 10:35 PM on October 4, 2023: contributor

Tested re-ACK d9c4e344d70bbf31ccb7162d83d4bd25762bc678
RandyMcMillan commented at 11:49 PM on October 4, 2023: contributor

ACK d9c4e34
MarnixCroes approved
MarnixCroes commented at 7:32 AM on October 5, 2023: contributor

tACK d9c4e344d70bbf31ccb7162d83d4bd25762bc678 nice!
hebasto merged this on Oct 5, 2023
hebasto closed this on Oct 5, 2023
hebasto deleted the branch on Oct 5, 2023
jarolrod commented at 2:15 AM on October 6, 2023: member

post merge ack d9c4e344d70bbf31ccb7162d83d4bd25762bc678
Frank-GER referenced this in commit ba1e2a1e3d on Oct 13, 2023
hebasto referenced this in commit 182983c6ab on May 11, 2024
bitcoin-core locked this on Oct 5, 2024