feat: ban libp2p peers on genesis hash mismatch #2184
Conversation
hanabi1224
requested review from
elmattic,
lemmih,
LesnyRumcajs,
jdjaustin,
tyshko5,
creativcoder and
sudo-shashank
as code owners
There was a problem hiding this comment.
In general, it looks great. Do we ban peers permanently or only for some time? Potentially some peers may "correct" their behaviour and we should perhaps bring them back. I could imagine storing those in some kind of TLRU cache. Not for this PR of course, but maybe as a potential improvement in the future.
node/forest_libp2p/src/service.rs
Outdated
| @@ -158,6 +158,7 @@ pub struct Libp2pService<DB, P: StoreParams> { | |||
| network_receiver_out: flume::Receiver<NetworkEvent>, | |||
| network_sender_out: flume::Sender<NetworkEvent>, | |||
| network_name: String, | |||
| genesis_hash: Cid, | |||
There was a problem hiding this comment.
maybe let's use genesis_cid, Cid is not precisely a hash (it does contain it, though)
node/forest_libp2p/src/service.rs
Outdated
| @@ -402,9 +406,10 @@ async fn handle_network_message<P: StoreParams>( | |||
|
|
|||
| #[allow(clippy::too_many_arguments)] | |||
| async fn handle_forest_behaviour_event<DB, P: StoreParams>( | |||
| bh_mut: &mut ForestBehaviour<P>, | |||
| st_mut: &mut Swarm<ForestBehaviour<P>>, | |||
There was a problem hiding this comment.
do we need those postfixes? For example just swarm, it's short enough.
There was a problem hiding this comment.
Fixed. That's a better name
node/forest_libp2p/src/service.rs
Outdated
| @@ -425,6 +430,7 @@ async fn handle_forest_behaviour_event<DB, P: StoreParams>( | |||
| ) where | |||
| DB: Blockstore + Store + BitswapStore<Params = P> + Clone + Sync + Send + 'static, | |||
| { | |||
| let bh_mut = st_mut.behaviour_mut(); | |||
| @@ -644,7 +658,13 @@ async fn handle_forest_behaviour_event<DB, P: StoreParams>( | |||
| ); | |||
| } | |||
| Err(err) => { | |||
| warn!("{err}:{}", ping_event.peer.to_base58()); | |||
| let err = err.to_string(); | |||
There was a problem hiding this comment.
I haven't confirmed but I guess without this line, warn! will make an extra allocation by calling err.to_string in Display(we still need err.to_string().contains below I guess`) . Not a big deal tho I can change it if you prefer fewer lines
There was a problem hiding this comment.
No, it's okay; I didn't notice the 2nd usage.
Looking at the source code, it's banned for the entire lifetime of the swarm instance.
Maybe we could propose some backward-compatible API changes to the upstream, e.g. adding |
I guess we could start by asking if there's a need for this, seems to me it may be pretty niche and it can be handled by the client code anyway (though with some overhead of an another hashmap). |
@LesnyRumcajs Yes, we need to achieve this with our code. libp2p/rust-libp2p#3140 |
|
@hanabi1224 thanks for looking into this! |
Summary of changes
Changes introduced in this pull request:
Based on #2154 Should let #2154 go first
Looking at the calibnet log, epoch too large errors during tipset validation are completely gone
Reference issue to close (if applicable)
Closes #2189
Other information and links