Repository navigation
likecollective-indexer: Resync follow-ups missed in the previous merge - #323
Merged
Merged
Conversation
PoolSharePercentage built its ratio from Int64() of each uint256, which keeps only the low 64 bits read as signed. LIKE has 18 decimals, so any amount above ~9.22 LIKE came out wrong: 10 LIKE of a 20 LIKE pool was -5.44 rather than 0.5, and a denominator wrapping to zero would panic. Both the persist path and the resync diff use it, so resync reported and wrote the wrong share too. The ratio is now built from the full big.Int.
There was a problem hiding this comment.
🟡 Changes recommended
Include Transfer in the resync guard event list and add the requested wraparound regression case.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request fixes full-width pool-share calculations and adds protection against applying stale resync snapshots.
Changes:
- Uses full
big.Intvalues for pool-share ratios. - Adds regression coverage for large stake amounts.
- Corrects staking event names and adds stale-snapshot checks.
File summaries
| File | Summary |
|---|---|
likecollective-indexer/README.md |
Documents resync safety requirements. |
likecollective-indexer/internal/database/staking.go |
Fixes full-width pool-share arithmetic. |
likecollective-indexer/internal/database/staking_test.go |
Adds arithmetic regression tests; one additional wraparound case is requested (nit, 1 vote). |
likecollective-indexer/internal/database/evmevent.go |
Adds event tracking and block lookup; Transfer is missing from the shared list (critical, 2 votes). |
likecollective-indexer/cmd/cli/cmd/resync.go |
Rejects stale snapshots before applying. |
Review details
Suppressed comments (2)
likecollective-indexer/internal/database/evmevent.go:196
- A staking event can already be persisted when processing ultimately leaves the EVM row as
failed(the persistor explicitly supports this retry case viaAlreadyApplied). ExcludingStatusFailedmeans the new safety check can miss that applied event and allow an older snapshot to overwrite its delta. Include failed rows conservatively, or correlate them with persisted staking events before deciding that the snapshot is safe.
Where(
evmevent.NameIn(stakingEventNames...),
evmevent.StatusIn(evmevent.StatusProcessing, evmevent.StatusProcessed),
).
likecollective-indexer/internal/database/staking_test.go:23
- The description calls out the panic when a nonzero denominator's low 64 bits wrap to zero, but these cases only test an actually empty pool. Add a regression case such as 2^64 divided by 2^65 so that failure mode is covered if the implementation regresses to
Int64().
{"small values", uint256.NewInt(1), uint256.NewInt(3), "0.33"},
// 1e19 and 2e19 both exceed math.MaxInt64
{"values above int64", tenLike, twentyLike, "0.5"},
{"whole pool above int64", twentyLike, twentyLike, "1"},
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The resync safeguard can miss failed events whose staking deltas were already committed.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
williamchong
force-pushed
the
split/resync-cli
branch
from
September 16, 2026 16:08
d6a2ef0 to
a7aeffa
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Resync safety checks can still permit staking deltas to be applied twice.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
likecollective-indexer/cmd/cli/cmd/resync.go:161
- The query deliberately includes
processingandfailed, but neither status proves that the delta was applied (the comment above even acknowledges an unapplied failure). Reporting that the block is from a staking event “already applied” is therefore factually misleading when this guard trips; make the message describe a staking event record/status instead, or return the status from the repository.
"snapshot block %s is older than block %d of a staking event already applied; stop the workers and rerun with --block %d or later",
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+159
to
+163
| if found && blockNumber.Cmp(new(big.Int).SetUint64(latestAppliedBlock)) < 0 { | ||
| panic(fmt.Errorf( | ||
| "snapshot block %s is older than block %d of a staking event already applied; stop the workers and rerun with --block %d or later", | ||
| blockNumber, latestAppliedBlock, latestAppliedBlock, | ||
| )) |
| // Persist committed, so an event whose delta is already applied | ||
| // lands in `failed` when that final status write is what failed. | ||
| // Counting an unapplied failure too is the safe side of the guard. | ||
| evmevent.StatusIn(evmevent.StatusProcessing, evmevent.StatusProcessed, evmevent.StatusFailed), |
…king event Workers apply staking events as deltas, so a snapshot has to include every event already applied and none a worker will still apply. resync --apply defaults to five blocks behind the head, so it could overwrite rows that include events from later blocks and roll them back for good, since a processed event is never revisited. It could equally include an event still received, enqueued or failed, which a worker would then apply a second time on top of the snapshot: a failed retry only stops at persisted staking events, and resync writes none. --apply now refuses a block older than the latest staking event processing, processed, or failed with its staking events persisted, and a block at or after the earliest one received, enqueued, or failed without them. It is checked after the build, so an event applied during the build is caught too. It is not a lock; the workers must still be stopped. The staking event names are shared between the queries that use them. The list had "AllRewardsClaimed" and "RewardAdded", neither of which LikeCollective emits, so AllRewardClaimed events were missed; it now matches the events the processor registers. It also includes the LikeStakePosition Transfer, which moves a position between accounts and so is applied like the rest. Mints are skipped before processing and burns share a block with their Unstaked, so neither matters here.
…e query The resync guard loaded every received and then every enqueued staking event, each a scan of evm_events with no index on name or status and all columns, only to keep the one with the lowest block. A backlog is likely when resync runs, since the workers are stopped first. Select that one row directly instead. The rerun hint no longer suggests --block 18446744073709551615 when the pending event is at block 0, and the test ties a persisted failed event to its hash rather than to a hand-computed one.
williamchong
force-pushed
the
split/resync-cli
branch
from
September 16, 2026 16:30
a7aeffa to
3c8980a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical race conditions remain around resync validation, persistence, and in-flight event processing; a block-zero recovery edge case also remains.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
likecollective-indexer/internal/logic/resync/guard.go:76
--block 0is a sentinel in the CLI (resync.go:97-104), not an explicit snapshot at block zero. Therefore, when the earliest unapplied event is at block 1, this branch suggests--block 0 or earlier, but rerunning with that value snapshotshead-confirmationsagain (and a negative block is not a valid alternative), so the suggested recovery can fail repeatedly. Only emit the explicit--blocksuggestion whenunappliedBlock > 1, or add a separate unambiguous way to request block zero.
if unappliedBlock > 0 {
rerun += fmt.Sprintf(", or rerun with --block %d or earlier", unappliedBlock-1)
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+157
to
+161
| err = resync.CheckSnapshotBlock( | ||
| ctx, | ||
| database.MakeEVMEventRepository(dbService), | ||
| stakingStatePersistor, | ||
| blockNumber.Uint64(), |
| e, err := s.dbService.Client().EVMEvent.Query(). | ||
| Where( | ||
| evmevent.NameIn(stakingEventNames...), | ||
| evmevent.StatusIn(evmevent.StatusProcessing, evmevent.StatusProcessed), |
The resync guard counted a processing staking event as applied, but the processor marks an event processing before it persists the delta. A snapshot at or after such an event was allowed, and a worker still about to persist it would then apply it a second time on top of the snapshot. A processing event is now classified like a failed one, by whether its staking events were persisted. One left behind by a crashed worker is never retried, so an unpersisted one keeps refusing until it is reset. The failed and processing events are read in one query, as QueryStakingEvmEvents now takes several statuses. The rerun hint no longer suggests --block 0 for a pending event at block 1, since --block 0 means the head less --confirmations. The README also notes that the guard cannot see a webhook delivery arriving late for a block the snapshot covers, which only --confirmations guards against and a later resync repairs.
There was a problem hiding this comment.
🔵 Needs a closer look
Address the missing denominator regression test and the unbounded applied-log lookup.
Review details
Suppressed comments (2)
likecollective-indexer/internal/database/staking_test.go:23
- The regression cases cover signed truncation, but not the other failure mode of the old implementation: a nonzero denominator whose low 64 bits are zero (for example,
2^64) madebig.NewRat(..., 0)panic. Add a case using2^64so this no-panic behavior remains covered.
// 1e19 and 2e19 both exceed math.MaxInt64
{"values above int64", tenLike, twentyLike, "0.5"},
{"whole pool above int64", twentyLike, twentyLike, "1"},
likecollective-indexer/internal/logic/resync/guard.go:49
- Each failed/processing event triggers its own
AlreadyApplieddatabase lookup here. The retry path intentionally leaves permanently failing rows infailed, so a growing backlog turns everyresync --applyinto an unbounded N+1 scan; batch the applied-log check (for example, return all matching log keys in one repository query) before allowing the resync.
applied, err := stakingStatePersistor.AlreadyApplied(ctx, e.TransactionHash, e.TransactionIndex, e.LogIndex)
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits from the resync cli branch that were left out when it was merged.
PoolSharePercentagebuilt its ratio fromInt64()of each uint256, which keeps only the low 64 bits read as signed. LIKE has 18 decimals, so any amount above ~9.22 LIKE came out wrong (10 LIKE of a 20 LIKE pool was -5.44 rather than 0.5), and a denominator wrapping to zero would panic. Both the persist path and the resync diff use it. The ratio is now built from the fullbig.Int.resync --apply(default five blocks behind head) could overwrite rows that already include events from those blocks and roll them back for good.--applynow refuses a snapshot block older than the latest staking event processing or processed, checked after the build. It is not a lock; the workers must still be stopped. Also fixes the shared staking event name list, which hadAllRewardsClaimed/RewardAdded(never emitted) instead ofAllRewardClaimed.