Use the stored CSubNet entry when unbanning #717

vasild commented at 1:27 PM on March 6, 2023: contributor

The previous code visualized the CSubNet object as string, then parsed that string back to CSubNet. This is sub-optimal given that the original CSubNet object can be used directly instead.

This avoids calling LookupSubNet() from the GUI.

DrahtBot commented at 1:27 PM on March 6, 2023: contributor

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	furszy, mzumsande

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin/bitcoin/26078 (p2p: return CSubNet in LookupSubNet by brunoerg)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

vasild commented at 1:39 PM on March 6, 2023: contributor

This would simplify https://github.com/bitcoin/bitcoin/pull/27071.

furszy commented at 2:09 PM on March 6, 2023: contributor

Concept ACK

Would be better to not access the ban model internals from outside, it breaks encapsulation. Could do https://github.com/furszy/bitcoin-gui/commit/81b667efe3a9cc74b04d22d32322ca790f7c0957

vasild force-pushed on Mar 6, 2023

gui: use the stored CSubNet entry when unbanning

The previous code visualized the `CSubNet` object as string, then
parsed that string back to `CSubNet`. This is sub-optimal given that
the original `CSubNet` object can be used directly instead.

This avoids calling `LookupSubNet()` from the GUI.

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

a981af4e6f

gui: fix comments for BanTableModel and BanTablePriv::refreshBanlist() 4be57a5df1

vasild force-pushed on Mar 6, 2023

vasild commented at 3:08 PM on March 6, 2023: contributor

@furszy, done, thanks!

furszy commented at 3:42 PM on March 6, 2023: contributor

utACK 4be57a5d

in src/qt/bantablemodel.h:41 in 4be57a5df1

  36 | @@ -37,7 +37,7 @@ class BannedNodeLessThan
  37 |  };
  38 |  
  39 |  /**
  40 | -   Qt model providing information about connected peers, similar to the
  41 | +   Qt model providing information about banned peers, similar to the
  42 |     "getpeerinfo" RPC call. Used by the rpc console UI.

mzumsande commented at 9:03 PM on March 8, 2023:

If this comment is fixed anyway (it was copied from peertablemodel.h), could also replace "getpeerinfo" with "listbanned".

in src/qt/rpcconsole.cpp:1312 in a981af4e6f outdated

1318 | -        {
1319 | -            clientModel->getBanTableModel()->refresh();
1320 | -        }
1321 | +    BanTableModel* ban_table_model{clientModel->getBanTableModel()};
1322 | +    bool unbanned{false};
1323 | +    for (const auto& node_index : nodes) {

mzumsande commented at 9:25 PM on March 8, 2023:

Not a major issue, but this looks like it's designed to support unbanning multiple nodes with one click, but the GUI doesn't allow me to select multiple entries when I try (unlike in the peertable where banning multiple nodes with one click works).

vasild commented at 8:26 AM on March 9, 2023:

Yes, I noticed the same - looks like we don't need a loop here since it is always just one being selected. But it was like this and I left it as is (with a loop) to minimize the amount of unnecessary changes that can have unexpected adverse effects. I do not know why the GUI wouldn't let me select more than one row. Maybe a change elsewhere in the code would allow that and then this code would be broken without a loop.

furszy commented at 12:14 PM on March 9, 2023:

to enable it, just need to set the table selection mode to multi selection. e.g. peerTable->setSelectionMode(QAbstractItemView::MultiSelection);.

I didn't mention it because the PR is fine, it is not changing behavior.

mzumsande commented at 1:39 PM on March 9, 2023:

Agree, no need to change it here, I just mentioned it because I found it a bit strange and maybe someone would be interested to change it in a follow-up.

mzumsande commented at 9:29 PM on March 8, 2023: contributor

Tested ACK 4be57a5df140f52628cab8a5ca5ddc208cd3f612

I tested that banning and unbanning still works, the code changes look good to me (although I don't know the GUI very well).

hebasto renamed this:
~~gui: use the stored CSubNet entry when unbanning~~
Use the stored CSubNet entry when unbanning
on Mar 9, 2023

hebasto merged this on Mar 9, 2023

hebasto closed this on Mar 9, 2023

vasild deleted the branch on Mar 9, 2023

sidhujag referenced this in commit 1d9aec0ce5 on Mar 9, 2023

Julianabrwon commented at 9:10 AM on May 1, 2023: none

13VYk4xFjMN8d3wXnMZzvm8cwBHp3mo3LP

bitcoin-core locked this on May 1, 2023