move TrieNodesCount to near-vm-errors #9208
Conversation
Ekleog-NEAR
added
the
C-housekeeping
See near#9176 for the rationale, near-vm-errors will be merged into near-vm-runner as soon as circular dependencies are done for.
Ekleog-NEAR
force-pushed
the
move-trie-nodes-count
branch
from
40dff5a
to
0eeaff8
Compare
Ekleog-NEAR
added
S-automerge
C-housekeeping
| @@ -7,6 +7,7 @@ use std::any::Any; | |||
| use std::fmt::{self, Error, Formatter}; | |||
| use std::io; | |||
|
|
|||
| // ----------8<---------- | |||
There was a problem hiding this comment.
A marker for the delimitation of the section of stuff that should be moved to near-vm-runner (ascii art for a line with scissors mark)
| @@ -30,6 +31,26 @@ impl fmt::Debug for dyn CompiledContractCache { | |||
| } | |||
| } | |||
|
|
|||
| /// Counts trie nodes reads during tx/receipt execution for proper storage costs charging. | |||
| #[derive(Debug, PartialEq)] | |||
| pub struct TrieNodesCount { | |||
There was a problem hiding this comment.
Sanity check: this introduces dependency of near-store -> near-primitives -> near-vm-errors. But if we merge near-vm-errors with near-vm-runner, does it introduce dependency of near-store on near-vm-runner?
There was a problem hiding this comment.
The end-goal of this is to make near-vm-logic, near-vm-errors, near-vm-runner and runtime into a single crate that will be depended on by the rest of nearcore. So yes, near-store will probably need to depend on near-transaction-runtime (or any other name we decide to give this crate), if only to implement the Store traits that will probably need to be defined in there.
Disclaimer: I’m not at all clear on what near-store exactly is, it’s currently outside my scope of perception; but the goal of all the current PRs is to make near-vm-runner depend on basically no other crate from nearcore. If the near-store -> near-vm-runner dependency is unwanted, we can introduce an intermediate crate that’d depend on both and make the translation from the structs of one to the other one.
(Also, I think near-primitives as a whole should completely cease to exist, and that might solve this specific issue if the dependency would not be by design. But that’s too much work to fit inside the resources allocated to limited replayability unfortunately, so I’m only dealing with the parts of near-primitives that we need to move in order to implement limited replayability)
There was a problem hiding this comment.
Just in case, the overall plan is currently described in #8197 (comment) though we’re still discussing alternative approaches for the later steps. But anyway, the first step is to make the codebase that we want to implement limited replayability for be completely scoped and independent from nearcore, because otherwise we’re sure we can’t do anything to improve on it (without such improvements being very wobbly and subject to breakage)
There was a problem hiding this comment.
Adding in comments now I’ve chatted with @jakmeier about it:
- Today’s situation is both runtime and store depend on primitives(-core)
- The ideal situation would be runtime and store are both completely isolated crates, and there is a third crate that depends on both to "tie the knot"
- This PR (and subsequent PRs merging crates) have as a goal to first make (contract) runtime be completely isolated. This naturally means that the shared dependency on primitives must become a dependency from store to runtime in order to keep things working
- A next step would bring us to the perfect world, by duplicating the
TrieNodesCountinto store and tying the knot from a crate that depends on both store and runtime - (If we wanted to make store isolated today, we would move
TrieNodesCountto near-store and make a dependency from runtime to store in the same way, but AFAICT we currently have no direct need of making store isolated, so it can wait until the "ideal situation" listed above)
There was a problem hiding this comment.
I see. Yeah I agree that near-store and near-vm-runner should be independent.
The ideal situation would be runtime and store are both completely isolated crates, and there is a third crate that depends on both to "tie the knot"
Question from a guy who never published a crate: why the third crate should go over first two, not under them, like near-primitives currently do?
Anyway there is no plan to make near-store independent so far. Still I would appreciate if all store&vm-related entities were in the same file, at least, to simplify lives of future engineers.
There was a problem hiding this comment.
Question from a guy who never published a crate: why the third crate should go over first two, not under them, like near-primitives currently do?
The reason is that if we want to publish runtime, then we also need to publish all its dependencies. Which would mean publishing primitives, and thus committing to a stable API for near-primitives.
Seeing how that’s probably not going to happen, going the other way around is much simpler: with a crate over the first two, the third crate doesn’t need to be published :)
Still I would appreciate if all store&vm-related entities were in the same file, at least, to simplify lives of future engineers.
Am I correct to assume that here you’re calling the runtime VM?
To sum up my plan for the "intermediate" architecture, it is currently "near-store is an implementation of the store abstraction exposed by the runtime." This would lead to all types and functions shared between store and runtime being in runtime for now, until we make store independent too and refactor things out
There was a problem hiding this comment.
No, I meant only VM, because TrieNodeCount is shared with VM. My understanding of what VM means is that it is the logic of contract execution and TrieNodeCount is used there to compute execution costs.
So if there is something else related to both store and VM, it seems nice to keep all that in the same place.
There was a problem hiding this comment.
Ok so what you’re talking about here is near-vm-runner, aka contract runtime. It’s actually not part of the wasm VM, but more a part of near-vm-logic; contract runtime is basically wasm VM + near-vm-logic (that implements host functions).
Anyway, I’m with you that anything else that’d relate both to store and VM should belong close together, probably in a single file if there’s not too much of it :)
See #9176 for the rationale, near-vm-errors will be merged into near-vm-runner as soon as circular dependencies are done for.
Here, the circular dependency comes from near-vm-runner depending on near-vm-logic, that needs TrieNodesCount.
see #8197