Add tracking for supply info inside verify_issue_bundle by dmidem · Pull Request #55 · QED-it/orchard

dmidem · 2023-04-19T15:17:43Z

This PR includes the following changes:

Added a new error, ValueSumOverflow, that occurs if the sum value overflows when adding new supply amounts.
Created a new supply_info module containing SupplyInfo and AssetSupply structures, with add_supply function and unit tests for it.
Renamed the are_note_asset_ids_derived_correctly function to verify_supply, changed its behavior to verify and compute asset supply, added unit tests for it.
Updated the verify_issue_bundle function to use the changes mentioned above, updated its description, and added new unit tests.
Renamed errors with ...NoteType suffix in the name to ...AssetBase.
Added update_finalization_set method to SupplyInfo and use after the calls of verify_issue_bundle function (if needed), instead of mutating the finalization set inside verify_issue_bundle.

PaulLaux

Please rename NoteType to AssetBase. We did a big rename recently, missed it. There is no NoteType term anymore.

PaulLaux · 2023-04-20T14:27:27Z

src/issuance.rs

-        .try_fold(s, |newly_finalized, action| {
+    let supply_info = bundle.actions().iter().try_fold(
+        HashMap::<AssetBase, (ValueSum, bool)>::new(),
+        // Rust uses move semantics here so supply_info is not copied but moved


remove this comment

PaulLaux · 2023-04-20T14:33:19Z

src/issuance.rs

    sighash: [u8; 32],
    finalized: &mut HashSet<AssetBase>, // The finalization set.
-) -> Result<(), Error> {
+) -> Result<HashMap<AssetBase, (ValueSum, bool)>, Error> {


we need a new type for HashMap<AssetBase, (ValueSum, bool)>:

pub struct AssetSupply { /// amount: ValueSum, /// is_finilized: bool, } pub struct SupplyInfo { /// a map ... assets : HashMap<AssetBase, AssetSupply> }

PaulLaux · 2023-04-20T14:50:53Z

src/issuance.rs

+        if asset.eq(&AssetBase::derive(ik, &self.asset_desc)) {
+            Ok((asset, value_sum))
+        } else {
+            Err(IssueBundleIkMismatchNoteType)


We don't like explicit ifs. Try to stick to the

asset // check that the asset was properly derived. .eq(&AssetBase::derive(ik, &self.asset_desc)) .then(|| asset) .ok_or(IssueBundleIkMismatchNoteType),

Pattern.

Use if only if it is much shorter then the alternative.

PaulLaux · 2023-04-20T14:55:16Z

src/issuance.rs

+    /// Return the (`AssetBase`, `ValueSum`) tuple if the provided `ik` is used to derive
+    /// the `asset_id` for **all** internal notes.
+    // FIXME: consider renaming this function as it not only validates assets in notes
+    // but also calculates the sum of the note values.


rename to compute_new_supply() and document all the possible exceptions.

Due to growing complexity, this function should be tested independently for all exceptions and functionality.

PaulLaux · 2023-04-20T15:11:29Z

src/issuance.rs

+        let (asset, value_sum) = self.notes.iter().try_fold(
+            (self.notes().head.asset(), ValueSum::zero()),
+            |(asset, value_sum), &note| {
+                // Update asset value (fail if not all note types are equal
+                // or if overflow occured)
+                if note.asset().eq(&asset) {
+                    let updated_value_sum =
+                        (value_sum + i128::from(note.value())).ok_or(ValueSumOverflow)?;
+                    Ok((asset, updated_value_sum))
+                } else {
+                    Err(IssueActionIncorrectNoteType)
+                }
+            },
+        )?;


Strive for a more compact code (and comments):

let (asset, value_sum) = self.notes.iter().try_fold( (self.notes().head.asset(), ValueSum::zero()), |(asset, value_sum), &note| { // all assets should have the same asset note.asset().eq(&asset).then(|| ()).ok_or(IssueActionIncorrectNoteType)?; // the total amount should not overflow Ok((asset, (value_sum + note.value()).ok_or(ValueSumOverflow)?)) }, )?;

…upply etc.)

PaulLaux

Very nice, added some more comments.

PaulLaux · 2023-04-23T05:56:36Z

src/issuance.rs

+impl SupplyInfo {
+    /// Attempts to insert an asset's supply information into the supply info map.
+    /// Returns true on success or false if the supply for the asset was already inserted and finalized.
+    pub fn try_insert_asset_supply(&mut self, asset: AssetBase, supply: AssetSupply) -> bool {


the function should be called add_supply() and the behavior should be:

if the assetBase already exists:

add existing supply and new supply

set is_finalize if supply.is_finilized is set

no change to is_finalize if supply.is_finilized is not set

it should not fail.

PaulLaux · 2023-04-23T06:01:39Z

src/issuance.rs

+/// Represents the amount of an asset and its finalization status.
+#[derive(Debug, Clone)]
+#[cfg_attr(test, derive(PartialEq))]
+pub struct AssetSupply {
+    /// The amount of the asset.
+    amount: ValueSum,
+    /// Whether or not the asset is finalized.
+    is_finalized: bool,
+}


Our new functionality justifies a new file. Please move all data structs and supporting functions/implementations/ tests into supply_info.rs.
Leave verify_supply() (see below) and verification tests for it in this file.

PaulLaux · 2023-04-23T06:05:53Z

src/issuance.rs

+                // previously finalized or if the supply for the asset was already
+                // inserted and finalized)
+                if finalized.contains(&asset)
+                    || (!supply_info.try_insert_asset_supply(asset, supply))


as discussed above, add_supply() should not fail.

The rational is that the consensus actually finalizes an assetBase at the end of each Block, not mid-block nor mid-transaction.

PaulLaux · 2023-04-23T06:08:44Z

src/issuance.rs

+    // FIXME: Evaluate the feasibility of implementing this test. Note values are
+    // represented by the NoteValue type (u64), while the supply amount uses the
+    // ValueSum type (i128). Due to the difference in types, simulating a supply
+    // amount overflow (i.e., the sum of values of notes) is challenging.
+    #[test]
+    fn test_compute_new_supply_value_sum_overflow() {}


Right, remove the test.

PaulLaux · 2023-04-23T06:12:31Z

src/issuance.rs

+    ///
+    /// * `IssueBundleIkMismatchAssetBase`: If the provided `ik` is not used to derive the
+    ///   `asset_id` for **all** internal notes.
+    fn compute_new_supply(


let's rename it again into: verify_supply(), since it is critical to consensus verification with the extra benefit of supply compute.

PaulLaux · 2023-04-23T06:19:44Z

Also, please update the description of the PR to include only the relevant information about the changes.

… change add_supply behavior etc.)

PaulLaux

Added my comments, one is significant.

PaulLaux · 2023-04-24T16:27:21Z

src/supply_info.rs

+    pub fn add_supply(&mut self, asset: AssetBase, new_supply: AssetSupply) -> Result<(), Error> {
+        match self.assets.entry(asset) {
+            hash_map::Entry::Occupied(mut entry) => {
+                let supply = entry.get_mut();


Somthing strange here, why we need .get_mut() if entry is already mut?

In general, I am using the entry() method now to avoid calling both get and insert. There is a more elegant way to work with entry result, using and_update and or_insert, but and_update does not allow returning an error. So, for our current logic, using a match with entry is the optimal choice. get_mut does require a mutable self reference, so entry needs to be mutable. However, in this particular case, we only need to use entry once, so we can convert it into AssetSupply using the into_mut method:

hash_map::Entry::Occupied(entry) => { let supply = entry.into_mut(); supply.amount = (supply.amount + new_supply.amount).ok_or(Error::ValueSumOverflow)?; supply.is_finalized |= new_supply.is_finalized; }

I have fixed it now.

src/supply_info.rs

PaulLaux · 2023-04-24T16:32:21Z

src/supply_info.rs

+
+    #[test]
+    fn test_add_supply_valid() {
+        let mut supply_info = SupplyInfo::default();


In this project, there are many custom types. Let's avoid default() and add an explicit new() constructor which is well defined in the scope of our new code.

It's recommended by Clippy:

error: you should consider adding a `Default` implementation for `SupplyInfo` --> src/supply_info.rs:35:5 | 35 | / pub fn new() -> Self { 36 | | Self { 37 | | assets: HashMap::new(), 38 | | } 39 | | } | |_____^ | = note: `-D clippy::new-without-default` implied by `-D warnings` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default help: try adding this | 34 + impl Default for SupplyInfo { 35 + fn default() -> Self { 36 + Self::new() 37 + } 38 + } |

To avoid the warning but still use new, we can either disable the Clippy warning:

#[allow(clippy::new_without_default)] pub fn new() -> Self { Self { assets: HashMap::new(), } }

Or add a default implementation as it recommends:

impl Default for SupplyInfo { fn default() -> Self { Self::new() } }

For now, I've fixed it by using the second option (added the default implementation that refers to new). And so I use new in the rest of code instead of default.

PaulLaux · 2023-04-24T16:33:15Z

src/lib.rs

 pub mod issuance;
 pub mod keys;
 pub mod note;
+mod supply_info;


should be pub like the rest

PaulLaux · 2023-04-24T16:35:11Z

src/issuance.rs

+        let (asset, value_sum) = self.notes.iter().try_fold(
+            (self.notes().head.asset(), ValueSum::zero()),
+            |(asset, value_sum), &note| {
+                // All assets should have the same asset


// All assets should have the same assetBase

PaulLaux · 2023-04-24T16:37:26Z

src/issuance.rs

-        })?;
+        self.actions
+            .iter()
+            .try_for_each(|action| action.verify_supply(&expected_ik).map(|_| ()))?;


I'm open to suggestions on how to avoid .map(|_| ())

I see two options.

Naive suggestion: we can use the ? operator to return early on an error and then return Ok(()) if the verification succeeds:

self.actions .iter() .try_for_each(|action| { action.verify_supply(&expected_ik)?; Ok(()) })?;

Alternatively, we can rename the verify_supply function to compute_supply, as this name more accurately reflects its purpose. And here we can add a comment to make it clear the successful result is intentionally not used:

// Make sure the `expected_ik` matches the `asset` for all notes. // To do that, simulate supply computation for the sake of verification // (ignore the successful result of `compute_supply`). self.actions .iter() .try_for_each(|action| action.compute_supply(&expected_ik).map(|_| ()))?;

The second option looks better to me. And, of course, we can combine both fixes:

// Make sure the `expected_ik` matches the `asset` for all notes. // To do that, simulate supply computation for the sake of verification // (ignore the successful result of `compute_supply`). self.actions .iter() .try_for_each(|action| { action.compute_supply(&expected_ik)?; Ok(()) })?;

By the way, do we need to perform all verification checks here or only check for uniqueness of asset base?

Tnx for the 2 options, I prefer to keep the verify_supply() name. Let's go with

// Make sure the `expected_ik` matches the `asset` for all notes. self.actions .iter() .try_for_each(|action| { action.verify_supply(&expected_ik)?; Ok(()) })?;

No extra text for the comment is needed.

src/issuance.rs

…o::new instead of default etc.)

…lls of verify_issue_bundle function (if needed), instead of mutating the finalization set inside verify_issue_bundle

PaulLaux

Approved pending fixes.

Great work, after fixes make sure to copy the PR description to the git commit msg (please ask me if this is not clear)

PaulLaux · 2023-05-01T11:20:21Z

src/issuance.rs

+    /// * `ValueSumOverflow`: If the total amount value of all notes in the `IssueAction` overflows.
+    ///
+    /// * `IssueBundleIkMismatchAssetBase`: If the provided `ik` is not used to derive the
+    ///   asset base for **all** internal notes.


let stick to AssetBase (with backticks), not asset base

PaulLaux · 2023-05-01T11:41:01Z

src/issuance.rs

-        })?;
+        self.actions
+            .iter()
+            .try_for_each(|action| action.verify_supply(&expected_ik).map(|_| ()))?;


Tnx for the 2 options, I prefer to keep the verify_supply() name. Let's go with

// Make sure the `expected_ik` matches the `asset` for all notes. self.actions .iter() .try_for_each(|action| { action.verify_supply(&expected_ik)?; Ok(()) })?;

No extra text for the comment is needed.

PaulLaux · 2023-05-01T15:43:32Z

src/issuance.rs

        (rng, isk, ik, recipient, sighash)
    }

+    fn setup_verify_supply_test_params(


Move to bottom together with the 3 new tests, the basic tests should come first.

PaulLaux · 2023-05-01T15:49:10Z

src/supply_info.rs

+
+    /// Updates the set of finalized assets based on the supply information stored in
+    /// the `SupplyInfo` instance.
+    pub fn update_finalized_assets(&self, finalized_assets: &mut HashSet<AssetBase>) {


let's go with something closer to the spec:
pub fn update_finalization_set(&self, fs: &mut HashSet<AssetBase>) {

PaulLaux · 2023-05-01T15:50:04Z

src/supply_info.rs

+
+impl SupplyInfo {
+    /// Creates a new, empty `SupplyInfo` instance.
+    #[allow(clippy::new_without_default)]


This is not required.

PaulLaux · 2023-05-01T15:53:50Z

src/supply_info.rs

+            rand::{rngs::StdRng, SeedableRng},
+        };
+
+        AssetBase::from_bytes(&Point::random(StdRng::seed_from_u64(seed)).to_bytes()).unwrap()


it's better to use the derive() function similar to arb_zsa_asset_id()

PaulLaux · 2023-05-01T15:54:15Z

src/supply_info.rs

+            Some(&AssetSupply::new(ValueSum::from_raw(70), true))
+        );
+
+        // Add supply4


Add supply5

PaulLaux · 2023-05-01T15:57:00Z

src/supply_info.rs

+            Some(&AssetSupply::new(ValueSum::from_raw(70), true))
+        );
+        assert_eq!(
+            supply_info.assets.get(&asset2),
+            Some(&AssetSupply::new(ValueSum::from_raw(50), false))


This will be much more convincing if 50 and 70 are replaced with computed variables based on the input. (supply1, supply2, supply1+supply2+...)

Done (added a function in tests called sum_amounts for that).

PaulLaux · 2023-05-01T16:04:50Z

src/issuance.rs

+        asset_desc: &str,
+        note1_value: u64,
+        note2_value: u64,
+        note2_asset_desc: Option<&str>, // if None, both notes use the same asset


let's go with

note1_value: u64, note2_value: u64, note1_asset_desc: &str, note2_asset_desc: Option<&str>,

…ase::derive in tests etc.)

PaulLaux

Approved with minor fixes.

PaulLaux · 2023-05-03T16:21:13Z

src/supply_info.rs

+    fn sum_amounts<'a, Supplies: IntoIterator<Item = &'a AssetSupply>>(
+        supplies: Supplies,
+    ) -> Option<ValueSum> {
+        supplies
+            .into_iter()
+            .map(|supply| supply.amount)
+            .try_fold(ValueSum::from_raw(0), |sum, value| sum + value)


Very nice,
I would remove repeated terms to allow the code speak for itself:

fn sum<'a, T: IntoIterator<Item = &'a AssetSupply>>( supplies: T, ) -> Option<ValueSum> { supplies .into_iter() .map(|supply| supply.amount) .try_fold(ValueSum::from_raw(0), |sum, value| sum + value) }

A sum over supplies.

Done - renamed.

PaulLaux · 2023-05-03T16:22:00Z

src/supply_info.rs


        // Add supply1
-        assert!(supply_info.add_supply(asset1, supply1).is_ok());
+        assert!(supply_info.add_supply(asset1, supply1.clone()).is_ok());


Let's add copy for AssetSupply and remove .clone() everywhere.

AssetSupply contains HashSet inside, which is not copyable, so AssetSupply can't be copyable either, but we can avoid using of .clone() by pre-computing expected sum amounts at the beginning of the test and then use them in asserts, like this:

let supply1 = AssetSupply::new(ValueSum::from_raw(20), false); let supply2 = AssetSupply::new(ValueSum::from_raw(30), true); let supply3 = AssetSupply::new(ValueSum::from_raw(10), false); let supply4 = AssetSupply::new(ValueSum::from_raw(10), true); let supply5 = AssetSupply::new(ValueSum::from_raw(50), false); let supply1_amount = sum([&supply1]).unwrap(); let supply12_amount = sum([&supply1, &supply2]).unwrap(); let supply123_amount = sum([&supply1, &supply2, &supply3]).unwrap(); let supply1234_amount = sum([&supply1, &supply2, &supply3, &supply4]).unwrap(); let supply5_amount = sum([&supply5]).unwrap(); ... assert_eq!( supply_info.assets.get(&asset1), Some(&AssetSupply::new(supply1234_amount, true)) );

In this case we don't need to clone supply... variables. By the way, if so, we can even remove that sum function and pre-compute sum amounts like this:

let supply1 = AssetSupply::new(ValueSum::from_raw(20), false); let supply2 = AssetSupply::new(ValueSum::from_raw(30), true); let supply3 = AssetSupply::new(ValueSum::from_raw(10), false); let supply4 = AssetSupply::new(ValueSum::from_raw(10), true); let supply5 = AssetSupply::new(ValueSum::from_raw(50), false); let supply1_amount = supply1.amount; let supply12_amount = (supply1_amount + supply2.amount).unwrap(); let supply123_amount = (supply12_amount + supply3.amount).unwrap(); let supply1234_amount = (supply123_amount + supply4.amount).unwrap(); let supply5_amount = supply5.amount; ... assert_eq!( supply_info.assets.get(&asset1), Some(&AssetSupply::new(supply1234_amount, true)) );

What do you think?

I tried adding copy and it went smoothly:

/// Represents the amount of an asset and its finalization status. #[derive(Debug, Clone, Copy)] #[cfg_attr(test, derive(PartialEq))] pub struct AssetSupply { /// The amount of the asset. pub amount: ValueSum, /// Whether or not the asset is finalized. pub is_finalized: bool, }

Am I missing something?

Get it now - I mixed it up with SupplyInfo. So, done: I added a Copy and removed clone calls.

PaulLaux · 2023-05-03T16:24:05Z

src/issuance.rs

+        let fvk = FullViewingKey::from(&sk);
+        let recipient = fvk.address_at(0u32, Scope::External);


Generally, the recipient is different from the sender. Here it has no effect but this function might be used elsewhere later so let's fix it.

Actually, I didn't change setup_params function, I only moved it down when I re-ordered functions in tests so git shows it in red, which is confusing. I will return it to the top of tests (which is more proper place for it as it is called from setup_verify_supply_test_params).

Anyway, I can fix it too - do you mean we need to return sender from setup_params?

Yes, please generate the recipient from a fresh sk. If other tests break then abort.

Fixed it this way:

let sk = SpendingKey::random(&mut rng); let isk: IssuanceAuthorizingKey = (&sk).into(); let ik: IssuanceValidatingKey = (&isk).into(); let fvk = FullViewingKey::from(&SpendingKey::random(&mut rng)); let recipient = fvk.address_at(0u32, Scope::External);

Local tests passed, CI tests are still running now.

…etSupply, fix setup_params)

1. Added a new error, `ValueSumOverflow`, that occurs if the sum value overflows when adding new supply amounts. 2. Created a new `supply_info` module containing `SupplyInfo` and `AssetSupply` structures, with `add_supply` function and unit tests for it. 3. Renamed the `are_note_asset_ids_derived_correctly` function to `verify_supply`, changed its behavior to verify and compute asset supply, added unit tests for it. 4. Updated the `verify_issue_bundle` function to use the changes mentioned above, updated its description, and added new unit tests. 5. Renamed errors with `...NoteType` suffix in the name to `...AssetBase`. 6. Added `update_finalization_set` method to `SupplyInfo` and use after the calls of `verify_issue_bundle function` (if needed), instead of mutating the finalization set inside `verify_issue_bundle`.

Add tracking for supply info inside verify_issue_bundle

fc2f811

dmidem requested a review from PaulLaux April 19, 2023 15:18

QED-it deleted a comment from what-the-diff bot Apr 20, 2023

PaulLaux reviewed Apr 20, 2023

View reviewed changes

Resolve review remarks (fix issues, add docs, tests for compute_new_s…

dc84dbf

…upply etc.)

PaulLaux reviewed Apr 23, 2023

View reviewed changes

Resolve review remarks 2 (rename compute_new_supply to verify_supply,…

10e7aa0

… change add_supply behavior etc.)

PaulLaux reviewed Apr 24, 2023

View reviewed changes

dmidem added 2 commits April 25, 2023 11:00

Partially resolve review remarks 3 (use into_mut for entry, SupplyInf…

a9f5097

…o::new instead of default etc.)

Add update_finalized_assets method to SupplyInfo and use after the ca…

9a5c853

…lls of verify_issue_bundle function (if needed), instead of mutating the finalization set inside verify_issue_bundle

PaulLaux approved these changes May 1, 2023

View reviewed changes

dmidem added 2 commits May 2, 2023 16:23

Resolve review 4 remarks (compute supply sum from vars and use AssetB…

9659d3c

…ase::derive in tests etc.)

Rename create_random_asset to create_test_asset in supply_info

ad32b7b

PaulLaux approved these changes May 3, 2023

View reviewed changes

Resolve review 5 remarks (rename sum_amounts to sum, add Copy for Ass…

8607581

…etSupply, fix setup_params)

dmidem merged commit ea0fd59 into zsa1 May 4, 2023

dmidem deleted the verify_issue_bundle_supply_info branch August 23, 2023 12:40

PaulLaux mentioned this pull request May 14, 2025

enable webassembly support #164

Closed

		let fvk = FullViewingKey::from(&sk);
		let recipient = fvk.address_at(0u32, Scope::External);

Conversation

dmidem commented Apr 19, 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!

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 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!

PaulLaux commented Apr 23, 2023

Uh oh!

PaulLaux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmidem Apr 25, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmidem Apr 25, 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!

dmidem commented Apr 19, 2023 •

edited

Loading

dmidem Apr 25, 2023 •

edited

Loading

dmidem Apr 25, 2023 •

edited

Loading

dmidem Apr 25, 2023 •

edited

Loading

dmidem Apr 26, 2023 •

edited

Loading

dmidem May 2, 2023 •

edited

Loading