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();
...
giwaov
force-pushed
the
fix/rpc-batch-budget-checks
branch
from
08d54af to
5faef20
Compare
|
Updated this to calculate the batch totals up front and compare them once, instead of decrementing the remaining budget in the loop. |
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.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
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.
|
Added the missing changelog entry. The new workflow runs are showing as action_required right now rather than failing, so they may need to be approved/restarted on GitHub. |
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.