-
Notifications
You must be signed in to change notification settings - Fork 187
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 all commits
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 |
---|---|---|
|
@@ -43,6 +43,9 @@ type queryOptions struct { | |
// that a query can be retried. If this is set then numRetries has no | ||
// effect. | ||
noRetryMax bool | ||
|
||
// errChan error channel with which the workmananger sends error. | ||
errChan chan error | ||
} | ||
|
||
// QueryOption is a functional option argument to any of the network query | ||
|
@@ -67,6 +70,14 @@ func (qo *queryOptions) applyQueryOptions(options ...QueryOption) { | |
} | ||
} | ||
|
||
// ErrChan is a query option that specifies the error channel which the workmanager | ||
// sends any error to. | ||
func ErrChan(err chan error) QueryOption { | ||
return func(qo *queryOptions) { | ||
qo.errChan = err | ||
} | ||
} | ||
|
||
// NumRetries is a query option that specifies the number of times a query | ||
// should be retried. | ||
func NumRetries(num uint8) QueryOption { | ||
|
@@ -107,25 +118,40 @@ func Cancel(cancel chan struct{}) QueryOption { | |
} | ||
} | ||
|
||
// Progress encloses the result of handling a response for a given Request, | ||
// determining whether the response did progress the query. | ||
type Progress struct { | ||
// Finished is true if the query was finished as a result of the | ||
// received response. | ||
Finished bool | ||
|
||
// Progressed is true if the query made progress towards fully | ||
// answering the request as a result of the received response. This is | ||
// used for the requests types where more than one response is | ||
// expected. | ||
Progressed bool | ||
} | ||
// Progress encloses the result of handling a response for a given Request. | ||
type Progress string | ||
|
||
var ( | ||
|
||
// Finished indicates we have received the complete, valid response for this request, | ||
// and so we are done with it. | ||
Finished Progress = "Received complete and valid response for request." | ||
|
||
// Progressed indicates that we have received a valid response, but we are expecting more. | ||
Progressed Progress = "Received valid response, expecting more response for query." | ||
|
||
// UnFinishedRequest indicates that we have received some response, but we need to rescheule the job | ||
// to completely fetch all the response required for this request. | ||
UnFinishedRequest Progress = "Received valid response, reschedule to complete request" | ||
|
||
// ResponseErr indicates we obtained a valid response but response fails checks and needs to | ||
// be rescheduled. | ||
ResponseErr Progress = "Received valid response but fails checks " | ||
|
||
// IgnoreRequest indicates that we have received a valid response but the workmanager need take | ||
// no action on the result of this job. | ||
IgnoreRequest Progress = "Received response but ignoring" | ||
|
||
// NoResponse indicates that we have received an invalid response for this request, and we need | ||
// to wait for a valid one. | ||
NoResponse Progress = "Received invalid response" | ||
) | ||
ProofOfKeags marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Request is the main struct that defines a bitcoin network query to be sent to | ||
// connected peers. | ||
type Request struct { | ||
// Req is the message request to send. | ||
Req wire.Message | ||
// Req contains the message request to send. | ||
Req ReqMessage | ||
|
||
// HandleResp is a response handler that will be called for every | ||
// message received from the peer that the request was made to. It | ||
|
@@ -138,7 +164,26 @@ type Request struct { | |
// should validate the response and immediately return the progress. | ||
// The response should be handed off to another goroutine for | ||
// processing. | ||
HandleResp func(req, resp wire.Message, peer string) Progress | ||
HandleResp func(req ReqMessage, resp wire.Message, peer Peer) Progress | ||
|
||
// SendQuery handles sending request to the worker's peer. It returns an error, | ||
// if one is encountered while sending the request. | ||
SendQuery func(peer Peer, request ReqMessage) error | ||
|
||
// CloneReq clones the message. | ||
CloneReq func(message ReqMessage) ReqMessage | ||
Comment on lines
+173
to
+174
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. But why do we need that? 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. Explained it in the code: Also reading the commit message that effected this change would help: 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 don't think you should be modifying the request. It seems you're using a common structure for what ought to be two different data types. One represents the request itself, the top level job. The other represents the remaining work on that job. The latter is conceptually an in-progress job. I think you should label it as such and then rather than talking about it as a "clone", it's really a construction of a "InProgressRequest" or something to that effect. |
||
} | ||
|
||
// ReqMessage is an interface which all structs containing information | ||
// required to process a message request must implement. | ||
type ReqMessage interface { | ||
|
||
// Message returns the message request. | ||
Message() wire.Message | ||
|
||
// PriorityIndex returns the priority the caller prefers the request | ||
// would take. | ||
PriorityIndex() float64 | ||
} | ||
|
||
// WorkManager defines an API for a manager that dispatches queries to bitcoin | ||
|
@@ -167,11 +212,6 @@ type Dispatcher interface { | |
// Peer is the interface that defines the methods needed by the query package | ||
// to be able to make requests and receive responses from a network peer. | ||
type Peer interface { | ||
// QueueMessageWithEncoding adds the passed bitcoin message to the peer | ||
// send queue. | ||
QueueMessageWithEncoding(msg wire.Message, doneChan chan<- struct{}, | ||
encoding wire.MessageEncoding) | ||
|
||
// SubscribeRecvMsg adds a OnRead subscription to the peer. All bitcoin | ||
// messages received from this peer will be sent on the returned | ||
// channel. A closure is also returned, that should be called to cancel | ||
|
@@ -184,4 +224,11 @@ type Peer interface { | |
// OnDisconnect returns a channel that will be closed when this peer is | ||
// disconnected. | ||
OnDisconnect() <-chan struct{} | ||
|
||
// IsPeerBehindStartHeight returns a boolean indicating if the peer's known last height is behind | ||
// the request's start Height which it receives as an argument. | ||
IsPeerBehindStartHeight(req ReqMessage) bool | ||
|
||
// IsSyncCandidate returns true if the peer is a sync candidate. | ||
IsSyncCandidate() bool | ||
} |
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)