[Runtime Epoch Split] (7/n) Split runtime everywhere else

[robin-near]

This does the bulk of the remaining split, which is all a bit intertwined. Although there are a ton of changes, the transformations are simple for the most part.

RuntimeWithEpochManagerAdapter was the previous combined trait. There were two implementations of that trait: NightshadeRuntime and KeyValueRuntime.

Now, RuntimeWithEpochManagerAdapter is split into three: EpochManagerAdapter, ShardTracker, and RuntimeAdapter:

• EpochManagerAdapter is a direct abstraction of EpochManagerHandle (which is basically a Mutex<EpochManager>). The reason why we cannot use EpochManagerHandle directly instead of a trait is because KeyValueRuntime cannot provide a concrete EpochManager implementation. It has a completely custom validator schedule that is not compatible at all. NightshadeRuntime, on the other hand, contains an embedded EpochManagerHandle.
• ShardTracker is just a TrackedConfig plus an Arc<dyn EpochManagerAdapter>. It doesn't belong to EpochManagerAdapter because the TrackedConfig is not inherent to the protocol. ShardTracker is responsible for one thing: calculating whether this node itself cares or will care about a specific shard. For querying whether some other validator is supposed to track a shard, use the EpochManagerAdapter, not ShardTracker.
• RuntimeAdapter is an abstraction of everything that has to do with the runtime and account storage, such as get_trie_for_shard. It does bleed a little bit into the EpochManagerAdapter responsibilities, more on that later.

How does this split work for the two implementations: NightshadeRuntime and KeyValueRuntime?

• NightshadeRuntime is split into EpochManagerHandle, ShardTracker, and NightshadeRuntime. So essentially, EpochManagerHandle is moved out (well, conceptually, but it still keeps a reference to it because some runtime methods depend on it - eventually this can be moved out as well and have the EpochManagerAdapter be passed in), ShardTracker is moved out, and NightshadeRuntime itself now only implements RuntimeAdapter.
  • When constructing the NightshadeRuntime, we must first construct the EpochManagerHandle. This is because we cannot have the NightshadeRuntime internally construct a different EpochManagerHandle, or else the two EpochManagers will not synchronize state, and things fail.
• KeyValueRuntime is split into KeyValueEpochManager, and KeyValueRuntime. There's no ShardTracker specific to KeyValueXXX anymore (because it's a concrete type to begin with). KeyValueRuntime takes as a constructor argument KeyValueEpochManager; but here it's really just to help initialize some fields (num_shards, epoch_length, etc.).

Now, we talk about how users of this split are modified accordingly:

• Wherever a struct used to keep a field runtime_adapter: Arc<dyn RuntimeWithEpochManagerAdapter>, it is now split into three fields, epoch_manager: Arc<dyn EpochManagerAdapter>, shard_tracker: ShardTracker, and runtime: Arc<dyn RuntimeAdapter>, but we can omit some of them if they are not actually used. Usages of the old field are individually changed to use only one of the new fields.
  • Note that queries to self.runtime_adapter.cares_about_shard(..., ..., ..., is_me: true) is changed to self.shard_tracker.care_about_shard(..., ..., ..., true) whereas self.runtime_adapter.cares_about_shard(..., ..., ..., is_me: false) is changed to self.epoch_manager.cares_about_shard_from_prev_block, because the latter does not actually need the ShardTracker.
  • Wherever we used to convert a runtime_adapter into one of the subtraits, we now must construct the subtrait directly:
    • any Arc<dyn RuntimeWithEpochManagerAdapter> argument that was passed in is split into up to three arguments depending on which are actually used.
    • any direct construction of NightshadeRuntime is easily converted to separate constructions of EpochManager, NightshadeRuntime, and ShardTracker.
    • any direct construction of KeyValueRuntime is easily converted to separate constructions of KeyValueEpochManager, KeyValueRuntime, and ShardTracker.
• The TestEnv is one big part of this refactoring. The TestEnvBuilder used to take a .runtime_adapters(Vec<Arc<dyn RuntimeWithEpochManagerAdapter>>), but now we don't have the RuntimeWithEpochManagerAdapters anymore. We could split this into three, of course, but this would present a pretty bad inconvenience to any tests that call helpers like create_nightshade_runtimes(...). So, the logic is now the following:
  • There are now .stores(...), .epoch_managers(...), .shard_trackers(...), and .runtimes(...) overrides available. These must be overridden in an appropriate partial order (epoch_managers > stores, shard_trackers > epoch_managers, runtimes > epoch_managers, and also, stores > clients, epoch_managers > validators).
  • Any override that is not explicitly provided will be constructed with a default (where N is the number of clients):
    • stores will be default-constructed as N elements of create_test_store().
    • epoch_managers will be default-constructed as N elements of KeyValueEpochManager using the stores we have (this is why the stores must be finalized, either default-constructed or overridden, first, thus the partial order)
    • shard_trackers will be default-constructed as N elements of ShardTracker using the epoch_managers we have, with empty tracking config.
    • runtimes will be default-constructed as N elements of KeyValueRuntime using the stores and epoch_managers we have.
  • In addition, there are helpers that allow overriding with something other than the default:
    • .track_all_shards(): constructs shard_trackers that use the epoch_managers we have but with AllShards tracking
    • .real_epoch_managers(): constructs epoch_managers as N Arc<EpochManagerHandle>s instead of KeyValueEpochManager
    • .nightshade_runtimes() (this is an extension method that lives in integration-tests, to avoid a dependency cycle): constructs runtimes as N Arc<NightshadeRuntime>s using the stores and epoch_managers we have.
  • As a result, most of the constructions in integration-tests have been converted to using only these helper functions, as opposed to constructing runtimes directly in the tests. Some tests outside of integration-tests still construct directly, which is OK as they used to do that as well; we just now have to make sure to override stores, epoch_managers, and runtimes together so they are consistent.

[mzhangmzz]

Overall the PR looks fine, since it's replacing runtime_adapter with epoch_manager and fixing tests. You don't need to split it more.

[mzhangmzz]

Could you rename this field back to runtime_adapter? I think that will decrease the number of changed lines in this PR and I don't see a concrete reason for the rename

[mzhangmzz]

Maybe MockEpochManager/SimpleEpochManager is a better name? It doesn't always need to be used together with KeyValueRuntime.

[robin-near]

Some Nayduck tests are failing. I'll look into why. https://nayduck.near.org/#/test/455339

[robin-near]

Latest run is Nayduck-neutral.

[Longarithm]

I didn't look into code, but I have questions based on PR description:

1. Who calls ShardTracker::care_about_shard with is_me = false? Looks like no one, can we omit this parameter then?
2. Getting rid of create_nightshade_runtimes and other helpers is awesome. Can we add partial order of epoch_managers, stores, shard_trackers, ... to the comments over TestEnvBuilder to make it obvious for future test writers?

Follow-up question: after this it should be easier to test protocol upgrades because we can use mock epoch managers, right?

[robin-near]

@Longarithm thanks for taking a look!

1. There's one place that uses is_me = false, but also with account_id = None. This... actually becomes equivalent to is_me = true and account_id = None. (ugh). I didn't bother changing this here, but I do plan to do another simple refactoring that changes:

   • ShardTracker::care_about_shard --> ShardTracker::care_about_shard_myself
   • EpochManagerAdapter::cares_about_shard_from_prev_block --> EpochManagerAdapter::validator_tracks_shard

   that way the naming is a lot clearer.

2. Sure. Let me add that in a separate PR, I've had enough of this sanity check nonsense lol

We should NOT use MockEpochManager. It's just soooo incorrect. Same with KeyValueRuntime. Please avoid them. The real EpochManager is actually pretty easy to use because it doesn't have many dependencies and only needs a few store columns, and we can still make it select a predictable set of block producers and chunk-only producers. Let me know if I can help there!

[Longarithm]

We should NOT use MockEpochManager. It's just soooo incorrect. Same with KeyValueRuntime. Please avoid them.

Ow, it wasn't obvious from description. Thank you!
In protocol upgrades tests I want to trigger protocol upgrade when I want instead of waiting for some magical number of blocks, but it is a separate issue then.

[robin-near]

In protocol upgrades tests I want to trigger protocol upgrade when I want instead of waiting for some magical number of blocks

I see, you can change the epoch height to be say 5, and then it shouldn't take very long. But in general forcing the protocol to do something that it isn't supposed to do will just break a hundred other things.