pczt: Define the structure and semantics of the PCZT format #1577
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| use std::collections::BTreeMap; | ||
|
|
||
| use crate::roles::combiner::merge_map; | ||
|
|
||
| /// Global fields that are relevant to the transaction as a whole. | ||
| #[derive(Clone, Debug)] | ||
| pub(crate) struct Global { | ||
|
There was a problem hiding this comment. Keystone's proposal included a There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 |
||
| // | ||
| // Transaction effecting data. | ||
| // | ||
| // These are required fields that are part of the final transaction, and are filled in | ||
| // by the Creator when initializing the PCZT. | ||
| // | ||
| pub(crate) tx_version: u32, | ||
| pub(crate) version_group_id: u32, | ||
|
Comment on lines
+14
to
+15
There was a problem hiding this comment. Should this pair of fields be optional independently of the others? There was a problem hiding this comment. Would it be better to infer the version group id from the tx version instead of having it as an explicit field?
Update: I see there's already a builder struct, so it seems unnecessary to make the field optional here as well. There was a problem hiding this comment.
That's more fragile across consensus forks, but I can see an argument that the tuple There was a problem hiding this comment. IMHO the arguments for encoding the version group ID explicitly in the transaction format apply equally to PCZTs. |
||
| /// The consensus branch ID for the chain in which this transaction will be mined. | ||
| /// | ||
| /// Non-optional because this commits to the set of consensus rules that will apply to | ||
| /// the transaction; differences therein can affect every role. | ||
| pub(crate) consensus_branch_id: u32, | ||
arya2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /// TODO: In PSBT this is `fallback_lock_time`; decide whether this should have the | ||
| /// same semantics. | ||
| pub(crate) lock_time: u32, | ||
| pub(crate) expiry_height: u32, | ||
|
Comment on lines
+23
to
+24
There was a problem hiding this comment. Should these be independently optional? There was a problem hiding this comment. It could be nice to type these as
If this needs to be able to produce v1 or v2 transactions, then at least There was a problem hiding this comment. @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 There was a problem hiding this comment. @arya2 I don't think it does need to be able to produce v1 or v2 transactions, or v3 for that matter. I only know of use cases for using PCZTs to produce new transactions, which would necessarily have to be at least v4 (required since Sapling). |
||
|
|
||
| pub(crate) proprietary: BTreeMap<String, Vec<u8>>, | ||
| } | ||
|
|
||
| impl Global { | ||
| pub(crate) fn merge(mut self, other: Self) -> Option<Self> { | ||
| let Self { | ||
| tx_version, | ||
| version_group_id, | ||
| consensus_branch_id, | ||
| lock_time, | ||
| expiry_height, | ||
| proprietary, | ||
| } = other; | ||
|
|
||
| if self.tx_version != tx_version | ||
| || self.version_group_id != version_group_id | ||
| || self.consensus_branch_id != consensus_branch_id | ||
| || self.lock_time != lock_time | ||
| || self.expiry_height != expiry_height | ||
| { | ||
| return None; | ||
| } | ||
|
|
||
| if !merge_map(&mut self.proprietary, proprietary) { | ||
| return None; | ||
| } | ||
|
|
||
| Some(self) | ||
| } | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq)] | ||
| pub(crate) struct Zip32Derivation { | ||
| /// The [ZIP 32 seed fingerprint](https://zips.z.cash/zip-0032#seed-fingerprints). | ||
| pub(crate) seed_fingerprint: [u8; 32], | ||
|
|
||
| /// The sequence of indices corresponding to the shielded HD path. | ||
| /// | ||
| /// Indices can be hardened or non-hardened (i.e. the hardened flag bit may be set). | ||
| /// When used with a Sapling or Orchard spend, the derivation path will generally be | ||
| /// entirely hardened; when used with a transparent spend, the derivation path will | ||
| /// generally include a non-hardened section matching either the [BIP 44] path, or the | ||
| /// path at which ephemeral addresses are derived for [ZIP 320] transactions. | ||
| /// | ||
| /// [BIP 44]: https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki | ||
| /// [ZIP 320]: https://zips.z.cash/zip-0320 | ||
| pub(crate) derivation_path: Vec<u32>, | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the
sapling/temporary-zcashdfeature dependency strictly necessary? Consider filing an issue (if there isn't one already) againstsaplingto make the necessary APIs stable.