feat: reserve-expanding sync eligibility for redistribution game (SWIP-44) - #5568
aloknerurkar wants to merge 2 commits into
Conversation
aloknerurkar
changed the title
sbackend123
left a comment
sbackend123
left a comment
There was a problem hiding this comment.
Nice job!
I would just clean up it a little bit - removing "formatting" changes, so PR looks bigger than it is.
752f1db to
791bd05
Compare
So this formatter is actually from our coding style doc. We used to follow it but I see now we dont. I had my editor setup to run these formatters as soon as I open any file. I will revert the changes for now but we should update the coding style and makefile for others in future. I personally prefer this formatting as it breaks down std library, external and internal imports. |
791bd05 to
6ea355a
Compare
6ea355a to
33bc0b7
Compare
33bc0b7 to
e867557
Compare
| reserveThreshold := reserveCapacity * 5 / 10 | ||
| logger.Debug("Sync status check evaluated", "stabilized", detector.IsStabilized()) | ||
| return localStore.ReserveSize() >= reserveThreshold && pullerService.SyncRate() == 0 && detector.IsStabilized() | ||
| return localStore.ReserveSize() >= reserveThreshold && pullerService.IsReserveSynced(depth) && detector.IsStabilized() |
There was a problem hiding this comment.
When a node uses Capacity Doubling, it stores chunks locally at radius 8, but commits on the blockchain at depth 9. It is responsible for 2 adjacent "sister neighborhoods". The PR only checks if bins >= 9 are synced, completely ignoring bin 8. Is this right?
There was a problem hiding this comment.
Did you check the SWIP?
I believe this is intentional and consistent with how the reserve digest itself is calculated.
CommittedDepth() (doublingFactor + StorageRadius()) is what the redistribution game and the sample are actually keyed on — see sample.go's own doc comment[L56-60]. With a physical radius of 8 and a doubling factor of 1, committed depth is 9, and ReserveSample only ever reads chunks at proximity ≥ 9 to the anchor — bin 8 chunks (which straddle both sister neighborhoods) simply aren't part of what gets hashed into the digest.
5aa1326 to
92c8a7f
Compare
|
@aloknerurkar please rebase |
92c8a7f to
ba4f3bb
Compare
| @@ -106,6 +107,9 @@ type Puller struct { | |||
|
|
|||
| rate *rate.Rate // rate of historical syncing | |||
There was a problem hiding this comment.
since you now count the active historical syncing using the number of active goroutines in radius bins, this thing is no longer used if i get it right. there are a couple of changes here - one is the fact we're no longer using these sliding window rate which is probably a good thing, but the problem is that the goroutine approach would be a bit tricky because it would basically flip on/off all the time. maybe a bit more thought needs to do be done here so we can get a smoothed reading that could be a bit more informative (but also that doesn't take half an hour to reach below the reading we need it to get to in order to conclude "not syncing")
5 participants
Checklist
Description
Implements SWIP-44: Reserve-Expanding Sync Eligibility.
Currently, Bee excludes a node from participating in the redistribution game whenever any historical pullsync is active (SyncRate() > 0). When a node is expanding its reserve into a lower bin ($d-1$ ) in preparation for a storage radius decrease, this sync can take hours, causing the node to miss rounds despite maintaining a complete and valid reserve at the current storage depth $d$ .
Because the reserve digest$H(R_N(d))$ is computed strictly over PO bins $p \geq d$ , chunks arriving in bins $p < d$ are disjoint from the reserve and do not influence the digest. This PR amends the eligibility check to allow participation during sub-depth sync:
Open API Spec Version Changes (if applicable)
N/A
Motivation and Context (Optional)
Addresses SWIP-44 (ethersphere/SWIPs#92).
Related Issue (Optional)
Screenshots (if appropriate):
N/A
AI Disclosure