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

Return of the Banman #14605

Merged
merged 12 commits into from Jan 21, 2019

Conversation

@dongcarl
Copy link
Contributor

commented Oct 30, 2018

Old English à la Beowulf

Banman wæs bréme    --blaéd wíde sprang--
Connmanes eafera    Coreum in.
aéglaéca            léodum forstandan
Swá bealdode        bearn Connmanes
guma gúðum cúð      gódum daédum·
dréah æfter dóme·   nealles druncne slóg

Modern English Translation

Banman was famed              --his renown spread wide--
Conman's hier,                in Core-land.
against the evil creature     defend the people
Thus he was bold,             the son of Connman
man famed in war,             for good deeds;
he led his life for glory,    never, having drunk, slew

--

With @theuni's blessing, here is Banman, rebased. Original PR: #11457

--

Followup PRs:

  1. Give CNode a Disconnect method (source)
  2. Add a comment to std::atomic_bool fDisconnect in net.h that setting this to true will cause the node to be disconnected the next time DisconnectNodes() runs (source)

@fanquake fanquake added the P2P label Oct 30, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Oct 30, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14987 (RPCHelpMan: Pass through Result and Examples by MarcoFalke)
  • #14929 (net: Allow connections from misbehavior banned peers by gmaxwell)
  • #14897 (randomize GETDATA(tx) request order and introduce bias toward outbound by naumenkogs)
  • #14856 (net: remove more CConnman globals (theuni) by dongcarl)
  • #14464 (refactor: make checkqueue manage the threads by itself (also removed some boost dependencies) by ken2812221)
  • #10785 (Serialization improvements by sipa)

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.

src/qt/rpcconsole.cpp Outdated
if(stats) {
// Find possible nodes, ban it and clear the selected node
const CNodeCombinedStats *stats = clientModel->getPeerTableModel()->getNodeStats(detailNodeRow);
if(stats) {

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Oct 30, 2018

Member

Accidental changes in the first commit?

src/addrdb.cpp Outdated
@@ -105,9 +105,8 @@ bool DeserializeFileDB(const fs::path& path, Data& data)

}

CBanDB::CBanDB()
CBanDB::CBanDB(fs::path ban_file) : pathBanlist(std::move(ban_file))

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 30, 2018

Member

Make sure parameter name matches between declaration and definition :-)

src/banman.h Outdated
public:
~BanMan();
BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);
void Ban(const CNetAddr& netAddr, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 30, 2018

Member

Make sure parameter names match between:

    void Ban(const CNetAddr& netAddr, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);
    void BanMan::Ban(const CNetAddr& addr, const BanReason &banReason, int64_t bantimeoffset, bool sinceUnixEpoch) {
src/banman.h Outdated
~BanMan();
BanMan(fs::path ban_file, CClientUIInterface* client_interface, int64_t default_ban_time);
void Ban(const CNetAddr& netAddr, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);
void Ban(const CSubNet& subNet, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 30, 2018

Member

Make sure these match:

    void Ban(const CSubNet& subNet, const BanReason& reason, int64_t bantimeoffset = 0, bool sinceUnixEpoch = false);
    void BanMan::Ban(const CSubNet& subNet, const BanReason &banReason, int64_t bantimeoffset, bool sinceUnixEpoch) {
src/banman.h Outdated
void ClearBanned();
bool IsBanned(CNetAddr ip);
bool IsBanned(CSubNet subnet);
bool Unban(const CNetAddr &ip);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 30, 2018

Member

Make sure these match:

    bool Unban(const CNetAddr &ip);
    bool BanMan::Unban(const CNetAddr &addr) {
src/banman.h Outdated
bool IsBanned(CNetAddr ip);
bool IsBanned(CSubNet subnet);
bool Unban(const CNetAddr &ip);
bool Unban(const CSubNet &ip);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 30, 2018

Member

Same here:

    bool Unban(const CSubNet &ip);
    bool BanMan::Unban(const CSubNet &subNet) {
src/banman.h Outdated
bool IsBanned(CSubNet subnet);
bool Unban(const CNetAddr &ip);
bool Unban(const CSubNet &ip);
void GetBanned(banmap_t &banmap);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 30, 2018

Member

Same here:

    void GetBanned(banmap_t &banmap);
    void BanMan::GetBanned(banmap_t &banMap)
src/banman.h Outdated
void DumpBanlist();

private:
void SetBanned(const banmap_t &banmap);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 30, 2018

Member

And here :-)

    void SetBanned(const banmap_t &banmap);
    void BanMan::SetBanned(const banmap_t &banMap)
src/banman.cpp Outdated
bool BanMan::Unban(const CSubNet &subNet) {
{
LOCK(m_cs_banned);
if (!m_banned.erase(subNet))

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 30, 2018

Member

Nit: if (m_banned.erase(subNet) == 0) to increase readability and avoid implicit conversion?

src/banman.cpp Outdated
m_is_dirty = true;
}
DumpBanlist(); //store banlist to disk
if(m_client_interface)

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 30, 2018

Member

Since this is new code - please run all new C++ files through clang-format (using the project local settings) to get the expected formatting.

src/banman.cpp Outdated
if (bantimeoffset <= 0)
{
bantimeoffset = m_default_ban_time;
sinceUnixEpoch = false;

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 30, 2018

Member

Could this be re-formulated to avoid reassigning parameters?

This comment has been minimized.

Copy link
@dongcarl

dongcarl Oct 31, 2018

Author Contributor

Reformulated in 36e6b24, let me know if you think that's good

This comment has been minimized.

Copy link
@practicalswift

practicalswift Nov 15, 2018

Member

@dongcarl Nice! Thanks! :-)

@dongcarl dongcarl force-pushed the dongcarl:banman branch 2 times, most recently Oct 31, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

thank you for picking this up!
We welcome the Ban Man to our kingdom

@laanwj laanwj added this to the 0.18.0 milestone Oct 31, 2018

@dongcarl dongcarl force-pushed the dongcarl:banman branch Nov 1, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 15, 2019

We want to have this in for 0.18.0, there's not that much time anymore.
This has had a fair amount of review.
Let's rebase, address the nits, do a last review round, and merge it.

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

@laanwj Yes. I will make it happen this week.

@dongcarl dongcarl force-pushed the dongcarl:banman branch Jan 15, 2019

@dongcarl

This comment has been minimized.

Copy link
Contributor Author

commented Jan 15, 2019

Rebased and addressed nits.

@DrahtBot DrahtBot removed the Needs rebase label Jan 15, 2019

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

Thank you @dongkarl!

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

utACK cb1ee04b6a7d56834a6447711ca5500fc8bd83cd

Original PR had utACKs by me, @ryanofsky, @jimpo. Please re-review so that we can merge this!

theuni added some commits Oct 4, 2017

net: Break disconnecting out of Ban()
These are separate events which need to be carried out by separate subsystems.

This also cleans up some whitespace and tabs in qt to avoid getting flagged by
the linter.

Current behavior is preserved.
src/net_processing.cpp Outdated
// Disconnect but don't ban _this_ local node
LogPrintf("Warning: disconnecting but not banning local peer %s!\n", pnode->addr.ToString());
} else {
// Disconnect and ban all nodes sharing the address
connman->Ban(pnode->addr, BanReasonNodeMisbehaving);
}

This comment has been minimized.

Copy link
@promag

promag Jan 16, 2019

Member

Commit b4ca7e2

Discussed with @dongcarl about not changing behavior in this commit, the conclusion is to call conman->DisconnectNode(pnode->addr) here and add pnode->fDisconnect = true; above.

@dongcarl dongcarl force-pushed the dongcarl:banman branch Jan 16, 2019

@Sjors

Sjors approved these changes Jan 16, 2019

Copy link
Member

left a comment

tACK 95b495b9697502b2b478dda8cece0ae9ffb6818f

@ryanofsky wrote:

is there a description of what high level goal of this PR is? Is it cleanup geared towards making code maintenance easier? [...]

I think reducing the 3000 lines in net.cpp by 200 is almost worth it by itself :-)

From the original description in #11457:

This also makes testing easier as different implementations can be dropped in.

So this is a step towards being able to mock network code and e.g. test banning behavior for a large number of simulated nodes.

LOCK(cs_vNodes);
for (CNode* pnode : vNodes) {
if (subnet.Match(pnode->addr)) {
pnode->fDisconnect = true;

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 16, 2019

Member

Maybe add a comment to std::atomic_bool fDisconnect in net.h that setting this to true will cause the node to be disconnected the next time DisconnectNodes() runs.

This comment has been minimized.

Copy link
@dongcarl

dongcarl Jan 16, 2019

Author Contributor

I think this belongs in another PR, but happy to do that PR :-)

src/net.cpp Outdated
// Dump addresses to peers.dat and banlist.dat every 15 minutes (900s)
#define DUMP_ADDRESSES_INTERVAL 900
// Dump addresses to peers.dat every 15 minutes (900s)
static constexpr int DUMP_PEERS_INTERVAL = 60 * 15;

This comment has been minimized.

Copy link
@Sjors

Sjors Jan 16, 2019

Member

Nit: 15 * 60 to be consistent with other _INTERVAL constants.

This comment has been minimized.

Copy link
@dongcarl

dongcarl Jan 16, 2019

Author Contributor

Done!

@@ -120,24 +120,24 @@ class NodeImpl : public Node
}
bool getBanned(banmap_t& banmap) override
{
if (g_connman) {
g_connman->GetBanned(banmap);
if (g_banman) {

This comment has been minimized.

Copy link
@Sjors

theuni and others added some commits Oct 5, 2017

banman: create and split out banman
Some say he has always been.
banman: pass the banfile path in
There's no need to hard-code the path here. Passing it in means that there are
no ordering concerns wrt establishing the datadir.
banman: pass in default ban time as a parameter
Removes the dependency on arg parsing.
scripted-diff: batch-rename BanMan members
-BEGIN VERIFY SCRIPT-
sed -i "s/clientInterface/m_client_interface/g" src/banman.h src/banman.cpp
sed -i "s/setBannedIsDirty/m_is_dirty/g" src/banman.h src/banman.cpp
sed -i "s/cs_setBanned/m_cs_banned/g" src/banman.h src/banman.cpp
sed -i "s/setBanned/m_banned/g" src/banman.h src/banman.cpp
-END VERIFY SCRIPT-
banman: add thread annotations and mark members const where possible
Also remove misleading comment. ClearBanned is used by rpc as well.
banman: reformulate nBanUtil calculation
Avoid reassigning parameters.
scripted-diff: batch-recase BanMan variables
-BEGIN VERIFY SCRIPT-
sed -i "s/banMap/banmap/g" src/banman.h src/banman.cpp
sed -i "s/netAddr/net_addr/g" src/banman.h src/banman.cpp
sed -i "s/sinceUnixEpoch/since_unix_epoch/g" src/banman.h src/banman.cpp
sed -i "s/bantimeoffset/ban_time_offset/g" src/banman.h src/banman.cpp
sed -i "s/subNet/sub_net/g" src/banman.h src/banman.cpp
sed -i "s/banReason/ban_reason/g" src/banman.h src/banman.cpp
sed -i "s/notifyUI/notify_ui/g" src/banman.h src/banman.cpp
sed -i "s/banEntry/ban_entry/g" src/banman.h src/banman.cpp
sed -i "s/nStart/n_start/g" src/banman.h src/banman.cpp
-END VERIFY SCRIPT-

@dongcarl dongcarl force-pushed the dongcarl:banman branch to 18185b5 Jan 16, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 16, 2019

re-tACK 18185b5

laanwj added a commit that referenced this pull request Jan 20, 2019

Merge #15194: Add comment describing fDisconnect behavior
5b4283c Add comment describing fDisconnect behavior (Carl Dong)

Pull request description:

  Motivated by @Sjors here: #14605 (comment)

Tree-SHA512: 8fc52eb4d3b5651c19c49b47fad75e8fb939cf524ada647e88d8d5aad7726052d94e500c1ebdb2a41b67bc4669ee61ff151a5cff81a52c68c900da562ef21751
@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

re-utACK 18185b5

@laanwj laanwj merged commit 18185b5 into bitcoin:master Jan 21, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 21, 2019

Merge #14605: Return of the Banman
18185b5 scripted-diff: batch-recase BanMan variables (Carl Dong)
c2e04d3 banman: Add, use CBanEntry ctor that takes ban reason (Carl Dong)
1ffa4ce banman: reformulate nBanUtil calculation (Carl Dong)
daae598 banman: add thread annotations and mark members const where possible (Cory Fields)
84fc3fb scripted-diff: batch-rename BanMan members (Cory Fields)
af3503d net: move BanMan to its own files (Cory Fields)
d0469b2 banman: pass in default ban time as a parameter (Cory Fields)
2e56702 banman: pass the banfile path in (Cory Fields)
4c0d961 banman: create and split out banman (Cory Fields)
83c1ea2 net: split up addresses/ban dumps in preparation for moving them (Cory Fields)
136bd79 tests: remove member connman/peerLogic in TestingSetup (Cory Fields)
7cc2b9f net: Break disconnecting out of Ban() (Cory Fields)

Pull request description:

  **Old English à la Beowulf**
  ```
  Banman wæs bréme    --blaéd wíde sprang--
  Connmanes eafera    Coreum in.
  aéglaéca            léodum forstandan
  Swá bealdode        bearn Connmanes
  guma gúðum cúð      gódum daédum·
  dréah æfter dóme·   nealles druncne slóg
  ```

  **Modern English Translation**
  ```
  Banman was famed              --his renown spread wide--
  Conman's hier,                in Core-land.
  against the evil creature     defend the people
  Thus he was bold,             the son of Connman
  man famed in war,             for good deeds;
  he led his life for glory,    never, having drunk, slew
  ```

  --

  With @theuni's blessing, here is Banman, rebased. Original PR: #11457

  --

  Followup PRs:
  1. Give `CNode` a `Disconnect` method ([source](#14605 (comment)))
  2. Add a comment to `std::atomic_bool fDisconnect` in `net.h` that setting this to true will cause the node to be disconnected the next time `DisconnectNodes()` runs ([source](#14605 (comment)))

Tree-SHA512: 9c207edbf577415c22c9811113e393322d936a843d4ff265186728152a67c057779ac4d4f27b895de9729f7a53e870f828b9ebc8bcdab757520c2aebe1e9be35

@fanquake fanquake moved this from In progress to Done in P2P refactor Jan 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.