Return of the Banman #14605
Merged
Return of the Banman #14605
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
This was referenced Oct 30, 2018
refactor: make checkqueue manage the threads by itself (also removed some boost dependencies)
#14464
Closed
maflcko
reviewed
Oct 30, 2018
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) { |
There was a problem hiding this comment.
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)) | |||
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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; |
dongcarl
force-pushed
the
banman
branch
2 times, most recently
from
973d78f
to
283dc16
Compare
|
thank you for picking this up! |
|
No merge conflicts, but it does need rebase [#14555]; current build fails wiith /.../bitcoin/src/banman.cpp:10:10: fatal error: util.h: No such file or directory
#include <util.h>
^~~~~~~~
compilation terminated. |
Sjors
approved these changes
Jan 16, 2019
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Nit: 15 * 60 to be consistent with other _INTERVAL constants.
| @@ -120,24 +120,24 @@ class NodeImpl : public Node | |||
| } | |||
| bool getBanned(banmap_t& banmap) override | |||
| { | |||
| if (g_connman) { | |||
| g_connman->GetBanned(banmap); | |||
| if (g_banman) { | |||
Some say he has always been.
There's no need to hard-code the path here. Passing it in means that there are no ordering concerns wrt establishing the datadir.
Removes the dependency on arg parsing.
-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-
Also remove misleading comment. ClearBanned is used by rpc as well.
Avoid reassigning parameters.
-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-
|
re-tACK 18185b5 |
laanwj
added a commit
that referenced
this pull request
Jan 20, 2019
5b4283c Add comment describing fDisconnect behavior (Carl Dong) Pull request description: Motivated by @Sjors here: #14605 (comment) Tree-SHA512: 8fc52eb4d3b5651c19c49b47fad75e8fb939cf524ada647e88d8d5aad7726052d94e500c1ebdb2a41b67bc4669ee61ff151a5cff81a52c68c900da562ef21751
|
re-utACK 18185b5 |
laanwj
added a commit
that referenced
this pull request
Jan 21, 2019
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
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Jun 9, 2020
Summary: 5b4283cb81b5d3023b9868d121b22b1f387a50ca Add comment describing fDisconnect behavior (Carl Dong) Pull request description: Motivated by @Sjors here: bitcoin/bitcoin#14605 (comment) Tree-SHA512: 8fc52eb4d3b5651c19c49b47fad75e8fb939cf524ada647e88d8d5aad7726052d94e500c1ebdb2a41b67bc4669ee61ff151a5cff81a52c68c900da562ef21751 Backport of core PR15194. Test Plan: ninja Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D6474
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 18, 2021
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: bitcoin#11457 -- Followup PRs: 1. Give `CNode` a `Disconnect` method ([source](bitcoin#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](bitcoin#14605 (comment))) Tree-SHA512: 9c207edbf577415c22c9811113e393322d936a843d4ff265186728152a67c057779ac4d4f27b895de9729f7a53e870f828b9ebc8bcdab757520c2aebe1e9be35
PastaPastaPasta
pushed a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 18, 2021
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: bitcoin#11457 -- Followup PRs: 1. Give `CNode` a `Disconnect` method ([source](bitcoin#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](bitcoin#14605 (comment))) Tree-SHA512: 9c207edbf577415c22c9811113e393322d936a843d4ff265186728152a67c057779ac4d4f27b895de9729f7a53e870f828b9ebc8bcdab757520c2aebe1e9be35
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Aug 21, 2021
5b4283c Add comment describing fDisconnect behavior (Carl Dong) Pull request description: Motivated by @Sjors here: bitcoin#14605 (comment) Tree-SHA512: 8fc52eb4d3b5651c19c49b47fad75e8fb939cf524ada647e88d8d5aad7726052d94e500c1ebdb2a41b67bc4669ee61ff151a5cff81a52c68c900da562ef21751
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Aug 23, 2021
5b4283c Add comment describing fDisconnect behavior (Carl Dong) Pull request description: Motivated by @Sjors here: bitcoin#14605 (comment) Tree-SHA512: 8fc52eb4d3b5651c19c49b47fad75e8fb939cf524ada647e88d8d5aad7726052d94e500c1ebdb2a41b67bc4669ee61ff151a5cff81a52c68c900da562ef21751
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Aug 24, 2021
5b4283c Add comment describing fDisconnect behavior (Carl Dong) Pull request description: Motivated by @Sjors here: bitcoin#14605 (comment) Tree-SHA512: 8fc52eb4d3b5651c19c49b47fad75e8fb939cf524ada647e88d8d5aad7726052d94e500c1ebdb2a41b67bc4669ee61ff151a5cff81a52c68c900da562ef21751
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Aug 24, 2021
5b4283c Add comment describing fDisconnect behavior (Carl Dong) Pull request description: Motivated by @Sjors here: bitcoin#14605 (comment) Tree-SHA512: 8fc52eb4d3b5651c19c49b47fad75e8fb939cf524ada647e88d8d5aad7726052d94e500c1ebdb2a41b67bc4669ee61ff151a5cff81a52c68c900da562ef21751
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Aug 24, 2021
5b4283c Add comment describing fDisconnect behavior (Carl Dong) Pull request description: Motivated by @Sjors here: bitcoin#14605 (comment) Tree-SHA512: 8fc52eb4d3b5651c19c49b47fad75e8fb939cf524ada647e88d8d5aad7726052d94e500c1ebdb2a41b67bc4669ee61ff151a5cff81a52c68c900da562ef21751
UdjinM6
pushed a commit
to UdjinM6/dash
that referenced
this pull request
Aug 24, 2021
5b4283c Add comment describing fDisconnect behavior (Carl Dong) Pull request description: Motivated by @Sjors here: bitcoin#14605 (comment) Tree-SHA512: 8fc52eb4d3b5651c19c49b47fad75e8fb939cf524ada647e88d8d5aad7726052d94e500c1ebdb2a41b67bc4669ee61ff151a5cff81a52c68c900da562ef21751
Munkybooty
pushed a commit
to Munkybooty/dash
that referenced
this pull request
Aug 24, 2021
5b4283c Add comment describing fDisconnect behavior (Carl Dong) Pull request description: Motivated by @Sjors here: bitcoin#14605 (comment) Tree-SHA512: 8fc52eb4d3b5651c19c49b47fad75e8fb939cf524ada647e88d8d5aad7726052d94e500c1ebdb2a41b67bc4669ee61ff151a5cff81a52c68c900da562ef21751
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Old English à la Beowulf
Modern English Translation
--
With @theuni's blessing, here is Banman, rebased. Original PR: #11457
--
Followup PRs:
CNodeaDisconnectmethod (source)std::atomic_bool fDisconnectinnet.hthat setting this to true will cause the node to be disconnected the next timeDisconnectNodes()runs (source)