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

[DRAFT] Add bootstrap handling #136

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gurghet
Copy link
Contributor

@gurghet gurghet commented Aug 6, 2021

This adds bootstrap handling in a built-in way.
I'm publishing this as a draft PR as I'm not sure this should be merged (wanted to build a dedicated module for bootstrap) but I don't want to lose track of the progress so far.

@@ -38,7 +38,7 @@ pub struct SendBlock {
}

impl SendBlock {
pub const LEN: usize = 152;
pub const LEN: usize = 32+32+16+64+8;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is never going to change, true, but I think it's clearer like this, each field its own number.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good idea!

debug_assert!(header.is_some());
let header = header.unwrap();
debug_assert_eq!(header.ext().block_type()?, BlockType::State);
if let Some(header) = header {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be without header in the bulk pull case

) -> anyhow::Result<()> {
// The rest of this connection will be a bunch of blocks without any headers.
if matches!(self.bootstrap_state, FrontierStream) {
panic!("Invalid bootstrap state transition FrontierStream => BulkPull");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably planned, but just suggesting this be an Err because a peer can trigger this, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, I'm moving stuff around right now. Could you elaborate anyways? I used panic because I thought this should be an irrecoverable programming error, the transition should not be even possible.

This is only so save progress. I was trying different things but I
think I will settle for the uncommented code. Just need some time to
clean it all up, give proper names to functions, and maybe refactor
a few things.
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 this pull request may close these issues.

None yet

2 participants