Rewrite internal logic of block sync client #2715
Conversation
schomatis
force-pushed
the
schomatis/blocksync/review
branch
from
bcf99c6
to
487b4f2
Compare
|
Not sure why |
Try rebasing / merging newer |
schomatis
force-pushed
the
schomatis/blocksync/review
branch
from
487b4f2
to
fc32410
Compare
|
@magik6k same error |
|
Working on review of this. |
There was a problem hiding this comment.
Looks like this introduces some protocol-breaknig changes, but i still managed to sync the calibration chain between 2 local nodes.
In general this looks good to me, but probably want @Kubuxu / @whyrusleeping to have a look too before merging
| if options.IncludeMessages { | ||
| validRes.messages = make([]*CompactedMessages, resLength) | ||
| for i := 0; i < resLength; i++ { | ||
| if res.Chain[i].Messages == nil { |
There was a problem hiding this comment.
what if there actually arent any messages?
There was a problem hiding this comment.
you'd have an empty structure but not a nil pointer
There was a problem hiding this comment.
but that's a good question, i now realize i had the implicit (unfounded) assumption that blocks couldn't be empty
| // FIXME: What's the point of setting a blank deadline that won't time out? | ||
| // Is this our way of clearing the old one? | ||
| client.peerTracker.logFailure(peer, build.Clock.Since(connectionStart)) | ||
| // FIXME: Should we also remove peer here? |
There was a problem hiding this comment.
i currently assume the peer tracker won't try the peer again unless they are our only other option
| // * BI: block index in the tipset. | ||
| // * MI: message index in `Bls` list | ||
| // | ||
| // FIXME: The logic to decompress this structure should belong |
| return | ||
| } | ||
|
|
||
| _ = stream.SetDeadline(time.Now().Add(WRITE_RES_DEADLINE)) |
There was a problem hiding this comment.
need to clear the deadline too
There was a problem hiding this comment.
addressed in 6551219
| // FIXME: Same, why are we doing this again here? | ||
| _ = stream.SetWriteDeadline(time.Time{}) | ||
| _ = stream.SetWriteDeadline(time.Time{}) // clear deadline // FIXME: Needs | ||
| // its own API (https://github.com/libp2p/go-libp2p-core/issues/162). |
There was a problem hiding this comment.
Clearing Deadline by empty time is the Go's standard, I agree specific API might be nice.
|
Still a lot of tests failing from unrelated reasons.. |
While trying not to modify too much of the external callers code. The only major change outside of
chain/blocksync/is extracing theFetchMessagesByCids/FetchSignedMessagesByCidsfunctions to the consumer. They did not actually used this protocol, just theBlockServiceembedded in one of its structures (that was also extracted since the server only needs theChainStore).