Skip to content
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 race condition in chain exchange #2934

Closed
lemmih opened this issue Jun 6, 2023 · 3 comments · Fixed by #2959
Closed

Fix race condition in chain exchange #2934

lemmih opened this issue Jun 6, 2023 · 3 comments · Fixed by #2959
Assignees

Comments

@lemmih
Copy link
Contributor

lemmih commented Jun 6, 2023

Issue summary

The handle_chain_exchange_request function downloads blocks (aka tipsets) from peers. It does this by sending the same request to multiple peers and accepting the first valid response. However, a concurrency bug is triggered if the number of peers is low.

As seen in the code snippet below, the function waits for a valid result OR all the tasks to finish. If all the tasks finish, it assumes no valid responses and returns an error. This logic needs to be corrected: We should only fail when we've received invalid responses from all peers.

Other information and links

tokio::select! {
result = result_rx.recv_async() => {
tasks.abort_all();
log::debug!("Succeed: handle_chain_exchange_request");
result.map_err(|e| e.to_string())?
},
_ = wait_all(&mut tasks) => return Err(make_failure_message()),
}

@lemmih
Copy link
Contributor Author

lemmih commented Jun 6, 2023

@elmattic Do you want to have a look at this one?

@elmattic
Copy link
Contributor

elmattic commented Jun 6, 2023

Sure.

@elmattic elmattic self-assigned this Jun 6, 2023
@LesnyRumcajs LesnyRumcajs mentioned this issue Jun 6, 2023
12 tasks
@hanabi1224
Copy link
Contributor

As a side note, the error message in #2755 looks related

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants