Check batch budget in RPC#2048
Conversation
| let mut remaining_input_notes = MAX_INPUT_NOTES_PER_BATCH; | ||
| let mut remaining_output_notes = MAX_OUTPUT_NOTES_PER_BATCH; | ||
|
|
||
| for tx in batch.transactions() { |
There was a problem hiding this comment.
Instead of a subtraction loop, lets rather calculate the totals and compare them once. i.e.
let accounts = batch.transactions().len();
let input_notes = batch.transactions().map(|tx| tx.input_notes().num_notes()).sum();
...
This comment was marked as spam.
This comment was marked as spam.
Mirko-von-Leipzig
added
the
no changelog
| }) | ||
| } | ||
|
|
||
| fn validate_batch_budget(batch: &ProposedBatch) -> Result<(), Status> { |
There was a problem hiding this comment.
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.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This comment was marked as spam.
This comment was marked as spam.
3 participants
Closes #2037.\n\nThis adds a batch budget check in the RPC batch submission path, before the batch proof is rebuilt and before anything is forwarded to the validator or block producer.\n\nThe check uses the protocol batch limits for account updates, input notes, and output notes, so batches that are already over budget get rejected early.\n\nI ran git diff --check. I also tried cargo check -p miden-node-rpc --lib --offline, but the local build is still blocked in miden-core-lib because the miden-core manifest path is missing.