[gacevicljubisa]

Checklist

• I have read the coding guide.
• My change requires a documentation update, and I have done it.
• I have added tests to cover my changes.
• I have filled out the description and linked the related issues.

Description

On master, pushsync can report a chunk as synced when no node in its neighborhood stores it. The upload returns 201 and the tag says Synced. No other node can retrieve such a chunk, and the storer evicts it when its reserve runs over capacity. This PR turns that false success into an error or a retry.

What changes

1. Receipt check. A receipt is valid only at po >= r. Master also accepted po >= r - 1 on a node without capacity doubling. The tolerance is removed from node.go, and checkReceipt also returns the PO.
2. Shallow receipt inside one call. A shallow receipt now uses one of the 32 attempts, like an error, and the call goes to the next peer. The call keeps the deepest shallow receipt. It returns ErrShallowReceipt only when the budget or the peer list runs out. On master the first shallow receipt ended the call.
3. Forwarder with no closer peer. On ErrWantSelf a forwarder now widens the peer filter, like on ErrNotFound, and waits 600 ms when peers are skipped for overdraft. A forwarder stores the chunk only inside its own AOR. Outside its AOR it returns ErrOutOfDepthStoring to the previous hop. On master it stored the chunk and signed a receipt.
4. Origin with no peers left. An origin outside its AOR, with every peer tried or skipped, returns ErrOutOfDepthStoring. The deferred pusher counts it as one of its 6 calls. A direct upload fails at once. On master the origin stored the chunk on itself.
5. Pusher after the last call. The pusher reports ChunkCouldNotSync instead of ChunkSynced. The tag's Synced count stays below Split. A direct upload returns ErrCouldNotSync, so the API answers 500 instead of 201.
6. Netstore. A direct upload waits 5 s on ErrNotFound before it submits the chunk again, instead of a tight loop.
7. Metrics. New counters: out_of_depth_storing and origin_no_peers_left in pushsync, total_could_not_sync in pusher. total_synced now counts only chunks with a valid receipt or stored by this node.

What does not change

• 32 attempts per call, 6 calls per chunk, 3 parallel sends when the first hop is in the neighborhood, the 5 s ticker, the 30 s push TTL, the 5 minute errSkip.
• Overdraft uses no budget. The netstore resubmits a direct chunk at once on ErrShallowReceipt.
• The maximum of paid attempts per chunk goes from about 6 to about 192, because a paid shallow receipt no longer ends the call. The measured average on mainnet is 1.4 per chunk.

Follow-up

PR #5667 is stacked on this PR. It gives a chunk one more round when a direct upload gets ErrOutOfDepthStoring, so the two PRs go together.

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

[acud]

you've removed the skiplist addition line that was on the left side of the diff (L493). any particular reason?

[gacevicljubisa]

actually it is not removed, it is on the common failure path at pushsync.go:524

[acud]

not sure if I understand this fully, but this statement and the next one in the else block seem like assigned but never used where is result.err used?

[gacevicljubisa]

This is used now. The shallow case sets result.err and falls through to the shared failure path. That path logs result.err in the "could not push to peer" debug line, counts one failed attempt and adds the peer to errSkip. So a shallow receipt is handled like any other failed attempt, and the deepest shallow receipt is kept for the ErrShallowReceipt return at the end of the call.

[martinconic]

Changing to ChunkCouldNotSync, will cause some false reporting because the ChunkCouldNotSync state is not checked and updated in uploadstore.go ->Report function. Maybe you would need to alsi include in that switch statement also ChunkCouldNotSync.

[gacevicljubisa]

Done in 19adb2c.

[martinconic]

What happens if you have doubling set on, and the chunk is saved into a sister neighborhood? Should we have here a check based on the committed depth?

[gacevicljubisa]

No change needed. The handler uses ps.radius(), which is the reserve radius. A doubling node has a reserve radius one bin shallower than its committed depth, so it stores both its own and its sister neighborhood. A chunk from the sister neighborhood passes the check and the node stores it. That is correct, because the node really holds that chunk and serves it on retrieval. A committed-depth check would refuse it.
The origin sees this receipt as shallow, because it is one bin below the network radius, and pushes again.

[gacevicljubisa]

Update: this is covered by SWIP-48 (ethersphere/SWIPs#97). There every full in-AOR node stores the chunk and keeps forwarding, and signs its own receipt only as a fallback. A doubling node is in AOR by its reserve radius, so it stores and forwards the chunk to the sister neighborhood and returns that receipt. The refuse check stays on the reserve radius. No change in this PR.

[lat-murmeldjur]

This looks good to me still.
see ethersphere/SWIPs#97 for a follow up logic extension