Bound Zcash batch signing resources - #2259
Conversation
There was a problem hiding this comment.
NACK 1f01865
As discussed in dm, the more appropriate change would be to refactor away from a PCZT-count limit, and instead use an action-count limit for batch sizes. This would not be urgent, though, because an action limit can be implemented easily on the client side.
On ZODL, we are limiting our batches to contain no more than 96 actions (regardless of tx composition), and we have not seen any OOM issues yet. This can be implemented by any wallet today, without a Keystone firmware change.
There was a problem hiding this comment.
I'm still not sure this is the correct contract to enforce. Let's agree on the contract first, and then my comments about how it's enforced may or may not go away.
The only thing I can say for certain, is that we have never seen a failure when constructing batches of shielded-only transactions with <= 96 actions. We sent at most 32 PCZTs (3 actions each). For now, lets back out a max batch size from that, and use that as our bound. Anything else is an over-constraint.
So, MAX_BATCH_SIZE would simply be:
96 x MAX_ACTION_SIZE # need to figure out the size from the actions we sent
+ 32 x PER_TX_OVERHEAD # need to figure this out too
And then the only constraint Keystone has to enforce, is that the max resolved size of the batch doesn't exceed that number. So throw an error if:
batch.bytes().len() # actual wire size
+ batch.actions().len() x MAX_ACTION_SIZE # action overhead
+ batch.transactions().len() x PER_TX_OVERHEAD # tx encoding overhead
>= MAX_BATCH_SIZE
(pardon my pseudo code)
This is the contract the client should respect when constructing a batch. We should do away with max wire size, PCZT count checks, and all the other growth checking, IMO.
|
Am testing this on ForgeBox before pinging for next review (feel free to add feedback as desired though) |
nullcopy
left a comment
There was a problem hiding this comment.
I like the structure of this a lot more! I think you're missing the actual max size check though
Co-authored-by: John <john@coldnoise.net>
The applied review suggestion renamed only the declaration, leaving the checked_add chain in estimate_zcash_batch_resolved_bytes referencing the old per_pczt_overhead name, which failed to compile (E0425).
Compact Zcash batch PCZTs expand when firmware restores redacted Orchard fields, while signed-response construction also retains one spend authorization signature per action. Oversized batches could therefore reach memory-intensive processing without a single enforceable resource contract.
This replaces the independent wire-size and PCZT-count limits with one aggregate pre-resolution budget: the request ID, canonical batch serialization using PCZT encoding v2, the exact 8-byte standalone header per PCZT, and worst-case 645-byte growth per Orchard action must fit the existing 512 KiB internal ceiling. The encoded envelope is bounded before PCZT parsing, and the aggregate estimate is checked before field resolution.
A separate ceiling of 96 Orchard actions across the Orchard and Ironwood value pools remains for response construction because 128-action hardware tests reached an SRAM allocation failure while 96-action batches have not. The 512 KiB request budget and 96-action response cap are intentionally independent; physical-device validation of this exact revision remains outstanding.