Update to upstream (zcash/main v0.8.0)

[dmidem]

This PR merges the latest updates from the zcash/orchard repository (version 0.8.0) into zsa1 branch. These updates include refactorings and enhancements which are detailed below. The integration has been completed in the zsa1-with-zcash-0.8.0 branch, where conflicts were resolved and all unit tests and CI checks pass successfully.

Key Changes from zcash/orchard v0.8.0

1. BundleType enum introduced: Replaces direct use of bundle flags.
2. Recipient renamed to Output: Updates type and function names accordingly.
3. New type Rho: Used for variables with rho value, keeping Nullifier for nullifiers.
4. External crate zip32 added and used: Contains parts of local zip32 module functionality and new features, while local zsa32 retains orchard-specific features now.
5. Refactoring of Builder::build function: Logic moved to new bundle function, introducing BundleMetadata to track indices pre-shuffle.

[PaulLaux]

Good overall, some more polish for clarity is required

[PaulLaux]

yes

[dmidem]

Got it, removed the FIXME comment.

And there're similar questions for bundle.rs: does zsa_enabled need to be false for SPENDS_DISABLED and OUTPUTS_DISABLED?

builder.rs:106:

    /// The flag set with spends disabled.
    pub const SPENDS_DISABLED: Flags = Flags {
        spends_enabled: false,
        outputs_enabled: true,
        zsa_enabled: false, // FIXME: is this correct?
    };

    /// The flag set with outputs disabled.
    pub const OUTPUTS_DISABLED: Flags = Flags {
        spends_enabled: true,
        outputs_enabled: false,
        zsa_enabled: false, // FIXME: is this correct?
    };

[PaulLaux]

yes, both are correct, the first is used for mining zec, the second is not used.

[dmidem]

Got it, thanks. Fixed (removed FIXME comments).

[PaulLaux]

why usize:MAX?

[PaulLaux]

see below

[dmidem]

Dummy spends/outputs should not participate in BundleMetadata indexing. To manage this, we need a way to mark them as a dummy in the indexed vectors of spends/outputs, so that in further code of bundle functions the following assignments will be skipped:

bundle_meta.spend_indices[spend_idx] = action_idx;
bundle_meta.output_indices[out_idx] = action_idx;

In the previous version of the code for this PR I simply used usize::MAX instead of the indices for marking dummy spends/outputs, so the checks:

if spend_idx < num_requested_spends {
if out_idx < num_requested_outputs {

skipped such items.

I agree that it was confusing, so now, I use the Option type to mark dummy items (for dummy items the index is None, for real ones - Some(...))

[PaulLaux]

nice, lets add comment

if hm.is_empty() {
        let dummy_spend = pad_spend(None, AssetBase::native(), rng);
        // dummy_spend should not be included in the indexing and marked as None.
        hm.insert(
            dummy_spend.note.asset(),
            (vec![(None, dummy_spend)], vec![]),
        );

[dmidem]

Fixed (added the comment).

[PaulLaux]

why Copy + Into ?

[PaulLaux]

previously we worked hard to avoid it + see comment below

[dmidem]

The Copy + Into<i64> constraint was added to the build function's signature in PR #35 of the QED-it/orchard repository.

The reason for this, as I understand it, is that in the Zcash repository, the bvk value was calculated separately within the build function instead of using the bundle.binding_validating_key() method (this method requires the Copy + Into<i64> constraint).

PR #35 added the burn functionality, making the separate calculation of bvk in build more complicated. To avoid duplicating code, the calculation was switched to use bundle.binding_validating_key(), which required the Copy + Into<i64> constraint.

Now, to fix this and remove the Copy + Into<i64> constraint while still using bundle.binding_validating_key() (and avoiding code duplication), I suggest calculating native_value_balance as an i64 first, then converting it to V, which doesn't require Into<i64>. Also, to reuse the binding_validating_key logic for bvk, we could modify it by adding a new free function called derive_bvk in the bundle.rs module. This function would take three parameters: an actions iterator, a generic value_balance, and a burn iterator. Using iterators would avoid extra memory usage.

We can call derive_bvk from binding_validating_key, and have bundle call derive_bvk instead of directly using binding_validating_key.

I've implemented this solution in the latest version of the code for this PR.

[PaulLaux]

I don't like the potential duplicated logic, can we use pad_spend() for this?

[PaulLaux]

instead SpendInfo::dummy(AssetBase::native(), rng);

[dmidem]

Fixed it in this way:

let dummy_spend = pad_spend(None, AssetBase::native(), rng);

[PaulLaux]

usize::MAX is unintuitive, use named var / constant (everywhere)
reading the code I need to understand at this point that this is the spend index but currently it is unclear
also unclear how multiple spends can have the same index (usize:max)

[dmidem]

Fixed (now it uses the Option type to mark dummy spends/outputs to skip them when populating BundleMetadata, see the comment above).

[PaulLaux]

one more refactor is required for clarity.
Also, see 2 answers for previous questions.

[PaulLaux]

let's add a named type to explain the logic.
let's switch location between the content and the index SpendInfo <-> MetadataIdx

/// The index of the attached spend or output in the bundle.
/// None indicates a dummy note.
/// The index is used to track the position of the note in the bundle.
type MetadataIdx = Option<usize>;

/// Partition a list of spends and recipients by note types.
/// Method creates single dummy ZEC note if spends and recipients are both empty.
#[allow(clippy::type_complexity)]
fn partition_by_asset(
    spends: &[SpendInfo],
    outputs: &[OutputInfo],
    rng: &mut impl RngCore,
) -> HashMap<
    AssetBase,
    (
        Vec<(SpendInfo, MetadataIdx)>,
        Vec<(OutputInfo, MetadataIdx)>,
    ),
> {

[dmidem]

Done.