Skip to content

gui: display mapped AS in peers info window #18402

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
Merged
merged 2 commits into from
Mar 25, 2020
Merged

gui: display mapped AS in peers info window #18402

merged 2 commits into from
Mar 25, 2020

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Mar 22, 2020 •
edited
Loading

Continuing the asmap integration of #16702 which added mapped_as to the rpc getpeerinfo output, this adds the mapped AS to the Peers detail window in the GUI wallet.

$ src/qt/bitcoin-qt -asmap=<path-to-asmap-file> (asmap on)

Screenshot from 2020-03-22 12-29-56

$ src/qt/bitcoin-qt (asmap off)

Screenshot from 2020-03-22 12-32-46

Added a tooltip and a couple of minor fixups.

@fanquake fanquake added the GUI label Mar 22, 2020
@hebasto
Copy link
Member

hebasto commented Mar 22, 2020

Concept ACK. Smth similar was in my task list ;)

@laanwj
Copy link
Member

laanwj commented Mar 22, 2020 •
edited
Loading

A good addition!
ACK 8580db1
(but see nits)

laanwj
laanwj reviewed Mar 22, 2020
View reviewed changes
laanwj
laanwj reviewed Mar 22, 2020
View reviewed changes
promag
promag reviewed Mar 22, 2020
View reviewed changes
Copy link
Contributor

@promag promag 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, honestly you could squash these commits.

@jonatack jonatack force-pushed the gui-peers-display-mapped_as branch from 8580db1 to 64f3838 Compare March 23, 2020 13:34
@jonatack
Copy link
Member Author

Thanks @hebasto, @laanwj, and @promag for reviewing. Feedback taken; done.

hebasto
hebasto approved these changes Mar 23, 2020
View reviewed changes
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 64f3838, tested on Linux Mint 19.3:

DeepinScreenshot_bitcoin-qt_20200323191834

@jonatack
While debugwindow.ui is touched mind adding a commit to fix annoying re-formatting by Qt Designer?

@laanwj #15220 (comment):

please just do this the next time you're editing these files.

promag
promag reviewed Mar 23, 2020
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK modulo suggestion.

jonatack and others added 2 commits March 24, 2020 14:04
@jonatack
gui: display Mapped AS in peers info window
aae2605
@jonatack @hebasto
gui: avoid QT Designer/Form Editor re-formatting
76db4b2 
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
@jonatack jonatack force-pushed the gui-peers-display-mapped_as branch from 64f3838 to 76db4b2 Compare March 24, 2020 13:06
@jonatack
Copy link
Member Author

Thanks @promag and @hebasto, done.

@laanwj
Copy link
Member

laanwj commented Mar 25, 2020 •
edited
Loading

ACK 76db4b2

@laanwj laanwj merged commit 244e88e into bitcoin:master Mar 25, 2020
@jonatack jonatack deleted the gui-peers-display-mapped_as branch March 25, 2020 14:38
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
@hebasto hebasto mentioned this pull request Apr 11, 2020
Closed
@andronoob
Copy link

I wonder if the string "Mapped AS" displayed in the GUI could be worded in other ways? Like "BGP AS number", so that it could be more self-explanatory, or at least more google-able?

@jonatack
Copy link
Member Author

@andronoob there is an explanatory tooltip -- "The mapped Autonomous System used for diversifying peer selection." -- see https://github.com/bitcoin/bitcoin/pull/18402/files#diff-2537e9bcbac21d35d8fa28ce1a35e89dR1508. In RPC getpeerinfo it is also named mapped_as. I think your translation is also inaccurate. See https://bitcoincore.reviews/16702 for more.

@andronoob
Copy link

andronoob commented Aug 14, 2020 •
edited
Loading

The mapped Autonomous System used for diversifying peer selection.

To my understanding, such phrase means:

"(Such number is) The Autonomous System (AS) (number, aka ASN) which this peer is mapped to (according to the map data). BGP AS is considered during peer selection, in order to diversify connected peers. (thus some risks related might be mitigated)"

Correct me if I'm wrong?

jonasschnelli
jonasschnelli reviewed Aug 14, 2020
View reviewed changes
src/qt/forms/debugwindow.ui
@@ -652,12 +652,12 @@
</item>
<item>
<widget class="QLineEdit" name="lineEdit">
<property name="placeholderText">
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the property order change has been done through QTCreator? I would have probably unstaged those.

jonasschnelli
jonasschnelli reviewed Aug 14, 2020
View reviewed changes
Copy link
Contributor

@jonasschnelli jonasschnelli left a comment

Choose a reason for hiding this comment

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

Core Review ACK 76db4b2

@jonatack jonatack mentioned this pull request Dec 25, 2020
Merged
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 12, 2021
@jonatack @PiRK
gui: display Mapped AS in peers info window
afdf7c7 
Summary:
> Continuing the asmap integration of [[bitcoin/bitcoin#16702 | PR16702]] which added mapped_as to the rpc getpeerinfo output, this adds the mapped AS to the Peers detail window in the GUI wallet.
> Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

This is a backport of Core [[bitcoin/bitcoin#18402 | PR18402]]

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

Menu `Window -> Peers`, select a peer, the "Mapped AS" field is now visible at the bottom of the right widget.

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D8876
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

@jonasschnelli jonasschnelli jonasschnelli left review comments

@promag promag promag left review comments

@hebasto hebasto hebasto approved these changes

Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jonatack @hebasto @laanwj @andronoob @jonasschnelli @promag @fanquake