Migrate batch kernel parts from miden-node #1112

PhilippGackstatter · 2025-01-28T15:41:39Z

Migrates the TransactionBatch and related types from miden-node to miden-base.

Mostly ready for review and tested.

A bunch of TODOs are left, mostly around documentation and on types that I'm not yet sure we will need in this form (NoteInclusionProof, BlockInclusionProof, ...) so I didn't want to document them yet.

Unrelated: Removes BlockNumber::from_usize which we don't need and is dangerous.

PhilippGackstatter · 2025-01-28T15:43:35Z

crates/miden-batch-prover/src/local_batch_prover.rs

+    /// Test that an authenticated input note that is also created in the same batch does not error
+    /// and instead is marked as consumed.
+    /// - This requires a nullifier collision on the input and output note which is very unlikely in
+    ///   practice.
+    /// - This makes the created note unspendable as its nullifier is added to the nullifier tree.
+    /// - The batch kernel cannot return an error in this case as it can't detect this condition due
+    ///   to only having the nullifier for authenticated input notes _but_ not having the nullifier
+    ///   for private output notes.
+    /// - We test this to ensure the kernel does something reasonable in this case and it is not an
+    ///   attack vector.
+    #[test]
+    fn authenticated_note_created_in_same_batch() -> anyhow::Result<()> {


This is a pretty tricky case I want to call out. This describes the best way I can think of to handle it and the rationale, but not sure if there's a better way.

PhilippGackstatter · 2025-01-28T15:46:46Z

crates/miden-objects/src/batch/batch_id.rs

+// TODO: Should this really be a Blake3 hash? We have to compute this in the block kernel
+// eventually, so we'd probably want RPO instead?
+// TODO: Compute batch ID as hash over tx ID _and_ account ID.
+#[derive(Debug, Copy, Clone, Eq, Ord, PartialEq, PartialOrd)]
+pub struct BatchId(Blake3Digest<32>);


I guess it was just a temporary thing to use a Blake3 hash for the Batch ID, so I have yet to update this.

Yes, we should switch to RPO. One potential concern is the performance of RPO vs. BLAKE3 - but I think until we are building hundreds of batches per second, the hashing cost should be negligible.

PhilippGackstatter · 2025-01-28T15:49:08Z

crates/miden-objects/src/batch/proposed_batch.rs

+/// A proposed batch of transactions with all necessary data to validate it.
+#[derive(Debug, Clone)]
+pub struct ProposedBatch {


ProposedBatch and ProvenBatch sound nice together, but I wonder, for consistency with other input types like TransactionInputs or BlockInputs, whether this should actually be called BatchInputs?

We could also try to go into the opposite direction. May work for ProposedBlock but not sure about ProposedTransaction.

PhilippGackstatter · 2025-01-28T15:51:10Z

@bobbinth I think this is ready for an initial review, but I still expect to change some things (BatchId computation and verifying the proofs of ProvenTransactions in the batch). I think it might still be beneficial to get an impression of whether this is the direction you were thinking of as well.

bobbinth

Looks good! Thank you! Not a full review, but I left some comments inline. Overall, this is pretty much how I was thinking about it as well.

crates/miden-objects/src/block/inclusion_proof.rs

crates/miden-objects/src/note/authentication_info.rs

bobbinth · 2025-01-29T09:48:44Z

crates/miden-objects/src/batch/proposed_batch.rs

+/// A proposed batch of transactions with all necessary data to validate it.
+#[derive(Debug, Clone)]
+pub struct ProposedBatch {


We could also try to go into the opposite direction. May work for ProposedBlock but not sure about ProposedTransaction.

crates/miden-objects/src/batch/proposed_batch.rs

bobbinth · 2025-01-29T09:55:11Z

crates/miden-objects/src/batch/batch_id.rs

+// TODO: Should this really be a Blake3 hash? We have to compute this in the block kernel
+// eventually, so we'd probably want RPO instead?
+// TODO: Compute batch ID as hash over tx ID _and_ account ID.
+#[derive(Debug, Copy, Clone, Eq, Ord, PartialEq, PartialOrd)]
+pub struct BatchId(Blake3Digest<32>);


Yes, we should switch to RPO. One potential concern is the performance of RPO vs. BLAKE3 - but I think until we are building hundreds of batches per second, the hashing cost should be negligible.

crates/miden-objects/src/batch/batch_id.rs

bobbinth · 2025-01-29T10:01:35Z

crates/miden-objects/src/batch/proven_batch.rs

+pub struct ProvenBatch {
+    id: BatchId,
+    account_updates: BTreeMap<AccountId, BatchAccountUpdate>,
+    input_notes: Vec<InputNoteCommitment>,


Should we use InputNotes<InputNoteCommitment> here instead of Vec<InputNoteCommitment>?

crates/miden-batch-prover/src/local_batch_prover.rs

bobbinth

Looks great! Thank you! I reviewed pretty much all non-testing code and left some small comments inline.

Before merging this, would be good to create a "parallel" PR in the miden-node repo which integrates this functionality into the node.

Also, we should probably create a good set of tests for the construction of the ProposedBatch - but this is probably best done in a different PR.

bobbinth · 2025-01-29T18:42:01Z

crates/miden-objects/src/batch/proposed_batch.rs

+    /// The header is boxed as it has a large stack size.
+    block_header: Box<BlockHeader>,


Would we still need Box here after #1076? If not, maybe we should quickly do #1076 first?

bobbinth · 2025-01-29T18:43:23Z

crates/miden-objects/src/batch/proposed_batch.rs

+    /// The note inclusion proofs for unauthenticated notes that were consumed in the batch which
+    /// can be authenticated.
+    authenticatable_unauthenticated_notes: BTreeMap<NoteId, NoteInclusionProof>,


I like the unauthenticated_note_proofs alternative better.

bobbinth · 2025-01-29T18:46:14Z

crates/miden-objects/src/transaction/chain_mmr.rs

+    /// Returns an iterator over the block headers in this chain MMR.
+    pub fn block_headers_iter(&self) -> impl Iterator<Item = &BlockHeader> {
+        self.blocks.values()
+    }


nit: I would probably name this just block_headers().

bobbinth · 2025-01-29T18:48:49Z

crates/miden-objects/src/batch/proposed_batch.rs

+    ///     - Another transaction in the batch produces the unauthenticated input note, in which
+    ///       case inclusion in the chain must not be proven.


I would maybe write this as: "Another transaction in the batch produces an output note matching an unauthenticated input note, in which case inclusion in the chain does not need to be proven.".

bobbinth · 2025-01-29T19:09:27Z

crates/miden-objects/src/batch/proposed_batch.rs

+                    let batch_account_update = BatchAccountUpdate::new(
+                        tx.account_id(),
+                        tx.account_update().init_state_hash(),
+                        tx.account_update().final_state_hash(),
+                        vec![tx.id()],
+                        tx.account_update().details().clone(),
+                    );


Should we move this logic into something like BatchAccountUpdate::from_transaction()?

bobbinth · 2025-01-29T19:23:45Z

crates/miden-objects/src/batch/proposed_batch.rs

+        // Remove all output notes from the batch output note set that are consumed by transactions.
+        //
+        // This still allows transaction `A` to consume an unauthenticated note `x` and output note
+        // `y` and for transaction `B` to consume an unauthenticated note `y` and output
+        // note `x` (i.e., have a circular dependency between transactions).
+        let mut output_notes = BatchOutputNoteTracker::new(transactions.iter().map(AsRef::as_ref))?;


This also makes sure that there are no duplicate output notes, right? If so, maybe worth mentioning this in the comments?

bobbinth · 2025-01-29T19:32:07Z

crates/miden-batch-prover/Cargo.toml

+[package]
+name = "miden-batch-prover"


I wonder if we should name this miden-tx-batch-prover to indicate that we are proving batches of transactions.

bobbinth · 2025-01-29T19:34:01Z

crates/miden-batch-prover/Cargo.toml

+anyhow = { version = "1.0", default-features = false, optional = true }
+assembly = { workspace = true }
+miden-crypto = { workspace = true }
+miden-lib = { workspace = true, optional = true, features = ["testing"] }
+miden-tx = { workspace = true }
+miden-objects = { workspace = true }
+miden-verifier = { workspace = true }
+rand = { workspace = true, optional = true, features = ["small_rng"] }
+thiserror = { workspace = true }
+vm-core = { workspace = true }
+vm-processor = { workspace = true }
+winterfell = { version = "0.11", optional = true }


Could we double-check if all of these are needed as dependencies or if some could go into dev-dependencies? (e.g., upon a quick look I didn't see if we are using rand in non-testing code).

PhilippGackstatter added 22 commits January 28, 2025 16:41

feat: Add miden-batch-prover crate

6a696fd

feat: Add BatchId

2b44b45

Introduce AccountUpdateError

71b5be9

feat: Add ProposedBatch

792369a

feat: Add LocalBatchProver

4ca86ef

feat: Add ProvenBatch

206e47f

feat: Migrate LocalBatchProver from node

cf580af

chore: Rename NoteAuthenticationInfo

f28aa6f

feat:Add batch expiration block num

4cc4453

chore: Use core instead of std for Display

53970f5

feat: Migrate MockProvenTxBuilder

5f50946

feat: Test tx ordering in batches

d961a78

feat: Add BatchAccountUpdate

c3b3fe2

chore: Extend test assertions

43cbb81

feat: Refactor and document batch output note tracker

c014be8

feat: Add input/output notes commitment test

9f1be8b

feat: Remove BlockNumber::from_usize

5887560

feat: Check for duplicate input notes

e8b4be4

feat: Add unauthenticated/authenticated scenario tests

45dfa3f

chore: Misc doc fixes

a7a9be2

feat: Move ProposedBatch and ProvenBatch to objects

8ca80cd

chore: Add changelog entry

921c6d9

PhilippGackstatter force-pushed the pgackst-batch-prover-crate branch from 8eeced0 to 921c6d9 Compare January 28, 2025 15:42

PhilippGackstatter commented Jan 28, 2025

View reviewed changes

PhilippGackstatter marked this pull request as ready for review January 28, 2025 15:49

bobbinth reviewed Jan 29, 2025

View reviewed changes

feat: Use MockChain for batch tests (partially)

a4d4b1e

PhilippGackstatter added 6 commits January 29, 2025 12:56

chore: Move most validation to ProposedBatch

fda5530

feat: Use MockChain for all tests

18817bf

feat: Implement note authentication and tests for it

63c0e08

feat: Verify transactions in batch

94d7382

chore: Document error conditions on proposed batch

46d01b0

feat: Check max input/output notes and account updates in batch

44d0330

bobbinth reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate batch kernel parts from miden-node #1112

Migrate batch kernel parts from miden-node #1112

PhilippGackstatter commented Jan 28, 2025 •

edited

Loading

PhilippGackstatter Jan 28, 2025

PhilippGackstatter Jan 28, 2025

bobbinth Jan 29, 2025

PhilippGackstatter Jan 28, 2025

bobbinth Jan 29, 2025

PhilippGackstatter commented Jan 28, 2025

bobbinth left a comment

bobbinth Jan 29, 2025

bobbinth Jan 29, 2025

bobbinth Jan 29, 2025

bobbinth left a comment

bobbinth Jan 29, 2025

bobbinth Jan 29, 2025

bobbinth Jan 29, 2025

bobbinth Jan 29, 2025

bobbinth Jan 29, 2025

bobbinth Jan 29, 2025

bobbinth Jan 29, 2025

bobbinth Jan 29, 2025

		/// The header is boxed as it has a large stack size.
		block_header: Box<BlockHeader>,

		/// - Another transaction in the batch produces the unauthenticated input note, in which
		/// case inclusion in the chain must not be proven.

Migrate batch kernel parts from miden-node #1112

Are you sure you want to change the base?

Migrate batch kernel parts from miden-node #1112

Conversation

PhilippGackstatter commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter commented Jan 28, 2025

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PhilippGackstatter commented Jan 28, 2025 •

edited

Loading