pczt: Define the structure and semantics of the PCZT format #1577

str4d · 2024-10-16T11:51:23Z

Todo:

Decide on an encoding (might not happen in this PR)

codecov · 2024-10-16T12:17:20Z

Codecov Report

Attention: Patch coverage is 0% with 519 lines in your changes missing coverage. Please review.

Project coverage is 54.80%. Comparing base (dd51c2a) to head (37fc6ce).
Report is 71 commits behind head on main.

Files with missing lines	Patch %	Lines
pczt/src/sapling.rs	0.00%	144 Missing ⚠️
pczt/src/orchard.rs	0.00%	142 Missing ⚠️
pczt/src/transparent.rs	0.00%	79 Missing ⚠️
pczt/src/roles/tx_extractor/mod.rs	0.00%	48 Missing ⚠️
pczt/src/roles/prover/sapling.rs	0.00%	39 Missing ⚠️
pczt/src/roles/combiner/mod.rs	0.00%	31 Missing ⚠️
pczt/src/common.rs	0.00%	18 Missing ⚠️
pczt/src/roles/creator/mod.rs	0.00%	6 Missing ⚠️
pczt/src/roles/tx_extractor/sapling.rs	0.00%	5 Missing ⚠️
...imitives/src/transaction/components/transparent.rs	0.00%	4 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1577      +/-   ##
==========================================
- Coverage   61.63%   54.80%   -6.83%     
==========================================
  Files         148      158      +10     
  Lines       18836    19581     +745     
==========================================
- Hits        11610    10732     -878     
- Misses       7226     8849    +1623

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

str4d · 2024-10-16T12:18:18Z

pczt/src/common.rs

+
+/// Global fields that are relevant to the transaction as a whole.
+#[derive(Clone)]
+pub(crate) struct Global {


Keystone's proposal included a network field (indicating mainnet vs testnet vs ?) at the global level, possibly because in my 2019 PCZT draft I suggested it. However, I note that PSBTs (BIP 174) do not include such a field (AFAICT). Is such a field necessary, and/or useful?

I don't see why it would be necessary or useful here, but it looks like BIP 174 does include an implicit indication of the network in the serialized extended public keys.

We meet a similar problem on Unisat's Fractal Bitcoin recently. PSBT doesn't have a network field, so we have to set it on the global map field which means it becomes a custom protocol between Unisat and us.
This can be useful if the chain have multiple sub/child chains like Bitcoin vs Litecoin or Dogecoin. In theory Litecoin can also use PSBT, but without network field, we cannot know some accurate info like coin precision.
Anyway, that's depends on Zcash's needs.

Hmm, this is a good point. And as @arya2 notes above and @pacu notes below, we will already be semi-explicitly encoding the network into the PCZT at some later stage. So I think it makes sense for the Creator to encode the network up-front, and then every later instance of data that includes network information can be checked against it for correctness.

Next question then is what format this network field should have. A SLIP 44 coin_type (as used in BIP 32 and ZIP 32 derivation paths) would make sense for mainnet applications, but SLIP 44 only defines a single testnet code for all coins. Would that be sufficient for Zcash (and potentially other) testnet uses? (I think so, because if you're running against testnet then you already likely know what network you're dealing with, and the important part is that we have domain separation between mainnet and non-mainnet, as well as between Zcash and non-Zcash mainnets.)

daira · 2024-10-16T19:48:15Z

pczt/src/common.rs

+    pub(crate) tx_version: u32,
+    pub(crate) version_group_id: u32,


Should this pair of fields be optional independently of the others?

Would it be better to infer the version group id from the tx version instead of having it as an explicit field?

Should this pair of fields be optional independently of the others?

~~Would it make sense to adding a builder struct where the tx_version field is optional instead of making it optional here?~~

Update: I see there's already a builder struct, so it seems unnecessary to make the field optional here as well.

Would it be better to infer the version group id from the tx version instead of having it as an explicit field?

That's more fragile across consensus forks, but I can see an argument that the tuple (tx_version, consensus_branch_id) might be sufficient to fully constrain the value of version_group_id such that every role can reliably deduce it. The main argument against this would be that it would force PCZT roles to be aware of more details about transaction formats, where they might otherwise just be able to focus on the PCZT.

daira · 2024-10-16T19:48:35Z

pczt/src/common.rs

+    pub(crate) lock_time: u32,
+    pub(crate) expiry_height: u32,


Should these be independently optional?

It could be nice to type these as LockTime and BlockHeight.

Should these be independently optional?

If this needs to be able to produce v1 or v2 transactions, then at least expiry_height should probably be optional.

@arya2 I'm intentionally not adding types to the core structure (at least at this stage), to keep its dependencies as minimal / non-existent as possible (because there are many roles that do not need to care about every type in the PCZT, e.g. an Orchard signer doesn't need to be able to parse Sapling-specific types). The Transaction Extractor role necessarily performs all conversions and checks, so that acts as a backstop.

Once we've figured out and agreed on the structure, then it may make sense to add certain newtypes that pczt can rely on (depending on who the downstream consumers of the crate will be).

daira · 2024-10-16T20:00:01Z

pczt/src/orchard.rs

+    ///
+    /// This is initialized by the Creator, and updated by the Constructor as spends or
+    /// outputs are added to the PCZT. It enables per-spend and per-output values to be
+    /// redacted from the PCZT after they are no longer necessary.


Maybe calculate this at the same time as bsk (in which case it would be an option of (value_balance, bsk)).

daira · 2024-10-16T20:08:16Z

pczt/src/sapling.rs

+    /// This is initialized by the Creator, and updated by the Constructor as spends or
+    /// outputs are added to the PCZT. It enables per-spend and per-output values to be
+    /// redacted from the PCZT after they are no longer necessary.
+    pub(crate) value_balance: u64,


Same comment as Orchard.

pczt/src/common.rs

pczt/src/roles/creator/mod.rs

pczt/src/lib.rs

pczt/src/roles/creator/mod.rs

pacu · 2024-10-28T18:02:59Z

pczt/src/orchard.rs

+    /// - After`zkproof` / `spend_auth_sig` has been set, this can be redacted.
+    pub(crate) alpha: Option<[u8; 32]>,
+
+    // TODO derivation path


wouldn't this account for network information (see comments on top of the PR)

conradoplg

Looks good, added some comments about FROST usage

pczt/src/orchard.rs

conradoplg · 2024-11-01T17:08:19Z

For ZSA issuance I think we need the fields required for computing the issuance_digest?

str4d · 2024-11-08T09:52:52Z

Force-pushed to fill in some more parts:

Transparent bundle fields for signing (adapting from BIPs 174 and 370)
Optional ZIP 32 paths (that Updaters can set to help Signers determine which keys to use)
Decided on semantics for merging data in proprietary maps (equal keys are required to have equal values, same as every other field and map).

str4d · 2024-11-08T10:02:24Z

I'm currently inclined to leave out both FROST support and ZSA issuance support from the first version of PCZTs, as we don't have experience with either of them yet, and trying to get them right at the same time as the base PCZT format is likely to end in sorrow.

As @conradoplg notes, we can in some cases have FROST internally pass around a PCZT, but that won't necessarily help for all possible PCZT use cases, so we'll likely want a v2 that adds those fields. That would also be the right time to adjust the core fields if we find weaknesses in the v1 PCZT format.

conradoplg · 2024-11-11T23:10:42Z

I'm happy with leaving FROST out of v1, we can make it work for now by sending additional data along with the PCZT.

These follow the equivalent fields in BIP 174 and BIP 370.

str4d · 2024-11-12T08:17:53Z

Force-pushed with more changes:

Anchors are now required and set by the Creator.
Ciphertexts are now Vec<u8> to accommodate upcoming plaintext changes.
Pczt struct no longer contains a Version; instead that will be explicitly part of the encoding.
Extracted some conversion logic from the Prover role to use it for consistency checks in the Signer role.

str4d force-pushed the pczt-format branch from cb5ba68 to 6ca070b Compare October 16, 2024 11:58

str4d commented Oct 16, 2024

View reviewed changes

daira reviewed Oct 16, 2024

View reviewed changes

arya2 reviewed Oct 16, 2024

View reviewed changes

pczt/src/common.rs Show resolved Hide resolved

arya2 reviewed Oct 16, 2024

View reviewed changes

pczt/src/roles/creator/mod.rs Show resolved Hide resolved

arya2 reviewed Oct 16, 2024

View reviewed changes

pczt/src/lib.rs Show resolved Hide resolved

arya2 reviewed Oct 16, 2024

View reviewed changes

pczt/src/roles/creator/mod.rs Show resolved Hide resolved

mpguerra requested a review from conradoplg October 22, 2024 13:26

nuttycom mentioned this pull request Oct 24, 2024

Expose the stages of transaction proving and signing via the zcash_client_backend API #1091

Open

pacu reviewed Oct 28, 2024

View reviewed changes

conradoplg reviewed Oct 28, 2024

View reviewed changes

pczt/src/orchard.rs Outdated Show resolved Hide resolved

str4d force-pushed the pczt-format branch from 6ca070b to eb4275f Compare November 8, 2024 09:50

str4d added 2 commits November 9, 2024 01:49

pczt: Create structure that can be converted into a v5 transaction

176db40

pczt: Add fields necessary for signing shielded spends

75bd49e

str4d added 7 commits November 12, 2024 08:09

pczt: Add fields necessary for creating proofs

6a7692f

pczt: Add fields necessary for decrypting ciphertexts

ec44463

pczt: Add fields necessary for signing transparent spends

7ab0249

These follow the equivalent fields in BIP 174 and BIP 370.

pczt: Add fields for pointing to derived signing keys

71a6498

pczt: Add fields for storing arbitrary proprietary data

cc2552f

TODO pczt: Define an encoding

16c2651

WIP unfinished changes

37fc6ce

str4d force-pushed the pczt-format branch from eb4275f to 37fc6ce Compare November 12, 2024 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pczt: Define the structure and semantics of the PCZT format #1577

pczt: Define the structure and semantics of the PCZT format #1577

str4d commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

str4d Oct 16, 2024

arya2 Oct 16, 2024

soralit Oct 28, 2024 •

edited

Loading

str4d Oct 29, 2024

daira Oct 16, 2024

arya2 Oct 16, 2024 •

edited

Loading

str4d Oct 16, 2024

daira Oct 16, 2024

arya2 Oct 16, 2024

str4d Oct 16, 2024

daira Oct 16, 2024 •

edited

Loading

daira Oct 16, 2024

pacu Oct 28, 2024

conradoplg left a comment

conradoplg commented Nov 1, 2024

str4d commented Nov 8, 2024

str4d commented Nov 8, 2024

conradoplg commented Nov 11, 2024

str4d commented Nov 12, 2024

		pub(crate) tx_version: u32,
		pub(crate) version_group_id: u32,

pczt: Define the structure and semantics of the PCZT format #1577

Are you sure you want to change the base?

pczt: Define the structure and semantics of the PCZT format #1577

Conversation

str4d commented Oct 16, 2024 • edited Loading

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soralit Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arya2 Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conradoplg left a comment

Choose a reason for hiding this comment

conradoplg commented Nov 1, 2024

str4d commented Nov 8, 2024

str4d commented Nov 8, 2024

conradoplg commented Nov 11, 2024

str4d commented Nov 12, 2024

str4d commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 16, 2024 •

edited

Loading

soralit Oct 28, 2024 •

edited

Loading

arya2 Oct 16, 2024 •

edited

Loading

daira Oct 16, 2024 •

edited

Loading