Skip to content

Add MEMW_R register fast-path table#477

Merged
jotabulacios merged 6 commits into
mainfrom
feat/memw_r
Apr 9, 2026
Merged

Add MEMW_R register fast-path table#477
jotabulacios merged 6 commits into
mainfrom
feat/memw_r

Conversation

@jotabulacios
Copy link
Copy Markdown
Collaborator

Implements the MEMW_R register fast path from spec PR #457.

Adds a new 10-column table for register accesses.
Registers are always aligned 2-word accesses with is_register=1, so this table strips the general-purpose memory columns (address decomposition, alignment mask, width flags, per-byte old timestamps)

  • 10 columns, 7 bus interactions, 3 transition constraints.
  • Timestamp ordering via IS_HALF lookup instead of LT, proving a delta in [1, 2^16].
  • Routing predicate is_register_op sends eligible register ops toMEMW_R at trace-build time.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 6, 2026

Codex Code Review

Findings

  1. Low - Security/Availability (panic on malformed input)
    In prover/src/tables/memw_register.rs:113, generate_memw_register_trace uses an unconditional assert_eq! on old_timestamp[0] == old_timestamp[1].
    If logs/witness data are malformed (or upstream routing regresses), this can crash the prover process (DoS) instead of returning an error.
    Action: replace with fallible handling (Result) or explicitly reroute/reject without panicking.

  2. Low - Potential bug (release-only invariant gap)
    In prover/src/tables/memw_register.rs:103, register-address parity is guarded only by debug_assert_eq!, but the code still does base_address / 2 in release mode.
    If an odd base_address slips through, it is silently truncated and can cause confusing downstream proof failures rather than a clear error at source.
    Action: enforce this invariant in release builds too (explicit check + error), not just debug builds.

No other concrete security/correctness/performance issues stood out in the PR diff.

Comment thread prover/src/tables/trace_builder.rs
Comment thread prover/src/tables/memw_register.rs
@jotabulacios
Copy link
Copy Markdown
Collaborator Author

/bench

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 6, 2026

Benchmark — fib_iterative_8M (median of 3)

Table parallelism: 32 (auto = cores / 3)

Metric main PR Δ
Peak heap 250294 MB 220983 MB -29311 MB (-11.7%) 🟢
Prove time 79.886s 47.095s -32.791s (-41.0%) 🟢

🎉 Improvement detected — heap or time decreased by more than 5%.

✅ Low variance (time: 2.5%, heap: 0.2%)

Commit: b1f365b · Baseline: cached · Runner: self-hosted bench

@nicole-graus nicole-graus marked this pull request as ready for review April 6, 2026 19:38
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 6, 2026

Codex Code Review

Findings

  1. Medium (Security/Robustness): panic on malformed register timestamp state
  • In memw_register.rs:113, generate_memw_register_trace uses assert_eq!(op.old_timestamp[0], op.old_timestamp[1], ...).
  • This is in non-test production code and will panic! instead of returning a recoverable error if the invariant is violated. In prover services, this is a crash/DoS vector.
  • Action: replace with explicit validation that returns Result<..., Error> (preferred), or at minimum downgrade to debug_assert_eq! and enforce routing invariants before this function is called.
  1. Low (Potential bug in test coverage): vacuous LT assertion
  • In trace_builder_tests.rs:495, the test asserts total_lt_rows > 0.
  • This can pass even when there are no real LT ops, because empty LT op sets still generate padded trace rows.
  • Action: assert on meaningful LT multiplicity or a specific non-padding LT condition, not row count.

Could not run tests in this environment due sandboxed read-only ~/.cargo/~/.rustup paths blocking dependency/toolchain setup.

Comment thread prover/src/tables/trace_builder.rs
Comment thread prover/src/tables/memw_register.rs Outdated
Comment thread prover/src/tables/memw_register.rs
@claude
Copy link
Copy Markdown

claude Bot commented Apr 6, 2026

Review: MEMW_R register fast-path table

Overall the design is sound and well-structured. The structural sharing of TIMESTAMP_1 as the high limb of both timestamps is a clever way to avoid an extra constraint, and the IS_HALFWORD substitution for LT is valid given the routing predicate enforces matching upper limbs. Three issues noted via inline comments:

Severity Issue
Medium collect_bitwise_from_memw_register: underflow in ts_lo - old_ts_lo - 1 is only safe because of implicit pre-filtering; silent wrong result in release mode if invariant breaks
Medium assert_eq! in generate_memw_register_trace panics in release builds; should be debug_assert_eq! since the routing predicate already guards this
Low constraints() is missing a comment explaining why no old_ts_hi == ts_hi constraint is needed (structural column sharing is the reason, but it's a non-obvious load-bearing invariant)

@diegokingston
Copy link
Copy Markdown
Collaborator

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

Comment thread prover/src/tables/memw_register.rs
Comment thread prover/src/tables/memw_register.rs
@jotabulacios
Copy link
Copy Markdown
Collaborator Author

/bench

@jotabulacios jotabulacios added this pull request to the merge queue Apr 9, 2026
Merged via the queue into main with commit 19133a6 Apr 9, 2026
9 checks passed
@jotabulacios jotabulacios deleted the feat/memw_r branch April 9, 2026 12:45
erik-3milabs pushed a commit that referenced this pull request Apr 10, 2026
* save work

* Fix MEMW_R routing predicate to allow timestamp delta = 2^16

* promote old_timestamp assert, add IS_HALF vs LT doc, and expand test coverage

* fix comment

* Add underflow guard and change assert to debug_assert

---------

Co-authored-by: Nicole <nicole.graus@lambdaclass.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants