Updating the encoding of the Issuance Authorization keys and signatures#182
Updating the encoding of the Issuance Authorization keys and signatures#182ConstanceBeguier merged 49 commits intozsa1from
Conversation
…ad of schnorr::Signature
…norr::VerifyingKey
…le to separate out the issuance authorization material
…rently mostly hardcoded to ZSASchnorr though)
…me ID enum, updating asset base derivation and test vectors
Co-authored-by: Constance Beguier <constance@qed-it.com>
|
I am surprised that only asset_base.rs was modified in the test_vectors folder. |
ConstanceBeguier
left a comment
There was a problem hiding this comment.
Approved pending the following comments
- Please add a description to the PR
- Check which files into test_vectors folder should be updated
- Check with Pablo whether the current trait architecture is acceptable (I was not able to simplify it)
There was a problem hiding this comment.
Let's remove
fn try_sign_with_aux(
isk: &Self::IskType,
msg: &[u8; 32],
aux_data: &[u8; 32],
) -> Result<Self::IssueAuthSigType, Error>;
/// Verifies a signature over a message and auxiliary data using the issuance validating key.
fn verify_with_aux(
ik: &Self::IkType,
msg: &[u8],
aux_data: &[u8; 32],
signature: &Self::IssueAuthSigType,
) -> Result<(), Error>;from the trait. I don't see a reason to keep it since we are providing this functions directly from the keys. this will simplify more unless I miss somthing.
+ fix the aux_random misunderstanding.
In the trait, I replaced try_with_with_aux and verify_with_aux with try_sign and verify. |
src/issuance_auth.rs
Outdated
| fn to_bytes(&self) -> Vec<u8> { | ||
| self.0.to_bytes().to_vec() | ||
| } | ||
|
|
||
| pub(crate) fn from_bytes(bytes: &[u8]) -> Option<Self> { | ||
| schnorr::Signature::try_from(bytes).ok().map(Self) | ||
| } |
There was a problem hiding this comment.
I think we should remove to_bytes() and from_bytes() and keep just encode() and decode() (potentialy encode_bytes() and decode_bytes()).
I don't want two options to go to bytes. What do you think?
(x3)
There was a problem hiding this comment.
I agree with you that we will have some confusion if we keep both the to/from_bytes and encode/decode functions.
For IssueAuthSig and IssueValidatingKey, I kept only the encode/decode functions because in the specs, ik and sig are always used with the ALGORITHM_BYTE.
For IssueAuthKey, I kept only the to/from_bytes functions because it is never used with ALGORITHM_BYTE in the spec.
There was a problem hiding this comment.
We have to update orchard_zsa_issuance_auth_sig.rs test vector file to add ALGORITHM_BYTE 0x00 at the beginning of ik and sig.
There was a problem hiding this comment.
Updated orchard_zsa_issuance_auth_sig test vectors
| } | ||
|
|
||
| fn verify(ik: &Self::IkType, msg: &[u8], sig: &Self::IssueAuthSigType) -> Result<(), Error> { | ||
| ik.verify_prehash(msg, sig) |
There was a problem hiding this comment.
We are using verify_prehash(), is this the best fit?
The lib has also fn verify_digest(() and fn verify()
If we stick to verify_prehash, our API +Docs should reflect it: limit the parameters to the correct size/type
There was a problem hiding this comment.
I will keep verify_prehash because we will verify a SIGHASH that is already a finalized digest.
verify_digest does not fit because it takes as input a digest that is not finalized
verify does not fit because it hashes the message before verifying it
There was a problem hiding this comment.
I will fix the size of the msg to the size of SIGHASH (32 bytes)
This is a companion PR to QED-it/orchard#182. It makes the changes to the specification made in zcash/zips#1042, namely the addition of a 0x00 byte to the start of the issuance validating key and the issuance authorization signature.
This PR makes the updates to the encoding of the issuance validating key and the issuance authorization signature, as done in the specification in zcash/zips#1042, along with the further updates in zcash/zips#1048 and zcash/zips#1053.
The test vectors are updated in QED-it/zcash-test-vectors#31.