feat: batch kernel skeleton#2904
Conversation
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.
Co-authored-by: Marti <marcin.gorny.94@protonmail.com>
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.
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.
The getter had no callers.
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.
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.
…h-kernel-skeleton
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good! I left a few comments with the goal to set up the right basis from which to evolve.
| #! pad(8), | ||
| #! ] | ||
| #! | ||
| #! Outputs: [ |
There was a problem hiding this comment.
I think the original intention here was for the batch kernel to output the root of the BatchNoteTree, rather than just a commitment to the output notes.
This way, the block kernel would have to do much less work and could aggregate the individual BatchNoteTrees into a single BlockNoteTree. This would have to be the MASM equivalent of BlockNoteTree::insert_batch_note_subtree.
I think the reason we don't currently do this at block level, is because we decided to also have note erasure at block level, and so if the block erases on of the batch's notes, the tree root would become stale and would have to be updated. Doing this update is probably still better than recomputing the entire tree from scratch, but we may want to think about that.
To start simple, I see two routes:
- Let the batch output the
BatchNoteTreeroot and aggregate at block level, don't do note erasure at block level for now. - Let the batch output a list of output notes and let the block kernel do the full aggregation. We can optimize this whole process later.
I think the first route is a bit closer to the end goal, so I'd go with that one. Just a suggestion, though.
I think I'm skeptical of the usefulness of note erasure at block level, given that batches are already incentivized to do as much note erasure as possible, and it just adds extra complexity.
There was a problem hiding this comment.
I think I'm skeptical of the usefulness of note erasure at block level, given that batches are already incentivized to do as much note erasure as possible, and it just adds extra complexity.
Agreed, my guess is that this will rarely if ever be used in practice. I'd be in favor of dropping it. Created #3008
There was a problem hiding this comment.
I think the original intention here was for the batch kernel to output the root of the BatchNoteTree, rather than just a commitment to the output notes.
Yes I know, I should have maybe mentioned this in the comments that the first version would just have a commitment to all output notes instead. Mostly because, AFAICS, the BatchNoteTree is not actually wired up anywhere during batch construction.
So what I'd suggest is:
- continue with the skeleton batch kernel to reference
OUTPUT_NOTES_COMMITMENT - wire up the construction of
BatchNoteTreeas part ofProposedBatch::new() - change the batch kernel to output (and verify)
BATCH_NOTE_TREE_COMMITMENT
WDYT? If so I'll create an issue for 2.
There was a problem hiding this comment.
that the first version would just have a commitment to all output notes instead
Does "first version" mean the version after this (or some subsequent) PR or the version we go to mainnet with?
If it's the former, then I'd already setup the outputs to reference what we want it to be, i.e. BATCH_NOTE_TREE_ROOT, but if it's the latter, then using OUTPUT_NOTES_COMMITMENT seems fine.
Your second step will basically imply removing note erasure support at the block level, but I think that's fine to do for now.
There was a problem hiding this comment.
I would have BATCH_NOTE_TREE_ROOT as an output of the kernel even now - even if it just getting there via advice stack or something similar.
Also, agree with the above conclusion that it is fine to disable not erasure at the block level for now. We may find a way to make it work in the future (assuming we want to) with batch kernel outputting BATCH_NOTE_TREE_ROOT as well.
There was a problem hiding this comment.
Does "first version" mean the version after this (or some subsequent) PR or the version we go to mainnet with?
One of the follow-up PRs. Mainnet version should have BatchNoteTree.
I would have
BATCH_NOTE_TREE_ROOTas an output of the kernel even now - even if it just getting there via advice stack or something similar.
ATM with this PR these are all just stack comment names anyway. As mentioned in another comment, we're not wiring anything up in this PR, all of which will be done in a follow-up.
So sure, I can change the name of the stack comment to say BATCH_NOTE_TREE_ROOT.
Anyway as mentioned above, we don't even have the construction of BatchNoteTree in Rust, so that would need to happen first before we can fully wire up BATCH_NOTE_TREE_ROOT in MASM.
There was a problem hiding this comment.
Makes sense. One correction though: we do already have BatchNoteTree in Rust.
There was a problem hiding this comment.
yes but it's not constructed anywhere meaningfully as part of building a batch
There was a problem hiding this comment.
(unless I missed it)
| fn run_kernel( | ||
| program: &Program, | ||
| stack_inputs: StackInputs, | ||
| advice_inputs: AdviceInputs, | ||
| ) -> Result<StackOutputs, miden_processor::ExecutionError> { | ||
| let mut host = DefaultHost::default(); | ||
| host.load_library(CoreLibrary::default().mast_forest()) | ||
| .expect("loading the core library into the test host should succeed"); | ||
|
|
||
| let processor = | ||
| FastProcessor::new_with_options(stack_inputs, advice_inputs, ExecutionOptions::default()) | ||
| .expect("failed to create processor") | ||
| .with_debugging(true); | ||
| let output = processor.execute_sync(program, &mut host)?; | ||
| Ok(output.stack) | ||
| } |
There was a problem hiding this comment.
Probably not for this PR, but I think we'll need something like this to inspect batch kernel memory for memory tests like in test_transaction_prologue and to run unit-like tests for individual procedures in the batch kernel (e.g. like test_is_slot_id_lt).
I think I would set up something similar as CodeExecutor but for the batch. Bascially:
- Rename
CodeExecutortoTransactionCodeExecutor. - Add
BatchCodeExecutor, with arunmethod similar to the currentTransactionCodeExecutor::run.
Lastly, we should use ExecError rather than the raw ExecutionError for better errors.
There was a problem hiding this comment.
let's defer this until it's necessary
…h-kernel-skeleton # Conflicts: # Cargo.lock # crates/miden-protocol/src/batch/mod.rs # crates/miden-testing/src/kernel_tests/batch/mod.rs # crates/miden-tx-batch-prover/Cargo.toml # crates/miden-tx-batch-prover/src/local_batch_prover.rs
…TMENT 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.
Prefer the associated constant `Felt::ZERO` over the bare `ZERO` import in the batch output padding check. Addresses a review comment on PR #2904.
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.
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.
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.
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.
| #! Inputs: [ | ||
| #! BATCH_ID, | ||
| #! BLOCK_COMMITMENT, | ||
| #! pad(8), | ||
| #! ] |
There was a problem hiding this comment.
I wonder if this should also include:
block_num- for the reference block number ofBLOCK_COMMITMENT.accountId- for the account ID for the batch builder. Though, this is not something we've been just mentioning in passing, and the current Rust implementation doesn't require it - so, maybe we skip it for now.
Also, I would maybe put the BLOCK_COMMITMENT on the top of the stack to stay consistent with the transaction kernel.
There was a problem hiding this comment.
AFAIU, block_num is not needed for the batch kernel itself, but rather for the downstream block kernel, right?
In that case I'd also wait until we start working on the block kernel.
There was a problem hiding this comment.
I guess the suggestion for block_num came from mirroring the transaction kernel inputs, which takes block num in addition to block_commitment. I don't fully recall why we added this, since it could technically be recovered from the unhashed reference block. I believe it had something to do with ease of getting the necessary batch inputs for a set of proven transactions, and we didn't want to just add block number in the transaction without having it be verified somewhere.
So, it will probably make sense to also do this for batches, but we can probably also do this when we get there.
| #! | ||
| #! Outputs: [ | ||
| #! INPUT_NOTES_COMMITMENT, | ||
| #! OUTPUT_NOTES_COMMITMENT, | ||
| #! batch_expiration_block_num, | ||
| #! pad(7), | ||
| #! ] |
There was a problem hiding this comment.
A couple of potential modifications here:
- As discussed in the other comment, I'd replace
OUTPUT_NOTES_COMMITMENTwithBATCH_NOTE_TREE_ROOT. - It may be good to add something like
ACCOUNT_UPDATES_COMMITMENTas another output element. This would commit to(accountId, initial_state_hash, final_state_hash)tuples for all updated accounts. The main purpose of this would be to make it simpler for the block kernel to verify consistency between included batches (e.g., "batchb1updates accounta1from statextoy).
There was a problem hiding this comment.
It may be good to add something like ACCOUNT_UPDATES_COMMITMENT as another output element. This would commit to (accountId, initial_state_hash, final_state_hash) tuples for all updated accounts. The main purpose of this would be to make it simpler for the block kernel to verify consistency between included batches (e.g., "batch b1 updates account a1 from state x to y).
I would keep things simple for now, especially since we don't have the block kernel even in its skeleton form. Once that starts taking shape, we'd be in a better position to know exactly what the batch kernel should output.
(I can include a stack comment that this is part of the output but anyway it won't be wired up in the next couple of PRs)
| proc main | ||
| dropw dropw | ||
| end | ||
|
|
||
| begin | ||
| exec.main | ||
| end |
There was a problem hiding this comment.
I know this is meant to be an initial skeleton - but I wonder if we should push it one step further and instead of just using empty words everywhere, set things to real inputs and real outputs (even if the output just come from the advice stack for now).
There was a problem hiding this comment.
This will be all wired up in a follow-up PR which is already in draft.
I kept this as minimal as possible for ease of review
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.
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.
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me!
Love that the proving step no longer needs to execute now!
| /// ```text | ||
| /// [INPUT_NOTES_COMMITMENT, OUTPUT_NOTES_COMMITMENT, batch_expiration_block_num] | ||
| /// ``` | ||
| pub fn build_output_stack( | ||
| input_notes_commitment: Word, | ||
| output_notes_commitment: Word, | ||
| batch_expiration_block_num: BlockNumber, | ||
| ) -> StackOutputs { |
There was a problem hiding this comment.
Docs and variable names might need an update if we change to BATCH_NOTE_TREE_ROOT.
| #! Inputs: [ | ||
| #! BATCH_ID, | ||
| #! BLOCK_COMMITMENT, | ||
| #! pad(8), | ||
| #! ] |
There was a problem hiding this comment.
I guess the suggestion for block_num came from mirroring the transaction kernel inputs, which takes block num in addition to block_commitment. I don't fully recall why we added this, since it could technically be recovered from the unhashed reference block. I believe it had something to do with ease of getting the necessary batch inputs for a set of proven transactions, and we didn't want to just add block number in the transaction without having it be verified somewhere.
So, it will probably make sense to also do this for batches, but we can probably also do this when we get there.
Take `BatchId` instead of a raw `Word` so the input contract is expressed through the domain type.
The type holds multiple outputs; rename it for consistency with `TransactionOutputs`.
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.
Store the parsed `BatchOutputs` on `ExecutedBatch` and expose it via a typed `batch_outputs()` accessor, replacing the raw `stack_outputs()`.
Add unit tests for the well-formed stack, non-zero padding rejection, and oversized expiration block number rejection.
partylikeits1983
left a comment
There was a problem hiding this comment.
Really cool! Still some questions about how the tx proofs will be pulled into the main procedure of the batch kernel, but as a batch kernel skeleton I think this a super solid first step!
Fumuran
left a comment
There was a problem hiding this comment.
Looks great! (Sorry for the late review)
First PR in a three-part split of #2884.
Establishes the public input/output contract for the batch kernel (#1122) plus the Rust plumbing that surrounds it. The kernel does not yet perform any verification — it drops its public inputs and emits the empty word output shape.
towards #1122