Skip to content

fix(sync): validate discovery hash responses #318

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.

Sign up for GitHub

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

Open
evan-forbes wants to merge 1 commit into
base: ironwood-main
Choose a base branch
from
Open

fix(sync): validate discovery hash responses #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 113 additions & 51 deletions zebrad/src/components/sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1266,8 +1266,9 @@ where
);

let tip_network = self.tip_network.clone();
let state = self.state.clone();
let tips = std::mem::take(&mut self.prospective_tips);
extend = Some(Box::pin(Self::build_extend(tip_network, tips)));
extend = Some(Box::pin(Self::build_extend(tip_network, state, tips)));
}

// Dispatch from the reserve while we're below the lookahead limit.
Expand Down Expand Up @@ -1398,6 +1399,101 @@ where
Ok(())
}

async fn first_unknown_in_find_blocks_response(
&mut self,
hashes: &[block::Hash],
) -> Result<Option<usize>, Report> {
let mut seen = HashSet::new();
let mut first_unknown = None;

for (index, &hash) in hashes.iter().enumerate() {
if !seen.insert(hash) {
debug!(?hash, "discarding FindBlocks response with duplicate hash");
return Ok(None);
}

if first_unknown.is_none() {
if !self.state_contains(hash).await? {
first_unknown = Some(index);
}
continue;
}

if self.state_contains(hash).await? {
debug!(
?hash,
"discarding FindBlocks response with known hash in advertised suffix"
);
return Ok(None);
}
}

Ok(first_unknown)
}

async fn validate_extend_find_blocks_response<'a>(
state: &mut ZS,
hashes: &'a [block::Hash],
locator: block::Hash,
expected_next: block::Hash,
) -> Result<Option<&'a [block::Hash]>, Report> {
let matched_hashes = match hashes {
[expected_hash, _rest @ ..] if *expected_hash == expected_next => hashes,
[first_hash, expected_hash, _rest @ ..] if *expected_hash == expected_next => {
debug!(?first_hash, ?expected_next, ?locator, "unexpected first hash, but the second matches: using the hashes after the match");
&hashes[1..]
}
[] => return Ok(None),
[single_hash] => {
debug!(
?single_hash,
?expected_next,
?locator,
"discarding response containing a single unexpected hash"
);
return Ok(None);
}
[first_hash, second_hash, rest @ ..] => {
debug!(?first_hash, ?second_hash, rest_len = ?rest.len(), ?expected_next, ?locator, "discarding response that starts with two unexpected hashes");
return Ok(None);
}
};

// We use the last hash for the tip, and we want to avoid bad tips.
// So we discard the last hash before checking duplicates or known hashes.
let matched_hashes = match matched_hashes {
[] => unreachable!("matched FindBlocks response contains the expected hash"),
[rest @ .., _last] => rest,
};

let mut seen = HashSet::new();
if matched_hashes.iter().any(|hash| !seen.insert(*hash)) {
debug!(
?matched_hashes,
"discarding FindBlocks extension response with duplicate hash"
);
return Ok(None);
}

let unknown_hashes = match matched_hashes {
[expected_hash, rest @ ..] if *expected_hash == expected_next => rest,
[] => return Ok(Some(&[])),
_ => unreachable!("matched FindBlocks response starts with the expected hash"),
};

for &hash in unknown_hashes {
if Self::state_contains_service(state, hash).await? {
debug!(
?hash,
"discarding FindBlocks extension response with known hash in advertised suffix"
);
return Ok(None);
}
}

Ok(Some(unknown_hashes))
}

/// Given a block_locator list fan out request for subsequent hashes to
/// multiple peers
#[instrument(skip(self))]
Expand Down Expand Up @@ -1493,13 +1589,7 @@ where
continue;
}

let mut first_unknown = None;
for (i, &hash) in hashes.iter().enumerate() {
if !self.state_contains(hash).await? {
first_unknown = Some(i);
break;
}
}
let first_unknown = self.first_unknown_in_find_blocks_response(hashes).await?;

debug!(hashes.len = ?hashes.len(), ?first_unknown);

Expand Down Expand Up @@ -1590,6 +1680,7 @@ where
#[instrument(skip_all)]
async fn build_extend(
mut tip_network: Timeout<ZN>,
mut state: ZS,
tips: HashSet<CheckedTip>,
) -> Result<(IndexSet<block::Hash>, HashSet<CheckedTip>, usize), Report> {
let stage_start = std::time::Instant::now();
Expand Down Expand Up @@ -1628,47 +1719,15 @@ where
// zcashd sometimes appends an unrelated hash at the
// start or end of its response. Check the first hash
// against the previous response, and discard mismatches.
let unknown_hashes = match hashes.as_slice() {
[expected_hash, rest @ ..] if expected_hash == &tip.expected_next => {
rest
}
// If the first hash doesn't match, retry with the second.
[first_hash, expected_hash, rest @ ..]
if expected_hash == &tip.expected_next =>
{
debug!(?first_hash,
?tip.expected_next,
?tip.tip,
"unexpected first hash, but the second matches: using the hashes after the match");
rest
}
// We ignore these responses
[] => continue,
[single_hash] => {
debug!(?single_hash,
?tip.expected_next,
?tip.tip,
"discarding response containing a single unexpected hash");
continue;
}
[first_hash, second_hash, rest @ ..] => {
debug!(?first_hash,
?second_hash,
rest_len = ?rest.len(),
?tip.expected_next,
?tip.tip,
"discarding response that starts with two unexpected hashes");
continue;
}
};

// We use the last hash for the tip, and we want to avoid
// bad tips. So we discard the last hash. (We don't need
// to worry about missed downloads, because we will pick
// them up again in the next ExtendTips.)
let unknown_hashes = match unknown_hashes {
[] => continue,
[rest @ .., _last] => rest,
let Some(unknown_hashes) = Self::validate_extend_find_blocks_response(
&mut state,
hashes.as_slice(),
tip.tip,
tip.expected_next,
)
.await?
else {
continue;
};

let new_tip = if let Some(end) = unknown_hashes.rchunks_exact(2).next() {
Expand Down Expand Up @@ -2094,8 +2153,11 @@ where
/// Returns `true` if the hash is present in the state, and `false`
/// if the hash is not present in the state.
pub(crate) async fn state_contains(&mut self, hash: block::Hash) -> Result<bool, Report> {
match self
.state
Self::state_contains_service(&mut self.state, hash).await
}

async fn state_contains_service(state: &mut ZS, hash: block::Hash) -> Result<bool, Report> {
match state
.ready()
.await
.map_err(|e| eyre!(e))?
Expand Down
Loading