ZSA integration (stage 2) - Issued assets state #29
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)
arya2
left a comment
arya2
left a comment
There was a problem hiding this comment.
This looks good, thank you for catching the mistake around converting negative i128s to u64s.
I addressed some of the FIXMEs added here in #32 after adding the RPC method. I'm happy to remove the issued_assets_change field on SemanticallyVerifiedBlock and get the changes from block transactions directly valid_burns_and_issuance() and prepare_issued_assets_batch(), the current code there is needlessly complicated (was this the simplification you were asking about during the Qedit-ZF sync?)
| // TODO: Reference ZIP | ||
| 0.. => signed.try_into().ok().map(Self::Issuance), | ||
| ..0 => signed.try_into().ok().map(Self::Burn), | ||
| // FIXME: (-signed) - is this a correct fix? |
There was a problem hiding this comment.
This is correct, thank you for catching it!
| }) | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| // FIXME: We use 0 as a value - is that correct? |
There was a problem hiding this comment.
It's correct but overly complicated, it should be refactored.
zebra-chain/src/orchard_zsa/burn.rs
Outdated
| ValueCommitment::with_asset(pallas::Scalar::zero(), amount, &asset) | ||
| ValueCommitment::with_asset( | ||
| pallas::Scalar::zero(), | ||
| // FIXME: consider to use TryFrom and return an error instead of using "expect" |
There was a problem hiding this comment.
This should be a u64 so the conversion should be infallible.
There was a problem hiding this comment.
That makes sense. But to make it work, the new ValueCommitment::with_asset constructor I added earlier should accept the value as a NoteValue instead of Amount (which is i64-based). I've fixed that and pushed the change into the zsa-integration-state branch.
| // FIXME: Not sure: it looks like semantically_verified.issued_assets_changes is already | ||
| // filled with burn and issuance items in zebra-consensus, see Verifier::call function in | ||
| // zebra-consensus/src/transaction.rs (it uses from_burn and from_issue_action AssetStateChange | ||
| // methods from ebra-chain/src/orchard_zsa/asset_state.rs). Can't it cause a duplication? | ||
| // Can we collect all change items here, not in zebra-consensus (and so we don't need | ||
| // SemanticallyVerifiedBlock::issued_assets_changes at all), or performing part of the | ||
| // checks in zebra-consensus is important for the consensus checks order in a some way? |
There was a problem hiding this comment.
Can't it cause a duplication?
It wouldn't be duplicated since the previous loop isn't mutating the asset states, it's just loading them into memory.
Can we collect all change items here, not in zebra-consensus
Yep, I'll try to make this change.
| // hand, there needs to be a check that denies duplicated burn records for the same | ||
| // asset inside the same transaction. |
There was a problem hiding this comment.
It does need to check that there are no duplicate burns in a single transaction somewhere, but that could be validated during deserialization or semantic validation, and this should update the asset state correctly otherwise.
I think we should update the Burn type to be Burn(HashMap<AssetBase, NoteValue>) and return an error during deserialization if any asset base is burned twice in the same transaction.
There was a problem hiding this comment.
That's a good idea to use a HashMap to ensure the uniqueness of AssetBase at the type level (the uniqueness would be validated in the deserialization function). However, I see two downsides:
-
Using a
HashMapmakes theburn.rsmodule code more bloated, as, for example, we can't directly take advantage of the automatic vector serialization provided byZcashSerializeandZcashDeserializeand have to implement it manually. But this isn't a major issue - I attempted to implement it today. -
A more serious problem is that
HashMapdoes not preserve the order of items. If we deserialize the transaction and then serialize it back or calculate the hash, the result would differ because the order of the burn items would change (most likely different, to be precise). To resolve this issue, we would need to add a new requirement to the specification that burn items must be sorted in the transaction by the bytes ofAssetBase, for example.
In my opinion, we could continue using a Vec for now but perform uniqueness validation within/after the deserializer. And we already have a burn validation function in the orchard crate. I will add this check.
…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
Great, thanks @arya2. Regarding the simplification I mentioned, my main idea was to reuse our work from the |
… provided and the finalize flag is set to true
…st data from files, introduce and use OrchardWorkflowBlock there
dmidem
changed the title
dmidem
changed the title
dmidem
changed the title
| fn as_bytes(&self) -> Self::Bytes { | ||
| [ | ||
| vec![self.is_finalized as u8], | ||
| self.total_supply.to_be_bytes().to_vec(), |
There was a problem hiding this comment.
Looks like resolved.
| self.issued_assets.get(asset_base).cloned() | ||
| } | ||
|
|
||
| /// Remove the History tree index at `height`. |
There was a problem hiding this comment.
@arya2 (Link): @dmidem I think it would be simpler to keep the issued assets maps on each ContextuallyValidatedBlock partial relative to the previous block instead of making them partial relative to the finalized tip, but if they need to be relative to the finalized tip, copying the AssetBases for every block would multiply the non-finalized state's theoretical max memory use, so those should go in Arcs.
| /// Remove the History tree index at `height`. | |
| /// When a block with the provided `transactions` and partial `issued_assets` state is removed | |
| /// from the chain tip, revert its changes from the [`Chain`]'s partial issued assets state. | |
| /// | |
| /// When a block is removed from the chain root to be committed to the finalized state, any | |
| /// assets in its partial `issued_assets` with an asset state matching the one in the chain's | |
| /// partial issued assets map is removed from the chain's partial issued assets map (since that | |
| /// it should match the finalized issued asset state.) |
dmidem
changed the title
dmidem
force-pushed
the
zsa-integration-state
branch
from
0a09a70 to
43ea314
Compare
dmidem
force-pushed
the
zsa-integration-state
branch
from
11e1c3e to
3ab6b56
Compare
2 participants
This pull request integrates the
zsa-issued-assetsbranch into thezsa-integration-consensusbranch by creating a newzsa-integration-statebranch based onzsa-integration-consensus.The
zsa-issued-assetsbranch was merged intozsa-integration-state, during which all merge conflicts were resolved, and compilation errors were fixed to ensure the code builds successfully.Additionally, several bugs introduced by the
zsa-issued-assetsbranch were identified and addressed using the newly implementedzebra-consensuscheck_zsa_workflowunit test from thezsa-integration-consensus.