Conversation
* feat: implement Clone for TransactionContextBuilder * chore: add unwrap_unauthorized_err
* feat(agglayer): reject duplicate GER insertions Adds a duplicate-detection guard to `bridge_config::update_ger`: after calling `set_map_item`, the OLD_VALUE returned is compared against EMPTY_WORD; if it is not empty the GER was already registered and the transaction panics with ERR_GER_ALREADY_REGISTERED. Note on Solidity divergence: the Solidity GlobalExitRootManager treats duplicate GER insertions as idempotent no-ops. Miden intentionally rejects them because a permanently-unconsumable network note is a worse failure mode than an explicit transaction revert. This divergence is documented in SPEC.md. Docs updated on `update_ger`, the `update_ger.masm` note script, and SPEC.md (Panics row + Section 2.3 prose). A new integration test (`update_ger_rejects_duplicate`) verifies that a second UPDATE_GER note carrying the same GER value is rejected with the expected error code. Closes #2708 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: add CHANGELOG entry for #2983 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review: combine padw+assert_eqw and use assert_transaction_executor_error! - bridge_config.masm: collapse the `padw` / `assert_eqw` pair onto a single line as suggested in review. - tests: replace the manual `is_err()` + `error_msg.contains(...)` block with the standard `assert_transaction_executor_error!` macro. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review: use word::eqz + assert instead of padw + assert_eqw Slightly cheaper than the `padw`-then-`assert_eqw` pair: `word::eqz` consumes the 4-felt `OLD_VALUE` and pushes a single 0/1 flag, then a plain `assert.err=...` verifies it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
The MASM doc-comment and formatting conventions previously captured in masm_doc_comment_fmt.md are now covered by the .claude/skills/masm-* skill files plus the MASM formatting tool. Maintaining a separate hand-written guide is redundant and prone to drift, so remove it. Closes #2021 Co-authored-by: Claude (Opus) <noreply@anthropic.com>
The protocol does not prevent adding multiple attachments with the same scheme to a note (e.g. two NetworkAccountTarget attachments). Document that doing so is discouraged as it brings no additional benefit and only increases public on-chain data and fees, and clarify in the rustdoc that the first matching attachment is treated as the canonical one. Closes #2873 Co-authored-by: Claude (Opus) <noreply@anthropic.com>
* refactor: include leaves number in BURN note ID computation * Apply suggestions from code review Co-authored-by: Marti <marcin.gorny.94@protonmail.com> --------- Co-authored-by: Marti <marti@miden.team> Co-authored-by: Marti <marcin.gorny.94@protonmail.com>
* Extend execution-error assertion macros with patter and any arms * Fix formating * Apply PR review feedback
…k level (#2993) * feat: reject consuming and creating same note in a tx * feat: erase notes on the fly * chore: remove unused `NoteCommitmentMismatch` error variants * chore: add changelog * chore: rename input/output note tracker functions * feat: reintroduce `NoteTracker` for readability * chore: improve clarity of comments * chore: format changelog * chore: add second cross-tx circular dep test
* feat: rename storage delta to patch * chore: add changelog
* refactor(protocol): relocate TransactionVerifier into miden-protocol Move TransactionVerifier and TransactionVerifierError from miden-tx to miden-protocol (transaction module + errors) with no behavior change. This lets ProposedBatch (in miden-protocol) verify transaction proofs in a follow-up without a miden-protocol -> miden-tx dependency cycle. TransactionVerifier is a thin wrapper over types already in miden-protocol (TransactionKernel, CoreLibrary, miden-verifier), so the move adds no new dependencies: miden-tx no longer needs miden-verifier, and miden-tx-batch-prover no longer needs miden-tx. No re-export is kept from miden-tx (a breaking change to its public surface); in-repo importers now use miden_protocol::transaction::TransactionVerifier. * refactor(protocol): verify transaction proofs in ProposedBatch::new Move per-transaction proof verification from LocalBatchProver into ProposedBatch construction. ProposedBatch::new now takes a proof_security_level and verifies each transaction's ExecutionProof (via the relocated TransactionVerifier) after the structural batch validation, which is factored into a private new_batch_inner. A testing-gated new_unverified skips verification for batches built from mock/dummy-proof transactions. Deserialization reconstructs via new_batch_inner (no re-verification; documented), so serialized batches do not require valid proofs to be read back. LocalBatchProver loses its verify loop and proof_security_level, collapsing to a single prove. ProposedBatchError gains a typed TransactionVerificationFailed variant; the now-unused ProvenBatchError::TransactionVerificationFailed is removed. MockChain and the batch structural tests build via new_unverified. * Apply suggestions from code review Co-authored-by: Marti <marcin.gorny.94@protonmail.com> --------- Co-authored-by: Claude (Opus) <noreply@anthropic.com>
…ss (#2999) The code-reviewer and security-reviewer agents treat every finding as blocking, with no notion of deliberately-staged work. A skeleton or placeholder that is explicitly documented as incomplete, warns against misuse, and is not wired into a path that relies on the missing behavior was being flagged as a blocking Critical/Important/Warning, forcing a bypass of the pre-push gate. Add a "documented, intentional incompleteness" rule to both agents: downgrade such a limitation to a Nit/Note (which the pre-push hook does not block on) when it is documented, warned about, and self-contained; keep the high severity when the documentation is missing or misleading, or when the incomplete code is actually reachable from a trust boundary. Co-authored-by: Claude (Opus) <noreply@anthropic.com>
…2988) (#3005) * feat(protocol): add active_note::is_public / is_private (#2988) Cherry-pick the reusable protocol-level building blocks from #2988, which was merged to the agglayer branch as an agglayer-specific B2AGG bridge-out hardening: - active_note::is_public - active_note::is_private Both procs read the active note's metadata and decode its note type via the existing note::metadata_into_note_type primitive (from #2738), then compare against NOTE_TYPE_PUBLIC / NOTE_TYPE_PRIVATE from miden::protocol::util::note. A parameterized kernel test covers the Public and Private cases. The agglayer-only files from #2988 (bridge_out.masm, B2AGG.masm, tests/agglayer/bridge_out.rs) and the constant-value flip the PR applied on agglayer's NoteType encoding are intentionally not brought along: next already encodes Private=0, Public=1, and the existing metadata_into_note_type is tailored to next's metadata bit layout. Co-Authored-By: Claude (Opus) <noreply@anthropic.com> * fix(agglayer): enforce NoteType::Public for B2AGG bridge-out notes (#2988) Cherry-pick the bridge-side half of #2988 to next. The B2AGG note type lives in NoteMetadata, not in the recipient commitment, so an attacker could submit a note with an identical recipient, attachment, and asset but NoteType::Private. consensus would accept it, the leaf would be folded into the on-chain Local Exit Tree, but aggkit could never recover the pre-image off-chain — the LET mirror would permanently desync and the bridge-out path would brick. Add an active_note::is_public assert at the top of bridge_out, extend the bridge_out and b2agg.masm doc comments, and parameterize the existing destination-network-is-Miden test into test_bridge_out_rejects_invalid_b2agg_note covering the recipient-identical private-note case too. Co-Authored-By: Claude (Opus) <noreply@anthropic.com> * Apply suggestions from code review Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com> * chore: fmt files --------- Co-authored-by: Claude (Opus) <noreply@anthropic.com> Co-authored-by: Marti <marti@miden.team> Co-authored-by: Philipp Gackstatter <PhilippGackstatter@users.noreply.github.com>
…2755) * Patch crypto with Clones * Split reader writer impls * Fix impl read write matching * Split NullifierTreeBackend reader writer * Replace write with reader traits * Lint * update crypto after revert * Back to v0.14 * Back to v0.15 * Fix trait bound readers * Update crypto dep * Changelog * Update crypto branch * Update deps * Update deps * Update deps * Update deps * Add reader() * Update deps * Bump crypto v0.24 * Changelog * Update changelog * Move id prefix to smt fn to account id key * Move account tree impls * Move nullifier tree impls * Add AccountTree::with_storage_from_entries * Changelog * fix docs --------- Co-authored-by: Marti <marti@miden.team>
…ng (#3015) * docs(overview): clarify transaction definition and execution vs proving * docs(overview): refine transaction definition per review Address Philipp's review note: a transaction is not always the state transition of an account. Reframe so account mutation is one of two possible effects (mutating storage/vault, or creating/consuming notes), and note that the account need not be mutated at all. * docs: add changelog entry for #3015
…g tooling (#2997) * fix(hooks): base pre-push review on the integration branch The pre-push reviewer derived its diff base from the branch's own upstream tracking ref (@{u}). For a branch that merged next in, the merge-base against its pre-merge remote tip swept the entire merged history into the reviewed diff, so the code-reviewer and security-reviewer ended up checking all of next instead of just the branch's own change. Resolve the review base from the remote default branch (origin/HEAD, i.e. origin/next) instead. merge-base(HEAD, origin/next) is then the branch's divergence point, so the reviewed diff is exactly this branch's contribution, even after merging next in. Adds unit tests for _diff_base. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Update .claude/hooks/pre_push_review.py * chore(hooks): drop the _diff_base unit test Not needed for this change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(reviewers): diff the integration branch / PR, not the local checkout Apply the same diff-base correction this PR makes to pre_push_review.py across the agents it spawns and the changelog agent: - code-reviewer / security-reviewer diffed `@{upstream}...HEAD`, which (like the hook before this PR) drags the whole merged history into the review once the integration branch is merged in. Point them at the remote default branch (`origin/HEAD`) instead. - changelog-manager classified a local `git diff origin/next...HEAD`, which analyzes whatever branch the cwd is on rather than the PR being created. Classify `gh pr diff <N>` instead, so it always sees the PR's own diff (this also fixes creating a PR for one branch from a worktree on another). --------- Co-authored-by: Claude (Opus) <noreply@anthropic.com>
… review (#3024) Co-authored-by: Claude (Opus) <noreply@anthropic.com>
* feat: add skeleton batch kernel + ProvenBatch proof field Establishes the public input/output contract for the batch kernel (#1122) plus the Rust plumbing that surrounds it, without any verification logic. - main.masm drops TRANSACTIONS_COMMITMENT + BLOCK_HASH and exits; the VM's depth >= 16 invariant leaves the all-zero 16-felt output region in place. - BatchKernel struct exposes prepare_inputs / build_input_stack / build_output_stack / parse_output_stack; build_advice_inputs returns the default empty AdviceInputs since the skeleton ignores advice data. - ProvenBatch carries a proof: ExecutionProof field through new_unchecked and serde. - LocalBatchProver::prove now runs the kernel via miden_prover::prove and attaches the proof to the returned ProvenBatch. The kernel's public outputs are not yet cross-checked against the proposed batch; that lands with the verification logic. - prove_dummy retained for tests that don't want proof generation. Smoke test exercises the full plumbing: builds a realistic two-transaction ProposedBatch, runs the kernel via FastProcessor, asserts the parsed outputs are empty / zero. TODO list in main.masm enumerates the checks the verification PR will introduce. * Apply suggestions from code review Co-authored-by: Marti <marcin.gorny.94@protonmail.com> * chore(protocol): drop premature BATCH error category The batch kernel is compiled separately and its MASM errors are not extracted by generate_error_constants (which only scans the transaction kernel dir), so the BATCH entry in TX_KERNEL_ERROR_CATEGORIES is inert. Batch error handling will be added properly alongside the verification logic. * refactor(protocol): order build_input_stack params to match stack layout Pass transactions_commitment before block_hash so the parameter order matches the documented input stack [TRANSACTIONS_COMMITMENT, BLOCK_HASH]. Also drop the now-stale reference to the main.masm TODO from the BatchKernel doc comment. * refactor(batch-prover): remove unused proof_security_level accessor The getter had no callers. * refactor(protocol): simplify batch output padding check Replace the two separate pad-word checks with a single scan over every cell after batch_expiration_block_num, dropping the EXPIRATION_PAD_WORD_* and TRAILING_PAD_WORD_FELT_IDX constants. * test(batch): reuse shared chain setup helpers Expose TestSetup and setup_chain from the proposed_batch test module and reuse them in the batch kernel smoke test instead of re-implementing the identical TestSetup/setup/generate_account fixtures. * docs: trim verbose batch kernel CHANGELOG entry * refactor(protocol): name batch kernel inputs BATCH_ID and BLOCK_COMMITMENT Rename the batch kernel's public inputs from TRANSACTIONS_COMMITMENT to BATCH_ID (consistent with the `BatchId` type the value comes from) and from BLOCK_HASH to BLOCK_COMMITMENT (we no longer use "hash"). Trim the over-detailed BATCH_ID doc comment down to just referencing `BatchId`. Addresses review comments on PR #2904. * refactor(protocol): use Felt::ZERO associated constant Prefer the associated constant `Felt::ZERO` over the bare `ZERO` import in the batch output padding check. Addresses a review comment on PR #2904. * refactor(protocol): type batch kernel errors instead of stringifying Stringifying the kernel error via `to_string` discarded the source error chain. Wrap the real source instead: `BatchKernelExecutionFailed` now carries `#[source] ExecutionError`, and a dedicated `BatchKernelOutputInvalid(#[source] BatchOutputError)` variant covers output-stack parsing failures. The batch prover maps these directly. Addresses a review comment on PR #2904. * test(batch): rename batch_kernel module to test_batch_kernel Adopt the `test_*` naming used by `kernel_tests/tx/test_*.rs` so the `kernel_tests::batch::batch_kernel` path is no longer redundant and future per-feature batch kernel test modules slot in consistently. Addresses a review comment on PR #2904. * chore: shorten comment * chore: fmt * refactor(batch-prover): split batch execution and proving Separate batch kernel execution from proof generation, mirroring the transaction flow's ExecutedTransaction: ProposedBatch -> BatchExecutor::execute -> ExecutedBatch ExecutedBatch -> LocalBatchProver::prove -> ProvenBatch `BatchExecutor::execute` runs the batch kernel (via the synchronous trace APIs, `execute_trace_inputs_sync`), validates the output stack shape, and returns an `ExecutedBatch` holding the trace inputs. `LocalBatchProver` now consumes an `ExecutedBatch` and only builds the trace + proof (via `prove_from_trace_sync`), so the flow is fully synchronous. Transaction proofs are already verified in `ProposedBatch::new`, so the executor does not re-verify them. This lets tests exercise raw batch execution without re-proving. The batch kernel smoke test now goes through `BatchExecutor`, removing the inline FastProcessor setup, and a new test covers the execute -> prove path. Addresses review comment T19 on PR #2904. * Update crates/miden-tx-batch-prover/src/batch_executor.rs * refactor(protocol): return a BatchOutput from parse_output_stack Introduce a `BatchOutput` type (mirroring `TransactionOutputs`) that encapsulates the batch kernel's parsed outputs — input/output note commitments and the batch expiration block number — instead of returning a raw `(Word, Word, BlockNumber)` tuple. The output-stack layout indices move onto `BatchOutput` as associated constants. Addresses review comment T15 on PR #2904. * refactor(protocol): put BLOCK_COMMITMENT on top of the batch input stack Order the batch kernel's public inputs as [BLOCK_COMMITMENT, BATCH_ID] to stay consistent with the transaction kernel's input layout. Addresses a review comment on PR #2904. * refactor(protocol): name the batch output BATCH_NOTE_TREE_ROOT Rename the batch kernel's second output from OUTPUT_NOTES_COMMITMENT to BATCH_NOTE_TREE_ROOT (and the `BatchOutput` field/accessor/layout const accordingly). The end-goal output is the root of the batch's note tree, which lets the block kernel aggregate per-batch note trees instead of a flat output-notes commitment; renaming now fixes the contract name even though the value is still wired up in a follow-up. Addresses a review comment on PR #2904. * refactor(batch): strong-type build_input_stack batch_id param Take `BatchId` instead of a raw `Word` so the input contract is expressed through the domain type. * refactor(batch): rename BatchOutput to BatchOutputs The type holds multiple outputs; rename it for consistency with `TransactionOutputs`. * refactor(batch): move output parsing onto BatchOutputs and return errors Replace `BatchKernel::parse_output_stack` with `BatchOutputs::parse`, and return `BatchOutputError::OutputStackInvalid` instead of panicking when a required output word or element is missing from the stack. * refactor(batch): expose typed BatchOutputs from ExecutedBatch Store the parsed `BatchOutputs` on `ExecutedBatch` and expose it via a typed `batch_outputs()` accessor, replacing the raw `stack_outputs()`. * test(batch): cover BatchOutputs::parse error branches Add unit tests for the well-formed stack, non-zero padding rejection, and oversized expiration block number rejection. * Apply suggestion from @mmagician --------- Co-authored-by: Claude (Opus) <noreply@anthropic.com>
…3032) Adds a regression test that pushes a marker word before calling active_note::get_metadata and asserts the metadata word is added directly on top of it. This catches the regression where an empty NOTE_ATTACHMENT word leaked onto the stack alongside the metadata. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d` traits (#3009) * refactor: unify account/nullifier SMT backends into shared SmtBackend traits Replaces the near-identical AccountTreeBackend/NullifierTreeBackend trait pairs with a single Word-valued SmtBackendReader + SmtBackend pair, impl'd once for Smt and once for LargeSmt. The NullifierBlock value wrapping moves from the backend into NullifierTree's own accessors, leaving the backend layer value-agnostic. Removes both per-tree backend.rs modules. * test: add reader() coverage; fix changelog and backend docs Addresses pre-push review findings on the SMT backend unification: - correct the CHANGELOG entry to name the shared SmtBackend/SmtBackendReader traits instead of the removed per-tree reader traits - disambiguate the writer-impl section headers in smt_backend.rs - document that with_storage_from_entries panics (not errors) on storage failures, matching large_smt_error_to_merkle_error behavior - add reader() tests asserting the read-only view shares root and entries * refactor: unify duplicate-entry error handling across account backends Removes the redundant AccountTreeError::DuplicateEntries variant and maps the LargeSmt-backed with_storage_from_entries duplicate case through the same prefix-extracting helper as the Smt-backed with_entries, so both constructors surface DuplicateStateCommitments { prefix }. Adds a duplicate-entry test, documents the read-only reader() invariant, and aligns the leaves()/entries() lifetime style. * docs: reference PR #3009 in changelog entry * test: drop tautological reader_matches_source_tree tests These only asserted that a freshly created reader view equals its source, which exercises no meaningful logic. The duplicate-entry test (real error path) is kept. * Apply suggestions from code review Co-authored-by: Marti <marcin.gorny.94@protonmail.com> * docs: fix unresolved intra-doc links in SmtBackendReader Use inline `[`leaves`](Self::leaves)` / `[`entries`](Self::entries)` links instead of reference-style links, which rustdoc failed to resolve under `-D warnings`. --------- Co-authored-by: Claude (Opus) <noreply@anthropic.com> Co-authored-by: Bobbin Threadbare <43513081+bobbinth@users.noreply.github.com>
* feat(batch-prover): verify batch kernel execution proofs Add `BatchVerifier`, the verification counterpart to `LocalBatchProver`. It reconstructs the batch kernel's public input stack from a `ProvenBatch` (`BatchId` + reference block commitment) and verifies the attached `ExecutionProof` against the batch kernel program via `miden_verifier::verify`. The skeleton kernel emits the all-zero output region, so verification checks the proof against that empty-output contract; binding the batch's real commitments lands with the kernel verification logic. The verifier is documented as not yet usable at a trust boundary. Tests build a real batch proof via `BatchExecutor` + `LocalBatchProver` and cover the positive path (verify + insufficient-security-level rejection) and the negative path (dummy proof rejected). * refactor(batch-prover): align BatchVerifier with new input order Adapt the verifier to the base-branch changes merged in: feed the kernel public inputs as [BLOCK_COMMITMENT, BATCH_ID] (matching the reordered build_input_stack signature) and update the docs/comments to reference the batch note tree root output. * test(batch-prover): assert against the proof's actual security level Use proven.proof_security_level() in the accept test: requiring exactly that level succeeds and requiring one more bit fails, instead of the arbitrary 0 / u32::MAX bounds. * refactor(protocol): replace build_output_stack with BatchOutput::into_stack_outputs Encoding a BatchOutput into the kernel's output stack belongs on BatchOutput itself, mirroring BatchKernel::parse_output_stack (the inverse) and reusing the layout constants it already owns. The verifier now constructs a typed all-zero BatchOutput and converts it, which reads more clearly than passing three loose words to a free function. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 9e91ae3) * fix(batch-prover): pass BatchId to build_input_stack after rebase next changed BatchKernel::build_input_stack to take a BatchId instead of a Word; the rebased BatchVerifier still passed batch.id().as_word(), which broke the build. Pass batch.id() directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude (Opus) <noreply@anthropic.com>
* init * changelog * collapse constructors into custom * add bon builder * lint * remove panic add error * apply suggestions * add AccountComponent::has_procedure --------- Co-authored-by: Bobbin Threadbare <43513081+bobbinth@users.noreply.github.com>
* feat: Add `AccountPatch` and `AccountVaultPatch` * feat: track vault updates for both delta and patch * chore: return initial asset value from asset_vault proc * chore: track initial and final asset values * chore: add changelog * fix: broken doc link * chore: unify separate maps in vault tracker into one * chore: new_nonce -> final_nonce, old_nonce -> initial_nonce * chore: move AccountStoragePatch to patch module * chore: remove buggy `assets` iterator * chore: leave note for why headers are appended * chore: vault_patch -> vault * chore: simplify `is_empty` impl * chore: clarify asset value / empty word in patch docs * chore: `AccountDeltaTracker` -> `AccountUpdateTracker` * feat: domain-separate delta and patch commitment headers Add distinct domain separators to the headers of `AccountDelta` and `AccountPatch` commitments so the two contexts can never collide, even when their bodies are identical. The asset, value-slot, and map-slot domains are renumbered to 3/4/5 to leave 1/2 reserved for the delta/patch headers, and the shared constants are centralized in a new `commitment_domain` module. * fix: docs on with_assets * fix: lower case domain_delta * fix: invert vault patch normalization to drop unchanged entries * chore: name add_asset/remove_asset output FINAL_ASSET_VALUE --------- Co-authored-by: Claude (Opus) <noreply@anthropic.com>
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
9 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.
This is a tracking PR for v0.16.0 release.