Skip to content
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.

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

[Qt] allow banning and unbanning over UI->peers table #6315

Merged
merged 20 commits into from Sep 22, 2015

Conversation

@jonasschnelli
Copy link
Member

jonasschnelli commented Jun 20, 2015

This adds 4 ban options to the peers tables context menu (1h, 24h, 7d, 365d). If there are active bans, a table with banned subnets will be shown.
Sync between the rpc bans and the ui bans are guaranteed over a new uiInterface signal.

Screenshots:
bildschirmfoto-2015-06-20-um-21 58 44
bildschirmfoto-2015-06-20-um-21 58 59

@jonasschnelli
jonasschnelli reviewed Jun 20, 2015
View changes
src/rpcnet.cpp Outdated
@@ -527,6 +528,7 @@ UniValue setban(const UniValue& params, bool fHelp)
throw JSONRPCError(RPC_MISC_ERROR, "Error: Unban failed");
}

uiInterface.BannedListChanged();

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 20, 2015

Author Member

I'm not sure if the locking of a signal emit is done within boost::signal of if this needs explicit locking of uiInterface?

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 23, 2015

Perhaps @laanwj can comment on this?

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2015/06/qt_ban branch Jun 20, 2015
@jonasschnelli
jonasschnelli reviewed Jun 20, 2015
View changes
src/qt/bantablemodel.cpp Outdated
case Bantime:
//show time in users local timezone, not 64bit compatible!
//TODO find a way to support 64bit timestamps
boost::posix_time::ptime pt1 = boost::posix_time::from_time_t(rec->bantil);

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 20, 2015

Author Member

i recognized that boost::posix_time::from_time_t is not 64bit capable. Maybe someone has a idea how to fix this.

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

What about QDateTime::fromTime_t()?

QDateTime::fromTime_t(rec->bantil).toString() didn't test it...

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 21, 2015

Author Member

I think this won't work either. The docs are saying: QDateTime QDateTime::fromTime_t(uint seconds) (uint!).

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 21, 2015

Author Member

Found a fix! Pushed.

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/bantablemodel.cpp Outdated
/** Pull a full list of banned nodes from CNode into our cache */
void refreshBanlist()
{
std::map<CSubNet, int64_t> banMap;

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

I was thinking about a typedef for this, what do you mean?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 21, 2015

Author Member

typedefs are useful but can also make reading the code more complicate. In this case (simple map) i don't see a usecase for a typedef.

This comment has been minimized.

Copy link
@prestonc1333

prestonc1333 Jun 23, 2015

Why would it make it more complicated? Worse comes to worse, you comment it, which is not really needed anyways.

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/bantablemodel.cpp Outdated
cachedBanlist.reserve(banMap.size());
#endif
std::map<CSubNet, int64_t>::iterator iter;
for (iter = banMap.begin(); iter != banMap.end(); ++iter) {

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

Could this be a Qt foreach? Would also be easy to convert to C++11 then :)?

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/bantablemodel.cpp Outdated
}
}

int size()

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

Could this be int size() const?

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/bantablemodel.cpp Outdated
{
if(idx >= 0 && idx < cachedBanlist.size()) {
return &cachedBanlist[idx];
} else {

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

That else can be removed. I see you copied most of the code here from peertablemodel.cpp where all these suggestions would also fit and perhaps we should clean it up, to keep the codebase close together?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 21, 2015

Author Member

Not sure if this else can be removed. What if the QTableView requests a row at index which is not in the cache QList. Obviously it should not happen because the table size is driven by the size of the cache QList. But I prefer keeping this.

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 21, 2015

Not sure if you're right here, when the if clause isn't true we always run into the else (as we have a return in the if we won't reach the return 0, if the clause is false). I'd rather remove it...

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/bantablemodel.cpp Outdated
}
} else if (role == Qt::TextAlignmentRole) {
if (index.column() == Bantime)
return (int)(Qt::AlignRight | Qt::AlignVCenter);

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

Is int the right type here? I tried (QVariant) which also works.

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/bantablemodel.cpp Outdated
{
return createIndex(row, column, data);
}
else

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

Same as above unneeded else. If we don't have data we can return just QModelIndex(). I think it's easier to read without else.

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/bantablemodel.h Outdated
class QTimer;
QT_END_NAMESPACE

struct CCombinedBan {

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

We have a struct here and a std::map for similar stuff... is there a way to unify these 2?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 21, 2015

Author Member

I think they should be separated and represent cores and ui form of keeping the data.

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/bantablemodel.cpp Outdated
#include "guiconstants.h"
#include "guiutil.h"

#include "net.h"

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

This is included in the header already.

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/rpcconsole.cpp Outdated

// create context menu actions
QAction* disconnectAction = new QAction(tr("&Disconnect Node"), this);
QAction* disconnectAction = new QAction(tr("&Disconnect Node"), this);
QAction* banAction1h = new QAction(tr("&Ban Node for 1 hour"), this);

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

You are repeating some translation text, perhaps there is an efficient way to reduce that?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 21, 2015

Author Member

split into two strings to reduce translation effort.

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/rpcconsole.cpp Outdated
SplitHostPort(nStr, port, addr);

CNode::Ban(CNetAddr(addr), bantime);
bannedNode->CloseSocketDisconnect();

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

Seems you reintroduced that over fDisconnect flag?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 21, 2015

Author Member

This was a serious overlook. Fixed. Thanks.

@Diapolo
Diapolo reviewed Jun 20, 2015
View changes
src/qt/rpcconsole.cpp Outdated

void RPCConsole::showOrHideBanTableIfRequired()
{
bool visible = clientModel->getBanTableModel()->shouldShow();

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 20, 2015

This could also use a NULL pointer check for clientModel IMHO.

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 21, 2015

Author Member

Nullpointer check added.

@Diapolo
Copy link

Diapolo commented Jun 20, 2015

Great pull IMHO, sorry for hitting you hard with that bunch of comments...

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Jun 21, 2015

Thanks @Diapolo for the review. Appricate detailed feedback.
Will fix all your points (especially the miss transaction to fDisconnect!).

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Jun 21, 2015

@Diapolo
Diapolo reviewed Jun 21, 2015
View changes
src/qt/bantablemodel.cpp Outdated
}
} else if (role == Qt::TextAlignmentRole) {
if (index.column() == Bantime)
return (int)(Qt::AlignRight | Qt::AlignVCenter);

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 21, 2015

Still the question: Is int the right type here? I tried (QVariant) which also works and is more the Qt style?

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jun 21, 2015

Author Member

Right. This is also the same as we have it in PeersTableModel, it works, but i agree that it should be QVariant (as the method definition says).

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 21, 2015

I opened #6317 and will try to keep that in sync with what we are doing here :).

@Diapolo
Diapolo reviewed Jun 21, 2015
View changes
src/qt/clientmodel.cpp Outdated
@@ -226,12 +240,19 @@ static void NotifyAlertChanged(ClientModel *clientmodel, const uint256 &hash, Ch
Q_ARG(int, status));
}

static void BannedListChanged(ClientModel *clientmodel)
{
qDebug() << "BannedListChanged";

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 21, 2015

You could also use something like:
qDebug() << QString("%1: Requesting update for peer banlist").arg(__func__); I intend to creat a pull to make use of that format so we have the debug logging include always the correct function name :).

@Diapolo
Diapolo reviewed Jun 21, 2015
View changes
src/qt/rpcconsole.cpp Outdated
@@ -8,6 +8,7 @@
#include "clientmodel.h"
#include "guiutil.h"
#include "peertablemodel.h"
#include "bantablemodel.h"

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 21, 2015

Not even a nit, but a please: Can you add #include "bantablemodel.h" above clientmodel :)?

@Diapolo
Diapolo reviewed Jun 21, 2015
View changes
src/qt/rpcconsole.cpp Outdated
connect(disconnectAction, SIGNAL(triggered()), this, SLOT(disconnectSelectedNode()));

//add a signal mapping to allow a dynamic argument

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 21, 2015

Nit: These comments are missing a whitespace at the front.

@Diapolo
Diapolo reviewed Jun 21, 2015
View changes
src/qt/rpcconsole.cpp Outdated
@@ -335,6 +380,9 @@ void RPCConsole::setClientModel(ClientModel *model)
ui->startupTime->setText(model->formatClientStartupTime());

ui->networkName->setText(QString::fromStdString(Params().NetworkIDString()));

connect(model, SIGNAL(banListChanged()), this, SLOT(showOrHideBanTableIfRequired()));

This comment has been minimized.

Copy link
@Diapolo

Diapolo Jun 21, 2015

I'd rather group these near the banlist stuff or add some small comment, before this is getting confusing here?

@Diapolo
Copy link

Diapolo commented Jun 21, 2015

Currently integrating this into my build to test, will report back :).

@Diapolo
Copy link

Diapolo commented Jun 21, 2015

Observations:

  1. Banning a node, selecting a new node (from connected peers) and then selecting a banned node leaves the details of the previous (not banned) node visible in the right area.
  2. Banning a node selects the first cell in the banlist table without selecting the whole line and without a focus (cell is not marked active). I'd suggest the banned node should be selected and active entirely.
  3. I'm using Tor and hidden-service addresses + netmask + banned until are that large that I'm unable to view the whole values with the default sizes.
  4. The banned peers list has a heading Banned peers, perhaps the connected peers also should get a heading? I'd vote for a much smaller font size, the current one looks rather too big.

Edit:

  1. There is a serious issue with banning incomming connections via Tor, these are all shown as 127.0.0.1 and handling them seems broken.
@Diapolo
Copy link

Diapolo commented Jun 21, 2015

There is also an issue with removing a peer from banned peers list when banned until is over:
ban

The peer should be removed from the list, which is not working currently.

listbanned also still shows them:
[
{
"address": "127.0.0.1/255.255.255.255",
"banned_untill": 1434891295
},
{
"address": "nkf5e6b7pl4jfd4a.onion/ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff",
"banned_untill": 1434891291
},
{
"address": "t6xj6wilh4ytvcs7.onion/ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff",
"banned_untill": 1434891400
}
]

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Jun 21, 2015

@Diapolo: for autocleaning then ban list see PR #6310

I think you point 1) is not a problem. The peer is still selected (gray) when you interact with the bantable. Point 2) and 3) are cosmetics and should not hold this PR back. Point 4) agreed.

Banning tor nodes needs testing (best first over RPC) and maybe needs more implementation. Would be good if you could see what's missing there.

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Jun 21, 2015

Tested also with Tor nodes. Works fine. @Diapolo: what issues did you encounter with tor nodes?

@laanwj laanwj added the GUI label Jun 22, 2015
@Diapolo
Copy link

Diapolo commented Jun 22, 2015

@jonasschnelli The problem are incoming Tor nodes with 127.0.0.1 blocking one connected node blocks any other incoming 127.0.0.1 node. Other connected 127.0.0.1 are not affected.

Also I'd suggest splitting of the autocleaning pull from the disk-store-pull as autocleaning is uncontroversial and also needed by this one.

I also disagree on ignoring UI or UX issues for now as often such "small" or "minor" things are just forgotton or I pick them up. I'd vote for making this pull good and not just "working okay" :).

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2015/06/qt_ban branch Jun 22, 2015
jonasschnelli and others added 14 commits Jun 21, 2015
- some code cleanups
- fix date formatting
- reduce header includes
Only use CIDR notation if the netmask can be represented as such.
- remove banListChanged signal from client model
- directly call clientModel->getBanTableModel()->refresh() without the way
  over clientModel->updateBanlist()

- also fix clearing peer detail window, when selecting (clicking)
  peers in the ban list
- add missing NULL pointer checks
- add better comments and reorder some code in rpcconsole.cpp
- remove unneeded leftovers in bantable.cpp
- update bantable column sizes to prevent cutting of banned until
- 1 (h)our
- 1 (d)ay
- 1 (w)eek
- 1 (y)ear
- this matches RPC call behaviour
@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2015/06/qt_ban branch to 7f90ea7 Sep 16, 2015
@jonasschnelli
Copy link
Member Author

jonasschnelli commented Sep 16, 2015

Rebased. Travis is happy now.

@jtimon
Copy link
Member

jtimon commented Sep 17, 2015

Concept ACK

@luke-jr
Copy link
Member

luke-jr commented Sep 20, 2015

Why does 7f90ea7 remove the DumpBanlist calls?

@jonasschnelli
Copy link
Member Author

jonasschnelli commented Sep 20, 2015

Thank @luke-jr for the precise review.
I think this sneaked in over a rebase because this PR started before the banlist was persisted to disk.

Just added a commit that re-implenents the DumpBanlist() calls in case of banning/unbanning over QT.

@luke-jr
Copy link
Member

luke-jr commented Sep 20, 2015

Note I did not do a precise review for this (yet?), just noticed those disappear from the previous version. ;)

@laanwj laanwj merged commit 7aac6db into bitcoin:master Sep 22, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Sep 22, 2015
7aac6db [QT] dump banlist to disk in case of ban/unban over QT (Jonas Schnelli)
7f90ea7 [QA] adabt QT_NO_KEYWORDS for QT ban implementation (Jonas Schnelli)
07f70b2 [QA] fix netbase tests because of new CSubNet::ToString() output (Jonas Schnelli)
4ed0510 [Qt] call DumpBanlist() when baning unbaning nodes (Philip Kaufmann)
be89292 [Qt] reenabling hotkeys for ban context menu, use different words (Jonas Schnelli)
b1189cf [Qt] adapt QT ban option to banlist.dat changes (Jonas Schnelli)
65abe91 [Qt] add sorting for bantable (Philip Kaufmann)
51654de [Qt] bantable polish (Philip Kaufmann)
cdd72cd [Qt] simplify ban list signal handling (Philip Kaufmann)
43c1f5b [Qt] remove unused timer-code from banlistmodel.cpp (Jonas Schnelli)
e2b8028 net: Fix CIDR notation in ToString() (Wladimir J. van der Laan)
9e521c1 [Qt] polish ban table (Philip Kaufmann)
607809f net: use CIDR notation in CSubNet::ToString() (Jonas Schnelli)
53caec6 [Qt] bantable overhaul (Jonas Schnelli)
f0bcbc4 [Qt] bantable fix timestamp 64bit issue (Jonas Schnelli)
6135309 [Qt] banlist, UI optimizing and better signal handling (Jonas Schnelli)
770ca79 [Qt] add context menu with unban option to ban table (Jonas Schnelli)
5f42132 [Qt] add ui signal for banlist changes (Jonas Schnelli)
ad204df [Qt] add banlist table below peers table (Jonas Schnelli)
50f0908 [Qt] add ban functions to peers window (Jonas Schnelli)
@Diapolo
Copy link

Diapolo commented Sep 22, 2015

@laanwj Now if #6371 could be merged, I'd be happy!

zkbot added a commit to zcash/zcash that referenced this pull request Apr 5, 2018
banlist.dat: store banlist on disk

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6310
- bitcoin/bitcoin#6315
  - Only the new signal and net changes, no QT
- bitcoin/bitcoin#7350
- bitcoin/bitcoin#7458

Part of #2074.
zkbot added a commit to zcash/zcash that referenced this pull request Mar 6, 2020
banlist.dat: store banlist on disk

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6310
- bitcoin/bitcoin#6315
  - Only the new signal and net changes, no QT
- bitcoin/bitcoin#7350
- bitcoin/bitcoin#7458

Part of #2074.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.