Implement ZIP-0227 global issued-assets state in Zebra#111
Implement ZIP-0227 global issued-assets state in Zebra#111
Conversation
…n-finalized chain.
…callyVerifiedBlock types, updates `IssuedAssetsChange::from_transactions()` method return type
…heckpointVerifiedBlock
…f BE for amount, read amount after asset base)
…state, add a couple of FIXMEs
…ror in the function of the crate (this may not be a fully correct fix). Add a couple of FIXME comments explaining the problem.
…64 to prevent serialization errors and enable defining BurnItem in orchard, facilitating its reuse along with related functions
…instead of try_from')
…instead of try_from') (2)
…tead of amount (with_asset is used to process orchard burn) - this allows avoiding the use of try_into for burn in binding_verification_key function
…action::V6 and serialization only — without changing consensus rules — they will be updated automatically after merging with the upstream version of Zebra, where those ZIP-233-related changes are already implemented)
…in Transaction::V6 and serialization only — without changing consensus rules — they will be updated automatically after merging with the upstream version of Zebra, where those ZIP-233-related changes are already implemented)" This reverts commit 8e3076f.
…action::V6 and serialization only — without changing consensus rules — they will be updated automatically after merging with the upstream version of Zebra, where those ZIP-233-related changes are already implemented)
PaulLaux
left a comment
There was a problem hiding this comment.
Added comments.
In addition:
- remove (to test) "--cfg", 'feature="tx_v6"' from
.cargo/config.tomland make sure it works. zebra_statewill not compile without--cfg feature="tx_v6":-
zebra-state/src/error.rs:12, 269— importorchard_zsathat needtx_v6
-
zebra-state/src/service/non_finalized_state.rs:13same
zebra-rpchas no tx_v6 feature but uses tx_v6 gated types. Compiles only because of the global rustflag.zebra-consensus/Cargo.toml:38-41:tx_v6 = ["zebra-state/tx_v6", "zebra-chain/tx_v6"]missingzebra-test/tx_v6(again the global flag
| const ENABLE_OUTPUTS = 0b00000010; | ||
| /// Enable ZSA transaction (otherwise all notes within actions must use native asset). | ||
| // FIXME: Should we use this flag explicitly anywhere in Zebra? | ||
| const ENABLE_ZSA = 0b00000100; |
There was a problem hiding this comment.
For tx_v5: this bit must be 0. In this implementation, we allow this bit to be 1 for v5. This is not acceptable. Probably need to define an independent Flags struct for V5 and for V6. (Version-aware Flags deserialization). Alternatively, check all flags rules for V5 after deserialization.
There was a problem hiding this comment.
Fixed.
For now, I chose the "check after deserialization" approach rather than splitting into two Flags structs, combined with gating ENABLE_ZSA behind #[cfg(feature = "tx_v6")].
Detail:
First, I gated ENABLE_ZSA with #[cfg(feature = "tx_v6")] in the Flags struct of shielded_data.rs:
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
pub struct Flags: u8 {
...
/// Enable ZSA transaction (otherwise all notes within actions must use native asset).
#[cfg(feature = "tx_v6")]
const ENABLE_ZSA = 0b00000100;
}This means when tx_v6 is disabled, bitflags::from_bits() automatically rejects bit 2 as undefined — no extra check needed in that case.
Second, when tx_v6 is enabled, ENABLE_ZSA becomes a valid defined bit so from_bits() accepts it. I therefore added an explicit consensus check in the OrchardVanilla deserializer in serialize.rs, where the transaction version context is known:
impl ZcashDeserialize for Option<orchard::ShieldedData<OrchardVanilla>> {
fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
...
let flags: orchard::Flags = (&mut reader).zcash_deserialize_into()?;
// `ENABLE_ZSA` is introduced in V6 (ZIP 230) and must be zero in V5.
#[cfg(feature = "tx_v6")]
if flags.contains(orchard::Flags::ENABLE_ZSA) {
return Err(SerializationError::Parse(
"ENABLE_ZSA is not allowed in V5 transactions",
));
}
...
}
}Open questions that need separate discussion:
-
has_enough_orchard_flags()— shouldENABLE_ZSAbe included there (see transaction.rs)? A V6 transaction with only ZSA actions might have neitherENABLE_SPENDSnorENABLE_OUTPUTSset, causing this check to incorrectly reject it. Needs confirmation against ZIP 230. -
Native asset enforcement — the
ENABLE_ZSAflag doc says (see shielded_data.rs):otherwise all notes within actions must use native asset. As discussed, this is enforced by the Halo2 proof circuit. Just to double-check: should we also add an explicit semantic check in Zebra as additional defense in depth? And should theENABLE_ZSAdoc comment mention that this constraint is enforced by the proof system? -
Coinbase rule for V6 — ZIP 230 states that
enableZSAsmust be0for coinbase transactions. Similar to the previous question, this is probably verified by the proof system, but just to double-check: should it also be explicitly enforced in Zebra?
There was a problem hiding this comment.
We discussed these three questions separately and agreed on the following:
-
has_enough_orchard_flags()is unrelated toENABLE_ZSA. It only checks that Orchard actions are accompanied by at least one ofENABLE_SPENDSorENABLE_OUTPUTS, soENABLE_ZSAshould not be included there. -
The native-asset restriction for transactions without
ENABLE_ZSAis enforced by the proof verification mechanism, so we do not need an additional Zebra check for that. -
The V6 coinbase
ENABLE_ZSA == 0rule should be enforced explicitly in Zebra, so I added that consensus check incoinbase_tx_no_prevout_joinsplit_spend().
What is still missing in this PR is a unit test for the new ENABLE_ZSA == 0 coinbase rule. That is difficult to add in zsa-integration-state, because:
- we cannot use our current Orchard ZSA workflow blocks for that, as they do not contain coinbase transactions,
- we could construct a block with a coinbase transaction, but that would require
Transaction::new_v6_coinbase(), which exists only in newer upstream Zebra. Reimplementing that function here would be unnecessary duplication and would bloat this PR.
So I added the test in the synced Zebra version instead. As a result, both the synced and unsynced zsa-integration-state branches contain the new rule, but the unit test for it is present only in the synced version, in a separate PR.
| assert_eq!( | ||
| transactions.len(), | ||
| sighashes.len(), | ||
| "Bug in caller: {} transactions but {} sighashes. Caller must provide one sighash per transaction.", | ||
| transactions.len(), | ||
| sighashes.len() | ||
| ); |
There was a problem hiding this comment.
We should not panic here and crash the node. The error should propagate up.
| if let Some(issue_data) = tx.orchard_issue_data() { | ||
| // ZIP-0227 defines issued-note rho as DeriveIssuedRho(nf_{0,0}, i_action, i_note), | ||
| // so we must pass the first Action nullifier (nf_{0,0}). We rely on | ||
| // `orchard_nullifiers()` preserving Action order, so `.next()` returns nf_{0,0}. | ||
| let first_nullifier = | ||
| // FIXME: For now, the only way to convert Zebra's nullifier type to Orchard's nullifier type | ||
| // is via bytes, although they both wrap pallas::Point. Consider a more direct conversion to | ||
| // avoid this round-trip, if possible. | ||
| &Nullifier::from_bytes(&<[u8; 32]>::from( | ||
| *tx.orchard_nullifiers() | ||
| .next() | ||
| // ZIP-0227 requires an issuance bundle to contain at least one OrchardZSA Action Group. | ||
| // `ShieldedData.actions` is `AtLeastOne<...>`, so nf_{0,0} must exist. | ||
| .expect("issuance must have at least one nullifier"), | ||
| )) | ||
| .expect("Bytes can be converted to Nullifier"); |
There was a problem hiding this comment.
- Remove
FIXME: For now, the... - A crafted V6 transaction with issuance data but no orchard shielded data
reaches this code and crashes the node. Should handle this case without a crash
There was a problem hiding this comment.
enforce the "issuance requires action groups" invariant during deserialization or in semantic verification, or return AssetStateError here instead of panicking. (make sure it is handled properly at the upper layer, no crash)
There was a problem hiding this comment.
Fixed - now it returns an error instead of this panic: .expect("issuance must have at least one nullifier").
Regarding the test - as we discussed, this test would likely require constructing a proper mock V6 transaction with issue_data but no nullifiers, which is non-trivial and would bloat this module. Added a TODO comment to revisit this test case later instead.
| // FIXME: Consider writing a leading version byte here so we can change AssetState's | ||
| // on-disk format without silently mis-parsing old DB entries during upgrades (and fix | ||
| // from_bytes accordingly). | ||
| let mut bytes = Vec::new(); |
There was a problem hiding this comment.
That is a good idea in general, but for the current development stage, it is much too speculative. It can also be handled by other means, like database schema versioning. Let's remove the fixme for now.
There was a problem hiding this comment.
Fixed - removed the comment.
| ValueBalance::from_orchard_amount(orchard_value_balance) | ||
| } | ||
|
|
||
| /// Returns the value balances for this transaction using the provided transparent outputs. |
There was a problem hiding this comment.
Not our responsibility, but let’s add a fixme: substruct zip233_amount if zip233 is approved.
There was a problem hiding this comment.
Added TODO comment:
// TODO: substruct zip233_amount if zip233 is approved.
self.transparent_value_balance_from_outputs(outputs)?
+ self.sprout_value_balance()?
+ self.sapling_value_balance()
+ self.orchard_value_balance()| None, // No sighashes - uses trusted validation without signature checks | ||
| |asset_base| zebra_db.issued_asset(asset_base), | ||
| ) | ||
| .map_err(|_| BoxError::from("invalid issued assets changes"))? |
There was a problem hiding this comment.
dont drop the original error. Use format!("invalid issued assets changes: {e:?}") or similar
There was a problem hiding this comment.
Fixed, now it is:
.map_err(|e| BoxError::from(format!("invalid issued assets changes: {e:?}")))?| action.notes().iter().map(|note| { | ||
| // TODO: FIXME: Make `ExtractedNoteCommitment::inner` public in `orchard` (this would | ||
| // eliminate the need for the workaround of converting `pallas::Base` from bytes | ||
| // here), or introduce a new public method in `orchard::issuance::IssueBundle` to | ||
| // retrieve note commitments directly from `orchard`. | ||
| pallas::Base::from_repr(ExtractedNoteCommitment::from(note.commitment()).to_bytes()) | ||
| .unwrap() |
There was a problem hiding this comment.
Replace this long fixme+todo with:
// Replace this workaround with Orchard `ExtractedNoteCommitment` if changed to be exposed publicly.
Does we discussed it with str4d? what was the outcome?
There was a problem hiding this comment.
we can add add a note_commitments() method to IssueBundle if needed. (we have room to improve issuance.rs in orchard much more then to change note struct)
There was a problem hiding this comment.
Fixed the TODO/FIXME comment as suggested.
Regarding eliminating the byte round-trip: yes, we can add IssueBundle::note_commitments(), and I have now implemented that in QED-it/orchard#245.
However, that change is based on the current Orchard zsa1, which is not compatible with zsa-integration-state (the source branch for this PR). So we agreed not to use it here and to keep the current Zebra-side implementation in this PR. We can switch to the Orchard method in later PRs for the synced Zebra branch.
| [workspace.dependencies] | ||
| incrementalmerkletree = "0.8.2" | ||
| orchard = "0.11.0" | ||
| orchard = { version = "0.11.0", features = ["zsa-issuance", "temporary-zebra"] } |
There was a problem hiding this comment.
do we need "temporary-zebra" at this point?
There was a problem hiding this comment.
Yes, as we use ValueSum::from_raw (it's under temporary-zebra flag in orchard):
https://github.com/QED-it/zebra/blob/zsa-integration-state/zebra-chain/src/orchard/commitment.rs#L246
https://github.com/QED-it/zebra/blob/zsa-integration-state/zebra-chain/src/orchard/shielded_data.rs#L136
zebra-state/src/request.rs
Outdated
| new_outputs, | ||
| transaction_hashes, | ||
| // Not used in checkpoint paths. | ||
| // FIXME: Is this correct? |
There was a problem hiding this comment.
Good. Removed FIXME.
zebra-chain/src/transaction.rs
Outdated
| fmter.field("expiry_height", &expiry_height); | ||
| } | ||
|
|
||
| // TODO: add zip233_amount formatting here |
There was a problem hiding this comment.
Added (copied from the upstream version of Zebra). Also added/copied the zip233_amount method of Transaction.
e3b61f2 to
8a78bda
Compare
…c ref for bytes OrchardWorkflowBlock instead of a vec, matching the approach used in block.rs there
8a78bda to
40ba2bd
Compare
dab5fd9 to
2634fd6
Compare
9406861 to
2634fd6
Compare
Thanks for the review. I addressed the review comments in this PR as much as makes sense for What is still not done here, but is already done in the synced Zebra branches (
So, as we discussed, I kept this PR focused on the functional changes relevant to |
#245) This adds `IssueBundle::note_commitments()` to expose extracted note commitments directly as `pallas::Base` values. This is useful in Zebra to recompute issuance-related commitments without the current `to_bytes`/`from_bytes` workaround: https://github.com/QED-it/zebra/blob/2634fd662a4eaca6df901c82b1bc05d2982636d7/zebra-chain/src/orchard_zsa/issuance.rs#L43 See also the related Zebra PR review comment: QED-it/zebra#111 (comment)
This PR adds global issued-asset state tracking for ZSA issuance per ZIP-0227.
Key changes:
AssetState/IssuedAssetChangesand integrates Orchard’s ZSA validation APIs to compute per-block issued-asset deltas.validate_bundle_burnandverify_issue_bundleto validate burns and issuances while building the globalissued_assetsstate. This relies on the Orchard refactoring in Refactor issue/burn validation for Zebra asset state integration orchard#199, which exposes/reworks these validation helpers for direct integration from Zebra.verify_issue_bundlecan check issuance authorization signatures (issueAuthSig).verify_trusted_issue_bundleinstead ofverify_issue_bundlebecause the current Zebra checkpoint-verification path does not provide transaction sighashes. This is intentional: checkpoint-verified blocks are treated as already verified and are only replayed through the global-state logic to (re)buildissued_assetsduring node startup, so signature checking is not required there.AssetBase, plus tests/snapshots and workflow vectors.Notes:
nf_{0,0}(the first Orchard Action nullifier) into issuance verification.