Implement Orchard signature validation consensus rules #5217
Conversation
- Currently, only RedPallas signatures are batch-validated. We can extend
this validator to cover Halo 2 proofs in the future.
- Signatures in a batch are not retried individually if the batch fails:
- For per-transaction batching (when adding to the mempool), we don't
care which signature within the transaction failed.
- For per-block batching, we currently don't care which transaction
failed. We might do so in future, at which point this behaviour can
be easily changed.
str4d
added
A-consensus
While these two are in flux, it's hard to keep these revisions consistent (e.g. zcash/zcash#5217 currently depends on two different versions of zcash_note_encryption). Using patches allows the downstream users (i.e. zcashd) to define its own set of patches, and keep everything in sync. This works fine now because we aren't actively making changes to the public APIs, only additions.
| (_, _, None) => error!("orchard_batch_add_bundle() called without txid!"), | ||
| (Some(_), None, Some(txid)) => debug!("Tx {} has no Orchard component", txid), | ||
| (None, Some(_), _) => debug!("Orchard BatchValidator not provided, assuming disabled."), | ||
| (None, None, _) => (), // Boring, don't bother logging. |
There was a problem hiding this comment.
This doesn't seem boring to me, it should never happen right?
There was a problem hiding this comment.
This case means that a disabled batch validator was given a transaction with no Orchard component. I can see that happening regularly.
There was a problem hiding this comment.
(specifically, during the earlier validation passes in ProcessNewBlock, since we only want to check signatures once in the last pass)
| #[derive(Debug)] | ||
| struct BundleSignature { | ||
| /// The signature item for validation. | ||
| signature: redpallas::batch::Item<SpendAuth, Binding>, |
There was a problem hiding this comment.
If this API were not being used in per-transaction or per-block contexts, I would be concerned about unbounded memory usage here.
There was a problem hiding this comment.
Yeah, if we end up wanting to use this in a cross-blocks context then on queue pushes we could check the size of the batch, and run the batch in-line if it's above some threshold. That would require the queue push logic to be fallible though, so it might actually be better to at that point expose the batch size to the caller so it can decide when to trigger batch validation.
|
@zkbot r+ |
|
📌 Commit 3fe8285 has been approved by |
|
⌛ Testing commit 3fe8285 with merge 17121d103616182e77c36e09c07fc51dec0775f0... |
|
💔 Test failed - pr-merge |
The orchard crate was pinning a specific rev of zcash_note_encryption which prevented CI from vendoring the crate dependencies. Now orchard uses a patch, which enables us to similarly patch here to get the correct crate versions throughout our tree (while the crates are still in flux).
|
Fixed CI build issues via zcash/orchard#117. @zkbot r+ |
|
📌 Commit 36b4d13 has been approved by |
Implemented via an
AuthValidatorclass that internally uses batch validation.Currently, only RedPallas signatures are batch-validated. We can extend
this validator to cover Halo 2 proofs in the future.
Signatures in a batch are not retried individually if the batch fails:
care which signature within the transaction failed.
failed. We might do so in future, at which point this behaviour can
be easily changed.
Closes #5194.