[Fix] Avoid `reqwest::blocking` when fetching parameters by kaimast · Pull Request #3241 · ProvableHQ/snarkVM

kaimast · 2026-05-04T22:41:23Z

This change is needed for ProvableHQ/snarkOS#4220.

#3169 changed snarkvm-parameters to use reqwest::blocking instead of curl. The former causes issues in some snarkOS test because it spawns a new tokio runtime, which does not work when called from an active tokio runtime.

f584af0 changes the parameter fetching logic to use ureq instead which does not spawn a tokio runtime under the hood.

Additionally, 47a827a cleans up the parameters macros and adds documentation. It also fixes #2931. The macros are still hard to read because of the many feature gates, but the change is an improvement.

Testing

All native parameter tests and ledger tests passed on my setup
I verified that wasm tests pass ( cd wasm && wasm-pack test --node)
For SGX, I verified that snarkvm-console builds. I could not build tests for parameters, but that seemed to be a pre-existing issue. I did not run any other tests, because I do not have SGX hardware,and did not have time to set up the simulator.

Notes

We should add documentation for wasm and sgx builds. I could not find any and used previous pull requests to figure out how to test them.
It is unclear to me why there is a wasm feature, and we do not solely rely on `target_arch='wasm32'.

iamalwaysuncomfortable

As is this will break mobile without MobileTLS and needs to be explicitly tested.

iamalwaysuncomfortable · 2026-05-05T18:53:59Z

-[target.'cfg(all(any(target_os = "android", target_os = "ios"), not(target_family = "wasm"), not(target_env = "sgx")))'.dependencies.reqwest]
-version = "0.13"
-default-features = false
-features = ["blocking", "native-tls"]
-
-[target.'cfg(all(not(any(target_os = "android", target_os = "ios")), not(target_family = "wasm"), not(target_env = "sgx")))'.dependencies.reqwest]
-version = "0.13"
-default-features = false
-features = ["blocking", "rustls"]
+[target.'cfg(all(not(target_family = "wasm"), not(target_env = "sgx")))'.dependencies.ureq]
+workspace = true
+features = ["rustls"]


This change will break mobile. We need to use nativetls for mobile targets, we'll test out the SDK changes based on this change, but the mobile changes need to be tested out as well if we're changing to ureq.

wasm should be safe since it uses a funky XMLHttpREquest object, but we'll test it explicitly.

It now uses native-tls as before for those targets

iamalwaysuncomfortable · 2026-05-05T19:03:10Z

+/// The remote URLs to fetch parameters from.
+/// (only used when the `filesystem` feature is active)
+#[cfg_attr(not(feature = "filesystem"), allow(dead_code))]


Why only gated for filesystem? Non-filesystems also download these.

This is just to hide a warning during test builds about the constant not being used.

The change does not remove the constant, but just suppresses the warning. I will update the comment.

iamalwaysuncomfortable · 2026-05-05T19:05:47Z

        }

-        #[cfg(all(not(feature = "wasm"), not(target_env = "sgx")))]
+        #[cfg(all(feature = "filesystem", not(feature = "wasm"), not(target_env = "sgx")))]


Why the sudden requirement of filesystem? Non-filesystem systems still perform downloads into buffers. I don't see any specific filesystem code in here, so why gate it behind this feature?

Suggested change

#[cfg(all(feature = "filesystem", not(feature = "wasm"), not(target_env = "sgx")))]

#[cfg(all(not(feature = "wasm"), not(target_env = "sgx")))]

Unless I misunderstood the impl_load_bytes_logic_remote macro, remote_fetch is only invoked when filesystem or wasm is active.

iamalwaysuncomfortable · 2026-05-05T19:06:56Z


+/// The remote URLs to fetch parameters from.
+/// (only used when the `filesystem` feature is active)
+#[cfg_attr(not(feature = "filesystem"), allow(dead_code))]


Why only when filesystem is active?

kaimast · 2026-05-06T20:58:07Z

We cannot test on mobile right now, but this PR still uses the same version of native-tls (0.2.18) under the hood. So the likelihood of mobile breaking is very small.

vicsn · 2026-05-07T11:55:52Z

-                        return Err($crate::errors::ParameterError::FilesystemDisabled);
-                    }
-                }
+                // We need either the `filesystem` or `wasm` feature to load parameters.


The error says FilesystemDisabled, the comment says either filesystem or wasm are needed. Should we re-use RemoteFetchDisabled or make a new error type?

vicsn · 2026-05-07T12:01:52Z

+/// On native `filesystem` targets: serves from the local cache directory if present; otherwise
+/// iterates through `$remote_urls` in order, retrying on transient errors, and caches the first
+/// verified download to disk.
+/// On wasm targets: iterates through `$remote_urls` using `XmlHttpRequest`, verifying the


Should we mention sgx as well?

vicsn · 2026-05-07T12:06:09Z

    ($remote_urls: expr, $local_dir: expr, $filename: expr, $metadata: expr, $expected_checksum: expr, $expected_size: expr) => {
        cfg_if::cfg_if! {
-            if #[cfg(all(feature = "filesystem", not(feature="wasm")))] {
+            if #[cfg(feature = "wasm")] {


The refactor 8684be5 is very hard to audit due to the inversion of multiple related feature flags and the number of lines of code. Can we leave a more comprehensive refactor for a separate PR so we don't block snarkOS?

raychu86 · 2026-05-15T22:51:36Z

How relevant is this PR now that #3248 is merged?

kaimast force-pushed the fix/parameter-fetch-ureq branch from 8caa7e3 to f584af0 Compare May 5, 2026 03:47

kaimast requested a review from iamalwaysuncomfortable May 5, 2026 04:05

kaimast marked this pull request as ready for review May 5, 2026 04:26

kaimast mentioned this pull request May 5, 2026

[Chore] Update snarkVM rev ProvableHQ/snarkOS#4220

Merged

iamalwaysuncomfortable requested changes May 5, 2026

View reviewed changes

kaimast force-pushed the fix/parameter-fetch-ureq branch 2 times, most recently from e9f8ba9 to d575dde Compare May 5, 2026 21:34

kaimast requested review from iamalwaysuncomfortable and raychu86 May 6, 2026 20:58

kaimast force-pushed the fix/parameter-fetch-ureq branch from 9cca4de to ead2d7c Compare May 6, 2026 21:43

vicsn requested changes May 7, 2026

View reviewed changes

kaimast mentioned this pull request May 7, 2026

[Fix] Use ureq instead of reqwest::blocking #3248

Merged

kaimast force-pushed the fix/parameter-fetch-ureq branch from 8080744 to 170fd55 Compare May 8, 2026 19:00

refactor(parameters): collapse nested cfg_if statments

2e50de9

kaimast force-pushed the fix/parameter-fetch-ureq branch from 170fd55 to 2e50de9 Compare May 19, 2026 20:22

	#[cfg(all(feature = "filesystem", not(feature = "wasm"), not(target_env = "sgx")))]
	#[cfg(all(not(feature = "wasm"), not(target_env = "sgx")))]

Conversation

kaimast commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Notes

Uh oh!

iamalwaysuncomfortable left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaimast commented May 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raychu86 commented May 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kaimast commented May 4, 2026 •

edited

Loading