make vat-timer upgradable

[warner]

What is the Problem Being Solved?

To satisfy #5666 and make vat-timer upgradable, it needs to be rewritten in terms of durable Kinds.

Description of the Design

The timer vat exposes four kinds of objects (search for Far() to find their definitions):

• root: this doesn't need changes, and the only method it exposes is createTimerService
• timer service: this needs to become a singleton durable Kind, with methods getCurrentTimestamp, setWakeup, removeWakeup, makeRepeater (unless can we get rid of Repeaters in vat-timer? #5555 allows us to remove it, which would be great), makeNotifier, and delay
  • the timer service receives the TimerDevice in createTimerService(), and it must either stash it in the singleton's state, or in baggage
• a vatRepeater will be an instance of a durable Kind, with schedule and disable methods
• a delayHandler will be the same, with a single wake method. This object is used internally.

Note that vat-timer communicates with device-timer, mostly using integer indices rather than object references. We shouldn't need to modify the device to complete this task, however we might consider rewriting it as well, especially if we found a good way to address problems like #4286.

Security Considerations

Now that I'm looking at vat-timer after several years (!), I notice that the authority to do a removeWakeup(handler) comes from holding the same handler object that was originally passed into setWakeup(baseTime, handler). This isn't the best pattern: you can imagine somebody incorporating the handler methods (just wake()) into some larger object, then sharing that object with other parties. This would inadvertently give those parties the authority to cancel the timer.

A better pattern would be for setWakeup to create and return a "remover" object, and for the caller to hold onto that object if they wanted to ever call E(remover).remove() in the future. That would require client changes, though, and I'm not sure how widespread setWakeup currently is within our codebase (and external contracts/dapps).

I'm also reminded of the following warts:

• the root object (available to bootstrap as vats.timer) is not the actual timer service
• userspace bootstrap is obligated to wire vats.timer and devices.timer together (see kernel should automatically connect its vat/devices at startup #4523 or convert timer device to "raw device", remove userspace wiring requirement #4492)
• E(vats.timer).createTimerService() could be called multiple times, with (maybe?) bad results

I don't know that the upgradable/durable changes would affect/improve any of these, but it'd be nice.

Test Plan

As with #5667, either unit tests of actual upgrade, or a careful audit for lingering non-durable state.

[warner]

More plans:

• to remove the object-reference leak that currently happens because devices don't do GC (and device-timer is given the wakeup handler object), I'm going to move most of the functionality out of device-timer and into vat-timer
  • this doesn't actually require the code of device-timer.js to change, although once I'm done I could remove some of it
  • the device will provide two services to the vat:
    • 1: D(devices.timer).getLastPolled(): fetch the current time
    • 2: D(devices.timer).setWakeup(when, handler): please send a message to handler at time when or later
  • vat-timer will implement a (durable) table of what needs to be woken up and when, plus the repeaters
  • device-timer will still cling to handlers forever, but the only handler it ever sees is a long-lived one in vat-timer
  • vat-timer sees all the userspace handlers, and drops them when appropriate
• the E(timerSvc).setWakeup() call will acquire a third "cancel token" argument, matching the pattern used by FinalizationRegistry (and others)
  • (actually maybe this is the time to introduce an options bag)
  • if provided, the cancel token must be a Far object
  • it can be the wakeup handler object, but it could also be something else
  • E(timerSvc).cancelWakeup() takes the cancel token, instead of the handler
  • this enables shared handlers that can be cancelled independently
    • although the handlers are called identically, as there's no additional context object (like FR's heldObject)

@gibson042 kindly found some docs that will need to be updated with this API change: https://agoric.com/documentation/repl/timerServices.html (with source in https://github.com/Agoric/documentation/blob/main/main/repl/timerServices.md)

[warner]

While I'm doing this rewrite, I'm also looking to clean up a misbehavior of the repeater.

The API was documented (https://agoric.com/documentation/repl/timerservices) as makeRepeater(delay, interval), but the prose said the wakeups happen "repeatedly at times that are a multiple of the specified interval following baseTime", even though baseTime is a parameter of setWakeup(), not makeRepeater(). I think this was a cut-and-paste error from the setWakeup() paragraph.

Internally, the code was written as addRepeater(startTime, interval), which implies the first argument specifies the earliest time the waker might possibly fire. The implementation used nextWakeup = now + interval - ((now - startTime) % interval) which cheerfully fires the wakeup earlier than startTime.

Given that confusion, I'd bet that any clients aren't really getting what they think they're getting. The only examples of makeRepeater uses in agoric-sdk seems to be tests or the one real call in

agoric-sdk/packages/zoe/src/contracts/priceAggregator.js

Line 88 in 3bc3005

const repeaterP = E(timer).makeRepeater(0n, POLL_INTERVAL);

which uses makeRepeater(0n, POLL_INTERVAL), which might mean "fire every POLL_INTERVAL starting now", or "fire every POLL_INTERVAL after the epoch", and I imagine either one is probably good enough for its purposes.

I'm guessing that the most useful API for contracts is "given startTime and interval, fire the wakeup at the earliest value of startTime + k * interval that is still in the future". So when now is the time at which the addRepeater request is received by vat-timer:

now	startTime	interval		wakeup
0	35	10	->	35
1	35	10	->	35
..	35	10	->	35
34	35	10	->	35
35	35	10	->	45
36	35	10	->	45
..	35	10	->	45
44	35	10	->	45
45	35	10	->	55

The algorithm is the same as before, except with an extra test before the math:

  if (now < start) {
    return start;
  }
  return now + interval - ((now - start) % interval);

That would allow clients who care about scheduling the first wakeup precisely can do so: as long as they call addRepeater early enough that their request arrives before startTime, the exact arrival time does not matter. If the set startTime to the current time, the wakeup will be scheduled one interval later, under the theory that if they wanted an immediate wakeup they could have just called their own handler themselves.

At @dtribble 's suggestion, I'm also going to make a more significant change to the repeater: we won't reschedule the repeater until the handler message's result promise is fulfilled. This is the result from the E(handler).wake(scheduledWakeupTime) message that vat-timer sends to the client. So if that client returns a Promise that is never resolved, the repeater will never be re-armed. If the client does the usual thing and doesn't fire their result promise until all their work is done, and that work takes longer than interval, then this will act as a kind of back-pressure: they'll miss updates, but we won't send more wakeups than they can handle. And if the client handler throws, then the repeater will be cancelled entirely, which guards against ghost repeaters that keep knocking forever without an answer.

I think it's unlikely that client code would accidentally return a never-to-be-resolved Promise. The delete-on-handler-rejection thing automatically takes care of handlers in dead vats. I'm a tiny bit worried about one-time handler errors causing the repeater to be deleted and the client doesn't realize that it's gone, but I think clients can protect themselves against that by using the common try/catch/log/succeed wrapper pattern.

I also want to change the cancellation API. Currently, you can call setWakeup(when, handler) and then remove it with removeWakeup(handler). I'd like to use setWakeup(when, handler, cancelToken) and removeWakeup(cancelToken) instead. The cancelToken will act a lot like the FinalizationRegistry API: you can use the same token for multiple handlers, or you can omit it (and then you can't cancel that wakeup). I don't see any uses of removeWakeup in agoric-sdk outside of the simulated timer service that

agoric-sdk/packages/zoe/tools/manualTimer.js

Line 132 in 3bc3005

removeWakeup(waker) {

creates.

@erights @Chris-Hibbert do these seem right to you? If so, I'll proceed with changing the API and docs to this more precise form.

[Chris-Hibbert]

Those all seem like good changes. Thanks for the thorough analysis, @warner

[warner]

I just finished implementing the E(timerService).makeNotifier API, and I need to decide whether it should accept startTime or delay.

The docs for this one are consistent: both the API signature and the prose use delay. The old implementation correctly treated the argument as delay (it used a different code path, with makeTimedIterable).

The five non-test uses of makeNotifier in agoric-sdk are all using a delay of 0n (and apparently makeNotifier is more convenient than makeRepeater, which has only one non-test use, which also uses 0n as the first argument).

• inter-protocol/src/feeDistributor.js

  agoric-sdk/packages/inter-protocol/src/feeDistributor.js

  Lines 76 to 79 in 9cb561b

  	const collectionNotifier = E(timerService).makeNotifier(
  	0n,
  	collectionInterval,
  	);

• inter-protocol/src/vaultFactory/vaultManager.js

  agoric-sdk/packages/inter-protocol/src/vaultFactory/vaultManager.js

  Lines 186 to 188 in 9cb561b

  	const periodNotifier = E(timerService).makeNotifier(
  	0n,
  	factoryPowers.getGovernedParams().getChargingPeriod(),

• inter-protocol/src/runStack/runStakeManager.js

  agoric-sdk/packages/inter-protocol/src/runStake/runStakeManager.js

  Line 116 in 9cb561b

  const periodNotifier = E(timerService).makeNotifier(0n, recordingPeriod);

• wallet/api/src/date-now.js

  agoric-sdk/packages/wallet/api/src/date-now.js

  Line 45 in 9cb561b

  const timerNotifier = E(timerService).makeNotifier(0n, timerPollInterval);

• zoe/tools/scriptedPriceAuthority.js

  agoric-sdk/packages/zoe/tools/scriptedPriceAuthority.js

  Line 69 in 9cb561b

  const notifier = E(timer).makeNotifier(0n, quoteInterval);

  • (ok that might be a testing tool)
• zoe/src/contracts/priceAggregator.js

  agoric-sdk/packages/zoe/src/contracts/priceAggregator.js

  Line 88 in 9cb561b

  const repeaterP = E(timer).makeRepeater(0n, POLL_INTERVAL);

So that made me question my assumption that makeRepeater(start, interval) is more useful than makeRepeater(delay, interval). Or at least I don't want to go through the whole monorepo and replace makeNotifier(0n, interval) with makeNotifier(await E(timerService).getCurrentTimestamp(), interval).

I have a new repeater API implemented, which accepts a cancelToken rather than returning a control object: E(timerService).repeat(start, interval, handler, cancelToken=undefined). Nobody is using it yet, so we can pick whichever of start or delay is most useful. It might be handy to have at least one API that allows for a very specific start time, but if everybody using this really wants delay=0, then only giving them start is one more hoop to jump through.

Is there some economic algorithm that would be better implemented with (start, interval)?

Our repeaters are necessarily laggy anyways (as is everything in a causal universe), and lossy (if the lag or handler execution time is large enough to miss the next update), so it's not like userspace can assume they'll get exactly N callbacks in (N*interval) time units. They'll get woken up soon-ish after each interval, and they must be prepared to do 1 or 2 or N or 0 actions, depending upon when exactly they woke up (which they can't know with precision anyways, because async; the wakeup event tells them the time the wakeup was scheduled for, which is probably good enough for figuring out how much work needs to be done now, but they can't assume that the argument will always be N units larger than the last time they were called).

@Chris-Hibbert @erights does that usage information update your opinions about the API? I think I want to leave makeNotifier alone (and using delay), but now I'm considering having both makeRepeater and repeat accept delay too. Or maybe removing the parameter entirely in the new repeat API, and only take interval.

Current API proposal would then be:

• (cancelToken is always optional)
• ts.getCurrentTimestamp() returns now (ish)
• ts.setWakeup(when, Handler, cancelToken)
• p = ts.wakeAt(when, cancelToken) fires with when
• p = ts.delay(delay, cancelToken) fires with now + delay
• r = makeRepeater(delay, interval)
  • r.schedule(handler) called with handler.wake(when)
  • r.disable()
• ts.repeat(interval, handler, cancelToken) calls with handler.wake(when)
• n = ts.makeNotifier(delay, interval, cancelToken)
  • p = n.getUpdateSince() fires with when or rejects with { name: 'TimerCancelled' }
  • iter = n[Symbol.asyncIterator]()
    • p = iter.next() fires with { value: when, done: false }
• ts.cancel(cancelToken) cancels anything that took a cancelToken
• brand = ts.getTimerBrand()
• clock = ts.getClock()
  • clock.getCurrentTimestamp() returns now (ish)
  • brand = clock.getTimerBrand()
  • and no other methods: no wakeups or delays or repeaters or notifiers
• brand.isMyTimerService(ts) returns boolean

[Chris-Hibbert]

I don't see a need for a start parameter at this point. I was probably thinking that there would be reasons to take some actions near the beginning of the business day, but you're right that our timers are not tied strongly enough to wall clock time for that to be sensible.

Our approach to charging interest takes the various kinds of jitter and drift you mention into account. Compare the time of notification with the last time that interest was charged, and charge for the number of complete interest periods that intervened, then update the most-recent time by that same number of intervals.

I would prefer to have delay available, but I can't think of anything that wouldn't be possible without it. It merely seems like something clients would expect to be able to specify.

[warner]

Ok, then I'll go with all the repeater calls (the old makeRepeater, the old-but-most-useful makeNotifier, and the new repeat) take (delay, interval), knowing that setting delay=0 is easy and common but not obligatory.

[warner]

That makes the new API:

• (cancelToken is always optional)
• ts.getCurrentTimestamp() returns now (ish)
• ts.setWakeup(when, Handler, cancelToken)
• p = ts.wakeAt(when, cancelToken) fires with when
• p = ts.delay(delay, cancelToken) fires with now + delay
• r = makeRepeater(delay, interval)
  • r.schedule(handler) called with handler.wake(when)
  • r.disable()
• ts.repeat(delay, interval, handler, cancelToken) calls with handler.wake(when) (note 'delay' not 'start')
• n = ts.makeNotifier(delay, interval, cancelToken)
  • p = n.getUpdateSince() fires with when or rejects with { name: 'TimerCancelled' }
  • iter = n[Symbol.asyncIterator]()
    • p = iter.next() fires with { value: when, done: false }
• ts.cancel(cancelToken) cancels anything that took a cancelToken
• brand = ts.getTimerBrand()
• clock = ts.getClock()
  • clock.getCurrentTimestamp() returns now (ish)
  • brand = clock.getTimerBrand()
  • and no other methods: no wakeups or delays or repeaters or notifiers
• brand.isMyTimerService(ts) returns boolean

[warner]

Hm, TODO: brand.isMyClock(clock). @erights ?

[Chris-Hibbert]

Hm, TODO: brand.isMyClock(clock)
I don' think we need it.