Implement PCZT support #440

str4d · 2024-11-27T10:03:01Z

No description provided.

str4d · 2024-11-27T10:12:35Z

src/pczt.rs

+    ///
+    /// - This is chosen by the Constructor.
+    /// - This is required by the IO Finalizer, and is cleared by it once used.
+    /// - Signers MUST reject PCZTs that contain `dummy_sk` values.


I both named this dummy_sk and added a "MUST reject" here because I do not want PCZTs to get serialized with non-dummy spending keys.

I stored the dummy note's sk instead of the smaller-scoped ask, because in Orchard we impose validity requirements at parse time of sk (specifically to ensure that a valid ivk is produced), and we already had APIs for handling that parsing here. If desired this can be scoped down to dummy_ask, but given that (again) this field is only for dummy notes, I think sk should be fine here.

str4d · 2024-11-27T10:14:09Z

src/pczt/parse.rs

+    pub fn parse(
+        actions: Vec<Action>,
+        flags: u8,
+        value_sum: (u64, bool),


I decided on this serialization format for ValueSum (rather than e.g. i128) because it has no edge cases; all values are valid.

nuttycom

utACK e7a2945 with minor questions / naming nits. Overall this looks excellent.

src/builder.rs

nuttycom · 2024-11-27T15:43:55Z

src/pczt.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_sum: ValueSum,


Doesn't the fact that a mut getter is exposed for actions make it so that this can get out of sync with the actions that have been added? Would it be safer to have explicit add_action and redact_action methods?

The mut getter here is actually for the Fun Third Option: mutating existing actions so that signatures can be applied. In the PCZT format PR I implemented all the roles as consuming / transforming, but the orchard APIs are what get used under the hood to do the transformations, and for the scale of PCZTs a mapping-based API with the necessary granularity would be ridiculously verbose and onerous to work with.

If we don't want to expose the dual capabilities of "modify existing Actions" and "add / remove Actions" here, then I can replace this with a method that returns &mut [Action] instead (hiding the Vec nature and only exposing the modification capability). That would be sufficient for now as the "add Actions" capability is currently provided by Builder (I have no plans to enable piece-wise PCZT building in this first pass).

Hiding the Vec nature (for mutation) sounds like the right approach to me. Right now it presents a footgun, which we can eliminate.

nuttycom · 2024-11-27T15:48:13Z

src/pczt.rs

+    /// TODO: This could be merged with `rseed` into a tuple. `recipient` and `value` are
+    /// separate because they might need to be independently redacted. (For which role?)


Suggested change

/// TODO: This could be merged with `rseed` into a tuple. `recipient` and `value` are

/// separate because they might need to be independently redacted. (For which role?)

// TODO: This could be merged with `rseed` into a tuple. `recipient` and `value` are

// separate because they might need to be independently redacted. (For which role?)

nuttycom · 2024-11-27T15:51:57Z

src/pczt.rs

+        let (anchor, merkle_path) = {
+            let cmx: ExtractedNoteCommitment = note.commitment().into();
+            let leaf = MerkleHashOrchard::from_cmx(&cmx);
+            let mut tree = BridgeTree::<MerkleHashOrchard, u32, 32>::new(100);


TODO: since BridgeTree is no longer being maintained, we should move away from using it.

nuttycom · 2024-11-27T18:06:20Z

src/pczt/tx_extractor.rs

+
+/// Authorizing data for a bundle of actions that is just missing a binding signature.
+#[derive(Debug)]
+pub struct Unbound {


nit: this name isn't particularly intuitive to me, but I don't have a better suggestion; maybe BindingSigInputs or something?

The name appears primarily in type signatures, i.e. Bundle<Unbound, ZatBalance>; I chose it to make sense there (same as Authorized / Unauthorized / EffectsOnly).

nuttycom · 2024-11-27T18:08:29Z

src/pczt/tx_extractor.rs

+    /// Verifies the given sighash with every `spend_auth_sig`, and then binds the bundle.
+    ///
+    /// Returns `None` if the given sighash does not validate against every `spend_auth_sig`.
+    pub fn bind<R: RngCore + CryptoRng>(


Maybe apply_binding_sig or bind_to_sighash?

str4d · 2024-11-28T00:09:25Z

src/pczt/io_finalizer.rs

+
+        // Add signatures to dummy spends.
+        for action in self.actions.iter_mut() {
+            if let Some(sk) = action.spend.dummy_sk.take() {


Point the take() out.

str4d · 2024-11-28T00:21:42Z

src/pczt.rs

+    /// The seed randomness for the output.
+    ///
+    /// - This is set by the Constructor.
+    /// - This is required by the Prover, instead of disclosing `shared_secret` to them.


Remove shared_secret reference.

Describe how the Signer can use rseed and recipient to decrypt and verify enc_ciphertext (in the case where ock is not present, or OvkPolicy::None was used to make the data unrecoverable from chain by the sender).

str4d · 2024-11-28T00:23:12Z

src/pczt.rs

+
+        // Run the Creator and Constructor roles.
+        let mut builder = Builder::new(BundleType::DEFAULT, anchor);
+        builder.add_spend(fvk, note, merkle_path).unwrap();


Modify this test to send an output to a different recipient (with change), and then verify that the PCZT contains sufficient information to decrypt and check enc_ciphertext.

str4d · 2024-11-28T02:00:12Z

src/pczt/tx_extractor.rs

+        sighash: [u8; 32],
+        rng: R,
+    ) -> Option<crate::Bundle<Authorized, V>> {
+        let bound = self.map_authorization(


Move this to after the check so we don't bother making the signature if the binding would be wrong.

str4d · 2024-11-28T02:03:17Z

src/pczt/tx_extractor.rs

+    MissingBindingSignatureSigningKey,
+    /// The Transaction Extractor role requires `zkproof` to be set.
+    MissingProof,
+    /// The Transaction Extractor role requires all `zkproof` fields to be set.


Suggested change

/// The Transaction Extractor role requires all `zkproof` fields to be set.

/// The Transaction Extractor role requires all `spend_auth_sig` fields to be set.

str4d · 2024-11-28T02:04:54Z

src/pczt/tx_extractor.rs

+                Ok(redpallas::Signature::from(
+                    action
+                        .spend
+                        .spend_auth_sig


Copy-pasta: this is already a redpallas::Signature.

str4d · 2024-11-28T05:27:31Z

src/pczt/parse.rs

+            .map(|r| {
+                Address::from_raw_address_bytes(r)
+                    .into_option()
+                    .ok_or(ParseError::InvalidSpendRecipient)


Rename this error to InvalidRecipient.

Implement PCZT support

e7a2945

str4d marked this pull request as draft November 27, 2024 10:03

str4d commented Nov 27, 2024

View reviewed changes

nuttycom approved these changes Nov 27, 2024

View reviewed changes

str4d commented Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement PCZT support #440

Implement PCZT support #440

str4d commented Nov 27, 2024

str4d Nov 27, 2024

str4d Nov 27, 2024

nuttycom left a comment

nuttycom Nov 27, 2024

str4d Nov 27, 2024

nuttycom Nov 27, 2024 •

edited

Loading

nuttycom Nov 27, 2024

nuttycom Nov 27, 2024

nuttycom Nov 27, 2024

str4d Nov 27, 2024 •

edited

Loading

nuttycom Nov 27, 2024

str4d Nov 28, 2024

str4d Nov 28, 2024

str4d Nov 28, 2024

str4d Nov 28, 2024

str4d Nov 28, 2024

str4d Nov 28, 2024

str4d Nov 28, 2024

		/// TODO: This could be merged with `rseed` into a tuple. `recipient` and `value` are
		/// separate because they might need to be independently redacted. (For which role?)

	/// The Transaction Extractor role requires all `zkproof` fields to be set.
	/// The Transaction Extractor role requires all `spend_auth_sig` fields to be set.

Implement PCZT support #440

Are you sure you want to change the base?

Implement PCZT support #440

Conversation

str4d commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nuttycom Nov 27, 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

str4d Nov 27, 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

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

nuttycom Nov 27, 2024 •

edited

Loading

str4d Nov 27, 2024 •

edited

Loading