Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
warner opened this issue Jun 9, 2022 · 2 comments
Open

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

warner opened this issue Jun 9, 2022 · 2 comments
Labels
enhancement New feature or request SwingSet package: SwingSet

Comments

@warner
Copy link
Member

warner commented Jun 9, 2022

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:

  • have @erights or @Chris-Hibbert look at vat-timer's makeNotifier, decide whether it meets the "cannot run away" criteria above
  • do a survey of our existing contracts, see if any of them are currently using repeaters (search for makeRepeater or makeNotifier)
  • do a survey of our contract authors (on Slack or something) and confirm that everybody is ok with removing makeRepeater (and maybe makeNotifier if it doesn't meet our criteria)
  • delete the feature
  • add a ticket to create the replacement, if the existing makeNotifier isn't sufficient

Description of the Design

Security Considerations

Test Plan

@warner warner added enhancement New feature or request SwingSet package: SwingSet labels Jun 9, 2022
@Chris-Hibbert
Copy link
Contributor

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
Copy link
Member Author

warner commented Jul 13, 2022

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)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request SwingSet package: SwingSet
Projects
None yet
Development

No branches or pull requests

3 participants