Skip to content
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

feat: verify chunk availability before concluding synced status #2036

Closed
wants to merge 25 commits into from

Conversation

metacertain
Copy link
Member

@metacertain metacertain commented Jun 9, 2021

This is an experimental change to increase push reliability
The availability check is done by attempting to retrieve through a peer farther away from the chunk as the node, to minimize the probability of merging/rejoining routes used for the push and verification


This change is Reviewable

Copy link
Member

@zelig zelig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @metacertain)

a discussion (no related file):
not sure why this would be needed



pkg/pusher/pusher.go, line 199 at r1 (raw file):

				if err = s.retrieval.CheckAvailableChunk(ctx, ch.Address()); err != nil {
					err = fmt.Errorf("independent verification of availability failed, %w", err)

use : instead of ,


pkg/retrieval/retrieval.go, line 462 at r1 (raw file):

}

func (s *Service) farthestPeer(addr swarm.Address) (swarm.Address, error) {

would be better to have this function in peerSuggester?

@metacertain
Copy link
Member Author

metacertain commented Jun 10, 2021

not sure why this would be needed

because, out of neighborhood peers creating storage receipts could be filtered out by checking the availability of the chunk,
so large files can be uploaded more reliably

use : instead of ,

?

would be better to have this function in peerSuggester?

no opinion, it was introduced here because closestPeer was used from local code as well, to maintain consistency

@metacertain metacertain changed the title feat: verify chunk availability before concludin synced status feat: verify chunk availability before concluding synced status Jun 10, 2021
@metacertain metacertain changed the base branch from master to retrieval_behaviour June 11, 2021 10:27
Base automatically changed from retrieval_behaviour to master June 11, 2021 14:03
@Eknir
Copy link
Contributor

Eknir commented Jun 16, 2021

Thanks for this PR @metacertain . Let's first roll out what we have to the testnet and then validate the effect of this change.

@Eknir Eknir added the on hold Temporarily halted by other development label Jun 17, 2021
@metacertain metacertain force-pushed the verify_independent branch 2 times, most recently from a985f14 to 2786702 Compare June 23, 2021 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on hold Temporarily halted by other development pull-request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants