ZSA integration (step 8): Merge zsa-issued-assets into zsa-integration-consensus with bug fixes #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)
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?)
| @@ -81,7 +83,8 @@ impl SupplyChange { | |||
| // Burn amounts MUST not be 0 | |||
| // 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!
| @@ -208,6 +211,7 @@ impl AssetStateChange { | |||
| }) | |||
| .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 |
…s) to Orchard ZSA
…plate-rpcs feature enabled
dmidem
changed the title
…rs came with Rust 1.83.0
dmidem
changed the title
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.