Add copy IP/Netmask action for banned peer

shaavan commented at 6:48 pm on July 20, 2021: contributor

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

jarolrod added the label UX on Jul 20, 2021

hebasto commented at 6:51 pm on July 20, 2021: member

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?

in src/qt/rpcconsole.cpp:710 in c56e082d8e outdated

705@@ -706,6 +706,11 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
706 
707         // create ban table context menu
708         banTableContextMenu = new QMenu(this);
709+        //: Context menu action to copy the subnet of a banned peer
710+        banTableContextMenu->addAction(tr("Copy subnet"), [this] {

jarolrod commented at 6:54 pm on July 20, 2021:

Since #362 we are now defaulting to adding mnemonic shortcuts for the context menus. This means that you should add one here

Below is how you would do so here:

0        banTableContextMenu->addAction(tr("&Copy subnet"), [this] {

shaavan commented at 8:04 pm on July 20, 2021:

this is addressed in d518d67

jarolrod commented at 6:55 pm on July 20, 2021: member

Concept ACK

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

hebasto commented at 6:58 pm on July 20, 2021: member

@ShaMan239

And warm welcome as a new contributor :fireworks:

shaavan commented at 7:05 pm on July 20, 2021: contributor

@ShaMan239

And warm welcome as a new contributor fireworks

Thank You very much @hebasto

shaavan force-pushed on Jul 20, 2021

shaavan commented at 8:01 pm on July 20, 2021: contributor

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

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

Sorry, this is now addressed.

kristapsk commented at 10:07 pm on July 20, 2021: contributor

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

RandyMcMillan commented at 10:26 pm on July 20, 2021: contributor

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 commented at 5:13 pm on July 21, 2021: member

@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 commented at 5:19 pm on July 21, 2021: member

@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 force-pushed on Jul 21, 2021

shaavan commented at 7:12 pm on July 21, 2021: contributor

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

jarolrod commented at 10:23 pm on July 21, 2021: member

tACK 32b4a54429278dcfb6be1fbaa10c2b91e73a9a1a

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 commented at 11:09 pm on July 21, 2021: contributor

tACK 32b4a54

Tested on macOS 10.15.7

psancheti110 commented at 9:13 am on July 28, 2021: contributor

Tested 32b4a54429278dcfb6be1fbaa10c2b91e73a9a1a

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: #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 commented at 8:15 pm on July 29, 2021: contributor

@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 commented at 10:27 am on August 2, 2021: contributor

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

Screenshot 2021-08-02 at 3 50 41 PM (2)

hebasto renamed this:
~~add copy subnet action for banned peer~~
Add copy subnet action for banned peer
on Aug 4, 2021

in src/qt/rpcconsole.cpp:712 in 32b4a54429 outdated

705@@ -706,6 +706,13 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
706 
707         // create ban table context menu
708         banTableContextMenu = new QMenu(this);
709+        /*: Context menu action to copy the subnet of a banned peer.
710+            Subnet is the combination of a peer's IP address and it's Netmask.
711+            Depending on the language, the translation of 'IP address' can be subbed in for subnet. */
712+        banTableContextMenu->addAction(tr("&Copy subnet"), [this] {

hebasto commented at 9:46 am on August 4, 2021:

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

hebasto commented at 10:27 am on August 4, 2021: member

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

jarolrod commented at 3:39 am on August 20, 2021: member

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 force-pushed on Aug 20, 2021

shaavan commented at 12:08 pm on August 20, 2021: contributor

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

in src/qt/rpcconsole.cpp:711 in 2da5d465c8 outdated

705@@ -706,6 +706,14 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
706 
707         // create ban table context menu
708         banTableContextMenu = new QMenu(this);
709+        /*: Context menu action to copy the IP/Netmask of a banned peer.
710+            IP/Netmask is the combination of a peer's IP address and it's Netmask.
711+            For IP Address see: https://en.wikipedia.org/wiki/Subnetwork

jarolrod commented at 2:37 pm on August 20, 2021:

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

shaavan renamed this:
~~Add copy subnet action for banned peer~~
Add copy IP/Netmask action for banned peer
on Aug 20, 2021

shaavan force-pushed on Aug 20, 2021

shaavan commented at 3:20 pm on August 20, 2021: contributor

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 commented at 4:11 pm on August 20, 2021: member

tACK a52f72f

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

in src/qt/rpcconsole.cpp:712 in a52f72f52d outdated

705@@ -706,6 +706,14 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
706 
707         // create ban table context menu
708         banTableContextMenu = new QMenu(this);
709+        /*: Context menu action to copy the IP/Netmask of a banned peer.
710+            IP/Netmask is the combination of a peer's IP address and it's Netmask.
711+            For IP Address see: https://en.wikipedia.org/wiki/IP_address
712+            For NetMask see: https://wiki.teltonika-networks.com/view/What_is_a_Netmask%3F */

hebasto commented at 9:15 pm on August 23, 2021:

I’d avoid mentioning any links to proprietary sites in the Bitcoin Core codebase.

Maybe, just skip last two lines of the comment?

jarolrod commented at 9:18 pm on August 23, 2021:

wikipedia or the other website? we already link to wikipedia articles to help with translations: https://github.com/bitcoin-core/gui/blob/d3203a99d886177eee9d1f9cd8411e215118a4e6/src/qt/addressbookpage.cpp#L285

hebasto commented at 9:37 pm on August 23, 2021:

At least, drop https://wiki.teltonika-networks.com, please.

shaavan commented at 5:27 pm on August 26, 2021:

At least, drop https://wiki.teltonika-networks.com, please.

shaavan force-pushed on Aug 26, 2021

hebasto approved

hebasto commented at 5:25 pm on August 26, 2021: member

ACK ec439332fc28280866b65a2c5b4a6efaf79391ed

shaavan commented at 5:25 pm on August 26, 2021: contributor

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 commented at 5:27 pm on August 26, 2021: member

tACK ec43933

nice work! 🥃

in src/qt/rpcconsole.cpp:710 in ec439332fc outdated

705@@ -706,6 +706,13 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
706 
707         // create ban table context menu
708         banTableContextMenu = new QMenu(this);
709+        /*: Context menu action to copy the IP/Netmask of a banned peer.
710+            IP/Netmask is the combination of a peer's IP address and it's Netmask.

hebasto commented at 5:41 pm on August 26, 2021:

nit:

0            IP/Netmask is the combination of a peer's IP address and its netmask.

?

jarolrod commented at 5:43 pm on August 26, 2021:

Netmask is a noun, so it should remain capitalized

0            IP/Netmask is the combination of a peer's IP address and its Netmask.

shaavan commented at 6:11 pm on August 26, 2021:

Addressed in ab1461d

qt: Add copy IP/Netmask action for banned peer

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

ab1461d5d3

in src/qt/rpcconsole.cpp:711 in ec439332fc outdated

705@@ -706,6 +706,13 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
706 
707         // create ban table context menu
708         banTableContextMenu = new QMenu(this);
709+        /*: Context menu action to copy the IP/Netmask of a banned peer.
710+            IP/Netmask is the combination of a peer's IP address and it's Netmask.
711+            For IP Address see: https://en.wikipedia.org/wiki/IP_address */

jarolrod commented at 5:45 pm on August 26, 2021:

nit

0            For IP address see: https://en.wikipedia.org/wiki/IP_address */

shaavan commented at 6:11 pm on August 26, 2021:

Addressed in ab1461d

shaavan force-pushed on Aug 26, 2021

shaavan commented at 6:10 pm on August 26, 2021: contributor

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 approved

hebasto commented at 6:11 pm on August 26, 2021: member

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

jarolrod commented at 7:21 pm on August 26, 2021: member

re-ACK ab1461d

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

hebasto merged this on Aug 26, 2021

hebasto closed this on Aug 26, 2021

sidhujag referenced this in commit dc1a1739e0 on Aug 28, 2021

bitcoin-core locked this on Aug 26, 2022

Add copy IP/Netmask action for banned peer #384