MAX_ENQUEUED_REMOVALS insufficient for gas limit errors. Datasets stuck

[ZenGround0]

We've observed an instance where the gas limit is being hit upon processing nextProvingPeriod. From @TippyFlitsUK:

Short version: while removing the duplicate pieces from Covalent's Storacha migration data set (1245), the scheduled removals made the next nextProvingPeriod tx too big to fit in a block. Gas estimation reverts, so nothing ever hits the chain... proving just silently stops. Every removal call was inside the allowed limits (500 per call, ~2k per proving period) - the caps happily let you build a state the chain can't actually execute.

This has always been my biggest outstanding worry about pdp. The guess at a safe value for the constant was wrong and so we are now observing a liveness issue. Moreover any guess for this constant is going to be wrong since PDPVerifier has no control over the listener contract which may happily eat up a large part of the message/block gas limit.

Here are some solution ideas

external processRemovals call processes the queue before nextProvingPeriod

One way to handle this is to allow partial queue draining via a separate call processRemovals which processes a user specified number of queue entries. When nextProvingPeriod fails with out of gas then callers like curio can move over to using this resumable queue draining call.

This maintains the invariant that all pieces added to the queue will eventually be processed while enusring livenss of nextProvingPeriod under the assumption that the listener call maintains liveness.

A listener call must be added for removalsProcessed so that it can handle tracking of removed pieces.

This new call would need to be handled with care to avoid grinding. It must be enforced that the proving set is not modified between challenge epoch sampling and proving. To accomplish this we will need a compliant listener that asserts that either proving has been missed or proving has completed this period before processing these calls. This is readily achievable with existing state in the filecoin-services contracts.

external cancelScheduledRemovals call empties queue before nextProvingPeriod

The other way to handle this is to simply cancel the queued removals to make space for nextProvingPeriod.

This removes the invariant that all pieces added to the queue are deleted before nextProvingPeriod which means grinding needs to be considered. Considering this further this operation is always safe. All manipulations on the proving set must occur between start of the current proving period and the call to nextProvingPeriod and operate only on the enqueued removals without impacting any proving state between challenge sampling and proving.

Note we'll still need a listener call scheduledRemovalsCancelled to help the listener revert any tracking that occured in the scheduling listener callback.

For this reason we could make cancleScheduledRemovals take in a count parameter or a list of piece ids and things would still be safe. But for simplicity I recommend just doing a single drop operation as an escape hatch guaranteeing liveness.

Recommendation

Cancellation is overall simpler for verifier and listener and reflects the fact that this is an escape hatch not a common case path. I also suspect it will be cleaner in curio.

We should also consider doing measurements on filecoin-services listener listener gas costs and PDPVerifier nextProvingPeriod gas costs and reduce the constant to something that better reflects a safe threshold in today's environment.

[snadrus]

The always-safe option sounds best to me
So Curio could send 1 remove when it gets the notice that Bulk Removes are cancelled, to verify. Then apply some edge-finding algorithm to find the smaller bulk level that'll work.

[rvagg]

Mostly this should be in the SP's hands to solve to keep things functioning. But, they do get paid for removals, so there should be ample remuneration for handling the hassle. One thing we should be aware of when making this flexible is that the removal fee is queued when schedulePieceDeletions is run, not nextProvingPeriod. So if we're going to allow the SP to de-schedule removals then we need to either move that fee or have a reverse rail or some other kind of debt .. we probably shouldn't move the fee into nextProvingPeriod, this issue shows why we try to keep things out of there.

I like a separate processRemovals(n) call prior to nextProvingPeriod if we can make it work with the proving system. That would solve the whole thing, if we shunt removals entirely to a separate call (make nextProvingPeriod block if there are removals pending) then we could even properly gas-account for the cost of removals and build that into any pricing update in the future, that would be neat, but would mean mandatory extra calls; so .. tradeoffs.

Another possibility might be a nextProvingPeriod(n) variant where the SP could choose to process some number of removals less than the total amount queued. But we'd need to consider how to hold the SP accountable for actually making the removals happen over time and not just collecting the removal fee, making a vague promise to the user, but letting pieces sit for many days, or even the entire term of the data set. The differential between "I want to delete these pieces" and when/if they are deleted is already annoying with the 2000 limitation; at least it's clear though, "no more than 2000 per period" is at least a coherent rule, if confusing for users.