Consider deduping ShardTries for Runtime and view Runtime

[Longarithm]

I recently noticed that if we use split store we have two different NightshadeRuntimes for Client and view Client, respectively:

nearcore/nearcore/src/lib.rs

Lines 239 to 266 in a5ede1d

	let runtime = NightshadeRuntime::from_config(
	home_dir,
	storage.get_hot_store(),
	&config,
	epoch_manager.clone(),
	);
	// Get the split store. If split store is some then create a new set of structures for
	// the view client. Otherwise just re-use the existing ones.
	let split_store = get_split_store(&config, &storage)?;
	let (view_epoch_manager, view_shard_tracker, view_runtime) =
	if let Some(split_store) = &split_store {
	let view_epoch_manager =
	EpochManager::new_arc_handle(split_store.clone(), &config.genesis.config);
	let view_shard_tracker = ShardTracker::new(
	TrackedConfig::from_config(&config.client_config),
	epoch_manager.clone(),
	);
	let view_runtime = NightshadeRuntime::from_config(
	home_dir,
	split_store.clone(),
	&config,
	view_epoch_manager.clone(),
	);
	(view_epoch_manager, view_shard_tracker, view_runtime)
	} else {
	(epoch_manager.clone(), shard_tracker.clone(), runtime.clone())
	};

AFAIK that's what we wanted to have for a long time, but there is a inaccuracy inside - we create separate ShardTries instances inside with ShardTries::new_with_state_snapshot. And this is bad because each instance has its own set of caches and view_caches. So now we have a copy of cache and view cache for each shard and each client. I don't think view client uses non-view cache though, it is only design issue.

I see two directions to resolve that:

• make ShardTries to store only caches of one type. But I'm not sure if non-view client wants to make view reads and whether it makes sense to do them and not use non-view cache. (Question: could it make sense to store ViewRuntimeAdapter for calls to view client inside non-view client?)
• pass existing ShardTries instance to NightshadeRuntime constructor.

cc @jbajic @wacban

[shreyan-gupta]

From my understanding about trie caches, we have two types of caches, caches and view_caches. Also, it seems like we have two instances of ShardTries, one for client and one of view_client.

Are we saying the view_client only uses view_caches and client only uses caches? In that case the ideal solution would be point 1, where we store cache of only one type.

[Longarithm]

Are we saying the view_client only uses view_caches and client only uses caches? In that case the ideal solution would be point 1, where we store cache of only one type.

Could be really nice to assume, but this is not the case yet.
RuntimeAdapter is too wide, you can create non-view and view tries using methods get_[|view_]trie_for_shard, which use non-view and view caches underneath, respectively.
I think it is good idea to separate them and store, say, is_view flag inside NightshadeRuntime, indicating which parameters for trie caches we should use.

We could start refactoring from separating non-view and view runtimes. To do so, we need to look over all places where get_view_trie_for_shard and new_trie_update_view are called, and replacing them with creating separate NightshadeRuntime instance and making view calls from it. Then perhaps we can remove get_view_trie_for_shard, deciding which trie we need in get_trie_for_shard and move all methods related to view calls to ViewRuntimeAdapter. It is worth double-checking with core team if this refactoring makes sense, @robin-near in particular. I can already confirm that supporting both kinds of caches in runtime is annoying and looks like a bad design.

...Or we can just cut corners and pass ShardTries instance to NightshadeRuntime::new :)

[shreyan-gupta]

I agree the right design should have a ViewRuntimeAdapter and perhaps a ViewShardTries interface as well, while we separate out the use of caches and view_caches to two different instances of ShardTries.

Passing ShardTries to NightshadeRuntime sounds like a good first step in either of these approaches. As discussed, we would like to work with the initialization order below

• Initialization order
  • genesis -> either from config or storage
  • flat storage manager
  • shardtrie (needs flat storage manager)
  • runtime adapter (needs FSM)
  • chain (needs runtime, genesis)