Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(TasmObject)!: Move checks to compile time #153

Merged
merged 3 commits into from
Jan 13, 2025

Conversation

jan-ferdinand
Copy link
Member

Add trait TasmStruct, moving struct-only capabilities out of TasmObject. This change neither adds nor removes functionality, it only moves checks from runtime to compile time.

It is possible that this is not the cleanest design yet. I'm happy about opinions and suggestions.

@jan-ferdinand
Copy link
Member Author

On the topic of design decisions:

I'm a little unsure about function compute_size_and_assert_valid_size_indicator. All implementations for TasmObject for “primitive types” (like u32, BFieldElement, Digest) simply panic, claiming a statically known size as the reason. Container types over generics, like Vec<T>, arrays, tuples, and Option<T>, happily start computing and asserting, using either the static length of type argument T or recursing to T::compute_size_and_assert_valid_size_indicator. This feels like primitive types should have a non-panicky implementation of compute_size_and_assert_valid_size_indicator after all.

Maybe there's some complication with BFieldCodec not being a zero-cost abstraction in cases where it could be?

@aszepieniec
Copy link
Contributor

I'm missing a answer to "why?" -- what benefits does this change generate? (Or is the runtime / compile-time shift the only one?)

it [...] moves checks from runtime to compile time.

How?

@jan-ferdinand
Copy link
Member Author

How?

Primitive types (like u32, BFieldElement, Digest, etc.) continue to implement TasmObject. Structs continue to implement TasmObject, but additionally implement the new trait TasmStruct. The functions that provide field accessor code are moved from TasmObject to TasmStruct, as they only make sense for structs.

is the runtime / compile-time shift the only [benefit]?

Of this PR: yes. In my opinion, this is a worthy benefit by itself.

Extended motivation

I have the suspicion, and @Sword-Smith has confirmed this suspicion, that many tasm snippets access most or all fields of some struct. To reduce the use of field getters in such cases, I want to add a function to destructure an object into pointers to all its fields. I want that function to look something like this:

[derive(BFieldCodec, TasmObject)]
struct Foo {
    bar: u32,
    baz: XFieldElement,
}

let foo = Foo { bar: 13, baz: xfe!(0) };
let foo_ptr = bfe!(42);
let mut non_determinism = NonDeterminism::default();
encode_to_memory(&mut non_determinism.ram, foo_ptr, &foo);

let program = triton_program! {
    read_io 1               // _ *foo
    {&Foo::destructure()}   // _ *bar *baz
    pop 1                   // _ *bar
    read_mem 1              // _ bar (*bar - 1)
    pop 1                   // _ bar
    write_io 1              // _
    halt
};

let output = VM::run(program, PublicInput::new(vec![foo_ptr]), non_determinism).unwrap();
let [bar] = output[..] else { panic!() };
assert_eq!(bfe!(foo.bar), bar);

I don't want to provide the destructure function for TasmObjects for which destructuring does not make sense. While developing a way to distinguish between types for which destructuring makes sense from those for which it doesn't, I realized that accessing fields already has this property: there are TasmObjects for which accessing fields makes sense, and ones for which it doesn't. All the TasmObjects for which accessing fields makes no sense currently panic at runtime. In my opinion, removing the functionality to even try to access fields is the better approach. That's this PR.

Add trait `TasmStruct`, moving struct-only capabilities out of
`TasmObject`. This change neither adds nor removes functionality, it
only moves checks from runtime to compile time.
Previously, the implementations of `TasmObject`'s function
`compute_size_and_assert_valid_size_indicator` for “primitive types”
like `u32`, `Digest`, etc., panicked, claiming a statically known size
as the reason.

Now, the statically know length is produced.

Also, improve code formatting and a tiny bit of assembly.
@jan-ferdinand jan-ferdinand merged commit a4a5617 into master Jan 13, 2025
3 checks passed
@jan-ferdinand jan-ferdinand deleted the tasm_struct_pr branch January 13, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants