fix: cap the amount of excess providers per block#480
Closed
pablocampogo wants to merge 1 commit into
Closed
Conversation
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.
fix(dex): drain over-cap liquidity pools across multiple blocks
Problem
PR #478 lowered
MaxLiquidityProvidersfrom 50,000 to 5,000 and added a one-timemigrateLegacyLPsstep to prune pools that were already over the new limit. That migrationevicted the entire excess in a single block.
A nested chain whose pool had reached the old 50,000 limit therefore had to evict 45,000
providers (each a state write + withdraw event, over a fully (de)serialized 50k-entry pool)
in one
ApplyBlock. That pushed block application to ~4.1s, which exceeds the per-phaseconsensus budgets (
ProposeTimeoutMS = 2500ms,ProposeVoteTimeoutMS = 4000ms): the blockcould never be applied-and-voted within a round, so it was re-proposed every round and never
committed — the chain livelocked / halted.
A sibling chain that had only ~23,000 providers pruned fine, because evicting ~18,000 fit
inside the round budget. The failure is purely a function of how much eviction work is packed
into one block — the migration path had no per-block bound (unlike order settlement, which is
already capped by
MaxOrdersSettledPerBlock).Change
Bound the legacy LP drain to a fixed number of evictions per block so applying the block
always stays within the consensus round budget. An over-cap pool now drains toward the limit
across multiple blocks instead of in one oversized (unappliable) block.
lib/certificate.go— addMaxLPEvictionsPerBlock = 5_000, mirroring the existingMaxOrdersSettledPerBlockrationale (cap per-block work to the consensus round budget).fsm/dex.go— inmigrateLegacyLPs, capexcessatMaxLPEvictionsPerBlock. Theexisting deterministic lowest-first ranking and eviction logic are unchanged, so each pass
removes the next-lowest 5,000 providers; the dead address is never evicted. Updated the
function doc since the drain is no longer one-shot.
fsm/dex_test.go— updatedTestHandleBatchDepositMigratesPoolBeforeDeposits(previously asserted a single call drained 50k→5k) to assert the new behavior: one batch
prunes exactly
MaxLPEvictionsPerBlock, and repeated batches converge to the 5k cap withoutever evicting the dead address.
Effect
comfortably inside the propose (2.5s) / propose-vote (4s) windows, so blocks commit and the
chain makes progress.
failure —
DexBatch.CheckBasicrejecting aRootDexBatchcarrying >5,000PoolPoints— isnever reached during the drain.
excess <= 0, migration is a no-op).Notes / follow-ups
out coordinated (same as the feat(dex): cap LP providers with deterministic replacement #478 deploy).
handleBatchDepositpath), i.e. it advances on blocks that process a liquidity deposit, notunconditionally every block. On a live DEX chain deposits flow continuously so it converges,
but a possible follow-up is to move the drain into an automatic begin_block step so it runs
independent of deposit flow (and to relax the
CheckBasicPoolPointsbound for pools largeenough that draining would exceed the ~60-block liveness window — not a concern at 50k).