Ns/chore/js cross origin sync

[nsarlin-zama]

closes: https://github.com/zama-ai/tfhe-rs-internal/issues/1232

PR content/description

The main change introduced by this pr is to use the wasm-par-mq crate introduced by #3296 to parallelize the zk proof msm in cross origin scenarios (in the absence of SharedArrayBuffer).

This pr includes 3 commits.
The first one is just a renaming, from "unsafe coop" to "cross origin", which seems to be a more commonly used term in the web world to describe this, and unsafe coop was a bit confusing

The second commit is the most important one, introducing the use of wasm-par-mq to parallelize the msm. The parallelization strategy is not exactly the same as the one used when we use rayon. The multithreaded rayon parallelism is done at the window level of the wnaf (windowed non adjacent form) algorithm. However, the issue here is that this cannot be simply chunked since the windows need to have access to the full array of inputs for arbitrary accesses. This is a perf issue without shared memory since it means that the full list of inputs need to be sent to all the workers.
But the msm itself is easily chunkable at a higher level. Indeed, msm([w, x, y, z], [a, b, c, d]) = msm([w, x], [a, b]) + msm([x, y], [c, d]). That way every input is only sent to one worker.
This commit also factorize the window part of the wnaf algorithm. It allowed me to easily compare the two approaches. And I think it makes the wnaf function a bit more readable.

The third commit use the wnaf algo to also compute the msm in the G2 group, since it was only implemented for G1. It just generefies the G1 code. This improves slightly the proving times on my laptop (a few 100ms, not very significant)

Benches

I ran benches on my laptop, since the result look more representative of the real life timings than the one we got on servers.
To give an idea, here are the timings for 256bits packed with 2048bits CRS, compute load verify:
Before:

256_bits_packed_2048_bits_crs_compute_load_verify_ZKV2_unsafe_coop_mean :  6522.639999999478 ms

After:

256_bits_packed_2048_bits_crs_compute_load_verify_ZKV2_cross_origin_mean :  2455.5600000000095  ms

Check-list:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)
• Relevant issues are marked as resolved/closed, related issues are linked in the description
• Check for breaking changes (including serialization changes) and add them to commit message following the conventional commit specification

---

This change is 

[IceTDrinker]

questions since it's still new and somewhat complicated to follow to me

@IceTDrinker partially reviewed 19 files and all commit messages, and made 26 comments.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on nsarlin-zama, soonum, SouchonTheo, and tmontaigu).

---

tfhe-zk-pok/src/curve_api/msm.rs line 55 at r2 (raw file):

impl MsmAffine for super::bls12_446::G1Affine {
    type Config = crate::curve_446::g1::Config;
    fn to_ark_affine(&self) -> &Affine<Self::Config> {

#[inline] could help for both, or just say to the compiler you expect the function to disappear

---

tfhe-zk-pok/src/curve_api/msm.rs line 67 at r2 (raw file):

}

fn compute_window<Aff: MsmAffine>(

#[track_caller] maybe ?

---

tfhe-zk-pok/src/curve_api/msm.rs line 218 at r2 (raw file):

fn msm_wnaf<A: MsmAffine>(bases: &[A], scalars: &[super::bls12_446::Zp]) -> Projective<A::Config> {
    let num_bits = 299usize;

unclear to me if that's true for both G1 and G2...

---

tfhe-zk-pok/src/curve_api/msm.rs line 227 at r2 (raw file):

    let lowest = *window_sums.first().unwrap();

    // We're traversing windows from high to low.

comments not useful ?

---

tfhe-zk-pok/src/curve_api/msm.rs line 405 at r2 (raw file):

        let chunk_results: Vec<SerializableG2Projective> = inputs
            .into_par_iter()
            .map(par_fn!(g2_msm_chunk_worker))

I feel like I'm missing the execute_async stuff from the previous PR ? not sure why

---

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 41 at r2 (raw file):

#[cfg(feature = "zk-pok")]
#[wasm_bindgen]
#[allow(clippy::future_not_send)] // JS event loop is single-threaded, Send is not needed

a bit confused as to why this would be needed ? The return type looks very simple ?

Also even if not multi threaded, no need for Send for consistency ?

---

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 42 at r2 (raw file):

#[wasm_bindgen]
#[allow(clippy::future_not_send)] // JS event loop is single-threaded, Send is not needed
pub async fn register_cross_origin_coordinator(coordinator_url: &str) -> Result<(), JsValue> {

would it be worth calling it _from_main or something like that ?

---

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 56 at r2 (raw file):

#[cfg(feature = "zk-pok")]
#[wasm_bindgen]
#[allow(clippy::future_not_send)] // JS event loop is single-threaded, Send is not needed

same confusion on my end

---

tfhe-zk-pok/Cargo.toml line 27 at r2 (raw file):

num-bigint = "0.4.5"
tfhe-versionable = { version = "0.7.0", path = "../utils/tfhe-versionable" }
[target.'cfg(target_family = "wasm")'.dependencies]

not sure we want this, since the wasm environment in which it can work is a browser only ?

---

tfhe/web_wasm_parallel_tests/worker.js line 720 at r2 (raw file):

          if (!supportsThreads) {
            common_bench_str += "_cross_origin";

likely breaks bench parsing/uploading to be checked with @soonum

---

ci/webdriver.py line 151 at r2 (raw file):

        # Needed for wasm-par-mq sync executor mode
        self.options.add_argument("--headless=new")
        self.options.add_argument("--enable-features=ServiceWorker")

compatible with both chrome and firefox ?

---

tfhe/docs/integration/js-on-wasm-api.md line 10 at r2 (raw file):

* Node.js: For use in Node.js applications or packages
* Web: For use in web browsers

I think the original web target has been replaced by the one with parallelism from par-mq in the Makefile ?

---

tfhe/docs/integration/js-on-wasm-api.md line 103 at r2 (raw file):

Standard multi-threading in WASM requires [cross-origin isolation](https://web.dev/articles/cross-origin-isolation-guide) (COOP/COEP headers) to enable `SharedArrayBuffer`. Some deployment environments cannot set these headers (e.g., when embedding in third-party pages or certain hosting platforms).

**TFHE-rs** provides an alternative parallelism mode that works **without cross-origin isolation** by using a Service Worker coordinator and a message-passing–based worker pool. This mode is currently used to accelerate **zero-knowledge proof** computation in the browser.

currently used ? seems a bit odd worded like that, it's "just" a capability of the crate ?

was this doc generated by an LLM first ?

---

tfhe/docs/integration/js-on-wasm-api.md line 193 at r2 (raw file):

    // Fallback: cross-origin worker pool
    await init_cross_origin_worker_pool(wasmUrl, bindgenUrl, "/sw.js");
}

missing imports like initThreadPool and init_cross_origin_worker_pool ?

---

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

* `make build_node_js_api` for the Node.js API
* `make build_web_js_api` for the browser API (also used for cross-origin parallelism)

ah so it supports both modes ? means we won't need a new package after all ?

---

tfhe/web_wasm_parallel_tests/sw.js line 1 at r2 (raw file):

import { setupCoordinator } from "./pkg/coordinator.js";

this is run on the user browser, should this import not be something like an import from the tfhe package ?

---

tfhe/Cargo.toml line 90 at r2 (raw file):

wasm-bindgen-rayon = { version = "1.3.0", optional = true }
js-sys = { workspace = true, optional = true }
wasm-bindgen-futures = { version = "0.4.56", optional = true }

should be workspaced, looks common to at least wasm-par-mq

---

tfhe/web_wasm_parallel_tests/index.js line 26 at r2 (raw file):

    // Register the coordinator Service Worker for cross-origin parallelism
    console.info("Running in cross-origin worker mode");
    await init();

is it the init from the wasm-par-mq ?

---

tfhe/web_wasm_parallel_tests/package.json line 7 at r2 (raw file):

  "main": "index.js",
  "scripts": {
    "build": "cp -r ../../tfhe/pkg ./ && webpack build ./index.js --mode production -o dist --output-filename index.js && cp index.html dist/ && cp favicon.ico dist/ && cp -r pkg dist/ && cp sw.js dist/",

the server needs the full tfhe JS package ?

---

tfhe-zk-pok/src/curve_api.rs line 364 at r2 (raw file):

    fn multi_mul_scalar(bases: &[Self::Affine], scalars: &[bls12_446::Zp]) -> Self {
        // wnaf overhead seems to not be worth it outside of wasm
        #[cfg(target_family = "wasm")]

not sure if we can still use the wasm family given the constraints of wasm-par-mq as indicated elsewhere

---

.github/workflows/benchmark_wasm_client_common.yml line 161 at r2 (raw file):

          BROWSER: ${{ matrix.browser }}

      - name: Run benchmarks (cross origin)

need an update in the grafana dashboard ?

---

Makefile line 688 at r2 (raw file):

.PHONY: build_web_js_api # Build the js API targeting the web browser
build_web_js_api: install_wasm_pack

so the web js API is the one you choose to be the cross origin friendly package ?

---

Makefile line 692 at r2 (raw file):

	RUSTFLAGS="$(WASM_RUSTFLAGS)" wasm-pack build --release --target=web \
		-- --features=boolean-client-js-wasm-api,shortint-client-js-wasm-api,integer-client-js-wasm-api,zk-pok,extended-types
	cp utils/wasm-par-mq/js/coordinator.js tfhe/pkg/

is there a way to have some sort of build step/script in wasm-bindgen ?

---

tfhe/web_wasm_parallel_tests/serve.cross-origin.json line 10 at r2 (raw file):

        {
          "key": "Content-Security-Policy",
          "value": "script-src 'self' 'wasm-unsafe-eval' blob:;"

is this blob requirement ok for BC or general good web practices ?

---

tfhe-zk-pok/src/serialization.rs line 853 at r2 (raw file):

pub type SerializableG1Projective = SerializableProjective<SerializableFp>;
pub type SerializableG2Projective = SerializableProjective<SerializableFp2>;

is it desirable to share the SerializableProjective among the two types ?

unclear on the implications, otherwise having two separate types is fine by me if it makes sense

[IceTDrinker]

@IceTDrinker reviewed 3 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on nsarlin-zama, soonum, SouchonTheo, and tmontaigu).

---

Makefile line 575 at r3 (raw file):

		-p tfhe-test-vectors -- --no-deps -D warnings

.PHONY: clippy_all # Run all clippy targets

let's maybe add a comment in ALL CAPS above here to not forget when adding clippy to also put it in the pcc batch, or we should make a make target that checks clippy_all recipes are all called in the batches (can be done in a later PR)

---

tfhe-zk-pok/Cargo.toml line 28 at r3 (raw file):

tfhe-versionable = { version = "0.7.0", path = "../utils/tfhe-versionable" }
[target.'cfg(target_family = "wasm")'.dependencies]
getrandom = { version = "0.2", features = ["js"] }

it's workspaced normally

[nsarlin-zama]

@nsarlin-zama made 24 comments and resolved 4 discussions.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

---

Makefile line 688 at r2 (raw file):

Previously, IceTDrinker wrote…

so the web js API is the one you choose to be the cross origin friendly package ?

The build for the cross origin mode does not require any specific flag like the multithreaded one, so I grouped sequential and cross origin parallel in the same target

Ideally, I'd rename the web_js_api_parallel to web_js_api_multithreaded since the base target now support some parallelism, but I'm afraid this would break a lot of things

---

Makefile line 692 at r2 (raw file):

Previously, IceTDrinker wrote…

is there a way to have some sort of build step/script in wasm-bindgen ?

I will look at that

---

.github/workflows/benchmark_wasm_client_common.yml line 161 at r2 (raw file):

Previously, IceTDrinker wrote…

need an update in the grafana dashboard ?

yes

---

ci/webdriver.py line 151 at r2 (raw file):

Previously, IceTDrinker wrote…

compatible with both chrome and firefox ?

it's chrome specific
fixed

---

tfhe/Cargo.toml line 90 at r2 (raw file):

Previously, IceTDrinker wrote…

should be workspaced, looks common to at least wasm-par-mq

done

---

tfhe/docs/integration/js-on-wasm-api.md line 10 at r2 (raw file):

Previously, IceTDrinker wrote…

I think the original web target has been replaced by the one with parallelism from par-mq in the Makefile ?

The way I see it, you can do sequential or cross origin parallelism, they both happen to use the same package. I regrouped in the Makefile them because they were doing effectively the same thing.
I reworded this.

---

tfhe/docs/integration/js-on-wasm-api.md line 103 at r2 (raw file):

Previously, IceTDrinker wrote…

currently used ? seems a bit odd worded like that, it's "just" a capability of the crate ?

was this doc generated by an LLM first ?

Yes I used an LLM to help me write this, I forgot to mention it because I used it after I wrote the pr description.

"currently" here means that it might be used to parallelize more stuff in the future, but I will reword this sentence if it feels weird.

---

tfhe/docs/integration/js-on-wasm-api.md line 193 at r2 (raw file):

Previously, IceTDrinker wrote…

missing imports like initThreadPool and init_cross_origin_worker_pool ?

yes indeed

---

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

Previously, IceTDrinker wrote…

ah so it supports both modes ? means we won't need a new package after all ?

I think the published one is the one with the atomics (currently called parallel ?).
The thing that does not work is using the package compiled with atomics without COOP/COEP. So we need at least:

• 1 package for node
• 1 package for multithreaded/COOP/SharedArrayBuffer
• 1 package for sequential and cross origin parallelism (can be the same package because they require the same options)

---

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 41 at r2 (raw file):

Previously, IceTDrinker wrote…

a bit confused as to why this would be needed ? The return type looks very simple ?

Also even if not multi threaded, no need for Send for consistency ?

This looks simple but somehow when it goes through wasm-bindgen and wasm-bindgen-futures, a RefCell is added that makes it !Send.

Clippy complains without the allow because in the general case, it makes the API less usable. For example, this function would not be usable in a tokio executor because it's multithreaded and requires Send futures. But here we don't care about that because this function is made to be called in a JS executor that is single threaded.

So I think silencing this warning is the best thing to do here

---

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 42 at r2 (raw file):

Previously, IceTDrinker wrote…

would it be worth calling it _from_main or something like that ?

For me it would imply that there is a similar function that does not need to be called from main if we do this

---

tfhe/src/js_on_wasm_api/js_high_level_api/keys.rs line 56 at r2 (raw file):

Previously, IceTDrinker wrote…

same confusion on my end

see my comment above

---

tfhe/web_wasm_parallel_tests/index.js line 26 at r2 (raw file):

Previously, IceTDrinker wrote…

is it the init from the wasm-par-mq ?

no this is a init generated by wasm-bindgen that we need to use before calling any function from tfhe-rs. We also use it in the other examples.

---

tfhe/web_wasm_parallel_tests/sw.js line 1 at r2 (raw file):

Previously, IceTDrinker wrote…

this is run on the user browser, should this import not be something like an import from the tfhe package ?

no since this is a service worker, and sadly I did not find any way to make it work directly from the tfhe package like I did for the other workers. It needs a specific js file.

---

tfhe/web_wasm_parallel_tests/worker.js line 720 at r2 (raw file):

Previously, IceTDrinker wrote…

likely breaks bench parsing/uploading to be checked with @soonum

Yes I will update grafana once this is ready

---

tfhe-zk-pok/Cargo.toml line 27 at r2 (raw file):

Previously, IceTDrinker wrote…

not sure we want this, since the wasm environment in which it can work is a browser only ?

I'm gonna check, but I don't think there is an issue in building it anyways for wasm targets, as long as you don't use it. How would you gate it?

---

tfhe-zk-pok/Cargo.toml line 28 at r3 (raw file):

Previously, IceTDrinker wrote…

it's workspaced normally

done

---

tfhe-zk-pok/src/serialization.rs line 853 at r2 (raw file):

Previously, IceTDrinker wrote…

is it desirable to share the SerializableProjective among the two types ?

unclear on the implications, otherwise having two separate types is fine by me if it makes sense

I'm not sure what would be the benefits of having different types ?
This pattern is already used for SerializableAffine

---

tfhe-zk-pok/src/curve_api/msm.rs line 55 at r2 (raw file):

Previously, IceTDrinker wrote…

#[inline] could help for both, or just say to the compiler you expect the function to disappear

done

---

tfhe-zk-pok/src/curve_api/msm.rs line 67 at r2 (raw file):

Previously, IceTDrinker wrote…

#[track_caller] maybe ?

done

---

tfhe-zk-pok/src/curve_api/msm.rs line 218 at r2 (raw file):

Previously, IceTDrinker wrote…

unclear to me if that's true for both G1 and G2...

yes, 299 is the size of the scalars, since it's the scalar that are decomposed in the wnaf. And they are the same for G1 and G2

---

tfhe-zk-pok/src/curve_api/msm.rs line 227 at r2 (raw file):

Previously, IceTDrinker wrote…

comments not useful ?

I felt like they were just describing the code but said nothing about why things are done like that or the general idea of the algorithm

---

tfhe-zk-pok/src/curve_api/msm.rs line 405 at r2 (raw file):

Previously, IceTDrinker wrote…

I feel like I'm missing the execute_async stuff from the previous PR ? not sure why

This is not strictly needed here because in the example the proof is directly run from a worker, we only needed if we want to run it from the main thread since it cannot block.

I fixed the pr to add this possibility, this gives users more flexibility if they don't want to manager their own workers.

There is a small perf impact since it means more data round trips.

---

tfhe/web_wasm_parallel_tests/serve.cross-origin.json line 10 at r2 (raw file):

Previously, IceTDrinker wrote…

is this blob requirement ok for BC or general good web practices ?

waiting for input from them. I thing it is because blob url is the main reason why they want to reduce the size of the TFHE wasm bundle AFAIK.
Here removing this would require a slightly more complex deployment scenario for the end user, meaning they would have to copy more files.

[tmontaigu]

tfhe/src/high_level_api/compact_list.rs line 999 at r5 (raw file):

Previously, IceTDrinker wrote…

this does not make sense to me, why do we need this for a builder ? we don't serialize it right ?

I think its serialized as part of the communication in JS webworkers, and then we have a lint for things that are serializable but not versionned ?

[nsarlin-zama]

@nsarlin-zama made 13 comments.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

---

Makefile line 688 at r2 (raw file):

Previously, IceTDrinker wrote…

putting comments should be enough

added a comment

---

Makefile line 575 at r3 (raw file):

Previously, IceTDrinker wrote…

let's maybe add a comment in ALL CAPS above here to not forget when adding clippy to also put it in the pcc batch, or we should make a make target that checks clippy_all recipes are all called in the batches (can be done in a later PR)

added a comment

---

.github/workflows/benchmark_wasm_client_common.yml line 161 at r2 (raw file):

Previously, IceTDrinker wrote…

please coordinate to make the update

I did the previous update for this dashboard so I will take care of this one too once it is merged

---

ci/webdriver.py line 151 at r2 (raw file):

Previously, IceTDrinker wrote…

nothing specific for service workers for Firefox ?

No with firefox it works out of the box

---

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

Previously, IceTDrinker wrote…

indeed, as dicussed, to check with BC the preferred naming for them

They don't have strong opinion about this.

It looks like a convention in js world for things that might work in more situations at a performance/feature cost might be to add a -compat suffix. So it could be tfhe-compat.

It looks like it would also be possible to bundle both wasm in the same npm package. Then the user could decide at import time between importing from pkg/tfhe.js or from pkg/compat/tfhe.js or pkg/tfhe-compat.js.

---

tfhe/src/high_level_api/compact_list.rs line 999 at r5 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

I think its serialized as part of the communication in JS webworkers, and then we have a lint for things that are serializable but not versionned ?

Yes that's the reason.

In the mode where we run the js user code from the main thread (using execute_async), we need to send the inputs of encrypt_and_prove to the sync executor. So we need to serialize them.

Technically, I could have made the encryption on the main thread and send the encrypted inputs to the sync executor, but it would have implied messing with some internal api, and meant more data (encrypted vs cleartext) sent on the message queue.

This does not strictly require versioning, since the data are only sent in a queue in the same binary. However, now that the type derives Serialize, someone might see it and want to store it in the future so I preferred to also derive Versionize now (not doing it would have meant silencing a lint for no good reason).

---

tfhe/src/high_level_api/backward_compatibility/compact_list.rs line 36 at r5 (raw file):

Previously, IceTDrinker wrote…

same thing

see my answer above

---

tfhe/src/integer/backward_compatibility/ciphertext/mod.rs line 140 at r5 (raw file):

Previously, IceTDrinker wrote…

same here

answered above

---

tfhe/src/integer/ciphertext/compact_list.rs line 159 at r5 (raw file):

Previously, IceTDrinker wrote…

why ?

answered above

---

tfhe/web_wasm_parallel_tests/index.js line 37 at r5 (raw file):

Previously, IceTDrinker wrote…

check tfhe/js_on_wasm_tests/test-hlapi-signed.js:493 ?

actually this one is a copy paste from the one in tfhe/web_wasm_parallel_tests/worker.js.
I can refactor it to make them both include it from another file, but I don't know if it's worth it for such a small function.

---

tfhe/web_wasm_parallel_tests/index.js line 237 at r5 (raw file):

Previously, IceTDrinker wrote…

did you check the main thread tests were run in the CI ?

they run when I call the command locally, but I believe it will be checked in CI only with the approved label.

---

tfhe/web_wasm_parallel_tests/package.json line 7 at r2 (raw file):

Previously, IceTDrinker wrote…

the server needs the full tfhe JS package ?

yes we need the urls reachable for the workers.
I tried to make the least assumptions about how

---

tfhe/web_wasm_parallel_tests/sw.js line 1 at r2 (raw file):

Previously, IceTDrinker wrote…

can't we bundle like wasm-rayon-bindgen ?

I'm looking at ways to mimic what wasm-rayon bindgen does to simplify the deployment

[tmontaigu]

tfhe/src/high_level_api/compact_list.rs line 999 at r5 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

Yes that's the reason.

In the mode where we run the js user code from the main thread (using execute_async), we need to send the inputs of encrypt_and_prove to the sync executor. So we need to serialize them.

Technically, I could have made the encryption on the main thread and send the encrypted inputs to the sync executor, but it would have implied messing with some internal api, and meant more data (encrypted vs cleartext) sent on the message queue.

This does not strictly require versioning, since the data are only sent in a queue in the same binary. However, now that the type derives Serialize, someone might see it and want to store it in the future so I preferred to also derive Versionize now (not doing it would have meant silencing a lint for no good reason).

Is the use of this serialization local to a file ?

Maybe we could have a private intermediate type with from/into ?

[nsarlin-zama]

@nsarlin-zama made 1 comment.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

---

tfhe/src/high_level_api/compact_list.rs line 999 at r5 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

Is the use of this serialization local to a file ?

Maybe we could have a private intermediate type with from/into ?

yes I think I can certainly do that if you prefer

[tmontaigu]

tfhe/src/high_level_api/compact_list.rs line 999 at r5 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

yes I think I can certainly do that if you prefer

That might be better as it's less work for us and less things to maintain and make sure it works correctly
otherwise, we would need an impl of Named for these type as I don't think they do, and potentially also a backward data test

lets wait for coffee drinker

[IceTDrinker]

tfhe/src/high_level_api/compact_list.rs line 999 at r5 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

That might be better as it's less work for us and less things to maintain and make sure it works correctly
otherwise, we would need an impl of Named for these type as I don't think they do, and potentially also a backward data test

lets wait for coffee drinker

I agree, we discussed it at the office and proposed the same thing with private types which would not be versioned

[IceTDrinker]

Makefile line 575 at r3 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

added a comment

I don't think I see the last revision, I don't see the comment here or below

[nsarlin-zama]

@nsarlin-zama made 1 comment.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

---

Makefile line 575 at r3 (raw file):

Previously, IceTDrinker wrote…

I don't think I see the last revision, I don't see the comment here or below

sorry, this should be fixed now

[IceTDrinker]

@IceTDrinker reviewed 1 file and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on nsarlin-zama, soonum, SouchonTheo, and tmontaigu).

---

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

They don't have strong opinion about this.

It looks like a convention in js world for things that might work in more situations at a performance/feature cost might be to add a -compat suffix. So it could be tfhe-compat.

It looks like it would also be possible to bundle both wasm in the same npm package. Then the user could decide at import time between importing from pkg/tfhe.js or from pkg/compat/tfhe.js or pkg/tfhe-compat.js.

if that's an option that's not too complicated why not, otherwise, have the tfhe-compat package and tfhe depending on it and re-exporting the compat package ?

[tmontaigu]

@tmontaigu reviewed 12 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on IceTDrinker, nsarlin-zama, soonum, and SouchonTheo).

---

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

  let result = BigInt(0);
  for (let i = 0; i < bitLen; i++) {
    result << 1n;

Aren't we missing an assigment here ? result <<= 1n;

[nsarlin-zama]

@nsarlin-zama made 1 comment.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

---

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

Aren't we missing an assigment here ? result <<= 1n;

Ha yes you're right !
The bad news is that this is copy pasted and the same mistake is present in the code that runs the existing tests 🥶

[tmontaigu]

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

Ha yes you're right !
The bad news is that this is copy pasted and the same mistake is present in the code that runs the existing tests 🥶

I kind of expected that 😁

[nsarlin-zama]

@nsarlin-zama made 1 comment.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on IceTDrinker, soonum, and SouchonTheo).

---

tfhe/src/high_level_api/compact_list.rs line 999 at r5 (raw file):

Previously, IceTDrinker wrote…

I agree, we discussed it at the office and proposed the same thing with private types which would not be versioned

Would it be ok to make the fields of this type pub(crate) for the from/into ? Or a into_raw_parts ?

[nsarlin-zama]

@nsarlin-zama made 4 comments.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

---

Makefile line 692 at r2 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

I will look at that

I don't think it's possible :\

---

tfhe/src/integer/ciphertext/compact_list.rs line 159 at r5 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

answered above

I removed the need to serialize this (I've lost the original thread in reviewable)

---

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

Previously, tmontaigu (tmontaigu) wrote…

I kind of expected that 😁

fixed

---

tfhe/web_wasm_parallel_tests/package.json line 7 at r2 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

yes we need the urls reachable for the workers.
I tried to make the least assumptions about how

this is now simplified a bit using #3383

[IceTDrinker]

Makefile line 692 at r2 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

I don't think it's possible :\

ok no problem

[IceTDrinker]

Last few questions for my understanding, otherwise I think we are good to go

Who did you get a review from ? (at least on the ZK pok crate)

@IceTDrinker partially reviewed 20 files and all commit messages, made 5 comments, and resolved 10 discussions.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on nsarlin-zama, soonum, SouchonTheo, and tmontaigu).

---

tfhe/docs/integration/js-on-wasm-api.md line 201 at r2 (raw file):

Previously, IceTDrinker wrote…

if that's an option that's not too complicated why not, otherwise, have the tfhe-compat package and tfhe depending on it and re-exporting the compat package ?

so tfhe-compat is considered the name to use ?

---

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

Previously, nsarlin-zama (Nicolas Sarlin) wrote…

fixed

fixed in the original code this was taken from as well ?

---

tfhe/src/js_on_wasm_api/js_high_level_api/integers.rs line 1534 at r10 (raw file):

    #[derive(Serialize, Deserialize)]
    #[cfg_attr(dylint_lib = "tfhe_lints", allow(serialize_without_versionize))]
    pub(super) struct SerializableCompactCiphertextListBuilder {

doesn't need to be pub(super) I think ?

the trait implem is enough ?

---

utils/wasm-par-mq/js/worker_helpers.js line 27 at r10 (raw file):

// Compute worker
// `self` is not defined in Node.js. Skip worker bootstrap here.

also I guess the service worker does not exist in node ?

[nsarlin-zama]

@nsarlin-zama made 3 comments.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on IceTDrinker, soonum, SouchonTheo, and tmontaigu).

---

tfhe/src/js_on_wasm_api/js_high_level_api/integers.rs line 1534 at r10 (raw file):

Previously, IceTDrinker wrote…

doesn't need to be pub(super) I think ?

the trait implem is enough ?

needs to be pub(super) since builder field in ProofInput is pub(super), and I need that to build it in build_with_proof_packed_async

---

tfhe/web_wasm_parallel_tests/index.js line 40 at r8 (raw file):

Previously, IceTDrinker wrote…

fixed in the original code this was taken from as well ?

yes

---

utils/wasm-par-mq/js/worker_helpers.js line 27 at r10 (raw file):

Previously, IceTDrinker wrote…

also I guess the service worker does not exist in node ?

Certainly but this is not what is blocking us here. The issue here is that the code is "top level js" and will run even if we don't call any function from this, so we need to prevent it to run in node