store: `TrieAccountingCache` is pretty iffy part of our codebase

[nagisa]

TrieAccountingCache (hereon TAC) is pretty iffy part of our codebase.

First, it is currently mostly unused for the purpose of caching, but rather is chiefly a tool for discriminating first and subsequent store accesses from contracts for the purposes of charging different amounts of gas. Since the accesses now always go to the memtrie, it is up for debate if these fees have any reason to be any different at all, since all accesses now cost the same. But at the same time we can't exactly get rid of TAC as it is necessary for support of older protocol versions (replayability.)

There is point to be made to replace TAC with just a mechanism that tracks whether access to a key happens for the first time or not, and just maintain the access type counters. In making this change the cache would stop storing the values and only store the keys that have been accessed.

Second, the current enable_switch mechanism that's part of TAC (and relatedly the Trie::charge_gas_for_trie_node_access field) is super iffy especially as we look towards sharing this type across multiple threads. #12981 made the switch somewhat more palatable by making the switch track state on a thread-local basis, but ideally each access should be passing an argument to describe whether an access should increment the TAC counters (mostly enabled just from the context in which contract execution occurs,) whether an access should incur a side effect of recording it to state witness (subsuming get_no_side_effect), etc. Passing an argument describing the effects makes the whole store concept much more friendly to it being used in a multi-threaded context.

[nagisa]

As a first attempt over today I have gone ham with adding an equivalent of

#[derive(Clone, Copy, PartialEq, Eq, Debug)]
pub enum KeyLookupSource {
    FlatStorage,
    Trie,
}

#[derive(Clone, Copy, Debug)]
pub struct LookupMode {
    pub(crate) source: KeyLookupSource,
    /// whether TAC gets used
    pub(crate) track_access: bool,
    /// replaces current `no_side_effects`
    pub(crate) record_witness: bool,
}

impl LookupMode {
    pub const FLAT_STORAGE: Self =
        Self { source: KeyLookupSource::FlatStorage, record_witness: true, track_access: false };
    pub const TRIE: Self = Self { source: KeyLookupSource::Trie, ..Self::DEFAULT };
    const DEFAULT: Self = Self::FLAT_STORAGE;
}

to all trie methods on recursive demand. Unfortunately in many of the call sites it is pretty difficult to determine whether LookupMode::TRIE or LookupMode::FLAT_STORAGE should be used, not to mention where the track_access should or shouldn't be set. I do have plan for track_access which is basically adding a thread-local dependent assert to make sure that all contract execution goes through tracking. Everything else is less straightforward unfortunately.

But I can see the light at the end of the tunnel.

[nagisa]

Alright, so here are a couple of things that lead me to thinking that an argument passing is not the right approach here. But what we have currently with regards to storage operations isn't right either. I think there are a couple of things that need to be addressed before anything else:

Get rid of any and all logic around different storage types from near-vm-runner: it doesn't need to and should not ever need to have this knowledge. It should not care about changes in storage backends as the protocol versions change and it should not care about specific details of how storage implementations behave with regards to fees and costs. Most notable offenders of this are:

• the "TTN" logic in near-vm-runner/src/logic/logic.rs;
• anything related to enum StorageGetMode;
• io_tracer stuff;

In order to do this it is going to be necessary to have different storage frontends/adapters that cover the different use-cases and hold the implementations above. That is, a contract runtime would be operating on TrieUpdate<ContractRuntimeStoreAdapter> which would handle the TTN logic, pick between different storage modes, recording/accounting settings based on the protocol version or parameters, etc. Until that point we'd have the transaction runtime operate on TrieUpdate<TransactionRuntimeStoreAdapter> which would e.g. keep TTN counting disabled.

Note that this is not a new idea -- we already have a caching storage adapter (TrieCachingStorage.)

These adapter types would expose the usual contains/get/write/remove operations that take just key value. No configuration. No fancy invariants or logical invariants to the callers. All of that is a concern for the store code to implement. The only place where the concerns would leak out is the code constructing these adapters.

Internally it might make sense to still pass information from an adapter to the store as arguments in order to simplify multi-thread safety of the implementations, but as long as its all self-contained it doesn't matter as much.

Which leads me to an update on the update above: I have managed to get to a point where only ~20 tests fail. But those that do fail now are pretty hard to deal with… And more importantly the more I look at the result, the more the approach looks suspicious. The current implementation takes the extreme of making it impossible to skip recording a witness (something we know is sometimes undesirable; see *_no_side_effects) or counting TTNs. My approach takes the opposite extreme of making it super flexible in every call site. The ideal option is something in the middle where there are a few places that exchange the storage adapters for the desired behaviour in that area of the code.

[nagisa]

I have https://github.com/nagisa/nearcore/tree/experiment-mt-sound-storage-api with my previous attempt at an improvement here that I've abandoned.

---

Okay, after staring at the code for a little while I figured out a small incremental step to take:

1. Introduce trait StorageFeeCharger which has methods charge_touching_trie_node and charge_read_cached_trie_node;
2. Implement this trait for our GasCounter (I don't want to make this into a public API);
3. Modify trait External storage APIs to take &dyn StorageFeeCharger which the implementation of the External would call with appropriate node counts. Remove get_node_count.

This in particular allows:

1. Removing any concerns about TTN from the implementation of Logic -- this would become entirely a concern of External and then later an implementation of a particular storage type.
2. Removing the TAC enable/disable switch -- whether TAC is enabled or not would be controlled entirely based on whether a storage access comes from an implementation of External or not (which then can control enablement by passing around arguments and whatnot.)
3. Removing StorageGetMode type (this would all be a concern of an implementation of External now that Logic doesn't need to worry about touching storage in Trie mode in particular instances to maintain accurate TTNs.

[nagisa]

With #13194 the remaining step for the few upcoming changes in TAC area is to equalize trie access fees. After that its a couple months of lull before we can remove TAC (limited replayability feels good) altogether.