Should the processor expose sync APIs?

[huitseeker]

Packages versions

miden-vm: 0.21.0 (upcoming)

Bug description

Processor API: Should it be async or sync?

We're hitting test failures in miden-base on miden-vm next (v0.21.0, unreleased) when using the P3 VM processor from async test functions. This raises a question about what the processor API should look like.

The test failure happens at processor/src/fast/mod.rs:923:

pub fn execute_for_trace_sync(...) -> Result<...> {
    let rt = tokio::runtime::Builder::new_current_thread().build().unwrap();
    rt.block_on(self.execute_for_trace(program, host, fragment_size))
}

When called from an async test function, it panics:

thread 'test_ecdsa_acl' panicked at processor/src/fast/mod.rs:925:12:
Cannot start a runtime from within a runtime.

Here's the call chain in miden-base:

#[tokio::test]
async fn test_ecdsa_acl() -> Result<()> {
    // ... setup ...
    prove_and_verify_transaction(tx)?;  // calls prover.prove()
                                        // which calls processor.execute_for_trace_sync()
                                        // which tries to create a new Tokio runtime
                                        // panic!
}

The processor has both async methods like execute_for_trace() and sync wrappers like execute_for_trace_sync(). The sync wrappers create their own Tokio runtime and call block_on(). This works from sync code but breaks from async contexts.

A few options:

Async only

Remove the *_sync() wrappers. Users who need sync can create their own runtime:

let rt = tokio::runtime::Runtime::new()?;
rt.block_on(processor.execute_for_trace(...))?;

Callers control async execution.

Detect existing runtime

Keep sync wrappers but check if we're already in a runtime:

  match tokio::runtime::Handle::try_current() {
      Ok(_handle) => {
          // We're in a runtime - use block_in_place to avoid blocking the worker thread
          tokio::task::block_in_place(|| {
              // Create a new runtime in this blocking context
              let rt = tokio::runtime::Builder::new_current_thread().build().unwrap();
              rt.block_on(self.execute(program, host))
          })
      },
      Err(_) => {
          // No runtime exists - create one and use it
          let rt = tokio::runtime::Builder::new_current_thread().build().unwrap();
          rt.block_on(self.execute(program, host))
      },
  }

Works in both contexts. Calling block_on() from within an existing runtime isn't ideal but technically works.

Spawn blocking

Use spawn_blocking() to move work to another thread when already in a runtime. More complex. Thread overhead.

What makes the most sense? The Winterfell VM was fully synchronous, so sync wrappers were probably added to match that API. Now that P3 uses async internally, maybe it's worth rethinking this.

The same pattern appears in execute_sync() and execute_sync_mut() in the same file.

[bobbinth]

A bit of context:

Initially, Miden VM was fully synchronous but earlier this year we introduced async API because this was needed downstream (in both miden-client and miden-node). Supporting both sync and async API "natively" was a bit too much work, and so, we went with async API as the primary API (as this is what is used downstream) and kept sync API for two reasons:

• This is what most tests used - and we didn't want to update all the tests.
• We were planning to eventually make sync API also "native" (@plafer did an experiment in Exploration of using the sans-IO pattern in the FastProcessor #2006 which suggested that doing so shouldn't be too much work). The primary motivation for supporting sync API was to avoid the async runtime overhead in cases where performance is critical and we don't care about the VM being async (this would specifically be relevant for batch and block proving; for transaction proving we will need the async API).

Assuming P3 uses async internally, it seems like having sync API no longer makes a lot of sense. So, we can probably remove it. But if that's too big of a chance now, we could go with the "detect existing runtime" approach first (as it seems like a smaller lift), and then revisit the need for sync API later.

[huitseeker]

Approach Taken on branch al-migrate-p3-ver2:

We implemented runtime detection that panics when called from within an existing runtime. Sync methods now check Handle::try_current() - if a runtime exists, they panic with a clear error message directing users to the async API. If no runtime exists, they create one.

The upstream (e.g. miden-base) calls the async API, see 0xMiden/protocol#2213

Why not block_in_place()?

• Requires rt-multi-thread (not available on wasm32)
• spawn_blocking() won't work - AsyncHost is !Send

For wasm32: All sync methods are gated behind #[cfg(not(target_arch = "wasm32"))]. The prover now uses async prove() as primary, with prove_sync() as a wrapper for native platforms only.

Limitations

• No nested runtime support: Calling sync methods from async contexts panics
• Sync API is now top-level only: CLI, tests, and synchronous main() functions
• Performance cost: Creates a new runtime on each sync call
• wasm32: No sync API at all

[bobbinth]

I think the approach taken in #2472 works for now. I guess the question is whether we want to remove sync functionality completely in the long term.

[bobbinth]

Merging #2527 into this. Basically, unless we decide to make sync execution a first-class citizen (similar to async execution), there is no reason to keep it at the processor level. Instead, we could provide execute_sync() free function that would "translate" async methods of the processor into sync execution.

[plafer]

I indeed only kept the execute_sync() methods for tests (initially private), and after some time made them public because IIRC some tests in other crates needed it.

As mentioned throughout this issue, if simply documenting the fact that the processor is async-first works, and that sync methods are only exposed for convenience (e.g. tests), then I would vote for that.

[bobbinth]

One route we can take here is to remove execute_sync() function from lib.rs and keep the test-only _sync() functions on the FastProcessor. For this, we should probably carry over the logic of execute_sync() function for detecting the current async runtime.

Also, do we still need SyncHost? If not, I'd get rid of it and rename AsyncHost to just Host.

Lastly, we can probably flatten out ProcessState - or rather delete it and rename FastProcessState into just ProcessState.