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.

Sign up for GitHub

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

Jump to bottom

Add BIP324-specific labels to peer details #754

Merged
merged 2 commits into from Oct 5, 2023
Merged

Add BIP324-specific labels to peer details #754

merged 2 commits into from Oct 5, 2023

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Sep 11, 2023 •
edited

This PR adds BIP324-specific labels to the peer details widget:

  • a transport version
  • a session id

See: bitcoin/bitcoin#28331 (comment).

image

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 11, 2023 •
edited

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.

@hebasto hebasto mentioned this pull request Sep 11, 2023
Merged
@sipa
Copy link
Contributor

sipa commented Sep 11, 2023 •
edited

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

hebasto commented Sep 11, 2023

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

A screenshot has been added to the PR description.

This was referenced Sep 11, 2023
Merged
Closed
Merged
Merged
Merged
Merged
Open
Closed
Closed
Closed
Merged
Open
Closed
Closed
Closed
Closed
Merged
jarolrod
jarolrod reviewed Sep 12, 2023
View reviewed changes
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.

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Sep 12, 2023
Merged
@Sjors
Copy link
Member

Sjors commented Sep 12, 2023

No strong feelings about the lock :-) But can you add the session id?

@DrahtBot DrahtBot mentioned this pull request Sep 12, 2023
Open
@jonatack
Copy link
Contributor

Concept ACK.

Am updating -netinfo as well (adding columns to the peer details, and adding P2P_V2 to the upcoming peer services column).

@Sjors
Copy link
Member

Sjors commented Sep 23, 2023

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

theStack
theStack reviewed Sep 23, 2023
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment •
edited

Choose a reason for hiding this comment

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

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

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.

src/qt/rpcconsole.cpp Outdated Show resolved Hide resolved
@hebasto
Copy link
Member Author

hebasto commented Sep 24, 2023

Updated 43268b0 -> f73f041 (pr754.03 -> pr754.04, diff):

@hebasto
Copy link
Member Author

hebasto commented Sep 24, 2023

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.

@hebasto hebasto marked this pull request as ready for review October 3, 2023 09:20
@hebasto
Copy link
Member Author

hebasto commented Oct 3, 2023 •
edited

Rebased on top of just merged bitcoin/bitcoin#28331 and undrafted.

cc @Sjors @theStack @jarolrod @sipa

@hebasto hebasto added this to the 26.0 milestone Oct 3, 2023
@Sjors
Copy link
Member

Sjors commented Oct 3, 2023

ACK 805273d

Tested on macOS Ventura 13.6 and lightly reviewed the code.

@DrahtBot DrahtBot removed the CI failed label Oct 3, 2023
theStack
theStack approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Tested ACK 805273d

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

MarnixCroes
MarnixCroes reviewed Oct 4, 2023
View reviewed changes
Copy link
Contributor

@MarnixCroes MarnixCroes left a comment •
edited

Choose a reason for hiding this comment

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

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

@hebasto
Copy link
Member Author

hebasto commented Oct 4, 2023

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

achow101 commented Oct 4, 2023

ACK d9c4e34

theStack
theStack approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Tested re-ACK d9c4e34

@RandyMcMillan
Copy link
Contributor

ACK d9c4e34

MarnixCroes
MarnixCroes approved these changes Oct 5, 2023
View reviewed changes
Copy link
Contributor

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

tACK d9c4e34
nice!

@hebasto hebasto merged commit 0e3de3b into bitcoin-core:master Oct 5, 2023
16 checks passed
@hebasto hebasto deleted the 230911-bip324-peer-details branch October 5, 2023 07:39
jarolrod
jarolrod reviewed Oct 6, 2023
View reviewed changes
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.

post merge ack d9c4e34

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023
@hebasto
Merge bitcoin-core/gui#754: Add BIP324-specific labels to peer details
ba1e2a1
@MarnixCroes MarnixCroes mentioned this pull request Jan 17, 2024
Merged
@124maa 124maa mentioned this pull request Feb 13, 2024
Closed
hebasto added a commit to bitcoin/bitcoin that referenced this pull request May 11, 2024
@hebasto
Merge bitcoin-core/gui#788: debugwindow: update session ID tooltip
182983c 
3bf00e1 gui: debugwindow: update session ID tooltip (Marnix)

Pull request description:

  When you have a v2 connection, there is always a session ID.

  the _if any_ is a leftover from bitcoin-core/gui#754, where the session ID property initially would always be displayed (transport v1 and v2).
  So the session ID could be empty when you have a v1 connection.

  As now the _Session ID_ property only is displayed for v2 connection, there will always be a session ID.

  master

  ![sessionIDifany](https://github.com/bitcoin-core/gui/assets/93143998/d4d7df43-8281-4b1e-83fc-5a3788d7724e)

  PR

  ![sessionID](https://github.com/bitcoin-core/gui/assets/93143998/221f6831-7d12-4913-be76-325a87baad2e)

  Session ID not shown when transport v1

  ![transportv1](https://github.com/bitcoin-core/gui/assets/93143998/6c067a08-4be4-4ce1-b514-80654ca5cd43)

  <!--
  *** Please remove the following help text before submitting: ***

  Pull requests without a rationale and clear improvement may be closed
  immediately.

  GUI-related pull requests should be opened against
  https://github.com/bitcoin-core/gui
  first. See CONTRIBUTING.md
  -->

  <!--
  Please provide clear motivation for your patch and explain how it improves
  Bitcoin Core user experience or Bitcoin Core developer experience
  significantly:

  * Any test improvements or new tests that improve coverage are always welcome.
  * All other changes should have accompanying unit tests (see `src/test/`) or
    functional tests (see `test/`). Contributors should note which tests cover
    modified code. If no tests exist for a region of modified code, new tests
    should accompany the change.
  * Bug fixes are most welcome when they come with steps to reproduce or an
    explanation of the potential issue as well as reasoning for the way the bug
    was fixed.
  * Features are welcome, but might be rejected due to design or scope issues.
    If a feature is based on a lot of dependencies, contributors should first
    consider building the system outside of Bitcoin Core, if possible.
  * Refactoring changes are only accepted if they are required for a feature or
    bug fix or otherwise improve developer experience significantly. For example,
    most "code style" refactoring changes require a thorough explanation why they
    are useful, what downsides they have and why they *significantly* improve
    developer experience or avoid serious programming bugs. Note that code style
    is often a subjective matter. Unless they are explicitly mentioned to be
    preferred in the [developer notes](/doc/developer-notes.md), stylistic code
    changes are usually rejected.
  -->

  <!--
  Bitcoin Core has a thorough review process and even the most trivial change
  needs to pass a lot of eyes and requires non-zero or even substantial time
  effort to review. There is a huge lack of active reviewers on the project, so
  patches often sit for a long time.
  -->

ACKs for top commit:
  vostrnad:
    ACK 3bf00e1
  kristapsk:
    ACK 3bf00e1
  jarolrod:
    ACK 3bf00e1
  pablomartin4btc:
    tACK 3bf00e1
  hebasto:
    ACK 3bf00e1.

Tree-SHA512: 4de0c56c070dc5d1cee73b629bdf3d1778c6d90d512337aa6cfd3eed4ce95cbcfbe5713e2942f6fc22907b2c4d9df7979ba8e9f91f7cc173b42699ea35113f96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jarolrod jarolrod jarolrod left review comments

@theStack theStack theStack approved these changes

@MarnixCroes MarnixCroes MarnixCroes approved these changes

@Sjors Sjors Awaiting requested review from Sjors

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
26.0
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@hebasto @DrahtBot @sipa @Sjors @jonatack @achow101 @RandyMcMillan @theStack @jarolrod @MarnixCroes