maybe we should just use the same class name from different packages?
not set on this but to demonstrate
statetransition.attestation.v1
statetransition.attestation.v2
may be cache.v1 and cache.v2 or something... just the concept...

Nice idea.. this way we can see that essentially V1 isn't changed.

i've been thinking about this a little.... i think if we only ever use the start slot of the current epoch or start slot of previous epoch, then that's probably safest (because of limited cache largely)
we've got 2 options, i think simplest is

• like above, if attestation is current or previous epoch use bestState
• otherwise use first slot of previous epoch (which generally is cached still)

if we can easily log cache misses here it may be a good thing...

i'd probably avoid doing the start slot of the attestation epoch because that may be 2 epochs behind (per comment) and almost certainly require a regeneration from ... probably the justified state...

i do think we need a set of lock logic around the clean... i think i had a cleanup function in my draft that used a lock here...

The reasoning is really the degenerate scenario where 2 onSlot run because of a long prune and hilarity ensues.

I would suggest breaking this cleanup logic out, and having a lock where if its already running then just return

final AtomicBoolean isCleanupRunning = new AtomicBoolean(false);
...
  void cleanupCache(final Optional<UInt64> maybeSlot) {
    // one cleanup at a time can run
    if (!isCleanupRunning.compareAndSet(false, true)) {
      return;
    }

    try {
      if (maybeSlot.isEmpty()) {
        while (dataHashBySlot.size() > 1 && size.get() > maximumAttestationCount) {
          LOG.trace("Attestation cache at {} exceeds {}, ", size.get(), maximumAttestationCount);
          removeAttestationsPriorToSlot(dataHashBySlot.firstKey().plus(1));
        }
      } else {
        removeAttestationsPriorToSlot(maybeSlot.get());
      }
    } finally {
      isCleanupRunning.set(false);
    }
  }

something like this concept...

i'd prefer to have a cache thats too large for a period than a broken cache or a bunch of onSlots taking a long time

So you are thinking about that because you think that two onSlot could run concurrently?
I actually don't think it can happen because IIRC event channel always queues pending events and execute them in sequence. I'll double check.

yeh so removeAttestationsPriorToSlot is actually a dangerous function, it cant run from multiple sources concurrently...
In v1 it ran in add and in onSlot, and now it just runs in onSlot... that's good, as long as onSlot is not able to run more than once at a time.
The safer thing is to have a cacheCleanup thats behind a lock, and then it just skips if its already running. It still probably needs a 'dont call this anywhere else because' on that removeAttestationsPriorToSlot
This could be done in the onSlot, just having the same lock concept and exiting if the cache cleanup is already running (so isCleanupRunning could easily be inside onSlot rather than its own function and would be equivalent)

actually i think more correctly that concept of the looping and reducing is the fun bit, 2 of them running at the same time we want to avoid as we'll flush out more than we want in the worst case.