Skip to content

Use the stored CSubNet entry when unbanning #717

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

Merged
hebasto merged 2 commits into from
Mar 9, 2023
Merged

Use the stored CSubNet entry when unbanning #717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a981af4
gui: use the stored CSubNet entry when unbanning
vasild Mar 6, 2023
4be57a5
gui: fix comments for BanTableModel and BanTablePriv::refreshBanlist()
vasild Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/qt/bantablemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ class BanTablePriv
/** Order (ascending or descending) to sort nodes by */
Qt::SortOrder sortOrder;

/** Pull a full list of banned nodes from CNode into our cache */
/** Pull a full list of banned nodes from interfaces::Node into our cache */
void refreshBanlist(interfaces::Node& node)
{
banmap_t banMap;
Expand Down Expand Up @@ -178,3 +178,9 @@ bool BanTableModel::shouldShow()
{
return priv->size() > 0;
}

bool BanTableModel::unban(const QModelIndex& index)
{
CCombinedBan* ban{static_cast<CCombinedBan*>(index.internalPointer())};
return ban != nullptr && m_node.unban(ban->subnet);
}
4 changes: 3 additions & 1 deletion src/qt/bantablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class BannedNodeLessThan
};

/**
Qt model providing information about connected peers, similar to the
Qt model providing information about banned peers, similar to the
"getpeerinfo" RPC call. Used by the rpc console UI.
Copy link
Copy Markdown
Contributor

@mzumsande mzumsande Mar 8, 2023

Choose a reason for hiding this comment

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

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

*/
class BanTableModel : public QAbstractTableModel
Expand Down Expand Up @@ -68,6 +68,8 @@ class BanTableModel : public QAbstractTableModel

bool shouldShow();

bool unban(const QModelIndex& index);

public Q_SLOTS:
void refresh();

Expand Down
19 changes: 7 additions & 12 deletions src/qt/rpcconsole.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

#include <chainparams.h>
#include <interfaces/node.h>
#include <netbase.h>
#include <qt/bantablemodel.h>
#include <qt/clientmodel.h>
#include <qt/guiutil.h>
Expand Down Expand Up @@ -1308,17 +1307,13 @@ void RPCConsole::unbanSelectedNode()

// Get selected ban addresses
QList<QModelIndex> nodes = GUIUtil::getEntryData(ui->banlistWidget, BanTableModel::Address);
for(int i = 0; i < nodes.count(); i++)
{
// Get currently selected ban address
QString strNode = nodes.at(i).data().toString();
CSubNet possibleSubnet;

LookupSubNet(strNode.toStdString(), possibleSubnet);
if (possibleSubnet.IsValid() && m_node.unban(possibleSubnet))
{
clientModel->getBanTableModel()->refresh();
}
BanTableModel* ban_table_model{clientModel->getBanTableModel()};
bool unbanned{false};
for (const auto& node_index : nodes) {
Copy link
Copy Markdown
Contributor

@mzumsande mzumsande Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor Author

@vasild vasild Mar 9, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

@furszy furszy Mar 9, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@mzumsande mzumsande Mar 9, 2023

Choose a reason for hiding this comment

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

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.

unbanned |= ban_table_model->unban(node_index);
}
if (unbanned) {
ban_table_model->refresh();
}
}

Expand Down