Skip to content

Bound Zcash batch signing resources - #2259

Open
czarcas7ic wants to merge 10 commits into
KeystoneHQ:masterfrom
valargroup:adam/zcash-batch-resource-limits
Open

Bound Zcash batch signing resources#2259
czarcas7ic wants to merge 10 commits into
KeystoneHQ:masterfrom
valargroup:adam/zcash-batch-resource-limits

Conversation

@czarcas7ic

@czarcas7ic czarcas7ic commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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.

@nullcopy nullcopy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@czarcas7ic czarcas7ic changed the title Bound Zcash batch signing resources Limit Zcash batch signing by wire size and action count Aug 5, 2026

@nullcopy nullcopy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread docs/protocols/ur_registrys/zcash.md Outdated
Comment thread docs/protocols/ur_registrys/zcash.md Outdated
Comment thread rust/apps/zcash/src/lib.rs Outdated
Comment thread rust/rust_c/src/zcash/mod.rs Outdated
Comment thread rust/rust_c/src/zcash/mod.rs Outdated
Comment thread rust/rust_c/src/zcash/mod.rs Outdated
Comment thread rust/apps/zcash/src/lib.rs Outdated
Comment thread rust/rust_c/src/zcash/mod.rs Outdated
Comment thread rust/rust_c/src/zcash/mod.rs Outdated
@czarcas7ic czarcas7ic changed the title Limit Zcash batch signing by wire size and action count Bound Zcash batch signing resources Aug 6, 2026
@czarcas7ic

Copy link
Copy Markdown
Contributor Author

Am testing this on ForgeBox before pinging for next review (feel free to add feedback as desired though)

@nullcopy nullcopy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like the structure of this a lot more! I think you're missing the actual max size check though

Comment thread rust/rust_c/src/zcash/mod.rs Outdated
Comment thread rust/rust_c/src/zcash/mod.rs
czarcas7ic and others added 2 commits August 6, 2026 21:25
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).
@czarcas7ic
czarcas7ic marked this pull request as ready for review August 7, 2026 04:56

@nullcopy nullcopy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

utACK c2196b2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants