-
Notifications
You must be signed in to change notification settings - Fork 147
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
fix(p2p): validate chain exchange responses for block headers #4454
Conversation
src/chain_sync/network_context.rs
Outdated
|tipsets: &Vec<Arc<Tipset>>| { | ||
if let Some(start) = tipsets.first() { | ||
if start.key() != tsk { | ||
tracing::warn!(epoch=%start.epoch(), expected=%tsk, actual=%start.key(), "start tipset key mismatch"); | ||
return false; | ||
} | ||
for (ts, pts) in tipsets.iter().zip(tipsets.iter().skip(1)) { | ||
if ts.parents() != pts.key() { | ||
tracing::warn!(epoch=%ts.epoch(), expected_parent=%pts.key(), actual_parent=%ts.parents(), "invalid chain"); | ||
return false; | ||
} | ||
} | ||
true | ||
} else { | ||
tracing::warn!(%count, "invalid empty chain_exchange_headers response"); | ||
false | ||
} | ||
}, |
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.
Would it make sense to extract this to a named method a write a unit test to check and document the behaviour? It also might prevent future regressions.
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.
fixed
…hain_exchange_headers
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'm still waiting for a unit test but manage my expectations. No bonusly to incentivize one. :(
@LesnyRumcajs unit tests added! |
@@ -562,4 +597,37 @@ mod tests { | |||
assert_eq!(batch.get_ok().await, None); | |||
assert!(exceeded.load(Ordering::Relaxed)); | |||
} | |||
|
|||
#[test] | |||
#[allow(unused_variables)] |
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.
What variables are unused?
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.
genesis_header
, first_header
etc.
c52a1c5
to
0acd8e2
Compare
src/libp2p/peer_manager.rs
Outdated
} | ||
let cost = if info.successes + info.failures > 0 { |
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.
wibble: this was changed to if info.successes > 0
to address #4376 (comment)
However, this unexpectedly changes the logic to favor peers that never succeed.
The original code does not panic but returns inf
fn main() {
let x = 100.0 + f64::from(1) / f64::from(0);
println!("{x}");
}
// Output: inf
…hain_exchange_headers
…hain_exchange_headers
Summary of changes
Part of #4441 investigation
Confirmed that
ChainExchange
failures should be in the console output or log. Theoretically, there is a chance that a remote peer always returns a successful empty response and leads the downloading code to a dead loop with no errors.This PR tries to mitigate the issue by adding validation logic to the chain exchange response
Changes introduced in this pull request:
NonEmptyU64
instead ofu64
forchain_exchange
request_len
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist