banman: Limit resources consumed by misbehaving node deprioitisation#19243
banman: Limit resources consumed by misbehaving node deprioitisation#19243luke-jr wants to merge 6 commits intobitcoin:masterfrom
Conversation
luke-jr
force-pushed
the
misbehaving_limit
branch
3 times, most recently
from
c232afc to
4574554
Compare
luke-jr
force-pushed
the
misbehaving_limit
branch
from
4574554 to
06cd6ff
Compare
luke-jr
force-pushed
the
misbehaving_limit
branch
from
06cd6ff to
885fbc7
Compare
|
Reviewed #19219 today, so with it fresh in mind will have a look at this version now. |
|
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. |
There was a problem hiding this comment.
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.
|
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). |
|
@TheBlueMatt By "current semantics", in particular I mean being able to list and remove entries, and a reliable 24 hour expiration. |
luke-jr
force-pushed
the
misbehaving_limit
branch
from
61a4854 to
9a58885
Compare
|
Also added a fix for a bug in the current code: Now manual bans take priority over misbehaving discouragement, regardless of expiry times. |
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
#19219 was merged, so this is no longer applicable |
5 participants
Potential alternative to #19219 that keeps the current semantics at the cost of more memory usage.
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).