fix: only in neighborhood replication

[metacertain]

This PR intends to improve pushsync behaviour by changing returned ErrWantSelf return ErrNoPush if chunk is not in our neighborhood (to avoid doing a failing replication and setting it as synced) from pushToClosest

---

This change is 

[acud]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @metacertain)

---

pkg/pushsync/pushsync.go, line 302 at r1 (raw file):

				}

				if ps.topologyDriver.IsWithinDepth(ch.Address()) {

it would be better to reduce nesting by checking if !WithinDepth() { return ErrNoPush }, also, the extra error is not needed, and existing package-level errors could be reused.

[zelig]

Reviewed 2 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acud)

[mrekucci]

Reviewed 2 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acud)

[acud]

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

---

pkg/pushsync/pushsync.go, line 369 at r4 (raw file):

				// if the node has warmed up AND no other closer peer has been tried
				if time.Now().After(ps.warmupPeriod) && !ps.skipList.HasChunk(ch.Address()) && ps.topologyDriver.IsWithinDepth(ch.Address()) {

this change is outside of the scope of this PR, since it changes the skip list behavior. It would be at least nice to document this change in the PR description, about why it is needed and what it affects.

---

pkg/topology/mock/mock.go, line 167 at r4 (raw file):

		return m.isWithinFunc(addr)
	}
	return true

this is changing the test setup for another test that uses this mock and used to rely on a false here. how is it that this needs to change?

[istae]

@acud The reason why the default is now true is because the chunk must be in the neighborhood of the storer peer with this change. The majority of the tests expect a valid receipt so the closest peers in the tests must be in the neighborhood. As of writing this, there is only one test that expects a peer to be out of depth.

[istae]

In short, the previous default false causes every test to fail.

[acud]

@istae then can we have an explicit change to the tests, in which a custom mocked IsWithinFunc is injected with a true? my concern is that with this change, also forwarder nodes (and there are forwarding tests) will see the chunk as within depth, leading to a change in the test cases' final state

[acud]

it is also counter-intuitive that we would now test everything with the following assumptions by default:

• originator node sees all chunks as within neighborhood
• fowarder node sees all chunks as within neighborhood
• storer node sees all chunks as within neighborhood
• replicating node sees all chunks as within neighborhood

a bit too permissive IMO. so I would vote for reverting this back to false and have an explicit true injected with a mock wherever needed

[acud]

@istae then can we have an explicit change to the tests, in which a custom mocked IsWithinFunc is injected with a true? my concern is that with this change, also forwarder nodes (and there are forwarding tests) will see the chunk as within depth, leading to a change in the test cases' final state

[acud]

it is also counter-intuitive that we would now test everything with the following assumptions by default:

• originator node sees all chunks as within neighborhood
• fowarder node sees all chunks as within neighborhood
• storer node sees all chunks as within neighborhood
• replicating node sees all chunks as within neighborhood

a bit too permissive IMO. so I would vote for reverting this back to false and have an explicit true injected with a mock wherever needed

[acud]

Great, thanks @istae 🙏