Refactor issue/burn validation for Zebra asset state integration by dmidem · Pull Request #199 · QED-it/orchard

dmidem · 2025-11-26T06:22:47Z

This PR refactors Orchard’s issue/burn validation so Zebra can call it directly when maintaining its issued_assets state and when processing checkpointed blocks.

The changes:

extend issuance and burn error types (derives + a few new variants)
make state lookup callbacks FnMut so Zebra can keep a local cache
split verify_issue_bundle to reuse core validation as verify_trusted_issue_bundle (no sighash/signature, for checkpoints)
update validate_bundle_burn to check burns against the asset state and return updated AssetRecords, making it symmetric with verify_issue_bundle

This change refactors Orchard issuance and burn validation so Zebra can reuse it for its issued_assets global state and checkpoint handling. - Add derives and a few new error variants for issuance and burn so Zebra can surface state-related failures explicitly. - Change state lookup callbacks to FnMut to allow caching on the Zebra side. - Split verify_issue_bundle into signature checking plus a verify_trusted_issue_bundle variant for checkpointed blocks. - Make burn validation state-aware and return updated AssetRecord values, symmetric to issuance.

… clean merge

…r471

…hout_sighash in issuance

…r471

PaulLaux

Overall good. Posted comments

PaulLaux · 2026-02-11T21:47:40Z

+    verify_trusted_issue_bundle(bundle, get_global_records, first_nullifier)
 }

+// FIXME: move it and add tests ...


remove or fix

Fixed (removed the comment).

PaulLaux · 2026-02-11T21:51:28Z

+
+    /// Strips reference notes, keeping only amounts (reference notes contain
+    /// randomness and can't be compared directly).
+    fn strip_reference_notes(


let's call it remove_reference_notes()

Fixed (renamed and updated the doc comment).

PaulLaux · 2026-02-11T21:54:14Z

+    /// Builds a BTreeMap of asset states from an array of (asset_desc, supply) tuples.
+    ///


Provide more context here. What is state in the context of orchard?

Let's call it build_tree() and avoid the term state and the discussion around it as much as possible for orchard.

Fixed. As we discussed, I renamed the build_state function to mock_issuance_records. I also replaced the remaining mentions of “state” in comments with “global issuance state”, following ZIP 0227’s terminology (Global Issuance State section).

PaulLaux · 2026-02-11T21:55:01Z

+    /// # Returns
+    ///
+    /// A `BTreeMap<AssetBase, AssetRecord>` containing the asset records.
+    fn build_state(data: &[(&[u8], u64)]) -> BTreeMap<AssetBase, AssetRecord> {


this untyped style data: &[(&[u8], u64)] does not work for Orchard. Please create a properly typed function signature.

Fixed - replaced the tuple with a new struct:

struct AssetSupply { asset: AssetBase, supply: u64, }

PaulLaux · 2026-02-11T22:00:42Z

                write!(f, "Cannot burn an asset with a zero value.")
            }
+            BurnError::AssetNotFoundInState => write!(f, "Asset not found in state."),
+            BurnError::InsufficientSupply => write!(f, "Insufficient supply for burn"),


This one does not end with period . but all the others does. Please check Orchard and align the style (I think we dont need . at the end)

Fixed (removed trailing periods in the fmt::Display strings).

I checked Orchard’s style and it’s mixed, but doc comments on error enum variants usually include periods (for example BuildError, SpendError, and OutputError in [builder.rs](https://github.com/zcash/orchard/blob/main/src/builder.rs)). The fmt::Display messages are more inconsistent and use trailing periods less often. So I removed trailing periods from the error strings, and kept periods in the error enum variant doc comments.

PaulLaux · 2026-02-11T22:01:42Z

+    /// Asset not found in state
+    AssetNotFoundInState,
+    /// Insufficient supply for burn
+    InsufficientSupply,


align . usage in docstrings as in Orchard

Fixed (added the missing trailing periods, see my previous comment).

PaulLaux · 2026-02-11T22:06:05Z

+            reference_note: current_record.reference_note,
+        };
+
+        if new_records.insert(asset, new_record).is_some() {


In this new version, you check if the asset is duplicated at the end, after get_current_record(&asset). Consider checking for duplicates (without insert) early and avoid expensive operations on duplicated assets.

Fixed - replaced with the following check before get_current_record(&asset):

if new_records.contains_key(&asset) { return Err(BurnError::DuplicateAsset); }

…quires MSRV 1.71

…0.115 requires MSRV 1.71" This reverts commit c8ba08c.

…s required by Zebra’s AssetStateError

…s so Zebra tests can reuse Orchard’s reference-note construction when building mock AssetRecords

…: add create_test_reference_note wrapper for Zebra tests

PaulLaux

very good

dmidem requested review from ConstanceBeguier and PaulLaux November 26, 2025 06:23

QED-it deleted a comment from what-the-diff Bot Nov 26, 2025

dmidem added 5 commits January 19, 2026 07:48

Refactor burn_validation to align with zcash_pr471_review branch for…

3401545

… clean merge

Merge branch 'zcash_pr471_review' into issue-burn-zebra-integration-p…

78fd192

…r471

Fix compilation errors from previous merge

e9a1bb4

Rename verify_trusted_issue_bundle function to check_issue_bundle_wit…

17b0b72

…hout_sighash in issuance

Merge branch 'zcash_pr471_review' into issue-burn-zebra-integration-p…

136e717

…r471

PaulLaux reviewed Feb 11, 2026

View reviewed changes

PaulLaux mentioned this pull request Feb 15, 2026

Add OrchardZSA zcash/orchard#471

Open

dmidem mentioned this pull request Feb 15, 2026

Implement ZIP-0227 global issued-assets state in Zebra QED-it/zebra#106

Closed

dmidem added 8 commits February 15, 2026 21:49

Merge branch 'zsa1' into issue-burn-zebra-integration-pr471

33dbfa3

Merge branch 'zsa1' into issue-burn-zebra-integration

e9c75ae

Pin syn dependency version in Cargo.toml as the latest one 2.0.115 re…

c8ba08c

…quires MSRV 1.71

Use ci.yml from the upstream (zsa1)

82d4df2

Revert "Pin syn dependency version in Cargo.toml as the latest one 2.…

3bd65cd

…0.115 requires MSRV 1.71" This reverts commit c8ba08c.

Fix an error in MAX_BURN_VALUE based check in burn_validation.rs

deaeeed

Restore ci.yml from zsa1

7be8a80

CI: pin syn to keep rustc 1.70 compatibility

5bc85a4

dmidem force-pushed the issue-burn-zebra-integration branch from d67d5ca to 5bc85a4 Compare February 17, 2026 10:04

dmidem added 6 commits February 18, 2026 04:19

Address PR #199 review comments

0c9d474

Merge branch 'zsa1' into issue-burn-zebra-integration

d8aa053

Make BurnError consistently Clone + PartialEq + Eq (not test-only), a…

ce29dc3

…s required by Zebra’s AssetStateError

Expose create_reference_note under temporary-zebra + test-dependencie…

33a1566

…s so Zebra tests can reuse Orchard’s reference-note construction when building mock AssetRecords

Fix feature-gated visibility for create_reference_note (which failed)…

b596ead

…: add create_test_reference_note wrapper for Zebra tests

Add docs for create_test_reference_note to satisfy missing_docs

20ac92b

PaulLaux approved these changes Feb 18, 2026

View reviewed changes

dmidem merged commit 7ec34c9 into zsa1 Feb 18, 2026
30 checks passed

dmidem deleted the issue-burn-zebra-integration branch February 18, 2026 11:17

dmidem mentioned this pull request Feb 20, 2026

Implement ZIP-0227 global issued-assets state in Zebra QED-it/zebra#111

Merged

		/// Builds a BTreeMap of asset states from an array of (asset_desc, supply) tuples.
		///

Conversation

dmidem commented Nov 26, 2025 • 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 Feb 16, 2026 • 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 Feb 17, 2026 • 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 Feb 16, 2026 • 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!

dmidem Feb 16, 2026 • 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!

PaulLaux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dmidem commented Nov 26, 2025 •

edited

Loading

dmidem Feb 16, 2026 •

edited

Loading

dmidem Feb 17, 2026 •

edited

Loading

dmidem Feb 16, 2026 •

edited

Loading

dmidem Feb 16, 2026 •

edited

Loading