banman: Limit resources consumed by misbehaving node deprioitisation

luke-jr commented at 7:22 am on June 11, 2020: member

Potential alternative to #19219 that keeps the current semantics at the cost of more memory usage.

Single-address bans are moved to a dedicated std::map to avoid iteration.
Misbehaving “ban” entries are limited to 50k using a circular buffer.
Bugfix: Manual bans can override misbehaving discouragement, even for a shorter period of time.

I’m not sure if the tradeoffs justify this approach.

Looking for concept ACK/NACK, as well as code review (will probably use this for at least Knots 0.20.0 temporarily).

fanquake added the label P2P on Jun 11, 2020

luke-jr force-pushed on Jun 11, 2020

banman: Use simple std::map lookup for single address bans e0baed63b6

luke-jr force-pushed on Jun 11, 2020

banman: Support a limit on number of misbehaving "bans" (default 50k) 45a328776f

banman: Rename m_banned to m_banned_subnets for clarity 29189085df

QA: Tests for banman misbehavinglimit 885fbc7ecd

luke-jr force-pushed on Jun 11, 2020

jonatack commented at 2:49 pm on June 11, 2020: member

Reviewed #19219 today, so with it fresh in mind will have a look at this version now.

in src/init.cpp:446 in 885fbc7ecd outdated

442@@ -443,6 +443,7 @@ void SetupServerArgs(NodeContext& node)
443     gArgs.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
444     gArgs.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by peers forward or backward by this amount. (default: %u seconds)", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
445     gArgs.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target (in MiB per 24h). Limit does not apply to peers with 'noban' permission. 0 = no limit (default: %d)", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
446+    gArgs.AddArg("-misbehavinglimit=<n>", strprintf("Maxmium number of misbehaving peers to deprioritise (default: %s)", DEFAULT_MISBEHAVING_LIMIT), ArgsManager::ALLOW_INT, OptionsCategory::CONNECTION);

jonatack commented at 3:36 pm on June 11, 2020:

45a328776 nit: Maximum

DrahtBot commented at 4:25 pm on June 11, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19219 (Replace automatic bans with discouragement filter by sipa)
#19191 (net: Extract download permission from noban by MarcoFalke)
#16878 (Fix non-deterministic coverage of test DoS_mapOrphans by davereikher)

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.

jonatack commented at 4:32 pm on June 11, 2020: member

Concept and light code review ACK with builds/tests ran. Good to see a unit test. This maintains the current interface/semantics with a bit more code complexity, more memory (4 MiB or more vs 0.5 MiB for the rolling bloom filter), but can erase from the set with fairly fast O(log(n)) lookups.

TheBlueMatt commented at 5:23 pm on June 11, 2020: member

Concept NACK - #19219 is essentially the current semantics with a node count limit, I don’t see any reason to use more memory for a similar thing, and I dont think it makes sense to go back to treating them as an actual ban (though I dont think this does that, either, just noting).

luke-jr commented at 6:57 pm on June 11, 2020: member

@TheBlueMatt By “current semantics”, in particular I mean being able to list and remove entries, and a reliable 24 hour expiration.

fixup! banman: Support a limit on number of misbehaving "bans" (default 50k) 9a58885df2

luke-jr force-pushed on Jun 11, 2020

Bugfix: banman: Allow manually banning misbehaving nodes, even for a shorter duration 923862aa8b

luke-jr commented at 9:09 pm on June 11, 2020: member

Also added a fix for a bug in the current code: Now manual bans take priority over misbehaving discouragement, regardless of expiry times.

DrahtBot commented at 6:29 pm on July 7, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Jul 7, 2020

luke-jr commented at 10:31 pm on August 19, 2020: member

#19219 was merged, so this is no longer applicable

luke-jr closed this on Aug 19, 2020

DrahtBot locked this on Feb 15, 2022

banman: Limit resources consumed by misbehaving node deprioitisation #19243

Conflicts