libp2p for running in parallel with the current peer loop
#729
Conversation
skaunov
requested review from
Sword-Smith and
aszepieniec
and removed request for
Sword-Smith
|
I looked into adapting/replicating I guess I'd divide this into two: the bare essentials and fine approach. Minimally adapt the current punishments right now; taking the most disrupting offenses and like ban the peer immediately. And in a separate issue study Gossip-sub scoring and the role of application score in overall to finely translate all the sanctions into the scheme they offer. If this sounds good to you: are there any immediate offenses which should be included in the module from the first day? |
|
I remember |
|
dealt in 7f65b8f
|
|
It's very hard to review this when you're changing so many unrelated comments. I'm OK getting rid of I'm a bit uneasy introducing this at the same time with a hardfork (#727). I think it's too many moving parts. Would it be an option to add this new peer loop definition and then default to the old one until we're past the hardfork? |
Sorry. =( In the core it's just how I understand what's going on. It's active reading where I get over the relevant thing and make it easier for me to work with. And I hope it's also just better / for the most. No way I "hunt" for those small change; but they accrue indeed. =( Let me reiterate on this -- I'd say it's the main problem here.
If that's possible it'd be awesome to invert, and identify the parts which needs more looking into -- passing everything else earlier.
I'd say I designed it to be introduced as the secondary circle/network and it's turned on basically in one line where its task is spawned. IIRC it should be enough to make that line conditional on an option/argument and all should be working as expected. I feel there's no need to switch that in run-time though and opting for it on P.S. Like I'm having really hard time to understand where the mark ending a sentence is forgotten and where is it omitted. (And it still leaves the feeling similar to a not closed parenthesis. It would be so much easier if English grammar require the sentences to always end with a non-null mark. :sigh: |
I understand. The fact is that many of these are small details are very much up to the opinion of the individual developer. And since this project has no formalized style guide (other than what cargo fmt and clippy enforce), I'll let it be up to the individual developer when they write their code (within reason). But please don't fix comments unrelated to this PR after your personal preferences. |
|
@skaunov Do you have any instructions on how to run end-to-end tests for this new peer loop? How can I test it on my node? |
|
I was asking for an advice on this few weeks ago in the dev Tg group. I mean I'd add Rust tests but I can't think out of how to do it with this thing. Any attempt/approach to it ends for me as building a separate container network which would be doing such testing. It looks like a right thing to do, but I had not enough motivation for it yet, and also wanted to have more eyes on this first, and discuss better ideas. |
===========
the adaptation I did won't be needed then
```rust
pub(crate) fn accept_block_proposal_from(
&self,
mut peer: Multiaddr,
) -> Result<(), BlockProposalRejectError> {
if self.ignore_foreign_compositions {
Err(BlockProposalRejectError::IgnoreAllForeign)
} else if self.whitelisted_composers.is_empty() {
Ok(())
} else {
// if let Some(peer_id) = peer.pop() {
// debug_assert!(
// matches!(peer_id, libp2p::multiaddr::Protocol::P2p(_)),
// r#"@skaunov can't find a case when it can come without `PeerId` ending, but if there is one this can be remade as `if let id @ = peer.iter().last()`"#
// );
// let peer_id = peer_id.into();
// if self.whitelisted_composers.iter().any(|w| w.ends_with(&peer_id) || w.ends_with(&peer)) {Ok(())}
// else {Err(BlockProposalRejectError::NotWhiteListed)}
// } else {
// tracing::debug!("something is wrong -- this Multiaddr must not be empty; hence rejecting the operation");
// // ~~seems like going with `anyhow` here to bubble up a deeper internal error which isn't critical~~
// Err(BlockProposalRejectError::NotWhiteListed)
// }
let (peer_id, peer) =
super::super::loops::main_loop::p2p::tmp_utils_multiaddr::peerid_split(&mut peer);
if self.whitelisted_composers.iter().any(|w| w.ends_with(peer)) {
Ok(())
} else {
if let Some(peer_id) = peer_id {
let peer_id = multiaddr::Protocol::P2p(peer_id).into();
if self
.whitelisted_composers
.iter()
.any(|w| w.ends_with(&peer_id))
{
return Ok(());
}
}
Err(BlockProposalRejectError::NotWhiteListed)
}
}
}
```
2 participants
continuation of #701 from a more recent commit
it has a lot of comments to delete when pass the reviews -- I left those to better convey the implementation decisions
Also given the size of this change it makes sense to split/separate some things which you're OK with to merge earlier. This should ease all the aspects of handling it; I just had some hard time guessing which parts could get a go --- let me listen from you first! 🎩
todos
I was surprised how many todos the code held. I thought just to invent some other tag for my current todos, but I truly think the system is beyond the point when this was acceptable. Some of them 3 y.o.! My point is they should be converted to the tracker issues and replaced by the reference to the issue. I'll bring them back if I'm wrong on this. But I hope after your first natural rejection reaction you would take less than 2 hours to map the issues to your todos. My first contribution was a small issue from the tracker, not a todo from the code, as an example.
And ofc I'm converting all the todos I've added around into the issues when the review is passed! ✏️