Conversation
|
@zelig Unit tested the light client request. But testing the retrieve from another full node looks hard in unit test. But i tested that in the Trinity client case.
|
bzzeth/bzzeth.go
Outdated
| batches := make(chan [][]byte) | ||
| defer close(batches) | ||
|
|
||
| // deliver in batches, this blocks until total number of requests are delivered or considered not found |
bzzeth/bzzeth.go
Outdated
| continue | ||
| } | ||
| // initiate request with the chosen peer | ||
| req, err := p.getBlockHeaders(ctx, header, deliveryC, giveBackC) |
There was a problem hiding this comment.
why not use just the same delivery channel? why do we need this givebackc?
There was a problem hiding this comment.
these two channels are different...
giveBackC is the actual channel to which we have to give back the header..
deliveryC is the channel for the second leg.. i.e. this Swarm node to Full node from which the header is requested...
so they have to be different..
bzzeth/bzzeth.go
Outdated
| } | ||
|
|
||
| wg.Add(1) | ||
| go b.getDelivery(ctx, req, &wg) |
There was a problem hiding this comment.
what is this doing? should we not just pipe delivered chunks to the original delivery chan?
There was a problem hiding this comment.
As i explained above.. deliveryC takes care of the second leg...
This function just acknowledges the receipt of the header from the full node and updates the req connection .. similar to how we acknowledge in handleNewBlockHeaders
There was a problem hiding this comment.
what more is needed other than open a request and let handleBlockHeaders do the checks and channel responses to the shared deliveryC assigned to req.c?
bzzeth/bzzeth.go
Outdated
| // - reads requested header hashes from a channel (headerC) and | ||
| // - creates batch requests and sends them to an adequate bzzeth peer | ||
| // - channels the responses into a delivery channel (deliveries) | ||
| func (b *BzzEth) getBlockHeadersEth(ctx context.Context, headersC, giveBackC chan []byte, pwg *sync.WaitGroup, rcvdPeer *Peer) { |
There was a problem hiding this comment.
you passing the requesting peer in order to filter but do we want to allow nodes that can serve as well as get headers?
There was a problem hiding this comment.
we want to allow nodes to serve as well as get headers... but a same node can't be the requestor and provider isn't it?
There was a problem hiding this comment.
but then this should be read from the peer, so no need to pass it to filter our a requestor
bzzeth/bzzeth.go
Outdated
| // this loop terminates when batches channel is closed as a result of input headersC being closed | ||
| requiredCount := 0 | ||
| var wg sync.WaitGroup | ||
| deliveryC := make(chan []byte) |
There was a problem hiding this comment.
still dont get it, you just need one for requests the other for responses that you then sent back to the requesting peer after validation and storage.
janos
left a comment
There was a problem hiding this comment.
It is a bit harder to review channels handling and their correctness. Maybe an interactive review would help.
No issues.. we can do it whenever you are free.. |
zelig
left a comment
There was a problem hiding this comment.
the major things are:
- I believe there is no need for two channels on a request . so we can get rid of givebackC.
- We can also get rid of passing the peer to getEth since if we have two types of peers, we will never try to request from the one that requested
bzzeth/bzzeth.go
Outdated
| // - reads requested header hashes from a channel (headerC) and | ||
| // - creates batch requests and sends them to an adequate bzzeth peer | ||
| // - channels the responses into a delivery channel (deliveries) | ||
| func (b *BzzEth) getBlockHeadersEth(ctx context.Context, headersC, giveBackC chan []byte, pwg *sync.WaitGroup, rcvdPeer *Peer) { |
There was a problem hiding this comment.
but then this should be read from the peer, so no need to pass it to filter our a requestor
bzzeth/bzzeth.go
Outdated
| // this loop terminates when batches channel is closed as a result of input headersC being closed | ||
| requiredCount := 0 | ||
| var wg sync.WaitGroup | ||
| deliveryC := make(chan []byte) |
There was a problem hiding this comment.
still dont get it, you just need one for requests the other for responses that you then sent back to the requesting peer after validation and storage.
|
@zelig I have addressed both of your major comments. Removed the excess channel and removed passing peer to the getPeer function. Also i tried to address most of the other comments also. |
|
The above screenshot shows the Trinity light client connects with Swarm node... Swarm node connects with the Trinity full node gets the Header... stores it in Swarm.. and Sends it to Trinity light client. |
bzzeth/bzzeth.go
Outdated
| @@ -296,7 +311,14 @@ func (b *BzzEth) validateHeader(ctx context.Context, header []byte, req *request | |||
| return nil, errDuplicateHeader | |||
| } else { | |||
There was a problem hiding this comment.
i am just saying there is no need for else since you return in the if clause.
|
|
||
| // requestAll requests each hash and channel | ||
| func (b *BzzEth) requestAll(ctx context.Context, deliveries chan []byte, hashes []chunk.Address, rcvdPeer *Peer) { | ||
| // requestAll requests each hash from Swarm (local or Network) and send t0 |
| // if this chunk is stored successfully, dont report error for it | ||
| if !flag { | ||
| continue | ||
| } |
There was a problem hiding this comment.
you still do this error checking on line 296 for every iteration. instead just put it before the loop.
and also we should not ignore other errors coming from the db
There was a problem hiding this comment.
Pushed the err outside the loop
Catch other errors also
The following tasks are in the Phase 2
= Use the actual eth block header and decode it.
= Handle GetBlockHeaders from a light client and serve the stored headers
= Update test cases to use the actual block header and other other stuff
= Test with Trinity client on the other side getting the blocks