ronantakizawa/codereview-bench huggingface dataset:
Repository: nexus-xyz/nexus-zkvm
File: runtime/src/alloc.rs (Rust)

Code before:
        heap_pos = &_end as *const u8 as usize;
    }

    let offset = heap_pos & (align - 1);
    if offset != 0 {
        heap_pos = heap_pos
            .checked_add(align - offset)
            .expect("No overflow happen");
    }

    let ptr = heap_pos as *mut u8;
    heap_pos = heap_pos.checked_add(bytes).expect("No overflow happen");

    // Get the current stack pointer
    let stack_ptr: usize;
    unsafe {
        core::arch::asm!(
            "mv {}, sp",
            out(reg) stack_ptr
        );
    }

    // Check if the heap is about to clash with the stack
    if heap_pos + MEMORY_GAP > stack_ptr {
        panic!(
            "Heap is about to clash with stack (heap: 0x{:x}, stack: 0x{:x})",
            heap_pos, stack_ptr
        );
    }

    HEAP_POS = heap_pos;
    ptr
}


Review comment (bug):
"Is about to" is odd for an error message, since it implies that you're panicking despite nothing having actually gone wrong yet. I think you can just write "Heap clashing with stack"

Code after:
    }

    let offset = heap_pos & (align - 1);
    if offset != 0 {
        heap_pos = heap_pos
            .checked_add(align - offset)
            .expect("Heap calculation has overflowed");
    }

    let ptr = heap_pos as *mut u8;
    heap_pos = heap_pos
        .checked_add(bytes)
        .expect("Heap calculation has overflowed");

    // Get the current stack pointer
    let stack_ptr: usize;
    unsafe {
        core::arch::asm!(
            "mv {}, sp",
            out(reg) stack_ptr
        );
    }

    // Check if the heap is about to clash with the stack
    let gap_check = heap_pos
        .checked_add(MEMORY_GAP)
        .expect("Heap calculation has overflowed");
    if gap_check > stack_ptr {
        panic!(
            "Heap clashing with stack (heap: 0x{:x}, stack: 0x{:x})",
            heap_pos, stack_ptr
        );
    }

    HEAP_POS = heap_pos;
    ptr
}
Repository: nexus-xyz/nexus-zkvm
File: runtime/src/alloc.rs (Rust)

Code before:
    extern "C" {
        // https://lld.llvm.org/ELF/linker_script.html#sections-command
        static _end: u8;
    }

    // Pointer to next heap address to use, or 0 if the heap has not yet been
    // initialized.
    static mut HEAP_POS: usize = 0;

    // SAFETY: Single threaded, so nothing else can touch this while we're working.
    let mut heap_pos = HEAP_POS;

    #[cfg(target_arch = "riscv32")]
    if heap_pos == 0 {
        heap_pos = &_end as *const u8 as usize;
    }

    let offset = heap_pos & (align - 1);
    if offset != 0 {
        heap_pos = heap_pos
            .checked_add(align - offset)
            .expect("No overflow happen");
    }

    let ptr = heap_pos as *mut u8;
    heap_pos = heap_pos.checked_add(bytes).expect("No overflow happen");

    // Get the current stack pointer
    let stack_ptr: usize;
    unsafe {
        core::arch::asm!(
            "mv {}, sp",
            out(reg) stack_ptr
        );
    }

    // Check if the heap is about to clash with the stack
    if heap_pos + MEMORY_GAP > stack_ptr {
        panic!(
            "Heap is about to clash with stack (heap: 0x{:x}, stack: 0x{:x})",
            heap_pos, stack_ptr
        );
    }

    HEAP_POS = heap_pos;
    ptr
}


Review comment (bug):
The argument to`.expect` is printed in case of an error, so it should be something like "heap calculation overflowed", i.e., the opposite of what you have now

Code after:
    extern "C" {
        // https://lld.llvm.org/ELF/linker_script.html#sections-command
        static _end: u8;
    }

    // Pointer to next heap address to use, or 0 if the heap has not yet been
    // initialized.
    static mut HEAP_POS: usize = 0;

    // SAFETY: Single threaded, so nothing else can touch this while we're working.
    let mut heap_pos = HEAP_POS;

    if heap_pos == 0 {
        heap_pos = &_end as *const u8 as usize;
    }

    let offset = heap_pos & (align - 1);
    if offset != 0 {
        heap_pos = heap_pos
            .checked_add(align - offset)
            .expect("Heap calculation has overflowed");
    }

    let ptr = heap_pos as *mut u8;
    heap_pos = heap_pos
        .checked_add(bytes)
        .expect("Heap calculation has overflowed");

    // Get the current stack pointer
    let stack_ptr: usize;
    unsafe {
        core::arch::asm!(
            "mv {}, sp",
            out(reg) stack_ptr
        );
    }

    // Check if the heap is about to clash with the stack
    let gap_check = heap_pos
        .checked_add(MEMORY_GAP)
        .expect("Heap calculation has overflowed");
    if gap_check > stack_ptr {
        panic!(
            "Heap clashing with stack (heap: 0x{:x}, stack: 0x{:x})",
            heap_pos, stack_ptr
        );
    }

    HEAP_POS = heap_pos;
    ptr
}
Repository: nexus-xyz/nexus-zkvm
File: runtime/src/alloc.rs (Rust)

Code before:
#[allow(clippy::missing_safety_doc)]
#[no_mangle]
pub unsafe extern "C" fn sys_alloc_aligned(bytes: usize, align: usize) -> *mut u8 {
    #[cfg(target_arch = "riscv32")]
    extern "C" {
        // https://lld.llvm.org/ELF/linker_script.html#sections-command
        static _end: u8;
    }

    // Pointer to next heap address to use, or 0 if the heap has not yet been
    // initialized.
    static mut HEAP_POS: usize = 0;

    // SAFETY: Single threaded, so nothing else can touch this while we're working.
    let mut heap_pos = HEAP_POS;

    #[cfg(target_arch = "riscv32")]
    if heap_pos == 0 {
        heap_pos = &_end as *const u8 as usize;
    }

    let offset = heap_pos & (align - 1);
    if offset != 0 {
        heap_pos = heap_pos
            .checked_add(align - offset)
            .expect("No overflow happen");
    }

    let ptr = heap_pos as *mut u8;
    heap_pos = heap_pos.checked_add(bytes).expect("No overflow happen");

    // Get the current stack pointer
    let stack_ptr: usize;
    unsafe {
        core::arch::asm!(
            "mv {}, sp",
            out(reg) stack_ptr
        );
    }

    // Check if the heap is about to clash with the stack
    if heap_pos + MEMORY_GAP > stack_ptr {
        panic!(
            "Heap is about to clash with stack (heap: 0x{:x}, stack: 0x{:x})",
            heap_pos, stack_ptr
        );
    }

    HEAP_POS = heap_pos;
    ptr
}

Review comment (bug):
The argument to.expect is printed in case of an error, so it should be something like "heap calculation overflowed", i.e., the opposite of what you have now


Code after:

#[allow(clippy::missing_safety_doc)]
#[no_mangle]
pub unsafe extern "C" fn sys_alloc_aligned(bytes: usize, align: usize) -> *mut u8 {
    extern "C" {
        // https://lld.llvm.org/ELF/linker_script.html#sections-command
        static _end: u8;
    }

    // Pointer to next heap address to use, or 0 if the heap has not yet been
    // initialized.
    static mut HEAP_POS: usize = 0;

    // SAFETY: Single threaded, so nothing else can touch this while we're working.
    let mut heap_pos = HEAP_POS;

    if heap_pos == 0 {
        heap_pos = &_end as *const u8 as usize;
    }

    let offset = heap_pos & (align - 1);
    if offset != 0 {
        heap_pos = heap_pos
            .checked_add(align - offset)
            .expect("Heap calculation has overflowed");
    }

    let ptr = heap_pos as *mut u8;
    heap_pos = heap_pos
        .checked_add(bytes)
        .expect("Heap calculation has overflowed");

    // Get the current stack pointer
    let stack_ptr: usize;
    unsafe {
        core::arch::asm!(
            "mv {}, sp",
            out(reg) stack_ptr
        );
    }

    // Check if the heap is about to clash with the stack
    let gap_check = heap_pos
        .checked_add(MEMORY_GAP)
        .expect("Heap calculation has overflowed");
    if gap_check > stack_ptr {
        panic!(
            "Heap clashing with stack (heap: 0x{:x}, stack: 0x{:x})",
            heap_pos, stack_ptr
        );
    }
Repository: nexus-xyz/nexus-zkvm
File: cli/src/command/prove.rs (Rust)

Code before:

    #[command(flatten)]
    pub local_args: LocalProveArgs,
}

#[derive(Debug, Args)]
pub struct LocalProveArgs {
    #[arg(short, name = "k")]
    pub k: Option<usize>,

    /// Path to public parameters file.
    #[arg(short = 'p', long = "public-params")]
    pub pp_file: Option<PathBuf>,

    #[arg(long("impl"))]
    pub prover_impl: Option<vm_config::ProverImpl>,

    /// Path to the SRS file: only needed when pp_file is None and nova_impl is ParallelCompressible.
    #[arg(long("srs-file"))]
    pub srs_file: Option<PathBuf>,
}

#[derive(Debug, Args)]
pub struct CommonProveArgs {
    /// Build artifacts with the specified profile. "release-unoptimized" is default.
    #[arg(long, default_value = "release")]
    pub profile: String,

    /// Name of the bin target to run.
    #[arg(long)]
    pub bin: Option<String>,
}

pub fn handle_command(args: ProveArgs) -> anyhow::Result<()> {
    let ProveArgs {
        common_args: CommonProveArgs { profile, bin },
        network,
        url,
        local_args,
    } = args;

    let path = path_to_artifact(bin, &profile)?;
    let vm_config = vm_config::VmConfig::from_env()?;

    if &profile == "dev" {
        tracing::warn!(
            target: LOG_TARGET,
            "proving debug build",
        )
    }


Review comment (performance):
You also need to modify `cli/src/command/new` to not use the `release-unoptimized` profile in guest projects.

See: https://github.com/nexus-xyz/nexus-zkvm/blob/main/cli/src/command/new.rs#L39-L50

Code after:

    #[command(flatten)]
    pub local_args: LocalProveArgs,
}

#[derive(Debug, Args)]
pub struct LocalProveArgs {
    #[arg(short, name = "k")]
    pub k: Option<usize>,

    /// Path to public parameters file.
    #[arg(short = 'p', long = "public-params")]
    pub pp_file: Option<PathBuf>,

    #[arg(long("impl"))]
    pub prover_impl: Option<vm_config::ProverImpl>,

    /// Path to the SRS file: only needed when pp_file is None and nova_impl is ParallelCompressible.
    #[arg(long("srs-file"))]
    pub srs_file: Option<PathBuf>,
}

#[derive(Debug, Args)]
pub struct CommonProveArgs {
    /// Build artifacts with the specified profile. "release" is default.
    #[arg(long, default_value = "release")]
    pub profile: String,

    /// Name of the bin target to run.
    #[arg(long)]
    pub bin: Option<String>,
}

pub fn handle_command(args: ProveArgs) -> anyhow::Result<()> {
    let ProveArgs {
        common_args: CommonProveArgs { profile, bin },
        network,
        url,
        local_args,
    } = args;

    let path = path_to_artifact(bin, &profile)?;
    let vm_config = vm_config::VmConfig::from_env()?;

    if &profile == "dev" {
        tracing::warn!(
            target: LOG_TARGET,
            "proving debug build",
        )
    }
Repository: nexus-xyz/nexus-zkvm
File: sdk/src/nova/seq.rs (Rust)

Code before:
                .as_slice(),
        );

        let tr = trace(&mut self.vm, K, false).map_err(ProofError::from)?;

        Ok(Self::Proof {
            proof: prove_seq(pp, tr).map_err(ProofError::from)?,
            view: Self::View {
                output: self.vm.syscalls.get_output(),
                logs: self
                    .vm
                    .syscalls
                    .get_log_buffer()
                    .into_iter()
                    .map(String::from_utf8)
                    .collect::<Result<Vec<_>, _>>()
                    .map_err(TapeError::from)?,
            },
        })
    }
}

impl Parameters for PP {
    type Error = Error;

    fn generate_for_testing(k: usize) -> Result<Self, Self::Error> {
        Ok(gen_vm_pp(k, &()).map_err(ProofError::from)?)
    }

    fn load(path: &Path) -> Result<Self, Self::Error> {
        if let Some(path_str) = path.to_str() {
            return Ok(load_pp(path_str).map_err(ProofError::from)?);
        }

        Err(Self::Error::PathError(
            crate::error::PathError::EncodingError,
        ))
    }

    fn save(pp: &Self, path: &Path) -> Result<(), Self::Error> {
        if let Some(path_str) = path.to_str() {
            return Ok(save_pp(pp, path_str).map_err(ProofError::from)?);
        }

        Err(Self::Error::PathError(
            crate::error::PathError::EncodingError,
        ))
    }
}

/// Generate a deployment-ready parameter set used for proving and verifying.

Review comment (suggestion):
You should also modify the `generate` function calls as well.

Code after:
                .as_slice(),
        );

        let tr = trace(&mut self.vm, K, false).map_err(ProofError::from)?;

        Ok(Self::Proof {
            proof: prove_seq(pp, tr).map_err(ProofError::from)?,
            view: Self::View {
                output: self.vm.syscalls.get_output(),
                logs: self
                    .vm
                    .syscalls
                    .get_log_buffer()
                    .into_iter()
                    .map(String::from_utf8)
                    .collect::<Result<Vec<_>, _>>()
                    .map_err(TapeError::from)?,
            },
        })
    }
}

impl Parameters for PP {
    type Error = Error;

    fn generate_for_testing() -> Result<Self, Self::Error> {
        Ok(gen_vm_pp(K, &()).map_err(ProofError::from)?)
    }

    fn load(path: &Path) -> Result<Self, Self::Error> {
        if let Some(path_str) = path.to_str() {
            return Ok(load_pp(path_str).map_err(ProofError::from)?);
        }

        Err(Self::Error::PathError(
            crate::error::PathError::EncodingError,
        ))
    }

    fn save(pp: &Self, path: &Path) -> Result<(), Self::Error> {
        if let Some(path_str) = path.to_str() {
            return Ok(save_pp(pp, path_str).map_err(ProofError::from)?);
        }

        Err(Self::Error::PathError(
            crate::error::PathError::EncodingError,
        ))
    }
}

/// Generate a deployment-ready parameter set used for proving and verifying.
Repository: nexus-xyz/nexus-zkvm
File: runtime/src/lib.rs (Rust)

Code before:
#![cfg_attr(target_arch = "riscv32", no_std)]
#![doc = include_str!("../README.md")]

#[cfg(target_arch = "riscv32")]
mod runtime;
#[cfg(target_arch = "riscv32")]
pub use runtime::*;

#[cfg(target_arch = "riscv32")]
mod alloc;

pub use nexus_rt_macros::{main, profile};

mod ecalls;
pub use ecalls::*;
pub use postcard::*;


Review comment (suggestion):
It would probably be preferable to just do `pub use postcard`, and have users access through `nexus_rt::postcard` to be clear what's going on here.

Code after:
#![cfg_attr(target_arch = "riscv32", no_std)]
#![doc = include_str!("../README.md")]

#[cfg(target_arch = "riscv32")]
mod runtime;
#[cfg(target_arch = "riscv32")]
pub use runtime::*;

#[cfg(target_arch = "riscv32")]
mod alloc;

pub use nexus_rt_macros::{main, profile};

mod ecalls;
pub use ecalls::*;
pub use postcard;
Repository: nginx/nginx
File: src/event/ngx_event_openssl_cache.c (C)

Code before:

    for ( ;; ) {

        pkey = PEM_read_bio_PrivateKey(bio, NULL, cb, pwd);
        if (pkey != NULL) {
            break;
        }

        if (tries-- > 1) {
            ERR_clear_error();
            (void) BIO_reset(bio);
            cb_data.pwd++;
            continue;
        }

        *err = "PEM_read_bio_PrivateKey() failed";
        BIO_free(bio);
        return NULL;
    }

    BIO_free(bio);

#ifdef ERR_R_OSSL_STORE_LIB
done:
#endif

    if (cb_data.encrypted) {
        *passwords = NGX_SSL_CACHE_DISABLED;
    }

    return pkey;
}


static int
ngx_ssl_cache_pkey_password_callback(char *buf, int size, int rwflag,
    void *userdata)
{
    ngx_ssl_cache_pwd_t  *data = userdata;

    ngx_str_t  *pwd;

    if (rwflag) {
        ngx_log_error(NGX_LOG_ALERT, ngx_cycle->log, 0,
                      "ngx_ssl_cache_pkey_password_callback() is called "
                      "for encryption");
        return 0;
    }

    data->encrypted = 1;


Review comment (suggestion):
I don't like the resulting goto wrapped in conditional compilation.

It may be a good idea to remove goto by duplicating the `if (cb_data.encrypted)` condition.

Code after:
    bio = ngx_ssl_cache_create_bio(id, err);
    if (bio == NULL) {
        return NULL;
    }

    for ( ;; ) {

        pkey = PEM_read_bio_PrivateKey(bio, NULL, cb, pwd);
        if (pkey != NULL) {
            break;
        }

        if (tries-- > 1) {
            ERR_clear_error();
            (void) BIO_reset(bio);
            cb_data.pwd++;
            continue;
        }

        *err = "PEM_read_bio_PrivateKey() failed";
        BIO_free(bio);
        return NULL;
    }

    if (cb_data.encrypted) {
        *passwords = NGX_SSL_CACHE_DISABLED;
    }

    BIO_free(bio);

    return pkey;
}


static int
ngx_ssl_cache_pkey_password_callback(char *buf, int size, int rwflag,
    void *userdata)
{
    ngx_ssl_cache_pwd_t  *data = userdata;

    ngx_str_t  *pwd;

    if (rwflag) {
        ngx_log_error(NGX_LOG_ALERT, ngx_cycle->log, 0,
                      "ngx_ssl_cache_pkey_password_callback() is called "
                      "for encryption");
        return 0;
    }

    data->encrypted = 1;
Repository: nginx/nginx
File: src/event/ngx_event_openssl_cache.c (C)

Code before:
            }

            return NULL;
        }

        pkey = NULL;

        while (pkey == NULL && !OSSL_STORE_eof(store)) {
            info = OSSL_STORE_load(store);

            if (info == NULL) {
                continue;
            }

            if (OSSL_STORE_INFO_get_type(info) == OSSL_STORE_INFO_PKEY) {
                pkey = OSSL_STORE_INFO_get1_PKEY(info);
            }

            OSSL_STORE_INFO_free(info);
        }

        OSSL_STORE_close(store);

        if (method != NULL) {
            UI_destroy_method(method);
        }

        if (pkey == NULL) {
            *err = "OSSL_STORE_load() failed";
            return NULL;
        }

        goto done;

#else

        *err = "loading \"store:...\" certificate keys is not supported";
        return NULL;

#endif
    }

    bio = ngx_ssl_cache_create_bio(id, err);
    if (bio == NULL) {
        return NULL;
    }

    for ( ;; ) {

        pkey = PEM_read_bio_PrivateKey(bio, NULL, cb, pwd);
        if (pkey != NULL) {

Review comment (suggestion):
JFTR, UI_destroy_method() accepts NULL since OpenSSL 1.1.1.
Unfortunately, this is not documented.
Also, this was changed silently in https://github.com/openssl/openssl/pull/2204
So we cannot rely on this and should check for NULL.

Code after:
            }

            return NULL;
        }

        pkey = NULL;

        while (pkey == NULL && !OSSL_STORE_eof(store)) {
            info = OSSL_STORE_load(store);

            if (info == NULL) {
                continue;
            }

            if (OSSL_STORE_INFO_get_type(info) == OSSL_STORE_INFO_PKEY) {
                pkey = OSSL_STORE_INFO_get1_PKEY(info);
            }

            OSSL_STORE_INFO_free(info);
        }

        OSSL_STORE_close(store);

        if (method != NULL) {
            UI_destroy_method(method);
        }

        if (pkey == NULL) {
            *err = "OSSL_STORE_load() failed";
            return NULL;
        }

        if (cb_data.encrypted) {
            *passwords = NGX_SSL_CACHE_DISABLED;
        }

        return pkey;

#else

        *err = "loading \"store:...\" certificate keys is not supported";
        return NULL;

#endif
    }

    bio = ngx_ssl_cache_create_bio(id, err);
    if (bio == NULL) {
        return NULL;
    }
Repository: nginx/nginx
File: src/event/quic/ngx_event_quic_ack.c (C)

Code before:
            if ((ngx_msec_int_t) wait > 0
                && ctx->largest_ack - start->pnum < NGX_QUIC_PKT_THR)
            {
                break;
            }

            if (start->send_time > qc->first_rtt) {

                if (oldest == NGX_TIMER_INFINITE || start->send_time < oldest) {
                    oldest = start->send_time;
                }

                if (newest == NGX_TIMER_INFINITE || start->send_time > newest) {
                    newest = start->send_time;
                }

                nlost++;
            }

            ngx_quic_resend_frames(c, ctx);
        }
    }


    /* RFC 9002, 7.6.2.  Establishing Persistent Congestion */

    /*
     * Once acknowledged, packets are no longer tracked. Thus no send time
     * information is available for such packets. This limits persistent
     * congestion algorithm to packets mentioned within ACK ranges of the
     * latest ACK frame.
     */

    if (st && nlost >= 2 && (st->newest < oldest || st->oldest > newest)) {

        if (newest - oldest > ngx_quic_pcg_duration(c)) {
            ngx_quic_persistent_congestion(c);
        }
    }

    ngx_quic_set_lost_timer(c);

    return NGX_OK;
}


static ngx_msec_t
ngx_quic_pcg_duration(ngx_connection_t *c)
{
    ngx_msec_t              duration;
    ngx_quic_connection_t  *qc;

Review comment (bug):
Nitpicking:
It makes sense either to put the assignment under condition as well, or wrap around the conditional block under NGX_DEBUG.

Code after:
void
ngx_quic_congestion_idle(ngx_connection_t *c, ngx_uint_t idle)
{
    ngx_msec_t              now;
    ngx_quic_congestion_t  *cg;
    ngx_quic_connection_t  *qc;

    qc = ngx_quic_get_connection(c);
    cg = &qc->congestion;

    ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0,
                   "quic congestion idle:%ui", idle);

    if (cg->window >= cg->ssthresh) {
        /* RFC 9438, 5.8. Behavior for Application-Limited Flows */

        now = ngx_current_msec;

        if (cg->idle) {
            cg->k += now - cg->idle_start;
        }

        cg->idle_start = now;
    }

    cg->idle = idle;
}


static void
ngx_quic_drop_ack_ranges(ngx_connection_t *c, ngx_quic_send_ctx_t *ctx,
    uint64_t pn)
{
    uint64_t               base;
    ngx_uint_t             i, smallest, largest;
    ngx_quic_ack_range_t  *r;

    ngx_log_debug4(NGX_LOG_DEBUG_EVENT, c->log, 0,
                   "quic ngx_quic_drop_ack_ranges pn:%uL largest:%uL"
                   " fr:%uL nranges:%ui", pn, ctx->largest_range,
                   ctx->first_range, ctx->nranges);

    base = ctx->largest_range;

    if (base == NGX_QUIC_UNSET_PN) {
        return;
    }

    if (ctx->pending_ack != NGX_QUIC_UNSET_PN && pn >= ctx->pending_ack) {
        ctx->pending_ack = NGX_QUIC_UNSET_PN;
    }