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

Added backward argument to block headers request #1100

Closed
wants to merge 3 commits into from
Closed

Added backward argument to block headers request #1100

wants to merge 3 commits into from

Conversation

yglukhov
Copy link

The backward option is useful for obtaining the branches we're missing for some reason, but getting attestations to.

@yglukhov yglukhov closed this May 20, 2019
@yglukhov yglukhov reopened this May 22, 2019
@atoulme
Copy link
Contributor

atoulme commented May 29, 2019

Looks ok to me. LES uses the word reverse instead of backward though.

@yglukhov
Copy link
Author

yglukhov commented Jun 3, 2019

I don't mind rewording to reverse but it seems a bit ambiguous to me, potentially meaning "reverse the order of the returned headers".

@@ -225,6 +225,7 @@ Requests a list of block roots and slots from the peer. The `count` parameter MU
start_slot: uint64
max_headers: uint64
skip_slots: uint64
backward: uint8 in {0, 1}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be of type bool?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, semantically this is a bool. Spec-wise though I didn't find any occurrences of bool to date, and went kinda the same way it was done in eth1 wire spec. Should I change it to bool instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Indeed. Changed to bool

@djrtwo
Copy link
Contributor

djrtwo commented Jul 10, 2019

closing in favor of #1281

@djrtwo djrtwo closed this Jul 10, 2019
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.

4 participants