ZSA burn functionality by alexeykoren · Pull Request #28 · QED-it/orchard

alexeykoren · 2022-11-22T21:32:21Z

Added a method to add assets to burn to the Builder
bvk computation now includes the burnt assets
Added Tests for bsk/bvk consistency for burning
Added E2E tests for assets burning

* Added .circleci/config.yml

Implements the issuer keys as IssuerAuthorizingKey -> isk IssuerVerifyingKey -> ik Test vectors generated with zcash_test_vectors repo

* Added NoteType to Notes * Added NoteType to value commitment derivation

* Circleci project setup (#1) * Added .circleci/config.yml * Added NoteType to Notes * reformated file * updated `derive` for NoteType * added note_type to value commit derivation * rustfmt * updated ci config * updated ci config * updated ci config * updated derive for note_type * added test for arb note_type * added test for `native` note type * zsa-note-encryption: introduce AssetType and encode and decode it in note plaintexts * zsa-note-encryption: extend the size of compact notes to include asset_type * fixed clippy warrnings * rustfmt * zsa-note-encryption: document parsing requirement * zsa-note-encryption: revert support of ZSA compact action * zsa_value: add NoteType method is_native * zsa-note-encryption: remove dependency on changes in the other crate * zsa-note-encryption: extract memo of ZSA notes * zsa-note-encryption: tests (zcash_test_vectors 77c73492) * zsa-note-encryption: simplify roundtrip test * zsa-note-encryption: more test vectors (zcash_test_vectors c10da464) * Circleci project setup (#1) * Added .circleci/config.yml * issuer keys implementation (#5) Implements the issuer keys as IssuerAuthorizingKey -> isk IssuerVerifyingKey -> ik Test vectors generated with zcash_test_vectors repo * Added NoteType to Notes (#2) * Added NoteType to Notes * Added NoteType to value commitment derivation * zsa-note-encryption: use both native and ZSA in proptests * zsa-note-encryption: test vector commit 51398c93 * zsa-note-encryption: fix after merge Co-authored-by: Paul <3682187+PaulLaux@users.noreply.github.com> Co-authored-by: Paul <lauxpaul@protonmail.com> Co-authored-by: Aurélien Nicolas <info@nau.re> Co-authored-by: Daniel Benarroch <danielbenarroch92@gmail.com>

+ Updated test bsk_consistent_with_bvk to verify mixed note types. + Added NoteType support to the builder and the bundle. + added split_flag to SpentInfo and as input to the Circuit (currently commented out) + added conditional cv_sum calculation (currently commented out) + added padding to actions

- added IssueBundle and IssueAction - added a builder for IssueBundle - added verify_issue_bundle() for consensus verification. - unit tests.

added tests in `tests/zsa.rs`

* disabled split notes and proof check for zsa transfer

* fixes and suggestions * changed "issuer" to "issuance" as per zcash#356 (comment) * terminology fixes * updated naming

* rename 2 note_type -> asset as per zcash#356 (comment) * added a dedicated type for "IssuanceAuth" * disabled codecov github action due to bad behavior. * extracted "is_asset_desc_of_valid_size()" into asset_id.rs

* improved `verify_issue_bundle()`

PaulLaux

Good first run, added the comments. In addition:

Avoid the term burnt since it is past tense. Use burn or for_burn or to_burn.

CHANGELOG.md

PaulLaux · 2022-11-23T15:11:06Z

src/builder.rs

    recipients: Vec<RecipientInfo>,
    flags: Flags,
    anchor: Anchor,
+    assets_burnt: Vec<(AssetId, i64)>,


Vec seems inappropriate since we can have multiple burns for the same asset. I would go with:

pub struct Builder { spends: Vec<SpendInfo>, recipients: Vec<RecipientInfo>, burn: HashMap<AssetId, ValueSum>, flags: Flags, anchor: Anchor, }

Order is also important, let's have flags and anchor at the end.

I was thinking its more of a feature that you can burn different amounts separately, but I agree that use case for that will be pretty artificial

Regarding ValueSum - yeah, actually first I implemented with it and then decided to simplify

src/builder.rs

PaulLaux · 2022-11-23T15:25:49Z

src/bundle.rs

        value_balance: V,
        anchor: Anchor,
        authorization: T,
+        assets_burnt: Vec<(AssetId, i64)>,


Let's keep similar terminology and order:

/// A bundle of actions to be applied to the ledger. #[derive(Clone)] pub struct Bundle<T: Authorization, V> { /// The list of actions that make up this bundle. actions: NonEmpty<Action<T::SpendAuth>>, /// Orchard-specific transaction-level flags for this bundle. flags: Flags, /// The net value moved out of the Orchard shielded pool. /// /// This is the sum of Orchard spends minus the sum of Orchard outputs. value_balance: V, /// Assets intended for burning. burn: Vec<(AssetId, V)>, /// The root of the Orchard commitment tree that this bundle commits to. anchor: Anchor, /// The authorization for this bundle. authorization: T, }

src/value.rs

tests/zsa.rs

PaulLaux · 2022-11-23T15:43:54Z

tests/zsa.rs

            )
        });
+        assets_to_burn
+            .iter()


try into_iter() to avoid dereferance.

tests/zsa.rs

PaulLaux

This is better, we are getting there.

PaulLaux · 2022-12-01T09:18:54Z

src/builder.rs


+    /// Add an instruction to burn a given amount of a specific asset.
+    pub fn add_burn(&mut self, asset: AssetId, value: NoteValue) -> Result<(), &'static str> {
+        if asset == AssetId::native() {


if asset.is_native().into()

PaulLaux · 2022-12-01T09:30:43Z

src/builder.rs

+        let sum = (*self.burn.get(&asset).unwrap_or(&ValueSum::zero()) + value)
+            .ok_or("Orchard value operation overflowed")?;


Let's have

let cur = *self.burn.get(&asset).unwrap_or(&ValueSum::zero()); let sum = (cur + value).ok_or("Orchard ValueSum operation overflowed")?;

for clarity.

PaulLaux · 2022-12-01T12:19:42Z

src/builder.rs

-        let value_balance = pre_actions
+        let native_value_balance = pre_actions
            .iter()
+            .filter(|action| action.spend.note.asset() == AssetId::native())


asset().is_native().into()

PaulLaux · 2022-12-01T12:25:46Z

src/builder.rs

+                        .and_then(|i| {
+                            V::try_from(i).map_err(|_| Error::ValueSum(value::OverflowError))
+                        })
+                        .unwrap();


unwrap() might panic, you should check and propagate with ? or similar.

PaulLaux · 2022-12-01T12:47:36Z

src/builder.rs

+                    let v_value: V = i64::try_from(value)
+                        .map_err(Error::ValueSum)
+                        .and_then(|i| {
+                            V::try_from(i).map_err(|_| Error::ValueSum(value::OverflowError))
+                        })


extract functionality here and in result_native_value_balance into somthing like

fn into<V: TryFrom<V>>(v: ValueSum) -> Result<V, Error> { i64::try_from(v) .map_err(Error::ValueSum) .and_then(|i| V::try_from(i).map_err(Error::ValueSum(OverflowError))) }

Located as part of the ValueSum functionality.

PaulLaux · 2022-12-01T13:44:03Z

src/value.rs

                AssetId::native(),
-            ))
+            )
+            - arb_values_burnt


arb_values_to_burn

Replace burnt into to_burn across the entire PR.

PaulLaux · 2022-12-01T13:48:18Z

src/value.rs

+
+    proptest! {
+        #[test]
+        fn bsk_consistent_with_bvk_native_with_zsa_transfer_and_burning(


Let's unify everything into one test that include 4 calls

_bsk_consistent_with_bvk(&native_values, &[], &[], &[]); _bsk_consistent_with_bvk(&native_values, &[], &[], &assets_burnt); _bsk_consistent_with_bvk(&native_values, &arb_values, &neg_trapdoors, &[]); _bsk_consistent_with_bvk(&native_values, &arb_values, &neg_trapdoors, &assets_burnt);

Please rename:
_bsk_consistent_with_bvk into check_binding_signature()
arb_values into asset_values
assets_burnt into burn_values

PaulLaux · 2022-12-01T13:57:58Z

tests/zsa.rs

+        .unwrap();
    });
    assert!(result.is_err());
+


use consistent style: zsa_note1 or zsa_spend_1 not both.

PaulLaux · 2022-12-01T13:58:32Z

tests/zsa.rs

+    let value_to_burn: u64 = 3;
+    let value_to_transfer: u64 = zsa_spend_1.note.value().inner() - value_to_burn;


redundant :u64 (all occurrences)

PaulLaux · 2022-12-01T14:00:44Z

tests/zsa.rs

+    )
+    .unwrap();
+
+    // 10. Burn a part of ZSA assets


Burn a partial amount of the ZSA assets

# Conflicts: # .circleci/config.yml # .github/workflows/bench.yml # .github/workflows/ci.yml # .github/workflows/lints-stable.yml # Cargo.toml # README.md # benches/note_decryption.rs # rust-toolchain # src/builder.rs # src/bundle.rs # src/note_encryption.rs # src/value.rs # tests/zsa.rs

PaulLaux · 2022-12-14T10:10:59Z

Closed in favor of #35

PaulLaux and others added 15 commits July 18, 2022 12:03

Circleci project setup (#1)

33a205b

* Added .circleci/config.yml

issuer keys implementation (#5)

71565ee

Implements the issuer keys as IssuerAuthorizingKey -> isk IssuerVerifyingKey -> ik Test vectors generated with zcash_test_vectors repo

Added NoteType to Notes (#2)

5ae5107

* Added NoteType to Notes * Added NoteType to value commitment derivation

Issuance (#12)

b90f88d

- added IssueBundle and IssueAction - added a builder for IssueBundle - added verify_issue_bundle() for consensus verification. - unit tests.

E2E tests for issuance (#20)

afaad60

added tests in `tests/zsa.rs`

disabled split notes (#22)

081b01b

* disabled split notes and proof check for zsa transfer

Review fixes (#23)

22377f2

* fixes and suggestions * changed "issuer" to "issuance" as per zcash#356 (comment) * terminology fixes * updated naming

Review fixes2 (#24)

4f9b589

* rename 2 note_type -> asset as per zcash#356 (comment) * added a dedicated type for "IssuanceAuth" * disabled codecov github action due to bad behavior. * extracted "is_asset_desc_of_valid_size()" into asset_id.rs

verify_issue_bundle() cleanup (#25)

4e23faf

* improved `verify_issue_bundle()`

WIP

2ed26d2

Fix bvk-bsk consistency test

dfa9a53

E2E test draft

e1d1dd7

PR preview

99789fd

alexeykoren requested a review from PaulLaux November 22, 2022 21:32

alexeykoren added 4 commits November 23, 2022 10:48

Rust toolchain 1.61.0

675c1da

Rust toolchain 1.61.0 EVERYWHERE

fef34e8

Not allow to burn native assets

7b5c32d

Fix burnt assets type

d282307

PaulLaux reviewed Nov 23, 2022

View reviewed changes

alexeykoren added 4 commits November 23, 2022 21:45

PR comments part 1

880ec64

TODO added

bb4bbba

PR fixes

cef9963

BVK building code deduplication

12e09da

PaulLaux reviewed Dec 1, 2022

View reviewed changes

alexeykoren added 2 commits December 2, 2022 18:40

PR fixes 2

c64af45

Generic addition for value sum

392478c

PaulLaux force-pushed the zsa1 branch 2 times, most recently from 5ec6aee to 9405f80 Compare December 6, 2022 19:04

alexeykoren added 3 commits December 7, 2022 11:22

Clippy

e91596b

Unify Rust version

e3deb03

alexeykoren closed this Dec 7, 2022

		let sum = (*self.burn.get(&asset).unwrap_or(&ValueSum::zero()) + value)
		.ok_or("Orchard value operation overflowed")?;

		let value_to_burn: u64 = 3;
		let value_to_transfer: u64 = zsa_spend_1.note.value().inner() - value_to_burn;

Conversation

alexeykoren commented Nov 22, 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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

PaulLaux commented Dec 14, 2022

Uh oh!

Reviewers

Assignees

Labels

alexeykoren commented Nov 22, 2022 •

edited by PaulLaux

Loading