Fix rebroadcast panic #4084
Fix rebroadcast panic #4084
Conversation
keorn
added
A0-pleasereview 🤓
| for _ in 0..peers { | ||
| let peer = random.gen_range(0, len); | ||
| self.peers.values_mut().nth(peer).map(|mut peer_info| { | ||
| let peers: Vec<PeerId> = ChainSync::select_random_peers(&self.peers.keys().cloned().collect::<Vec<PeerId>>()) |
There was a problem hiding this comment.
i think Vec<_> might be more idiomatic...
gavofyork
added
A8-looksgood 🦄
| .take(3) | ||
| .collect(); | ||
| trace!(target: "sync", "Re-broadcasting transactions to random peers: {:?}", peers); | ||
| for peer in peers { |
There was a problem hiding this comment.
can avoid the allocation by moving the .into_iter().take portion to here.
There was a problem hiding this comment.
did it for tracing, will do though
|
|
| self.peers.values_mut().nth(peer).map(|mut peer_info| { | ||
| let peers: Vec<PeerId> = ChainSync::select_random_peers(&self.peers.keys().cloned().collect::<Vec<_>>()); | ||
| trace!(target: "sync", "Re-broadcasting transactions to random peers."); | ||
| for peer in peers.iter().take(3) { |
There was a problem hiding this comment.
Previous solution was sending to 0-2 peers after each block, current is sending to 3 peers after each block.
I think it would be better to keep average number the same, and perhaps to send only to one peer on each block (we can also get rid of unnecessary allocation in such case):
if !is_syncing && !enacted.is_empty() && !self.peers.is_empty() {
let peer = random::new().get_rng(0, self.peers.len());
...
}There was a problem hiding this comment.
I'm good with deterministic behaviour, just think that 3 peers at block might be a little bit too much (transactions will be rebroadcasted too often), that's why I suggested to send to only 1 peer - in such case we won't have two peers selected twice hence we can get rid of the allocation.
|
Changes Unknown when pulling 1a3c30e on fix-rebroadcast into ** on master**. |
No description provided.