Skip to content

[V132-specific] RollupSwapSlip calls only once per block (in spawnDerivedAssets)

Multipartite requested to merge Multi/single-block-rollupSwapSlips into develop

[V132-specific]

This is intended to close #1955 (closed)
'Uncontrolled growth of CurrentRollup'.


2024-04-24 update:
Coming back to this, I have simplified my original (total) diffs by call each RollupSwapSlip from within spawnDerivedAssets,
without duplicating getLayer1Assets code calling.

This has the effect of reverting the changed TestSpawnDerivedAssets unit test behaviour
(since multiple spawnDerivedAssets calls would again imply multiple RollupSwapSlip calls),
but at this time I have kept the earlier commits present in case of historical curiosity.
(Please tell me if commit squashing is preferred for reviewing.)

I also added a map[common.Asset]bool to reassure myself that this would not be able to call RollupSwapSlip for the same Asset if in future one were somehow an anchor for two different Assets.


[Update: The below is the initial description for if moving RollupSwapSlip calls out of spawnDerivedAssets to BeginBlock.]

Following conversation with @pluto-9r , and related to !3496 (merged) and !3450 (merged),
this aims to fix the uncontrolled-currRollup-growth bug
(see the discussion threads of both other MRs)
by calling rollupSwapSlips once in every NetworkMgr BeginBlock
(ideally rolling up once for each gas asset and once for each TOR Anchor asset, but feedback welcome)
and by in CalcAnchor (called multiple times a block) calling GetCurrentRollup (a read-online function) instead of RollupSwapSlip (which adds the previous block's slip to the rollup).

Perhaps following merging of !3496 (merged) and observation that all is as intended (the discrepancy no longer incrementing),
store migration SetCurrentRollup flushes will be appropriate (also resetting rollup counts to not then go negative,
or else (perhaps easier) DeletePoolSwapSlip for some previous blocks to avoid becoming negative)
are appropriate to shed the accumulated excess rollup.

(Please note the TestSpawnDerivedAssets unit test updates before and after.)

Edited by Multipartite

Merge request reports