Skip to content

Commit 1debbbf

Browse files
committed
Update comment
1 parent f77bc24 commit 1debbbf

File tree

3 files changed

+12
-15
lines changed

3 files changed

+12
-15
lines changed

beacon_node/network/src/sync/backfill_sync/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -858,7 +858,7 @@ impl<T: BeaconChainTypes> BackFillSync<T> {
858858
Err(e) => match e {
859859
RpcRequestSendError::NoPeer(no_peer) => {
860860
// If we are here the chain has no more synced peers
861-
info!(self.log, "Backfill sync paused"; "reason" => ?no_peer);
861+
info!(self.log, "Backfill sync paused"; "reason" => format!("insufficient_synced_peers({no_peer:?})"));
862862
self.set_state(BackFillState::Paused);
863863
return Err(BackFillError::Paused);
864864
}

beacon_node/network/src/sync/network_context.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -420,7 +420,9 @@ impl<T: BeaconChainTypes> SyncNetworkContext<T> {
420420
.min()
421421
.map(|(_, _, _, peer)| *peer)
422422
else {
423-
// TODO(das): is it safe to error here?
423+
// Backfill and forward sync handle this condition gracefully.
424+
// - Backfill sync: will pause waiting for more peers to join
425+
// - Forward sync: can never happen as the chain is dropped when removing the last peer.
424426
return Err(RpcRequestSendError::NoPeer(NoPeerError::BlockPeer));
425427
};
426428

@@ -530,9 +532,7 @@ impl<T: BeaconChainTypes> SyncNetworkContext<T> {
530532
.min()
531533
.map(|(_, _, peer)| *peer)
532534
else {
533-
// TODO(das): is it safe to error here?
534535
// TODO(das): this will be pretty bad UX. To improve we should:
535-
// - Attempt to fetch custody requests first, before requesting blocks
536536
// - Handle the no peers case gracefully, maybe add some timeout and give a few
537537
// minutes / seconds to the peer manager to locate peers on this subnet before
538538
// abandoing progress on the chain completely.

beacon_node/network/src/sync/range_sync/chain.rs

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -910,17 +910,14 @@ impl<T: BeaconChainTypes> SyncingChain<T> {
910910
return Ok(KeepChain);
911911
}
912912
Err(e) => match e {
913-
RpcRequestSendError::NoPeer(no_peer) => {
914-
// Not possible to reach this condition with NoPeer::BlockPeer. For this
915-
// case the chain should have 0 peers and would be dropped already
916-
debug!(self.log, "Error sending batch no peers"; "epoch" => batch_id, &batch, "no_peer" => ?no_peer);
917-
// Set batch in stale state
918-
// What to return here?
919-
// Not necessary to return a `RemoveChain::EmptyPeerPool` here. If the
920-
// chain actually has 0 peers it will be removed on the remove_peer call
921-
return Ok(KeepChain);
922-
}
923-
RpcRequestSendError::InternalError(e) => {
913+
// TODO(das): Handle the NoPeer case explicitly and don't drop the batch. For
914+
// sync to work properly it must be okay to have "stalled" batches in
915+
// AwaitingDownload state. Currently it will error with invalid state if
916+
// that happens. Sync manager must periodicatlly prune stalled batches like
917+
// we do for lookup sync. Then we can deprecate the redundant
918+
// `good_peers_on_sampling_subnets` checks.
919+
e
920+
@ (RpcRequestSendError::NoPeer(_) | RpcRequestSendError::InternalError(_)) => {
924921
// NOTE: under normal conditions this shouldn't happen but we handle it anyway
925922
warn!(self.log, "Could not send batch request";
926923
"batch_id" => batch_id, "error" => ?e, &batch);

0 commit comments

Comments
 (0)