David-Petrov commented 3 months ago

Description

A rather big refactor striving to make runtime invocations sandboxable, i.e. decouple them from the spring app context (dependencies on its beans) and any dependencies on global singletons. We do this by introducing an explicit Context object to manage those dependencies. It gets injected into the runtime instance on construction, thus enabling us to create instances with different execution contexts.

Detailed description

A detailed description can be found in each of the commit's messages:

refactor(TrieStorage)!: remove BlockState dependency

The TrieStorage class should serve as an abstraction over the DB (KVRepository db) for trie-related queries (thus handling finalised blocks only) and must not depend on BlockState related logic (which generally manages unfinalised blocks also).
refactor(TrieStorage)!: remove singleton

TrieStorage will not have a static singleton, will be a bean in production instead, and otherwise independently instantiable with the db dependency now in its constructor rather than in an initialize() method.
refactor(runtimeVersionV1): use the runtime builder instead of manually building the instance
- additionally, change around the scale encoding of the RuntimeVersion
The main benefit is we can take advantage of the wasm custom sections in newer runtimes instead of always calling Core_version. Although, for this particular hostapi endpoint, it could be more ergonomic to not tie ourselves to the builder... anyhow, did it.
refactor!: add runtime execution context
- StateRpcImpl: fix a bug in stateGetReadProof - block trie accessor must be initialized with state root for the block instead of the block hash
- runtime.allocator and runtime.memory packages: abstract the FreeingBumpHeapAllocator behind an Allocator interface. Also, abstract the org.wasmer.Memory behind a Memory interface, with which the Allocator operates.
- runtime.hostapi package:
- an Endpoint enum was introduced to hold the signatures for all Host API endpoints. Think of this class as containing "just data" - minimally necessary to define what a "Host API endpoint" is.
- an abstract HostApi class was introduced to define what an "implementation" of these endpoints must consist of. Basically, it requires an execution context be passed in order to even construct an implementation; and forces child classes (actual implementations) to provide a total list of import objects for every Endpoint.
- DefaultHostApi is the default production implementation, which has been decomposed into the previously known ...HostFunctions classes (as they're partitioned in the spec). All of those classes have been minimally changed just to adapt them to the new concept (no actual endpoint implementations have been severely changed within them). As a notable novelty, a PartialHostApi interface was introduced for ease during development and migration, but I find it apt and decided it's a nice addition. It's basic idea was similar to HostApi's, but with the crucial difference that it explicitly allows implementations to be partial (i.e. not for all endpoints). For now, all its implementors are the parts of the DefaultHostApi, so it's simply an implementation detail of the DefaultHostApi, but could easily be promoted to the public interface of runtime instantiation.
- Notable changes amongst the ...HostFunctions classes:
  - Each of them now accepts the minimal necessary dependencies on construction (which they receive from DefaultHostApi's Context) instead on relying on global static singletons or Spring app beans.
  - Writing/Reading shared memory is no longer a responsibility of Runtime, but has been extracted into a separate SharedMemory class doing only that.
  - Their constructors are intentionally made package protected (to emphasize their current encapsulation as an implementation detail of DefaultHostApi)
  - OffchainHostFunctions now operates with OffchainStorages, which will be described below (see storage.offchain package).
  - StorageHostFunctions experienced a little refactor in its scaleEncodedOption helper.
- runtime.Context class: introduced (roughly) to:
- hold the necessary dependencies from the outer world;
- hold the SharedMemory which is an internal component of the runtime instance, but exposes "public" functionality (public to implementors of the host API endpoints)
- cache the runtime version (and potentially other stuff in the future, which does not exactly belong as a field in Runtime) It is intended as an implementation detail of the Runtime class (not directly accessible to the outer world). It does have public getters since the current package decomposition inside runtime is more granular than java's opinion on nesting folders and visibility within nested packages, but it's not intended to be visible outside the runtime package. The only exception to this "intention" is the third overload of RuntimeBuilder::buildRuntime which for now (and that's not very nice) publicly exposes the context for the very eventual and rare case that someone would want to provide a custom implementation of the Host API (for which they'll need things like SharedMemory, which is internally constructed by RuntimeBuilder since it depends on the underlying org.wasmer.Instance for the memory export). That's a bit of mixture of intent, but I won't dig any deeper into that for now (not a problem).
- runtime.Runtime class: Intended as a wrapper around the underlying org.wasmer.Instance, with added execution context (and, for now, a reference to the org.wasmer.Module it originated from... but I'm not sure that's necessary since it's only used in .close()). Anyways, it doesn't handle memory operations anymore, and is rather empty for now, but this is the place where the public interface for interactions with the instance would go (methods like .executeBlock(Block block) (in future), getVersion(), etc.). Note that its constructor is package private, since constructing it is a concern of:
- runtime.RuntimeBuilder class: it's actually more of a RuntimeFactory in standard Java terms, but essentially it's the public constructor for runtime instances. Source code should be self-explanatory here.
- storage.offchain package: the previous OffchainStore has been abstracted to an implementor of the new BasicStorage interface, which was introduced to more simply represent the necessary for OffchainHostFunctions dependencies, which reside collected inside OffchainStorages. That's rather a leftover from my initial prototyping, but I like the simplicity this introduces in the supply chain through the Context.
- compareAndSet method was moved as default in the interface.
- package trie:
- .*TrieAccessor: trieStorage was made an explicit dependency injected on construction
- AccessorHolder: obtaining the TrieStorage from as spring bean has been moved here (scope boundary for my refactor).
---- TESTS ----
- BlockExecutorTest: a showcase of explicitly configured runtime instance executing a block in isolation. We want more of those.
- InMemoryDB introduced as a light testing repository
- RuntimeWasmTest: although disabled, rises an interesting question (bring up a separate discussion in the PR)
- all other tests have been refactored to adapt to changes, nothing noteworthy there.

Anything not explicitly mentioned in the above list is trivial (or I've missed to mention it, so feel free to raise any noticed oddities).

Fixes #375, #376, and the first big half of #355

For traceability's sake: this PR is the realisation of the POC showcased in #413.

Checklist:

[x] I have read the contributing guidelines.
[x] My PR title matches the Conventional Commits spec.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

David-Petrov commented 3 months ago

For some reason, not yet known to me, BlockExecutorTest doesn't pass in GitHub's test environment. We gotta fix this before merging.

David-Petrov commented 3 months ago

I also don't like the structure of the runtime package as-is. It was initially decomposed more sanely, but when I got to staging files I figured we'd lose the entire history (+ the PR's diffs would've become incomprehensible with all of the moved files), so I left this as a final step for a separate PR (only moving things around) after this one's done being reviewed.