Agoric / agoric-sdk

monorepo for the Agoric Javascript smart contract platform
Apache License 2.0
322 stars 202 forks source link

can we get rid of Repeaters in vat-timer? #5555

Open warner opened 2 years ago

warner commented 2 years ago

What is the Problem Being Solved?

I'm a bit nervous about the possibility of abandoned Repeaters. If a contract talks to vat-timer and sets up a Repeater, then forgets about it (or dies) without shutting it down, it will keep running forever. Even if it only fires once a day, it will be adding to the background load of our chain, and the problem will just get worse and worse over time.

We talked about this in the last one or two kernel meetings. We suspect that most contracts are not using repeaters: one possibility is a daily interest calculation (@Chris-Hibbert ?). @erights suggested that we remove them entirely, and instead provide something closer to a Notifier, in which the recipient must explicitly re-register their interest after every wakeup. That would solve the runaway problem.

To avoid drift (since notifications might be delayed on the run-queue and arrive in a later block then they were requred), we'd need to tell the recipient when their wakeup was scheduled, so they could add the delta to the original time instead of the arrival time. We'd also need to handle the case where the delay was so large than an entire notification was skipped. We might want to build some sort of client library to encapsulate this logic, to make things easier.

vat-timer already has something named makeNotifier , whose implementation might already do this.. I can't quite tell.

So the task is:

Description of the Design

Security Considerations

Test Plan

Chris-Hibbert commented 2 years ago

Of the run-protocol contracts, only feeDistributor, runStake, and vaultManager rely on the timerService, and the only recurring service they use is E(timerService).makeNotifier(...).

I haven't looked at DApps, but I don't know of any that are reliant on chain-based timers.

@mfig says the priceAggregator (the only contract that uses makeRepeater()) would be fine rewritten with a notifier instead of a repeater.

I don't know how to tell if the Notifier produced byvat-timer.js is GC-able. @erights?

warner commented 2 years ago

Ok, short answer is "not now", I don't want to incur the work on contract authors before MN-1. I'll keep makeRepeater around, but before we open the chain up to other contracts, let's consider deprecating it or otherwise encouraging the use of something with less potential for long-term load.

Relatedly, maybe we should have the repeater pay attention to whether the handler succeeds or fails, and delete the repeater if it fails? That would at least give us automatic cleanup when the handling vat is terminated. We can't do this easily with the current implementation (devices don't do Promises), but in #5668 I'm planning to move most of the functionality from device-timer to vat-timer, which would enable E(handler).wakeup(time).catch(_err => deleteHandler(handler)).