Skip to content

Commit

Permalink
Fix issue when theres nothing new to learn from a peer
Browse files Browse the repository at this point in the history
When processing epoch ids it could happen that by the time we requested the epoch ids
and the time we received those we no longer have anything new to learn from the peer,
so we need to emit the peer as good.
This caused the peer to hang within the macro-sync process, increasing the time to consensus
  • Loading branch information
viquezclaudio authored and hrxi committed Feb 21, 2025
1 parent 676bda8 commit eaf5bc6
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 7 deletions.
15 changes: 11 additions & 4 deletions consensus/src/sync/light/sync_requests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::{
sync::{EpochIds, PeerMacroRequests},
LightMacroSync,
},
syncer::MacroSync,
syncer::{MacroSync, MacroSyncReturn},
},
};

Expand Down Expand Up @@ -180,7 +180,7 @@ impl<TNetwork: Network> LightMacroSync<TNetwork> {
pub(crate) fn request_macro_headers(
&mut self,
mut epoch_ids: EpochIds<TNetwork::PeerId>,
) -> Option<TNetwork::PeerId> {
) -> Option<MacroSyncReturn<TNetwork::PeerId>> {
// Read our current blockchain state.
let (our_epoch_id, our_epoch_number, our_block_number) = {
let blockchain = self.blockchain.read();
Expand All @@ -204,7 +204,7 @@ impl<TNetwork: Network> LightMacroSync<TNetwork> {
peer = %epoch_ids.sender,
"Peer is behind"
);
return Some(epoch_ids.sender);
return Some(MacroSyncReturn::Outdated(epoch_ids.sender));
} else {
// Check that the epoch_id sent by the peer at our current epoch number corresponds to
// our accepted state. If it doesn't, the peer is on a "permanent" fork, so we ban it.
Expand All @@ -219,7 +219,7 @@ impl<TNetwork: Network> LightMacroSync<TNetwork> {
peer = %epoch_ids.sender,
"Peer is on a different chain"
);
return Some(epoch_ids.sender);
return Some(MacroSyncReturn::Outdated(epoch_ids.sender));
}

epoch_ids.ids = epoch_ids
Expand All @@ -236,6 +236,13 @@ impl<TNetwork: Network> LightMacroSync<TNetwork> {
}
}

// After discarding all epoch_ids & checkpoint prior to our current state
// It could happen that we don't have anything new to learn from this peer
if epoch_ids.ids.is_empty() && epoch_ids.checkpoint.is_none() {
log::debug!("Nothing new to learn from this peer, emit it as good");
return Some(MacroSyncReturn::Good(epoch_ids.sender));
}

let mut peer_requests = PeerMacroRequests::new();

log::trace!(%epoch_ids.sender,
Expand Down
23 changes: 20 additions & 3 deletions consensus/src/sync/light/sync_stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,26 @@ impl<TNetwork: Network> LightMacroSync<TNetwork> {
}
}

// If the macro header process deems a peer useless, it is returned here and we emit it.
if let Some(agent) = self.request_macro_headers(epoch_ids) {
return Poll::Ready(Some(MacroSyncReturn::Outdated(agent)));
// Request the macro headers from the peer or emit it accordingly
if let Some(macro_sync_return) = self.request_macro_headers(epoch_ids) {
match macro_sync_return {
MacroSyncReturn::Good(peer_id) => {
// We are synced with this peer.
debug!(?peer_id, "Finished macro syncing with peer");

if self.blockchain.read().can_enforce_validity_window() {
// We can enforce the validity window, so we are done.
return Poll::Ready(Some(MacroSyncReturn::Good(peer_id)));
} else {
#[cfg(feature = "full")]
self.start_validity_synchronization(peer_id);
}
}
_ => {
// Propagate any other sync result (outdated is the only one that can be emitted by the macro headers process)
return Poll::Ready(Some(macro_sync_return));
}
}
}
}

Expand Down

0 comments on commit eaf5bc6

Please sign in to comment.