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

Chain syncing verification fixes #503

Merged
merged 65 commits into from
Sep 4, 2020
Merged

Chain syncing verification fixes #503

merged 65 commits into from
Sep 4, 2020

Conversation

ec2
Copy link
Member

@ec2 ec2 commented Jun 16, 2020

Summary of changes
Changes introduced in this pull request:

  • Fixes block verification and response handling

Reference issue to close (if applicable)

Closes

Other information and links

},
)
.await
}
NetworkEvent::PubsubMessage { source, topics, message } => {
match topics[0].as_str() {
"/fil/blocks/interop" => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the topic depends on the network name from genesis (in case that helps debugging)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah. I will polish this more later. Just wanted to make it clear that we call sync when we receive something over gossip.

Comment on lines 190 to 194
let new_blk = Tipset::new(vec![new_blk]).unwrap();
let chain_syncer = chain_syncer.clone();
task::spawn(async move {
chain_syncer.sync(&new_blk).await.unwrap();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't think this is what we want to do, we shouldn't be manually constructing the tipset and doing a full sync from it, can you explain reasoning?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think we actually are supposed to call inform_new_head here instead. Does that answer your question?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that makes a lot more sense, calling sync I'm pretty sure isn't what we want

@austinabell austinabell changed the base branch from master to main June 24, 2020 15:25
@ec2 ec2 changed the title Basic concurrency for ChainSyncer Chain syncing verification fixes Sep 3, 2020
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending functional testing

blockchain/chain_sync/src/sync.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dutterbutter dutterbutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just those two comments

blockchain/blocks/src/header/mod.rs Show resolved Hide resolved
blockchain/chain_sync/src/sync.rs Show resolved Hide resolved
@ec2 ec2 merged commit 0be6b76 into main Sep 4, 2020
@ec2 ec2 deleted the ec2/sync/optimize branch September 4, 2020 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants