reboot interleaving spec and instrument rt next - #185
Conversation
| // TODO: The hook receives the newly created component `Instance`, but | ||
| // Wasmtime currently has no API for obtaining the core debug instances | ||
| // instantiated underneath it. Until it does, scan the entire store and | ||
| // require exactly one core instance without registered trace state. This | ||
| // detects uninstrumented contracts, but the repeated full-store scan still | ||
| // makes n component instantiations cumulatively O(n²). Ideally Wasmtime | ||
| // would expose the component-to-core relationship (for example, | ||
| // `Instance::debug_core_instances`), allowing direct registration here. | ||
| let core_instances = store.as_context_mut().debug_all_instances(); | ||
| let unregistered_core_instances = core_instances | ||
| .into_iter() | ||
| .filter(|instance| { | ||
| store | ||
| .data() | ||
| .wasm_trace_state(instance.debug_index_in_store()) | ||
| .is_none() | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| let [core_instance] = unregistered_core_instances.as_slice() else { | ||
| bail!( | ||
| "instruction tracing requires exactly one unregistered core instance after component \ | ||
| instantiation, but found {}", | ||
| unregistered_core_instances.len() | ||
| ) | ||
| }; |
There was a problem hiding this comment.
@rvolosatovs this is low priority, but maybe you have any thoughts on this.
basically the tracer needs to know which core instance corresponds to which core program/source code
if you are running plain core wasm, normally you call instantiate and you can just attach the program metadata to that instance id
the problem is that a component instance is different from a core instance
because of this, here I'm basically doing:
-> instantiate component -> loop through all the current (core) instances -> find which one still does not have source metadata attached
this works, but feels kind of hacky, and also doesn't support the case where a component has more than one core module inside
There was a problem hiding this comment.
It looks like you could use https://docs.rs/wasmtime/latest/wasmtime/struct.Store.html#method.debug_register_component before instantiation, which will internally call https://docs.rs/wasmtime/latest/wasmtime/struct.Store.html#method.debug_register_module per each module in the component. This way you can establish the module -> component mapping.
At a later point you could maybe get the module of the instance through https://docs.rs/wasmtime/latest/wasmtime/struct.Instance.html#method.module and maybe use https://docs.rs/wasmtime/latest/wasmtime/struct.Module.html#method.same to establish a mapping from instance -> component
Would that work?
| let mut target_params = params.to_vec(); | ||
| target_params[0] = Val::Resource(utxo.resource()); | ||
| f.call_async(&mut store, &target_params, results).await?; |
There was a problem hiding this comment.
I don't think the implementation should be cloning the parameters and/or modifying them in any way. Why is this required?
| let (_, index) = self | ||
| .component | ||
| .get_export(Some(&self.instance_idx), name) | ||
| .context("function export not found in UTXO instance")?; | ||
| self.instance | ||
| .get_func(store, name) | ||
| .get_func(store, index) |
There was a problem hiding this comment.
this should be redundant, name should already carry the instance information. This lookup happens ahead-of-time
| component: Component, | ||
| instance_idx: ComponentExportIndex, |
There was a problem hiding this comment.
we should not need these, the export information is stored in the various *Export structs
| pub struct ConstructorExport { | ||
| ty: types::ComponentFunc, | ||
| idx: ComponentExportIndex, | ||
| instance_idx: ComponentExportIndex, |
There was a problem hiding this comment.
The idx should already encode this
There was a problem hiding this comment.
that's the constructor idx, not the utxo idx (which obviously doesn't make sense to have here of course)
tbh most of the change to this package were AI, and I didn't particularly pay that much attention to it because I was planning on dropping these (that's why I labelled the commit as tmp?). Same for the compiler changes.
the reason for this is that I wanted a test case of a coord script calling a utxo method (kind of dynamic dispatch, let's say?), which was not supported, so I hacked it together with a handwritten example.
this was added here to pass it down, to eventually be able to resolve the function from it by its name...
I guess there could be a pre-resolution step at some point and a lookup from the string out of it
In my case I just needed something to start playing around with the import/export instrumentation.
There was a problem hiding this comment.
You can use get_utxo_method to get the export index by name
Starstream/starstream-runtime-next/src/lib.rs
Lines 547 to 558 in 736af00
The key point is that the idx stored here uniquely identifies the export on the component, the instance index is irrelevant for the lookup. You do need the instance index if you are doing a string lookup, of course, but this API exists precisely to avoid string lookups.
There was a problem hiding this comment.
Yeah, I guess this needs to be exposed through UtxoHandler so that this function can reach that function somehow? Instead of putting it into the resource.
Starstream/starstream-runtime-next/src/lib.rs
Line 148 in 736af00
I'll assess this later, although maybe things will change again before I get to that
There was a problem hiding this comment.
You could maybe take a look at the ledger to see how this fits together:
| ty: types::Record, | ||
| get: ComponentExportIndex, | ||
| set: ComponentExportIndex, | ||
| instance_idx: ComponentExportIndex, |
There was a problem hiding this comment.
same as in others, this should be redundant
| async fn instantiate( | ||
| &self, | ||
| mut store: impl AsContextMut<Data = T>, | ||
| ) -> wasmtime::Result<Instance> |
There was a problem hiding this comment.
Instead of adding the hook support, let's make this function public and make it return a ContractInstance, which in itself would contain the contract and instance fields and provide accessors to these.
We should then move all methods doing the call to ContractInstance
| #[must_use] | ||
| pub fn new_wasmtime_config() -> wasmtime::Config { | ||
| let mut config = wasmtime::Config::new(); | ||
| config.wasm_component_model(true); |
There was a problem hiding this comment.
| config.wasm_component_model(true); |
Component model is enabled by default
Make `Contract::instantiate` public and have it return a new `repr(transparent)` `ContractInstance` wrapper around the wasmtime `Instance`. Move `create_utxo`, `load_utxo` and `call_coordination_script` onto `ContractInstance` and drop the `Contract`-level convenience wrappers, so callers instantiate explicitly and the runtime keeps track of the instance. This addresses #185 (comment) Signed-off-by: Roman Volosatovs <rvolosatovs@riseup.net>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
…strumented Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
…crates to legacy the circuit and the spec are still good references while we port things to the new package/runtime the runtimes however were mostly exploratory work, and while they provide some test coverage, it's for a circuit that would be deleted anyway the ledger integration is minimal and we'll need a new ledger spec too anyway Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
…names Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
pre-run quint typecheck on the input improve error reporting forbid traces with invalid init behavior (init is not circuit related) Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
the spec just needs to specify which orders are valid (like the circuit will do too), but we still need to be able to compute the right order Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
…ic one similar to the existing interleaving circuit, using pending cells for validating future actions Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
…or event template assignment) Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
… coverage Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
…asmtime_config Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
…te) + wire provisional io values in the runtime Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
Signed-off-by: Enzo Cioppettini <48031343+ecioppettini@users.noreply.github.com>
1929314 to
d8f179d
Compare
(still WIP/experimental)
Adds a new "interleaving" spec for the (still non existing) new circuit.
The reason for this is not that the existing circuit is necessarily wrong, a lot of the code will just be re-used, but the idea is to write something closer to the current state of the compiler + runtime, now that we have the first designs of actual execution.
The core part of this PR is the
starstream-interleaving-spec/spec/starstream.qntfile.It's role is the one that
interleaving/starstream-interleaving-spec/EFFECTS_REFERENCE.mdcurrently has (now renamed to starstream-interleaving-spec-legacy), plus themocked_verifier.rsin that package (which is a transliteration of the markdown spec)The reasoning for using quint is that
Extra notes:
This has some unrelated changes to the compiler and runtime. Mostly of things I needed to be able to get a somewhat interesting example to run.
The
abis-clearwas based on this Discord discussionFor now it's mainly a temporary patch, but the reason for it still stands.
This also removes the runtimes in
interleaving. I don't think those are worth keeping at this stage, we lose some small test coverage, but for a circuit/set of instructions that will get rewritten anyway. They were mainly useful for exploration of the problem. Still keeping the circuit code and the ledger specs as a reference.