-
Notifications
You must be signed in to change notification settings - Fork 129
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(dot/sync): fix block request and response logic #1907
Changes from 6 commits
8951703
f0250e9
7b07d9d
bdc5565
3e64089
ad9815b
38f917b
21cb17d
5d921eb
2312609
95431ce
2a9afb9
8985831
866d76a
8e46510
997f988
c2425d8
08d513c
d735eef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -897,6 +897,12 @@ func workerToRequests(w *worker) ([]*network.BlockRequestMessage, error) { | |
} else { | ||
// in tip-syncing mode, we know the hash of the block on the fork we wish to sync | ||
start, _ = variadic.NewUint64OrHash(w.startHash) | ||
|
||
// if we're doing descending requests and not at the last (highest starting) request, | ||
// then use number as start block | ||
if w.direction == network.Descending && i != numRequests-1 { | ||
start, _ = variadic.NewUint64OrHash(startNumber) | ||
} | ||
} | ||
|
||
var end *common.Hash | ||
|
@@ -911,7 +917,21 @@ func workerToRequests(w *worker) ([]*network.BlockRequestMessage, error) { | |
Direction: w.direction, | ||
Max: &max, | ||
} | ||
startNumber += maxResponseSize | ||
|
||
switch w.direction { | ||
case network.Ascending: | ||
startNumber += maxResponseSize | ||
case network.Descending: | ||
startNumber -= maxResponseSize | ||
} | ||
Comment on lines
+921
to
+926
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not if/else? Are we expecting more directions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no, but I think in general we kinda like switch statements here :p what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think
But I'm happy with |
||
} | ||
|
||
// if our direction is descending, we want to send out the request with the lowest | ||
// startNumber first | ||
if w.direction == network.Descending { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we directly do this inside loop instead of outside?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you mean exactly? |
||
for i, j := 0, len(reqs)-1; i < j; i, j = i+1, j-1 { | ||
reqs[i], reqs[j] = reqs[j], reqs[i] | ||
} | ||
EclesioMeloJunior marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
return reqs, nil | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use the
Must
equivalent? Just in case we modifyNewUint64OrHash
eventually, to avoid bad surprises.