Issuance tests and Issue commitments. by alexeykoren · Pull Request #17 · QED-it/orchard

alexeykoren · 2022-09-19T09:38:03Z

Unit and property tests for issue bundle
Commitments for issuance.

PaulLaux

Great first PR!
Added some comments and additions that we will need.

PaulLaux · 2022-09-20T07:15:23Z

src/bundle/commitments.rs

 }
+
+/// Construct the commitment for the issue bundle
+/// TODO - investigate if we need different personalizations


yes, lets go with

const ZCASH_ORCHARD_ZSA_ISSUE_PERSONALIZATION: &[u8; 16] = b"ZTxIdOrcZSAIssue";

PaulLaux · 2022-09-20T07:16:07Z

src/bundle/commitments.rs

@@ -1,8 +1,10 @@
 //! Utility functions for computing bundle commitments

+use bitvec::macros::internal::funty::Fundamental;


This line doe not seem to be related to anything.

It was needed for "bool.as_u8()" call, but probably importing it is a bit overkill for such a small conversion so I changed it to a simpler way

PaulLaux · 2022-09-20T07:33:45Z

src/bundle/commitments.rs

+    }
+    h.update(&bundle.ik().to_bytes());
+    h.finalize()
+}


While at it, please add

pub(crate) fn hash_issue_bundle_auth_data(bundle: &IssueBundle<Signed>) -> Blake2bHash { let mut h = hasher(ZCASH_ORCHARD_ZSA_ISSUE_SIG_PERSONALIZATION); h.update(bundle.authorization().signature().to_bytes()); h.finalize() } const ZCASH_ORCHARD_ZSA_ISSUE_SIG_PERSONALIZATION: &[u8; 16] = b"ZTxAuthZSAOrHash";

to comply with zip-244 similar to hash_bundle_auth_data().

Done, using "&<[u8; 64]>::from..." as they do in 'hash_bundle_auth_data'

PaulLaux · 2022-09-20T07:41:15Z

src/issuance.rs

+/// change if the effects of the bundle are altered.
+#[derive(Debug)]
+pub struct IssueBundleCommitment(pub Blake2bHash);
+


please add

/// A commitment to the authorizing data within a bundle of actions. #[derive(Debug)] pub struct IssueBundleAuthorizingCommitment(pub Blake2bHash);

impl IssueBundle<Signed> { pub fn authorizing_commitment(&self) -> IssueBundleAuthorizingCommitment { BundleAuthorizingCommitment(hash_issue_bundle_auth_data(self)) } }

PaulLaux · 2022-09-20T07:42:28Z

src/issuance.rs

 ) -> Result<&'a mut HashSet<NoteType>, Error> {
    if let Err(e) = bundle.ik.verify(&sighash, &bundle.authorization.signature) {
-        return Err(IssueBundleInvalidSignature(e));
+        return Err(Error::IssueBundleInvalidSignature(e));


It's better to import then Error::.

100% agree, but before in most cases it was implemented with Error:: so I decided to change to a single standard. Changing all to import now

PaulLaux · 2022-09-20T07:49:51Z

src/issuance.rs

+            )
+            .unwrap();
+
+        let sighash: [u8; 32] = <[u8; 32]>::try_from(bundle.commitment().0.as_bytes()).unwrap();


should add somthing like

impl From<IssueBundleCommitment> for [u8; 32] { fn from(commitment: BundleCommitment) -> Self { // The commitment uses BLAKE2b-256. commitment.0.as_bytes().try_into().unwrap() } } let sighash: [u8 ;32] = bundle.commitment().into();

to avoid the custom conversion. We will need it anyway.

PaulLaux · 2022-09-20T07:53:46Z

src/issuance.rs

+    prop_compose! {
+        /// Generate a uniformly distributed RedDSA issuer authorizing key.
+        pub fn arb_issuer_authorizing_key()(rng_seed in prop::array::uniform32(prop::num::u8::ANY)) -> IssuerAuthorizingKey {
+            let mut rng = StdRng::from_seed(rng_seed);
+            IssuerAuthorizingKey::from(&SpendingKey::random(&mut rng))
+        }
+    }
+
+    prop_compose! {
+        /// Generate a uniformly distributed RedDSA issuer validating key.
+        pub fn arb_issuer_validating_key()(isk in arb_issuer_authorizing_key()) -> IssuerValidatingKey {
+            IssuerValidatingKey::from(&isk)
+        }
+    }


let's move those to keys.rs

PaulLaux · 2022-09-20T07:59:02Z

src/issuance.rs

+    pub fn arb_issue_action_n(n_actions: usize) -> impl Strategy<Value = IssueAction> {
+        let value_gen = Strategy::boxed(arb_note_value_bounded(MAX_NOTE_VALUE / n_actions as u64));
+        value_gen.prop_flat_map(arb_issue_action)
+    }


Nice execution but we do not need to be constrained by MAX_NOTE_VALUE / n_actions. We just need to have valid notes. Please remove and simplify since it might confuse.

PaulLaux · 2022-09-20T07:59:47Z

src/issuance.rs

+    prop_compose! {
+        /// Generate an arbitrary issue bundle with fake authorization data. This bundle does not
+        /// necessarily respect consensus rules; for that use
+        /// TODO [`crate::builder::testing::arb_issue_bundle`]


why the TODO? x3

Repeating the same design they had - current proptests create invalid random data structures, while realistic ones are part of e2e and builder.rs. Since we have separate PRs for units/props and e2e - I decided to work on realistic data structures in e2e context

got it, but lets remove the TODO, I feel that this is quite complete.

src/bundle/commitments.rs

PaulLaux

looks good, please resolve #17 (comment) and merge.

alexeykoren added 2 commits September 13, 2022 13:47

Add unit tests for issuance bundle

74f3b8c

Add tests for issuance bundle

6824b98

alexeykoren requested a review from PaulLaux September 19, 2022 09:38

Fix linter warning

529b69d

PaulLaux requested changes Sep 20, 2022

View reviewed changes

PaulLaux changed the title ~~Issuance tests~~ Issuance tests and Issue commitments. Sep 20, 2022

PaulLaux reviewed Sep 20, 2022

View reviewed changes

src/bundle/commitments.rs Outdated Show resolved Hide resolved

alexeykoren added 2 commits September 20, 2022 14:24

Small fixes and improvements

cb6251c

Fix fmt

77bcac4

PaulLaux approved these changes Sep 21, 2022

View reviewed changes

Remove TODOs

a623485

alexeykoren merged commit 46085d7 into zsa1-issue2 Sep 21, 2022

		@@ -1,8 +1,10 @@
		//! Utility functions for computing bundle commitments

		use bitvec::macros::internal::funty::Fundamental;

Conversation

alexeykoren commented Sep 19, 2022 • edited by PaulLaux Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PaulLaux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PaulLaux left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexeykoren commented Sep 19, 2022 •

edited by PaulLaux

Loading