feat: bans for bad incoming blocks #5934
Conversation
Co-authored-by: stringhandler <mikethetike@tari.com>
ghpbot-tari-project
added
P-acks_required
Test Results (Integration tests) 2 files + 2 11 suites +11 29m 15s ⏱️ + 29m 15s For more details on these failures, see this check. Results for commit fff14f0. ± Comparison against base commit 27f78de. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
I'll be honest I don't love this PR.
We're letting the ban periods be configurable per node which is okay, but then we're passing those periods around anytime we need to fetch reasons for a ban. These reasons don't actually need to know the configuration details.
On the BanReason struct, we could use an enum to define the expected ban duration. BanDuration::Short, BanDuration::Long and BanDuration::Permenant.
This makes it easy to manage in different services without creating a heavy requirement of config details. Once the BanReason reaches the ConnectivityManager, it can read the enum, and apply the correct configuration. The ConnectivityManager is the only one that needs to be able to translate that a BanDuration::Short means 60 seconds.
There was a problem hiding this comment.
It's a lot cleaner
I would personally try to go one step closer, and get the timing config into the ConnectivityManager so ban_peer_until can take the BanReason and we still don't need to destructure the duration until inside ban_peer_until
ConnectivityManager is brought up with the CommsNode spawn. So we have lots of config options at hand, and which we could pass directly to the ConnectivityManager.
At the point the destructure is managed by the ConnectivityManager it can be a simple helper function. Pass in the BanDuration enum, and receive a Duration as the return.
|
I am going to leave the config to move to this issue:#5933 |
ghpbot-tari-project
removed
the
P-reviews_required
Description
Bans peers who send bad incoming blocks.
Consolidates banning a bit more.
Motivation and Context
Created an issue to fix the ban period in the config: #5933
Fixes: #5795