Implement bundle burn validation and serialization (new) by dmidem · Pull Request #38 · QED-it/librustzcash

dmidem · 2023-06-26T09:08:30Z

This pull request introduces handling of the 'burn' field of the bundle. The changes are split across two new modules: burn_validation and burn_serialization.

Burn Validation
The burn_validation module ensures the correctness of burn operations. Specifically, it validates that each asset to be burned is unique, non-native, and has a non-zero amount.

Burn Serialization
The burn_serialization module is responsible for reading and writing assets and burns from/to a stream.

Additionally, I have made modifications to the arb_amount function in zcash_primitives/src/transaction/components/amount.rs to generate amounts from the 1..MAX_MONEY range instead of the -MAX_MONEY..MAX_MONEY range (to pass new validation checks for burn items, whch require amounts to be positive).

Furthermore, I have updated the test vectors for zip_0244 to accommodate the changes in the transaction structure. I have included an empty burn field in the test vectors to ensure alignment with the updated transaction format.

…array)

PaulLaux

Added some comments, plus we need a meaningful test vector in the python tests. QED-it/zcash-test-vectors#10 (comment)

PaulLaux · 2023-06-29T09:03:06Z


        writer.write_all(&[bundle.flags().to_byte()])?;
        writer.write_all(&bundle.value_balance().to_i64_le_bytes())?;
+        write_bundle_burn(&mut writer, bundle.burn())?;


Let's inline this function, similar to two lines below.

PaulLaux · 2023-06-29T09:11:09Z

+        // FIXME: check for negative amounts?
+        if i64::from(amount) == 0 {
+            return Err(BurnError::ZeroAmount);
+        }


Yes, and change the error to NonPossitiveAmount

Done. Additionally, I have to modify arb_amount function to generate amount values from tests from range 1..MAX_MONEY instead of -MAX_MONEY..MAX_MONEY, i.e. not generate negative or zero amount values.

PaulLaux · 2023-06-29T09:12:50Z

+    }
+}
+
+/// Validates burns for a bundle by ensuring each asset is unique, non-native, and has a non-zero value.


Validates the burn values for a bundle by ensuring each asset is unique, non-native, and has a possitive value.

PaulLaux · 2023-06-29T09:14:46Z

+/// Each burn element is represented as a tuple of `AssetBase` and `Amount`, where `AssetBase` identifies
+/// the asset to be burned and `Amount` is the quantity to burn.


replace with

For burn, each asset is represented as a tuple of `AssetBase` and `Amount`.

PaulLaux · 2023-06-29T09:16:25Z

+///
+/// # Arguments
+///
+/// * `burns` - A vector of burns, where each burn is represented as a tuple of `AssetBase` and `Amount`.


burns is not a good word.

* `burn` - A vector of assets, where each asset is represented as a tuple of `AssetBase` and `Amount`.

PaulLaux · 2023-06-29T09:19:03Z

+    use super::super::burn_validation::BurnError;
+
+    #[test]
+    fn test_read_write_bundle_burn_success() {


remove test_ from all tests

PaulLaux · 2023-06-29T09:21:11Z

+
+    AssetBase::derive(&IssuanceValidatingKey::from(&isk), asset_desc)
+}
+


Insert new work at the bottom of the file, not at the top. (the entire new function)

PaulLaux · 2023-06-29T09:28:19Z

 #[cfg(feature = "zfuture")]
 use super::components::tze;

+pub fn create_test_asset(asset_desc: &str) -> orchard::note::AssetBase {


this function will be better utilized if returns the burn tupple:

get_burn_tupple(asset_desc: &str, amount u64) -> (AssetBase, Amount) {

please add a comment that the function is deterministic by using a static sk_iss.

PaulLaux · 2023-06-29T09:37:17Z

+
+use super::Amount;
+
+// FIXME: Consider making tuple (AssetBase, Amount) a new type.


Think a tuple is simple enough for this.

PaulLaux · 2023-06-29T09:37:58Z

+    // This test implementation covers only one failure case intentionally,
+    // as the other cases are already covered in the validate_bundle_burn tests.


Either describe this test or not at all.
No need to discuss other tests.

Done - removed this comment.

- Fix comments. - Rename `create_test_asset` to `get_burn_tuple` and move it to the end of the file. - Inline `write_bundle_burn`. - Remove `test_` prefix for test functions.

…vity instead of non-zero

…range 1..MAX_MONEY instead of -MAX_MONEY..MAX_MONEY, i.e. not generate negative or zero amount values

dmidem added 4 commits June 26, 2023 10:43

Implement bundle burn validation and serialization

435ce73

Fix clippy errors

301f6de

Fix test vectors for zip_0244 (add empty burn field - i.e. zero-size …

c791bea

…array)

Use IssuanceKey instead of SpendingKey to create assets for tests

c602928

dmidem requested a review from PaulLaux June 26, 2023 09:08

dmidem mentioned this pull request Jun 26, 2023

Implement bundle burn validation and serialization #30

Closed

Fix clippy error for tests

59f024b

PaulLaux reviewed Jun 29, 2023

View reviewed changes

QED-it deleted a comment from what-the-diff Bot Jun 29, 2023

dmidem added 6 commits June 30, 2023 12:29

Update zip_0244 test vectors to add cases of non-empty burn vector

4c048cc

Update the code according to PR #38 review:

0790863

- Fix comments. - Rename `create_test_asset` to `get_burn_tuple` and move it to the end of the file. - Inline `write_bundle_burn`. - Remove `test_` prefix for test functions.

Add updated orchard.rs file (missed in previous commit)

d19d5a5

Modify burn validation routines to check burn item amounts for positi…

6218ddb

…vity instead of non-zero

Update test vector for zsa_0244

4ac1aab

Modify arb_amount function to generate amount values from tests from …

10aa57d

…range 1..MAX_MONEY instead of -MAX_MONEY..MAX_MONEY, i.e. not generate negative or zero amount values

PaulLaux closed this Jul 1, 2024

		/// Each burn element is represented as a tuple of `AssetBase` and `Amount`, where `AssetBase` identifies
		/// the asset to be burned and `Amount` is the quantity to burn.


		AssetBase::derive(&IssuanceValidatingKey::from(&isk), asset_desc)
		}


		use super::Amount;

		// FIXME: Consider making tuple (AssetBase, Amount) a new type.

		// This test implementation covers only one failure case intentionally,
		// as the other cases are already covered in the validate_bundle_burn tests.

Conversation

dmidem commented Jun 26, 2023 • edited 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!

dmidem Jul 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dmidem commented Jun 26, 2023 •

edited

Loading

dmidem Jul 4, 2023 •

edited

Loading