Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
305 stars 194 forks source link

Liveslots finalizers run metered #6795

Open mhofman opened 1 year ago

mhofman commented 1 year ago

Describe the bug

I don't see any code disabling / or restoring metering around finalizers execution.

Since metering differences may result in execution differences through the run policy (and in the future will be directly checked to be in consensus with #6770), we should be careful to exempt gc related tasks from metering. Conceptually related to #6784, but differently observable.

I have confirmed from looking at the xsnap-worker code that finalizers run after draining of the promise queue, but before executing any setImmediate callbacks. The callbacks themselves don't disable the metering, and I'm not sure if the callback invocation itself may not increment the meter.

Of course the finalizer itself should strive to not produce an observably different execution, which is currently covered by #6760.

mhofman commented 1 year ago

A possible solution is to:

warner commented 1 year ago

If we add computrons usage to consensus (perhaps via the transcript holding the delivery results), this becomes even more important.

warner commented 1 year ago

@mhofman and I discussed this yesterday. I'm kinda inclined to WONTFIX this, and stop trying to make our metering insensitive to GC.

We put a bunch of energy into making the computron usage not be affected by code running in GC-sensitvity situtations, but as this ticket notes, that work wasn't complete. And we haven't put any energy into trying to make allocation counters be unaffected (and it's probably impossible, since allocation affects the future much more than a computron counter).

So I'm not sure it's worth trying to hide that usage. And it would simplify the code a fair bit if we removed the unmetered wrappers.

warner commented 1 year ago

@mhofman and I agreed to move this out of Vaults, while we think about how to handle GC metering in general