Add copy IP/Netmask action for banned peer by shaavan · Pull Request #384 · bitcoin-core/gui

shaavan · 2021-07-20T18:48:22Z

This PR adds a Copy IP/Netmask context menu action to the Banned Peers Table.

This feature is helpful if a node using GUI might want to alert its peer about a particular malicious user. So it can copy that user’s IP/Netmask and broadcast it to its peers so they can ban it instantly using the setban command in the console.

Master	PR

hebasto · 2021-07-20T18:51:44Z

Concept ACK.

This PR adds a Copy Subnet context menu action to the Banned Peers Table.

But on the screenshot a menu item is called "Copy Address". Which variant is correct?

jarolrod

Concept ACK

Looking good, one note: https://github.com/bitcoin-core/gui/pull/384/files#r673396366

hebasto · 2021-07-20T18:58:06Z

@ShaMan239

And warm welcome as a new contributor 🎆

shaavan · 2021-07-20T19:05:51Z

@ShaMan239

And warm welcome as a new contributor fireworks

Thank You very much @hebasto

shaavan · 2021-07-20T20:01:06Z

Updated from c56e082 -> d518d67 (pr384.01 -> pr384.02)
changes: addressed #384 (comment)

But on the screenshot a menu item is called "Copy Address". Which variant is correct?

Sorry, this is now addressed.

kristapsk · 2021-07-20T22:07:41Z

Concept ACK. I think "Copy address" should be added to connected peers list's context menu too, both for consistency and usefulness.

RandyMcMillan · 2021-07-20T22:26:57Z

Concept ACK

I will be curious to see how "Copy subnet" translates.
"Copy address" or simply "Copy" will be ok if there is a translation issue.

I lean toward simply "Copy".

It would be great to link a key command to the action as well - such as <control> + c maybe?

jarolrod · 2021-07-21T17:13:03Z

@RandyMcMillan

I will be curious to see how "Copy subnet" translates.

That's a valid concern. Maybe we can add additional context to the translator comment stating that the subnet is a combination of its IP address and it's Netmask. Additionally we can add that if there is no clear translation in the target language the translation of IP address can be used.

hebasto · 2021-07-21T17:19:23Z

@RandyMcMillan

I will be curious to see how "Copy subnet" translates.

That's a valid concern. Maybe we can add additional context to the translator comment stating that the subnet is a combination of its IP address and it's Netmask. Additionally we can add that if there is no clear translation in the target language the translation of IP address can be used.

If the translator comment will be touched, please, end it with a full stop / period.
Rationale: it is presented to translators in Transifex.com web-interface, and they usually do not look into the source code. The absence of a full stop / period could make an impression that Transifex.com just trimmed out a part of a comment.

Disclaimer: I'm a translator to Ukrainian :)

shaavan · 2021-07-21T19:12:42Z

updated from 7c21de2 -> 32b4a54 (pr384.02 -> pr384.03)
changes: addressed #384 (comment) and #384 (comment)

jarolrod

tACK 32b4a54

Tested on macOS 11.3. Banned a peer, then was able to successfully copy the Peer's subnet by using the new Copy subnet context menu action.

The new translator comment looks good to me. I think the additional context behind it's meaning and a possible alternative will help with any translator confusion.

RandyMcMillan · 2021-07-21T23:09:53Z

tACK 32b4a54

Tested on macOS 10.15.7

psancheti110 · 2021-07-28T09:13:57Z

Tested 32b4a54

I tried to build the PR on macOS 11.4. It delivers what is projected in the attached screenshots. The code patch looks neat and consistent. Congratulations on that 🎉

Although I had two doubts about the functionality and the use-case mentioned for the added feature.

The functionality implemented with the patch does not seem a new feature, instead just another way to copy the address. The Qt widget banlistWidget by default supports keyboard shortcuts like Ctrl+C on Linux/Windows and ⌘+C on macOS. You could look at this comment for a better understanding of what I am trying to say: Long(V3) Onion address is not wrapped in peer details #262 (comment)
The use-case mentioned states about a node using GUI who might want to alert its peer about a particular malicious user. Would other nodes really welcome/interest such messages about a fellow peer, where they know nothing else about the user but its IP Address and assume it to be malicious by Trusting some other user?

In case I understood the use-case wrongly, would appreciate it if you'd correct me.

Happy Coding!

shaavan · 2021-07-29T20:15:45Z

@psancheti110

Thank you for addressing these doubts. Let me try to answer them one by one:

Though we can directly copy addresses using keyboard shortcuts, this PR makes it more accessible for those who need to use the mouse.
Here is an example of when this is useful:
- Sometimes, there is a list of peers known to spy that is passed around. This makes it more convenient to add to the list.
- You have two GUI nodes and banned a peer on one node. You can copy the subnet from GUI Node A, take it to GUI Node B, and ban the peer there as well.

psancheti110 · 2021-08-02T10:27:21Z

Hello @ShaMan239

Appreciate you clearing my doubts and concerns 🙌🏻

Agreed. It can be a good addition for people who want to use a mouse for a change.

I'd like to suggest something maybe you could add to make the use-case mentioned even better.

Currently, in the banned peers' list, it is not possible to select multiple addresses and copy them together. For now one has to copy individual addresses one by one.
You can also checkout rpcconsole. Try running listbanned and that shall return a list of all banned peers in JSON format, which can be copied with just one click of the mouse. Attaching a screenshot for your reference.

rpcconsole : listbanned

hebasto

I think "Copy address" should be added to connected peers list's context menu too, both for consistency and usefulness.

Yes, I guess it is an improvement in terms of accessibility (that is why I concept ACKed this pr).

OTOH, such changes open a door for prs that suggest to add a "Copy some_another_fancy_column_content" into the context menus. It could ends with ugly bloated menus.

I don't think the use cases discussed above are a good practice for the following reasons:

for an attacker it is easy and cheap to switch to another IP address in another subnet, and it seems unfeasible to track attacker manually
aggressive banning (I believe it is the case when we use setban RPC call) increases the risk of splitting the network

hebasto · 2021-08-04T09:46:32Z

+        /*: Context menu action to copy the subnet of a banned peer.
+            Subnet is the combination of a peer's IP address and it's Netmask.
+            Depending on the language, the translation of 'IP address' can be subbed in for subnet. */
+        banTableContextMenu->addAction(tr("&Copy subnet"), [this] {


Should "XXX" in "Copy XXX" menu action match a header title of the column?

jarolrod · 2021-08-20T03:39:00Z

Should "XXX" in "Copy XXX" menu action match a header title of the column?

I think that in the scope of this PR it should. While the combination of IP and Netmask is called subnet in other parts of the code, the column header here is 'IP/Netmaskand notSubnet`. @hebasto is completely correct here. Having different names for the same data here can be confusing for both users and translators.

For now lets keep the same name as in the header. If there is valid motivation to rename this header to subnet, then a follow up PR can do so and update the name of this action from Copy IP/Netmask to Copy subnet.

For now lets rename this action to Copy IP/Netmask

shaavan · 2021-08-20T12:08:12Z

updated from 32b4a54 -> 2da5d46 (pr384.03 -> pr384.04)
changes: addressed the comments 1 and 2 by @hebasto and @jarolrod

Updated Name of Action from copy subnet to copy IP/Netmask
Updated Translator's Comments accordingly

jarolrod · 2021-08-20T14:37:16Z

        banTableContextMenu = new QMenu(this);
+        /*: Context menu action to copy the IP/Netmask of a banned peer.
+            IP/Netmask is the combination of a peer's IP address and it's Netmask.
+            For IP Address see: https://en.wikipedia.org/wiki/Subnetwork


This is similar to what was done in #332, see: https://github.com/bitcoin-core/gui/pull/332/files#diff-fe41db46679f280131a7b0a04d39383dd4d0ab623cec8de8ddc74f79b163dabfR285

But, is this an appropriate link for more information on IP Address? What about: https://en.wikipedia.org/wiki/IP_address

shaavan · 2021-08-20T15:20:12Z

Updated from 2da5d46 to a52f72f
Addressed this comment by @jarolrod

Updated translator comment to now direct towards the correct link with the information regarding the meaning of IP Address.

jarolrod · 2021-08-20T16:11:31Z

tACK a52f72f

tested on Ubuntu 20.04 and macOS 12. PR works as intended. Translator comments look good to me

hebasto

ACK ec43933

shaavan · 2021-08-26T17:25:58Z

Updated from a52f72f to ec43933 (pr384.04 -> pr384.05)
Addressed comment by @hebasto

Changes:

Removed proprietary link: https://wiki.teltonika-networks.com from the translator comment.

jarolrod

tACK ec43933

nice work! 🥃

This adds a copy IP/Netmask context menu action for peers in the banned peer table

shaavan · 2021-08-26T18:10:49Z

Updated from ec43933 to ab1461d (pr384.05 -> pr384.06)
Address comments by @hebasto and @jarolrod

Changes:

Fixed grammatical error: it’s -> its
Fixed unnecessary capitalisation: Address -> address
Updated commit message title.

hebasto

re-ACK ab1461d, tested on Linux Mint 20.2 (Qt 5.12.8).

jarolrod · 2021-08-26T19:21:58Z

re-ACK ab1461d

Only change since last review is update to translator comment and update to the commit message.

ab1461d qt: Add copy IP/Netmask action for banned peer (Shashwat) Pull request description: This PR adds a Copy IP/Netmask context menu action to the Banned Peers Table. This feature is helpful if a node using GUI might want to alert its peer about a particular malicious user. So it can copy that user’s IP/Netmask and broadcast it to its peers so they can ban it instantly using the setban command in the console. | Master | PR | | ----------- | ----------- | | ![Screenshot_from_2021-07-21_00-01-331](https://user-images.githubusercontent.com/23396902/126377808-bd23bb19-3f47-4f1b-8371-39baa9747bbe.png) | ![Screenshot from 2021-08-20 20-13-28(1)(1)](https://user-images.githubusercontent.com/85434418/130251441-a8d0f816-a2e9-4e63-a22d-94885c5cec98.png) | ACKs for top commit: jarolrod: re-ACK ab1461d hebasto: re-ACK ab1461d, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: a528f089bd4cb5b51fec987550d21c2587459ad80f854b55850bc62c776c21f3fa31052a17e2b0e9e9d0b3468799c8070ed306543730fb7b324f283847151e17

jarolrod added the UX All about "how to get things done" label Jul 20, 2021

jarolrod reviewed Jul 20, 2021

View reviewed changes

Comment thread src/qt/rpcconsole.cpp Outdated

jarolrod reviewed Jul 20, 2021

View reviewed changes

shaavan force-pushed the copy-subnet branch from c56e082 to d518d67 Compare July 20, 2021 19:41

shaavan force-pushed the copy-subnet branch 3 times, most recently from 7c21de2 to 32b4a54 Compare July 21, 2021 19:07

jarolrod reviewed Jul 21, 2021

View reviewed changes

hebasto changed the title ~~add copy subnet action for banned peer~~ Add copy subnet action for banned peer Aug 4, 2021

hebasto reviewed Aug 4, 2021

View reviewed changes

shaavan force-pushed the copy-subnet branch from 32b4a54 to 2da5d46 Compare August 20, 2021 12:02

jarolrod reviewed Aug 20, 2021

View reviewed changes

shaavan changed the title ~~Add copy subnet action for banned peer~~ Add copy IP/Netmask action for banned peer Aug 20, 2021

shaavan force-pushed the copy-subnet branch from 2da5d46 to a52f72f Compare August 20, 2021 15:17

hebasto reviewed Aug 23, 2021

View reviewed changes

Comment thread src/qt/rpcconsole.cpp Outdated

shaavan force-pushed the copy-subnet branch from a52f72f to ec43933 Compare August 26, 2021 17:21

hebasto approved these changes Aug 26, 2021

View reviewed changes

jarolrod reviewed Aug 26, 2021

View reviewed changes

hebasto reviewed Aug 26, 2021

View reviewed changes

Comment thread src/qt/rpcconsole.cpp Outdated

jarolrod reviewed Aug 26, 2021

View reviewed changes

Comment thread src/qt/rpcconsole.cpp Outdated

qt: Add copy IP/Netmask action for banned peer

ab1461d

This adds a copy IP/Netmask context menu action for peers in the banned peer table

shaavan force-pushed the copy-subnet branch from ec43933 to ab1461d Compare August 26, 2021 18:09

hebasto approved these changes Aug 26, 2021

View reviewed changes

hebasto merged commit b8d45a3 into bitcoin-core:master Aug 26, 2021

bitcoin-core locked as resolved and limited conversation to collaborators Aug 26, 2022

Conversation

shaavan commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jul 20, 2021

Uh oh!

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jul 20, 2021

Uh oh!

shaavan commented Jul 20, 2021

Uh oh!

shaavan commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristapsk commented Jul 20, 2021

Uh oh!

RandyMcMillan commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod commented Jul 21, 2021

Uh oh!

hebasto commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaavan commented Jul 21, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

RandyMcMillan commented Jul 21, 2021

Uh oh!

psancheti110 commented Jul 28, 2021

Uh oh!

shaavan commented Jul 29, 2021

Uh oh!

psancheti110 commented Aug 2, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarolrod commented Aug 20, 2021

Uh oh!

shaavan commented Aug 20, 2021

Uh oh!

jarolrod Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaavan commented Aug 20, 2021

Uh oh!

jarolrod commented Aug 20, 2021

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan commented Aug 26, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shaavan commented Aug 26, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod commented Aug 26, 2021

Uh oh!

Reviewers

Assignees

Labels

shaavan commented Jul 20, 2021 •

edited

Loading

shaavan commented Jul 20, 2021 •

edited

Loading

RandyMcMillan commented Jul 20, 2021 •

edited

Loading

hebasto commented Jul 21, 2021 •

edited

Loading

hebasto Aug 4, 2021 •

edited

Loading

jarolrod Aug 20, 2021 •

edited

Loading