-
Notifications
You must be signed in to change notification settings - Fork 186
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
Parallelize header download across peers to fetch headers within a chain's checkpointed region. #282
base: master
Are you sure you want to change the base?
Parallelize header download across peers to fetch headers within a chain's checkpointed region. #282
Changes from 1 commit
40670d1
9ab1d8b
101c690
f168538
dd02e22
2285ce7
64b2787
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,7 +95,7 @@ func (w *worker) Run(results chan<- *jobResult, quit <-chan struct{}) { | |
msgChan, cancel := peer.SubscribeRecvMsg() | ||
defer cancel() | ||
|
||
nexJobLoop: | ||
nextJobLoop: | ||
for { | ||
log.Tracef("Worker %v waiting for more work", peer.Addr()) | ||
|
||
|
@@ -154,7 +154,7 @@ nexJobLoop: | |
case <-quit: | ||
return | ||
} | ||
goto nexJobLoop | ||
goto nextJobLoop | ||
} | ||
} | ||
|
||
|
@@ -308,6 +308,28 @@ nexJobLoop: | |
} | ||
} | ||
|
||
func (w *worker) IsSyncCandidate() bool { | ||
return w.peer.IsSyncCandidate() | ||
} | ||
|
||
func (w *worker) IsPeerBehindStartHeight(req ReqMessage) bool { | ||
return w.peer.IsPeerBehindStartHeight(req) | ||
} | ||
|
||
// IsWorkerEligibleForBlkHdrFetch is the eligibility function used for the BlockHdrWorkManager to determine workers | ||
// eligible to receive jobs (the job is to fetch headers). If the peer is not a sync candidate or if its last known | ||
// block height is behind the job query's start height, it returns false. Otherwise, it returns true. | ||
func IsWorkerEligibleForBlkHdrFetch(r *activeWorker, next *queryJob) bool { | ||
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. This shouldn't be needed. The workers should never be given peers that aren't eligible to be synced off of in the first place. 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. The work manager is not exclusively for fetching block headers. So it would receive connected peer messages for all peers. It just happens that in this use case where it is fetching block headers we are only interested in peers that fulfill the condition as specified in that function . That is why I included that there. So a worker could not be useful in fetching block headers but it could be eligible to fetch cfheaders or filters for example. 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. judging by the name, the "next" argument should be known to be a block header fetch. If you accept all types of queries here then the function should be renamed and reworked to decide if the peer is eligible for that request in general 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. The name of the function is literally, 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. The thing is difference between the 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. Maybe there is a better way to go about it. |
||
if !r.w.IsSyncCandidate() { | ||
return false | ||
} | ||
|
||
if r.w.IsPeerBehindStartHeight(next.Req) { | ||
return false | ||
} | ||
return true | ||
} | ||
|
||
// NewJob returns a channel where work that is to be handled by the worker can | ||
// be sent. If the worker reads a queryJob from this channel, it is guaranteed | ||
// that a response will eventually be deliverd on the results channel (except | ||
|
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.
just pass in the start height, this function should take two arguments: the server peer, and the start height. This is also doing a downcasting operation and that clutters the logic, do that at the call site. You are duplicating the downcasting logic in a lot of places and it isn't necessary. Also returning true in the case that the downcast fails clashes strongly with the name here. This function could just be
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.
#282 (comment)