feat(pushsync): forward chunk after storing#5081
Conversation
| // act as the multiplexer and push the chunk in parallel to multiple peers | ||
| if swarm.Proximity(peer.Bytes(), ch.Address().Bytes()) >= rad { | ||
| if swarm.Proximity(peer.Bytes(), ch.Address().Bytes()) >= rad && | ||
| swarm.Proximity(ps.address.Bytes(), ch.Address().Bytes()) < rad { |
There was a problem hiding this comment.
This change in logic should be formally validated, either by a regression unit test or an integration test.
I would suggest that the research team evaluates this logic change as well as the change #5037 that allegedly caused the issue.
From the technical point of view, regression test is missing, but it is just as important to have an approval from the research.
There was a problem hiding this comment.
@zelig @significance your input about these changes please
There was a problem hiding this comment.
@acha-bill we discussed this a little in research sync, could you give us a little extra commentary on what's going on here and amend the comment please dude? just so we are sure we're all on the same page
There was a problem hiding this comment.
amended the descr
nugaon
left a comment
nugaon
left a comment
There was a problem hiding this comment.
a test case may be needed where peers pass the chunk to node with addr 71 and that is in connection with node 70 and in the same neighborhood. node 71 should forward the chunk to node 70 since that is the closest to the chunk address.
acha-bill
changed the title
It was already tested in the other PR. |
| closestPeer := swarm.MustParseHexAddress("70") | ||
| closestPeer1 := swarm.MustParseHexAddress("72") | ||
|
|
||
| psClosestPeer, closestPeerStorer, closestAccounting := createPushSyncNode(t, closestPeer, defaultPrices, nil, nil, defaultSigner(chunk), mock.WithClosestPeerErr(topology.ErrWantSelf)) |
There was a problem hiding this comment.
this ErrWantSelf mocking is strange, it should work without that by setting neighbor peers that may return this error.
| recorder1 := streamtest.New(streamtest.WithProtocols(psClosestPeer.Protocol()), streamtest.WithBaseAddr(pivotPeer)) | ||
|
|
||
| // pivot connects to closestpeer and closestpeer1 but should only forward once to closestpeer after storing | ||
| // closestpeer should not forward the chunk to closestpeer1 as it is already the closest peer |
There was a problem hiding this comment.
anyway it cannot, because it does not have peers defined.
| return | ||
| } | ||
|
|
||
| ps.logger.Debug("chunk sent", "chunk_address", ch.Address().String(), "peer_address", peer) |
There was a problem hiding this comment.
this can be handy on manual test, also shallow receipt logs should be checked.
gacevicljubisa
requested review from
sbackend123
and removed request for
istae
|
LGTM , but needs more testing |
9 participants
Checklist
Description
Currently, we return from pushsync as soon as node lands in a neighborhood and is stored by the first node which is not necessarily the closest to the chunk. One effect of this is that gsoc does not work reliably with pushsync as the listening node does not receive the chunk.
In this PR, the in-neighborhood node will forward the chunk to its closest peer after storing it.
Also, a node will multiplex only if the node is not in AOR to prevent circular pushes. So it's 1 extra push from before.
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
fixes #5067
Screenshots (if appropriate):