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

gui: display mapped AS in peers info window #18402

laanwj 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 8580db114ae30459af245f2bf0bfcc5f8c85b612
(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

jonatack commented Mar 23, 2020

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 64f383857d6f3412e432e737c298ff17f84c48d3, 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 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

jonatack commented Mar 24, 2020

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

andronoob commented Aug 13, 2020

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

jonatack commented Aug 13, 2020

@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
</item>
<item>
<widget class="QLineEdit" name="lineEdit">
<property name="placeholderText">
Copy link
Contributor

@jonasschnelli jonasschnelli Aug 14, 2020

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 <[email protected]>

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

@hebasto hebasto hebasto approved these changes

+2 more reviewers

@jonasschnelli jonasschnelli jonasschnelli left review comments

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

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