HotShot icon indicating copy to clipboard operation
HotShot copied to clipboard

[<VID>] - Segregate VID construction into a separate crate with WASM compatibility

Open ggutoski opened this issue 1 year ago • 8 comments

What is this task and why do we need to work on it?

Downstream applications need the vid_scheme constructor to compile to WASM: https://github.com/EspressoSystems/HotShot/blob/ddfaf0d12326b5122b2f2a6837ee0e78820a71f8/crates/types/src/vid.rs#L52

The best way to do this is to move it to a new crate that supports WASM. cc @nomaxg @alxiong has experience compiling to WASM. I don't know whom to assign so putting @shenkeyao for now. 🙏

What work will need to be done to complete this task?

No response

Are there any other details to include?

No response

What are the acceptance criteria to close this issue?

Noah needs to be able to execute vid_scheme via WASM.

Branch work will be merged to (if not the default branch)

No response

ggutoski avatar Feb 29 '24 21:02 ggutoski

@nomaxg do we still need this? Is it already done? Let's close this issue if we can.

ggutoski avatar Jun 10 '24 20:06 ggutoski

yes we still need this at some point, but it will likely be part of a larger plan to have a pure data structures crate that can compile to WASM.

nomaxg avatar Jun 10 '24 20:06 nomaxg

To whomever closes this issue

Please also close #3298 if you can. 🥺

ggutoski avatar Jun 10 '24 21:06 ggutoski

most of jellyfish is wasm-compilable. just be aware that browser only supports 32-bit wasm whereas normal wasm runtime should support both 32 bit and 64 bit.

I just successfully build jf-vid targeting wasm32: cargo build -p jf-vid --target wasm32-unknown-unknown --no-default-features --release (artifacts at ./target/nix_rustc/wasm32-unknown-unknown/releases/)

☝️ @ggutoski jf-vid is already wasm compatible, don't think we need to "separate out"?

I documented my prior lesson in this blog post.

alxiong avatar Jun 19 '24 14:06 alxiong

☝️ @ggutoski jf-vid is already wasm compatible, don't think we need to "separate out"?

True, but the function vid_scheme from the original post lives in the hotshot repo, not jf-vid. Seems to me that this function should be deleted from hotshot and we should make an equivalent in jf-vid, which would eliminate the need to "separate out" as you say. wdyt @nomaxg @shenkeyao ?

ggutoski avatar Jun 19 '24 16:06 ggutoski

☝️ @ggutoski jf-vid is already wasm compatible, don't think we need to "separate out"?

True, but the function vid_scheme from the original post lives in the hotshot repo, not jf-vid. Seems to me that this function should be deleted from hotshot and we should make an equivalent in jf-vid, which would eliminate the need to "separate out" as you say. wdyt @nomaxg @shenkeyao ?

sounds good to me

nomaxg avatar Jun 19 '24 17:06 nomaxg

☝️ @ggutoski jf-vid is already wasm compatible, don't think we need to "separate out"?

True, but the function vid_scheme from the original post lives in the hotshot repo, not jf-vid. Seems to me that this function should be deleted from hotshot and we should make an equivalent in jf-vid, which would eliminate the need to "separate out" as you say. wdyt @nomaxg @shenkeyao ?

Sounds good. When do we want this to be removed? Perhaps after the equivalent is added in jf-vid?

shenkeyao avatar Jun 20 '24 14:06 shenkeyao

Ugh now that I look more carefully I see that this entire file vid.rs is full of boilerplate code needed to properly abstract away the implementation details of the underlying ADVZ scheme. The code comments explain:

//! This module provides:
//! - an opaque constructor [`vid_scheme`] that returns a new instance of a
//!   VID scheme.
//! - type aliases [`VidCommitment`], [`VidCommon`], [`VidShare`]
//!   for [`VidScheme`] assoc types.
//!
//! Purpose: the specific choice of VID scheme is an implementation detail.
//! This crate and all downstream crates should talk to the VID scheme only
//! via the traits exposed here.

and

/// VID scheme constructor.
///
/// Returns an opaque type that impls jellyfish traits:
/// [`VidScheme`], [`PayloadProver`], [`Precomputable`].
///
/// # Rust forbids naming impl Trait in return types
///
/// Due to Rust limitations the return type of [`vid_scheme`] is a newtype
/// wrapper [`VidSchemeType`] that impls the above traits.
///
/// We prefer that the return type of [`vid_scheme`] be `impl Trait` for the
/// above traits. But the ability to name an impl Trait return type is
/// currently missing from Rust:
/// - [Naming impl trait in return types - Impl trait initiative](https://rust-lang.github.io/impl-trait-initiative/explainer/rpit_names.html)
/// - [RFC: Type alias impl trait (TAIT)](https://github.com/rust-lang/rfcs/blob/master/text/2515-type_alias_impl_trait.md)
///
/// # Panics
/// When the construction fails for the underlying VID scheme.
#[must_use]
pub fn vid_scheme(num_storage_nodes: usize) -> VidSchemeType {

Options:

  1. Move this entire file into jf-vid. (Thus exposing all these newtypes in the jf-vid API.)
  2. Make a completely separate crate (inside the hotshot workspace) that contains only this file.

I think I prefer option 1. @mrain do you have an opinion here?

ggutoski avatar Jun 20 '24 16:06 ggutoski