Migrate batch kernel parts from miden-node #1112
Conversation
PhilippGackstatter
force-pushed
the
pgackst-batch-prover-crate
branch
from
8eeced0 to
921c6d9
Compare
| /// 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<()> { |
There was a problem hiding this comment.
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.
| // 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>); |
There was a problem hiding this comment.
I guess it was just a temporary thing to use a Blake3 hash for the Batch ID, so I have yet to update this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yep, makes sense.
One question about the order here: Should it be either one of those (or something else entirely?):
hash(TRANSACTION_ID || [account_id_prefix, account_id_suffix, 0, 0])hash(TRANSACTION_ID || [0, 0, account_id_suffix, account_id_prefix])
I think the second one should be more consistent, right? Because the suffix is at a lower "memory address" than the prefix, and on the stack it would be [account_id_prefix, account_id_suffix, 0, 0], which is in line with how account IDs are usually laid out on the stack.
For compute_tx_hash however, we actually use the first one (which I changed in the account ID PR), but not sure if that was actually consistent, so maybe it should be changed?
miden-base/crates/miden-objects/src/block/mod.rs
Lines 229 to 240 in fdfbd3c
I'm still confused how these layouts should be structured 😅.
|
@bobbinth I think this is ready for an initial review, but I still expect to change some things ( |
| // 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>); |
There was a problem hiding this comment.
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.
There is a test for almost every error condition checked by |
Migrates the
TransactionBatchand related types from miden-node to miden-base.Implements all the checks of a transaction batch in
ProposedBatch::new, except for verifying the transactions.A
LocalBatchProverthen turns this proposed batch into aProvenBatchby verifying all transactions in the batch.Adds a few public methods on existing types to faciliate tests.
Unrelated to this PR: Removes
BlockNumber::from_usizewhich we don't need and is dangerous.