Skip to content

Check batch budget in RPC #2048

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
giwaov wants to merge 3 commits into
base: next
Choose a base branch
from
Open

Check batch budget in RPC #2048

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5faef20
Check batch budget in RPC
giwaov May 5, 2026
1d51974
Merge branch 'next' into fix/rpc-batch-budget-checks
Mirko-von-Leipzig May 7, 2026
148c964
Check batch budget in RPC
giwaov May 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion crates/rpc/src/server/api.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ use miden_protocol::transaction::{
TxAccountUpdate,
};
use miden_protocol::utils::serde::{Deserializable, Serializable};
use miden_protocol::{MIN_PROOF_SECURITY_LEVEL, Word};
use miden_protocol::{
MAX_ACCOUNTS_PER_BATCH,
MAX_INPUT_NOTES_PER_BATCH,
MAX_OUTPUT_NOTES_PER_BATCH,
MIN_PROOF_SECURITY_LEVEL,
Word,
};
use miden_tx::TransactionVerifier;
use miden_tx_batch_prover::LocalBatchProver;
use tonic::{IntoRequest, Request, Response, Status};
Expand Down Expand Up @@ -565,6 +571,8 @@ impl api_server::Api for RpcService {
)));
}

validate_batch_budget(&proposed_batch)?;

// Only allow deployment transactions for new network accounts.
for tx in proposed_batch.transactions() {
if tx.account_id().is_network()
Expand Down Expand Up @@ -716,6 +724,31 @@ fn strip_output_note_decorators<'a>(
})
}

fn validate_batch_budget(batch: &ProposedBatch) -> Result<(), Status> {
Copy link
Copy Markdown
Contributor

@bobbinth bobbinth May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for this PR, but I wonder if this should actually live in the protocol repo - i.e., as something like ProposedBatch::validate_budge().

At the same time - don't we already kind of have these checks implemented? I thought these limits are imposed at batch construction time.

cc @PhilippGackstatter

Copy link
Copy Markdown
Author

@giwaov giwaov May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I kept it here because this is the RPC boundary for client-submitted batches, and the goal is to return a clear invalid_argument before forwarding the batch. If protocol grows a ProposedBatch::validate_budget() helper, this should just call that instead.

I think the normal construction path already enforces the limits, but this keeps the boundary explicit for submitted batches.

Copy link
Copy Markdown
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is checked in the constructor yeah, I think the original suggestion was to check the BatchBudget we use in the mempool.

Currently the mempool's batch budget is simply the same as these checks so its all a bit redundant.

Copy link
Copy Markdown
Author

@giwaov giwaov May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that lines up with how I read the issue: keep the mempool check, but reject the same over-budget batch earlier in RPC before it gets forwarded.

I avoided pulling BatchBudget into the RPC crate because it currently lives in block-producer, so this keeps the boundary check small and local. If you’d rather have this call a shared helper instead, I can adjust it that way.

Copy link
Copy Markdown
Contributor

@bobbinth bobbinth May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, shouldn't we move the function to some centralized location so that the same function is used in both places? Or maybe we just need it in the RPC but not in the mempool? Basically, we should try to avoid code duplication so that when we update the logic we need to do it only in one place.

Either way, if we keep this, I'd add some comments explaining the intent - otherwise, it is not clear why we need it.

Copy link
Copy Markdown
Collaborator

@Mirko-von-Leipzig Mirko-von-Leipzig May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I would just leave it in the mempool until there is an actual reason to move it. As in, not merge this PR.

Its a struct in the mempool, not a function, and its technically configurable. So we could end up with mismatches which would be worse. And this will change completely with fees, or once we change how user batches are allowed.

let accounts = batch.transactions().len();
let input_notes = batch
.transactions()
.iter()
.map(|tx| tx.input_notes().num_notes() as usize)
.sum::<usize>();
let output_notes = batch
.transactions()
.iter()
.map(|tx| tx.output_notes().num_notes())
.sum::<usize>();

if accounts > MAX_ACCOUNTS_PER_BATCH
|| input_notes > MAX_INPUT_NOTES_PER_BATCH
|| output_notes > MAX_OUTPUT_NOTES_PER_BATCH
{
return Err(Status::invalid_argument(format!(
"batch exceeds batch budget: max {MAX_ACCOUNTS_PER_BATCH} account updates, {MAX_INPUT_NOTES_PER_BATCH} input notes, and {MAX_OUTPUT_NOTES_PER_BATCH} output notes"
)));
}

Ok(())
}

// LIMIT HELPERS
// ================================================================================================

Expand Down