Draft
Conversation
Implements the public-input/output interface for issue #1122 plus the minimal MASM that verifies every advice-provider input back to the public TRANSACTIONS_COMMITMENT. Stack interface (per #919): Inputs: [BLOCK_HASH, TRANSACTIONS_COMMITMENT] Outputs: [INPUT_NOTES_COMMITMENT, OUTPUT_NOTES_COMMITMENT, batch_expiration_block_num] The kernel never trusts advice data. Each layer is loaded via adv.push_mapvaln keyed by the previously-verified hash, piped to memory while sequentially hashed, and assert_eqw'd against the key: Layer 1 (TRANSACTIONS_COMMITMENT) -> (tx_id, account_id) tuples Layer 2 (each tx_id) -> per-tx header (init/final, per-tx note commitments, fee asset) Layer 3 (per-tx INPUT_NOTES_C) -> (nullifier, commit_or_zero) tuples Layer 3' (per-tx OUTPUT_NOTES_C) -> (note_id, metadata_commit) tuples Module split mirrors ProposedBatch::new sections (prologue, note_tracker) so future PRs can lift each TODO check 1:1 from Rust. Also: - Extracts BatchId::tuple_elements / TransactionId::input_elements as pub(crate) helpers shared between the Rust hashers and the kernel advice builder (single source of truth for felt layouts). - Adds proof: ExecutionProof to ProvenBatch; LocalBatchProver::prove now runs the kernel via miden_prover::prove and sanity-checks the parsed batch_expiration_block_num against the proposed batch. - prove_dummy retained for tests that don't want proof-generation cost. - BLOCK_HASH consumed as public input but not yet verified; see TODO list in main.masm. Tests: 5 unit tests in batch::kernel (happy path + four negative cases covering each ERR_BATCH_*). Full nextest suite passes (993 tests, prove-tests filtered). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Remove the pad(N) and pad... shorthands; comment only the elements that participate in the kernel's logic. The depth-floor zero-fill is implicit and doesn't need to be repeated in every stack annotation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…chain The previous main.masm header dove straight into the unhashing chain without first explaining why a batch kernel exists. Add a two-step high-level description (reconstruct per-tx data, aggregate into batch-wide commitments) so readers landing here cold can orient themselves before parsing the layer-by-layer details. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The "Where:" block leaked implementation history ("currently consumed
and dropped", "does not yet match…") and project-management framing
("each will be lifted in a follow-up issue") that aren't useful to
external readers. Trim each entry to what the value is, and rewrite the
follow-up list as plain TODO comments.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Remove inline comments that restate what the operation right below them already says: "Bring TRANSACTIONS_COMMITMENT to the top so the prologue can consume it" before a `swapw`, "Compute the batch INPUT_NOTES_COMMITMENT" before `exec.compute_input_notes_commitment`, "Truncate the stack to the canonical 16-element output…" before the truncate idiom, etc. Stack-state comments (`# Stack: [...]`) and notes that explain non-obvious choices (e.g. the `u32lt` argument ordering) are kept. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pinning citations to specific Rust line ranges (e.g. "ProposedBatch::new lines 184-193") rots as soon as the Rust file is edited. The TODO text itself names what the eventual check needs to do; readers who want the Rust counterpart can grep for the type / function name. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous name described the *shape* of the returned value (a flat sequence of `(tx_id, account_id_pair)` tuples) but not its *purpose*. The function returns the felt sequence that `from_ids` hashes; readers shouldn't have to chase the call site to learn that. Aligns with the existing `TransactionId::input_elements` convention elsewhere in the crate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Keep one mention on the `BatchKernel` Rust struct (the type readers land on first when looking up the kernel from a downstream crate) and drop it everywhere else. The MASM source describes what the program *does* without editorializing on its scope, and the prover docstring explains the divergence between kernel-side and Rust-side note commitments without labelling the kernel itself. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… data The earlier tests in `miden-protocol::batch::kernel` constructed felt sequences by hand and asserted the kernel produced the same hash — which only proved the test's expected-value helper agreed with the kernel, not that either matched real-batch semantics. Move the kernel tests next to the existing `proposed_batch` tests in `miden-testing::kernel_tests::batch` and drive them through the same `MockChain` + `MockProvenTxBuilder` pipeline used by the rest of the batch tests: - Two real accounts created from the genesis block. - One real authenticated input note (P2ID), consumed by tx1. - One synthesised unauthenticated input note, consumed by tx2. - Three synthesised output notes (one in tx1, two in tx2). - Two distinct expirations so the min reduction has something to do. The expected note commitments are still computed in Rust for comparison (the kernel emits the raw, un-erased aggregation), but they are derived from the real `ProposedBatch`'s transactions rather than fabricated tuples. Negative tests retained: corrupting `TRANSACTIONS_COMMITMENT`, the Layer 2 entry, and either Layer 3 entry must each abort the kernel with the corresponding `ERR_BATCH_*`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Second pass over the PR after the earlier main.masm cleanup. Drops: - "initial, minimal implementation … defers a number of checks" from the BatchKernel rustdoc; the TODO list in main.masm already covers what is and isn't enforced. - "Any refactor that drops the leading `swapw` must update this builder to match" — addresses future maintainers; replaced with a fact-stated cross-reference to main.masm. Drops the leftover `pad(8)` notation along with it. - "Exposed for use by the batch kernel which …" on `BatchId::hash_input_elements` and `TransactionId::input_elements`; the docstring should describe what the function returns, not justify the visibility. - "`prove` is `async` because the underlying `miden_prover::prove` is `async`" — the signature already says it's async; the reason adds no value to a downstream caller. - "sanity-checked" / "intentionally diverges" / "(indicates a kernel/advice-builder bug)" / "Exposed for tests that want a ProvenBatch without paying the cost of proof generation" — defensive / scope-cut / project-internal phrasing throughout `LocalBatchProver` docs. - "currently being absorbed" → "being absorbed" in memory.masm. - "Runs the equivalent of the leading sections of `ProposedBatch::new`" → "Performs three steps" in prologue.masm. - "once recursive proof verification ships" — project-management framing on a TODO; dropped. - "(mirrors `InputOutputNoteTracker::from_transactions`)" → "(see `InputOutputNoteTracker::from_transactions`)" — keeps the useful pointer, drops the "mirrors" framing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Flip the input order so `TRANSACTIONS_COMMITMENT` is at the top and `BLOCK_HASH` is below. The first thing main does is `swapw dropw` to discard `BLOCK_HASH` (which the kernel does not yet verify), leaving `TRANSACTIONS_COMMITMENT` ready for the prologue to consume. Side effects of the rearrangement: - The input/output type signatures in main.masm now show the trailing `pad(N)` like the transaction kernel header. - The closing `repeat.3 movupw.3 dropw end` truncate carries a comment explaining why it's needed (depth-floor invariant means `consume`-only operations don't shrink the stack below 16, so the depth has grown above 16 by the end and needs to be brought back down). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Rephrase the prologue's running-min description from "accumulates the batch expiration min" to "tracks the smallest expiration_block_num across all transactions". - Replace "Layer 1 / Layer 2 / Layer 3 / Layer 3' tuple data" in the prologue and note_tracker error and input descriptions with the actual shape of the data — e.g. "the (tx_id, account_id) tuple list", "(INIT, FINAL, INPUT_NOTES_COMMITMENT, OUTPUT_NOTES_COMMITMENT, FEE_ASSET) data", "(NULLIFIER, EMPTY_OR_COMMITMENT) tuple list" — so readers who haven't internalised the layer numbering can still tell what the kernel is checking against. - Drop the inline `u32lt` semantics-explainer comment in prologue.masm. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tures, add changelog entry
…_dummy compiles in test builds with --no-default-features
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #1122