-
Notifications
You must be signed in to change notification settings - Fork 1
fix: prevent sync deadlock when no peer can serve the requested header range #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: optimism
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,8 @@ use std::{ | |
| /// does](https://github.com/ethereum/go-ethereum/blob/f53ff0ff4a68ffc56004ab1d5cc244bcb64d3277/les/server_requests.go#L245). | ||
| /// All errors regarding the response cause the peer to get penalized, meaning that adversaries | ||
| /// that try to give us bodies that do not match the requested order are going to be penalized | ||
| /// and eventually disconnected. | ||
| /// and eventually disconnected. The exception is an empty response, which is the | ||
| /// protocol-correct answer of a peer that does not have the requested bodies. | ||
| pub(crate) struct BodiesRequestFuture<B: Block, C: BodiesClient<Body = B::Body>> { | ||
| client: Arc<C>, | ||
| consensus: Arc<dyn Consensus<B>>, | ||
|
|
@@ -89,7 +90,12 @@ where | |
| fn on_error(&mut self, error: DownloadError, peer_id: Option<PeerId>) { | ||
| self.metrics.increment_errors(&error); | ||
| tracing::debug!(target: "downloaders::bodies", ?peer_id, %error, "Error requesting bodies"); | ||
| if let Some(peer_id) = peer_id { | ||
| // An empty response is the protocol-correct answer of a peer that does not have the | ||
| // requested bodies and must not accrue ban-worthy reputation; the request is simply | ||
| // resubmitted. | ||
| if let Some(peer_id) = peer_id && | ||
| !matches!(error, DownloadError::EmptyResponse) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fable 5 (high): question: only the Headers stage gets stall detection — bodies (and era) keep the wait-forever behavior, so the same "no peer can serve" shape can still deadlock one stage later. In practice headers gate bodies for this incident, and peers that served the headers virtually always have the bodies — which is presumably the justification — but state it in the PR, or upstream will ask why the fix is asymmetric. |
||
| { | ||
| self.client.report_bad_message(peer_id); | ||
| } | ||
| self.submit_request( | ||
|
|
@@ -308,4 +314,29 @@ mod tests { | |
| (headers.into_iter().filter(|h| !h.is_empty()).count() as u64).div_ceil(2) | ||
| ); | ||
| } | ||
|
|
||
| /// Empty responses are the protocol-correct answer of peers that do not have the requested | ||
| /// bodies and must not be reported as bad messages while the request is retried. | ||
| #[tokio::test] | ||
| async fn empty_responses_are_not_penalized() { | ||
| // Generate some random blocks | ||
| let (headers, mut bodies) = generate_bodies(0..=19); | ||
|
|
||
| let client = Arc::new( | ||
| TestBodiesClient::default() | ||
| .with_bodies(bodies.clone()) | ||
| .with_max_batch_size(5) | ||
| .with_empty_responses(2), | ||
| ); | ||
| let fut = BodiesRequestFuture::<Block, _>::new( | ||
| client.clone(), | ||
| Arc::new(TestConsensus::default()), | ||
| BodyDownloaderMetrics::default(), | ||
| ) | ||
| .with_headers(headers.clone()); | ||
|
|
||
| assert_eq!(fut.await.unwrap(), zip_blocks(headers.iter(), &mut bodies)); | ||
| assert!(client.times_requested() > 1); | ||
| assert_eq!(client.bad_messages(), 0); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -539,7 +539,17 @@ where | |
| fn penalize_peer(&self, peer_id: Option<PeerId>, error: &DownloadError) { | ||
| // Penalize the peer for bad response | ||
| if let Some(peer_id) = peer_id { | ||
| trace!(target: "downloaders::headers", ?peer_id, %error, "Penalizing peer"); | ||
| if matches!(error, DownloadError::EmptyResponse) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Instead of adding an exception inside the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fable 5 (high): question: worth disclosing explicitly in the upstream PR text: an always-empty peer is now completely penalty-free — it can hold a peer slot forever, with only soft deprioritization (the fetcher's |
||
| // An empty response is the protocol-correct answer of a peer that does not have | ||
| // the requested range and must not accrue ban-worthy reputation: if no peer can | ||
| // serve the range yet (e.g. all peers are equally out of sync), banning honest | ||
| // peers guarantees the download can never complete. The fetcher already | ||
| // deprioritizes peers whose last response was unsatisfactory when picking a peer | ||
| // for the resubmitted request. | ||
| debug!(target: "downloaders::headers", ?peer_id, %error, "Peer unable to serve requested headers"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fable 5 (high): should-fix: with the penalty gone, nothing rate-limits the retry loop. |
||
| return | ||
| } | ||
| debug!(target: "downloaders::headers", ?peer_id, %error, "Penalizing peer"); | ||
| self.client.report_bad_message(peer_id); | ||
| } | ||
| } | ||
|
|
@@ -1251,7 +1261,15 @@ mod tests { | |
| use alloy_eips::{eip1898::BlockWithParent, BlockNumHash}; | ||
| use assert_matches::assert_matches; | ||
| use reth_consensus::test_utils::TestConsensus; | ||
| use reth_network_p2p::test_utils::TestHeadersClient; | ||
| use reth_network_p2p::{download::DownloadClient, test_utils::TestHeadersClient}; | ||
| use reth_network_peers::WithPeerId; | ||
| use std::{ | ||
| collections::VecDeque, | ||
| sync::{ | ||
| atomic::{AtomicU64, Ordering as AtomicOrdering}, | ||
| Mutex, | ||
| }, | ||
| }; | ||
|
|
||
| /// Tests that `replace_number` works the same way as `Option::replace` | ||
| #[test] | ||
|
|
@@ -1547,4 +1565,144 @@ mod tests { | |
|
|
||
| assert!(downloader.next().await.is_none()); | ||
| } | ||
|
|
||
| /// A client that serves a scripted sequence of responses, pends once the script is | ||
| /// exhausted, and counts how often it was reported for a bad message. | ||
| #[derive(Clone, Debug)] | ||
| struct ScriptedHeadersClient { | ||
| responses: Arc<Mutex<VecDeque<Vec<Header>>>>, | ||
| total_requests: Arc<AtomicU64>, | ||
| bad_messages: Arc<AtomicU64>, | ||
| } | ||
|
|
||
| impl ScriptedHeadersClient { | ||
| fn new(responses: Vec<Vec<Header>>) -> Self { | ||
| Self { | ||
| responses: Arc::new(Mutex::new(responses.into())), | ||
| total_requests: Arc::new(AtomicU64::new(0)), | ||
| bad_messages: Arc::new(AtomicU64::new(0)), | ||
| } | ||
| } | ||
|
|
||
| fn total_requests(&self) -> u64 { | ||
| self.total_requests.load(AtomicOrdering::Relaxed) | ||
| } | ||
|
|
||
| fn bad_messages(&self) -> u64 { | ||
| self.bad_messages.load(AtomicOrdering::Relaxed) | ||
| } | ||
| } | ||
|
|
||
| impl DownloadClient for ScriptedHeadersClient { | ||
| fn report_bad_message(&self, _peer_id: PeerId) { | ||
| self.bad_messages.fetch_add(1, AtomicOrdering::Relaxed); | ||
| } | ||
|
|
||
| fn num_connected_peers(&self) -> usize { | ||
| 1 | ||
| } | ||
| } | ||
|
|
||
| impl HeadersClient for ScriptedHeadersClient { | ||
| type Header = Header; | ||
| type Output = Pin<Box<dyn Future<Output = PeerRequestResult<Vec<Header>>> + Send + Sync>>; | ||
|
|
||
| fn get_headers_with_priority( | ||
| &self, | ||
| _request: HeadersRequest, | ||
| _priority: Priority, | ||
| ) -> Self::Output { | ||
| self.total_requests.fetch_add(1, AtomicOrdering::Relaxed); | ||
| let next = self.responses.lock().unwrap().pop_front(); | ||
| Box::pin(async move { | ||
| match next { | ||
| Some(headers) => Ok(WithPeerId::from((PeerId::default(), headers))), | ||
| None => std::future::pending().await, | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| /// Polls the downloader once without awaiting its next item. | ||
| async fn poll_downloader_once<S: Stream + Unpin>(downloader: &mut S) { | ||
| std::future::poll_fn(|cx| { | ||
| let _ = Pin::new(&mut *downloader).poll_next(cx); | ||
| Poll::Ready(()) | ||
| }) | ||
| .await | ||
| } | ||
|
|
||
| /// Empty responses are the protocol-correct answer of peers that do not have the requested | ||
| /// range and must not be reported as bad messages, which would otherwise ban the entire | ||
| /// honest peer set whenever no peer can serve the range yet. | ||
| #[tokio::test] | ||
| async fn empty_responses_are_not_penalized() { | ||
| reth_tracing::init_test_tracing(); | ||
|
|
||
| const EMPTY_RESPONSES: usize = 10; | ||
|
|
||
| let p3 = SealedHeader::default(); | ||
| let p2 = child_header(&p3); | ||
| let p1 = child_header(&p2); | ||
| let p0 = child_header(&p1); | ||
|
|
||
| // a valid sync target response followed by only empty responses for the range request | ||
| let mut responses = vec![vec![p0.as_ref().clone()]]; | ||
| responses.extend(std::iter::repeat_n(Vec::new(), EMPTY_RESPONSES)); | ||
| let client = Arc::new(ScriptedHeadersClient::new(responses)); | ||
|
|
||
| let mut downloader = ReverseHeadersDownloaderBuilder::default() | ||
| .stream_batch_size(3) | ||
| .request_limit(3) | ||
| .build(Arc::clone(&client), Arc::new(TestConsensus::default())); | ||
| downloader.update_local_head(p3); | ||
| downloader.update_sync_target(SyncTarget::Tip(p0.hash())); | ||
|
|
||
| // drive the downloader until every scripted empty response has been consumed and the | ||
| // failed request resubmitted | ||
| let expected_requests = 2 + EMPTY_RESPONSES as u64; | ||
| for _ in 0..100 { | ||
| if client.total_requests() >= expected_requests { | ||
| break | ||
| } | ||
| poll_downloader_once(&mut downloader).await; | ||
| } | ||
|
|
||
| assert_eq!(client.total_requests(), expected_requests); | ||
| assert_eq!(client.bad_messages(), 0); | ||
| } | ||
|
|
||
| /// A malformed response (wrong start block) is still reported as a bad message. | ||
| #[tokio::test] | ||
| async fn malformed_responses_are_penalized() { | ||
| reth_tracing::init_test_tracing(); | ||
|
|
||
| let p3 = SealedHeader::default(); | ||
| let p2 = child_header(&p3); | ||
| let p1 = child_header(&p2); | ||
| let p0 = child_header(&p1); | ||
|
|
||
| // a valid sync target response, then a response that starts at the wrong block: the | ||
| // range request asks for blocks 2 and 1 but the response starts at block 1 | ||
| let client = Arc::new(ScriptedHeadersClient::new(vec![ | ||
| vec![p0.as_ref().clone()], | ||
| vec![p2.as_ref().clone(), p3.as_ref().clone()], | ||
| ])); | ||
|
|
||
| let mut downloader = ReverseHeadersDownloaderBuilder::default() | ||
| .stream_batch_size(3) | ||
| .request_limit(3) | ||
| .build(Arc::clone(&client), Arc::new(TestConsensus::default())); | ||
| downloader.update_local_head(p3); | ||
| downloader.update_sync_target(SyncTarget::Tip(p0.hash())); | ||
|
|
||
| for _ in 0..100 { | ||
| if client.bad_messages() > 0 { | ||
| break | ||
| } | ||
| poll_downloader_once(&mut downloader).await; | ||
| } | ||
|
|
||
| assert_eq!(client.bad_messages(), 1); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,14 @@ pub enum StageError { | |
| /// Download channel closed | ||
| #[error("download channel closed")] | ||
| ChannelClosed, | ||
| /// The stage made no progress towards becoming ready for execution, e.g. because no peer | ||
| /// can currently serve the data it is waiting on. | ||
| /// | ||
| /// This error is recoverable: the pipeline retries the stage, and ends the current | ||
| /// pipeline run with no progress once the stage keeps stalling beyond the pipeline's | ||
| /// readiness timeout, returning control to the caller for re-targeting. | ||
| #[error("stage stalled while waiting to become ready: {0}")] | ||
| Stalled(String), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fable 5 (high): nit: stringly-typed payload, and the call site allocates a formatted string every 30s in the retry loop. |
||
| /// The stage encountered a database integrity error. | ||
| #[error("database integrity error occurred: {0}")] | ||
| DatabaseIntegrity(#[from] ProviderError), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this case should be handled before even calling the
on_errorfunction.