Skip to content

More verbose and stricter transcripts. #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

recmo
Copy link
Contributor

@recmo recmo commented May 20, 2025

Make transcripts stricter by:

  1. Type checking using Rust's TypeId type_name.
  2. Requiring labels in prover and verifier.
  3. Hierarchically grouping interactions (begin/end delimiters).

This addresses #6.

It's still a work in progress.

@recmo recmo changed the title Add strongly typed transcripts. More verbose and stricter transcripts. May 21, 2025

/// Errors when validating a transcript.
#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash, Error)]
pub enum TranscriptError {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, this was clearly missing and the previous way of doing things made identifying and generalizing errors difficult because if I'm not mistaken, before that, errors in the domain separator were just strings

Comment on lines 24 to 27
// #[cfg(feature = "serde-postcard")]
// /// Use [`zerocopy`] to convert types to/from bytes.
// pub mod zerocopy;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be removed?

}

/// Extension trait for [`Transcript`].
pub trait TranscriptExt {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is an extension trait, shouldn't it look like this?

Suggested change
pub trait TranscriptExt {
pub trait TranscriptExt: Transcript {

/// Begin of a hint interaction.
fn begin_hint<T>(&mut self, label: &'static str) -> Result<(), Self::Error>;

/// End of a hint interaction..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// End of a hint interaction..
/// End of a hint interaction.

/// End of a hint interaction..
fn end_hint<T>(&mut self, label: &'static str) -> Result<(), Self::Error>;

/// Begin of a challenge interaction..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Begin of a challenge interaction..
/// Begin of a challenge interaction.

/// Begin of a challenge interaction..
fn begin_challenge<T>(&mut self, label: &'static str) -> Result<(), Self::Error>;

/// End of a challenge interaction..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// End of a challenge interaction..
/// End of a challenge interaction.

Comment on lines 11 to 35
pub trait ZeroCopyPattern<T>: Transcript {
fn message(&mut self, label: &'static str) -> Result<(), Self::Error>;
fn hint(&mut self, label: &'static str) -> Result<(), Self::Error>;
fn challenge(&mut self, label: &'static str) -> Result<(), Self::Error>;
}

pub trait ZeroCopyMessageProver<T: Immutable + IntoBytes>: Transcript {
fn message(&mut self, label: &'static str, value: &T) -> Result<(), Self::Error>;
}

pub trait ZeroCopyMessageVerifier<T: IntoBytes + FromBytes>: Transcript {
fn message(&mut self, label: &'static str) -> Result<T, Self::Error>;
}

pub trait ZeroCopyHintProver<T: Immutable + IntoBytes>: Transcript {
fn hint(&mut self, label: &'static str, value: &T) -> Result<(), Self::Error>;
}

pub trait ZeroCopyHintVerifier<T: IntoBytes + FromBytes>: Transcript {
fn hint(&mut self, label: &'static str) -> Result<T, Self::Error>;
}

pub trait ZeroCopyChallenge<T: IntoBytes + FromBytes>: Transcript {
fn challenge(&mut self, label: &'static str) -> Result<T, Self::Error>;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe can we have a very small one line header doc for each to explain the principle for the reader to better understand

Comment on lines +86 to +109
pub trait BytesPattern {
fn add_bytes(&mut self, count: usize, label: &str);
fn hint(&mut self, label: &str);
fn challenge_bytes(&mut self, count: usize, label: &str);
}

pub trait BytesMessageProver {
fn message(&mut self, input: &[u8]) -> Result<(), DomainSeparatorMismatch>;
}

pub trait BytesMessageVerifier {
fn message(&mut self, output: &mut [u8]) -> Result<(), DomainSeparatorMismatch>;
}
pub trait BytesHintProver {
fn hint(&mut self, input: &[u8]) -> Result<(), DomainSeparatorMismatch>;
}

pub trait BytesHintVerifier {
fn hint(&mut self, output: &mut [u8]) -> Result<(), DomainSeparatorMismatch>;
}

pub trait BytesChallenge {
fn challenge(&mut self, output: &mut [u8]) -> Result<(), DomainSeparatorMismatch>;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a short header doc for each?

pub enum InteractionKind {
/// A protocol containing mixed interactions.
Protocol,
/// A message send in-band from prover to verifier.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// A message send in-band from prover to verifier.
/// A message sent in-band from prover to verifier.

Protocol,
/// A message send in-band from prover to verifier.
Message,
/// A hint send out-of-band from prover to verifier.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// A hint send out-of-band from prover to verifier.
/// A hint sent out-of-band from prover to verifier.

Challenge,
}

/// Kinds of prover-verifier interactions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the header comment should be different that the one for InteractionKind no?

Challenge,
}

/// Kinds of prover-verifier interactions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Kinds of prover-verifier interactions
/// Where in the transcript the interaction sits relative to other actions.

position: usize,
/// Wheter the transcript playback has been finalized.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Wheter the transcript playback has been finalized.
/// Whether the transcript playback has been finalized.

default = []
default = ["arkworks-algebra", "arkworks-rand", "zerocopy"]
arkworks-rand = [
# Workaround to also implement arkworks version of rand as exported by ark_std
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should solve this no? arkworks-rs/std#60

Comment on lines 30 to 31
/// [`ProverState`] does not implement [`Clone`] or [`Copy`] to prevent accidental leaks.
#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the fact that we derive Clone seems contradictory with the comment just above no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Cloneing the prover state is a footgun, why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm following the Rust API Guideline C-COMMON-TRAITS here.

Is the concern here that the user may forget to Zeroize the clones causing them to potentially live longer in memory than necessary?

Copy link
Member

@mmaker mmaker May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's because if the prover is erroneously copied by the user down the line, and the proof system satisfies special soundness, an adversary can extract the witness. See e.g. https://media.ccc.de/v/27c3-4087-en-console_hacking_2010 around minute 37


impl<R: RngCore + CryptoRng> CryptoRng for ProverRng<R> {}

/// Implements the version of `rand` arkworks usses
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Implements the version of `rand` arkworks usses
/// Implements the version of `rand` arkworks uses

Comment on lines +44 to +52
#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)]
pub enum Hierarchy {
/// A single interaction.
Atomic,
/// Start of a sub-protocol.
Begin,
/// End of a sub-protocol.
End,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here maybe this could be useful to have an implementation like

impl Hierarchy {
    /// Returns true if this is an atomic (single) interaction.
    pub fn is_atomic(&self) -> bool {
        matches!(self, Hierarchy::Atomic)
    }

    /// Returns true if this marks the beginning of a sub-protocol.
    pub fn is_begin(&self) -> bool {
        matches!(self, Hierarchy::Begin)
    }

    /// Returns true if this marks the end of a sub-protocol.
    pub fn is_end(&self) -> bool {
        matches!(self, Hierarchy::End)
    }
}

length: Length,
) -> Result<(), Self::Error>;

/// Begin of a subprotocol.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Begin of a subprotocol.
/// Beginning of a subprotocol.

self.end::<T>(label, Kind::Protocol, Length::None)
}

/// Begin of a message interaction.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Begin of a message interaction.
/// Beginning of a message interaction.

self.end::<T>(label, Kind::Message, length)
}

/// Begin of a hint interaction.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Begin of a hint interaction.
/// Beginning of a hint interaction.

self.end::<T>(label, Kind::Hint, length)
}

/// Begin of a challenge interaction..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Begin of a challenge interaction..
/// Beginning of a challenge interaction.

self.begin::<T>(label, Kind::Challenge, length)
}

/// End of a challenge interaction..
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// End of a challenge interaction..
/// End of a challenge interaction.

assert_ne!(buf, [0; 8]);
}

// #[test]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either add these tests or remove them

Copy link
Contributor Author

@recmo recmo May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course! Once the approach and traits are stabilized I'll make the tests work again. Right now everything is still in flux.

@mmaker
Copy link
Member

mmaker commented May 30, 2025

Hi @recmo, thank you so so much for this incredible work! For me this is starting to become really too big to review. Is there any chance you can split it into multiple PR's? Here's some examples that would make sense:

  • place all the changes related to the hint into a separate PR (if possible),
  • place all codec-related changes into a separate PR, excluding the arkworks serialization one,
  • place all codec-related ark-serialize changes into a separate PR.

If that helps, we could schedule a call (potentially with @WizardOfMenlo) for speeding things up and not slow you down?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants