Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,11 @@ where

setup_timer.stop();

SharedMemoryEngine::read_pcs_setup_from_shared_memory()
// Prover setup not needed on client side (server does the proving).
// Verifier setup is required for verification, so read it from shared memory.
let (_prover_setup, verifier_setup) =
SharedMemoryEngine::read_pcs_setup_from_shared_memory::<C::FieldConfig, C::PCSConfig>();
(ExpanderProverSetup::default(), verifier_setup)
}

pub fn client_send_witness_and_prove<C, ECCConfig>(
Expand All @@ -152,8 +156,39 @@ where
{
let timer = Timer::new("prove", true);

// Reset ack signal, then write witness
SharedMemoryEngine::reset_witness_ack();
SharedMemoryEngine::write_witness_to_shared_memory::<C::FieldConfig>(device_memories);
wait_async(ClientHttpHelper::request_prove());

#[cfg(all(target_os = "linux", target_env = "gnu"))]
{
extern "C" {
fn malloc_trim(pad: usize) -> i32;
}
unsafe {
malloc_trim(0);
}
}
Comment on lines +163 to +171
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The extern "C" declaration for malloc_trim is repeated twice in this function (here and at line 188). It is better to declare it once at the beginning of the function scope. Additionally, the second call to malloc_trim (after dropping the shared memory handle) may have negligible impact compared to the first one, as shared memory is typically managed via mmap rather than the malloc heap.


// Async: send prove request + poll for witness ack to release shared memory early
let rt = tokio::runtime::Runtime::new().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Creating a new Runtime and calling block_on inside a function can cause a panic if this code is executed within an existing Tokio runtime. It is generally better to use the current handle if available or structure the API to be async.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Creating a new tokio::runtime::Runtime on every call to client_send_witness_and_prove is expensive and introduces significant overhead. Consider using a shared runtime or obtaining a handle to the current runtime if one exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Creating a new Tokio runtime with tokio::runtime::Runtime::new().unwrap() inside this function can be inefficient if it's called multiple times. This pattern is also repeated in the wait_async helper function. It's generally better to manage the runtime at a higher level in the application, for instance by creating it once and reusing it across calls.

Consider using once_cell::sync::Lazy to initialize a static runtime that can be shared.

Example:

use once_cell::sync::Lazy;

static RUNTIME: Lazy<tokio::runtime::Runtime> = Lazy::new(|| tokio::runtime::Runtime::new().expect("Failed to create Tokio runtime"));

// Then in your function:
// RUNTIME.block_on(async { ... });

Additionally, .unwrap() will panic on failure. Using .expect() with a descriptive message would be more robust.

Suggested change
let rt = tokio::runtime::Runtime::new().unwrap();
let rt = tokio::runtime::Runtime::new().expect("Failed to create Tokio runtime");

rt.block_on(async {
let prove_handle = tokio::spawn(async {
ClientHttpHelper::request_prove().await;
});

// Poll witness_ack; once server confirms read, release witness shared memory
tokio::task::spawn_blocking(|| {
SharedMemoryEngine::wait_for_witness_read_complete();
unsafe {
super::shared_memory_utils::SHARED_MEMORY.witness = None;
}
})
Comment on lines +181 to +186
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The malloc_trim(0) call should be executed after the witness shared memory is released to be effective. This suggestion moves it to the correct place. Please also remove the original malloc_trim block from lines 163-171.

        tokio::task::spawn_blocking(|| {
            SharedMemoryEngine::wait_for_witness_read_complete();
            unsafe {
                super::shared_memory_utils::SHARED_MEMORY.witness = None;
            }
            #[cfg(all(target_os = "linux", target_env = "gnu"))]
            {
                extern "C" {
                    fn malloc_trim(pad: usize) -> i32;
                }
                unsafe {
                    malloc_trim(0);
                }
            }
        })

.await
.expect("Witness cleanup task failed");

prove_handle.await.expect("Prove task failed");
});

let proof = SharedMemoryEngine::read_proof_from_shared_memory();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ where
let mut witness_win = state.wt_shared_memory_win.lock().await;
S::setup_shared_witness(&state.global_mpi_config, &mut witness, &mut witness_win);

// Signal client: witness has been read, shared memory can be released
SharedMemoryEngine::signal_witness_read_complete();

let prover_setup_guard = state.prover_setup.lock().await;
let computation_graph = state.computation_graph.lock().await;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@ pub struct SharedMemory {
pub pcs_setup: Option<Shmem>,
pub witness: Option<Shmem>,
pub proof: Option<Shmem>,
/// 1-byte signal: 0 = witness not read, 1 = server finished reading witness
pub witness_ack: Option<Shmem>,
}

pub static mut SHARED_MEMORY: SharedMemory = SharedMemory {
pcs_setup: None,
witness: None,
proof: None,
witness_ack: None,
};

pub struct SharedMemoryEngine {}
Expand Down Expand Up @@ -106,6 +109,56 @@ impl SharedMemoryEngine {
Self::read_object_from_shared_memory("pcs_setup", 0)
}

/// Client: reset witness_ack to 0 (call before writing witness)
pub fn reset_witness_ack() {
unsafe {
Self::allocate_shared_memory_if_necessary(
&mut SHARED_MEMORY.witness_ack,
"witness_ack",
1,
);
let ptr = SHARED_MEMORY.witness_ack.as_mut().unwrap().as_ptr();
std::ptr::write_volatile(ptr, 0u8);
}
}

/// Server: set witness_ack to 1 (call after reading witness)
pub fn signal_witness_read_complete() {
let shmem = ShmemConf::new()
.flink("witness_ack")
.open()
.expect("Failed to open witness_ack shared memory");
unsafe {
std::ptr::write_volatile(shmem.as_ptr(), 1u8);
}
}

/// Client: poll until witness_ack becomes 1, with a timeout to avoid hanging
/// if the server crashes.
pub fn wait_for_witness_read_complete() {
const TIMEOUT: std::time::Duration = std::time::Duration::from_secs(300);
let start = std::time::Instant::now();
unsafe {
let ptr = SHARED_MEMORY
.witness_ack
.as_ref()
.expect("witness_ack not initialized, call reset_witness_ack first")
.as_ptr() as *const u8;
loop {
if std::ptr::read_volatile(ptr) != 0 {
break;
}
if start.elapsed() > TIMEOUT {
panic!(
"Timed out waiting for server to read witness ({}s)",
TIMEOUT.as_secs()
);
}
std::thread::sleep(std::time::Duration::from_millis(10));
}
Comment on lines 147 to 158
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The polling loop lacks a timeout. If the server process crashes or fails to signal the witness_ack for any reason, the client will hang indefinitely. Adding a timeout (e.g., 5 minutes) would make the system more robust.

            let start = std::time::Instant::now();
            loop {
                if std::ptr::read_volatile(ptr) != 0 {
                    break;
                }
                if start.elapsed().as_secs() > 300 {
                    panic!("Timeout waiting for witness ack from server");
                }
                std::thread::sleep(std::time::Duration::from_millis(10));
            }

}
}
Comment on lines +138 to +160
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This function panics on timeout. It's more idiomatic in Rust to return a Result to allow the caller to handle the error gracefully. This makes the function more reusable and robust. This change will require updating the call site in client_utils.rs to handle the Result, for example by using .expect().

    pub fn wait_for_witness_read_complete() -> Result<(), String> {
        const TIMEOUT: std::time::Duration = std::time::Duration::from_secs(300);
        let start = std::time::Instant::now();
        unsafe {
            let ptr = SHARED_MEMORY
                .witness_ack
                .as_ref()
                .expect("witness_ack not initialized, call reset_witness_ack first")
                .as_ptr() as *const u8;
            loop {
                if std::ptr::read_volatile(ptr) != 0 {
                    return Ok(());
                }
                if start.elapsed() > TIMEOUT {
                    return Err(format!(
                        "Timed out waiting for server to read witness ({}s)",
                        TIMEOUT.as_secs()
                    ));
                }
                std::thread::sleep(std::time::Duration::from_millis(10));
            }
        }
    }


pub fn write_witness_to_shared_memory<F: FieldEngine>(values: Vec<Vec<F::SimdCircuitField>>) {
let total_size = std::mem::size_of::<usize>()
+ values
Expand Down