A cache for saving public params and commitment keys to local disk #79

winston-h-zhang · 2023-10-22T22:37:22Z

Addresses #67.

This PR is largely a port of the already existing infrastructure from lurk-rs. We implement Cache as an interface to read, write, and update public parameters and commitment keys as the user needs. We also declare a global ARECIBO_CACHE for any downstream arecibo users to access, although users can also just declare a cache themselves.

We modify the Group trait by adding a way to extend commitment keys without having to recompute the entire string.
We add a new CommitmentKeyTrait to define the behavior of commitment keys.

huitseeker · 2023-10-22T23:35:52Z

src/cache.rs

@@ -0,0 +1,300 @@
+//! This module provides caching utils


Here's some inspiration to solve your imminent issue of compiling on WASM in a transparent manner:
https://github.com/rust-lang/rust/blob/master/compiler/rustc_data_structures/src/memmap.rs

huitseeker · 2023-10-22T23:41:25Z

src/traits/circuit_supernova.rs

@@ -25,6 +27,15 @@ pub trait StepCircuit<F: PrimeField>: Send + Sync + Clone {
    pc: Option<&AllocatedNum<F>>,
    z: &[AllocatedNum<F>],
  ) -> Result<(Option<AllocatedNum<F>>, Vec<AllocatedNum<F>>), SynthesisError>;
+
+  /// AHHH


This feels like it's halfway towards a confirmation of Strehmel's results. Pray tell us more.

src/traits/mod.rs

huitseeker · 2023-10-22T23:44:29Z

src/constants.rs

+/// The threshold of elements we check when extending a `CommitmentKey`
+/// to be convinced that they were generated from the same label
+///
+/// TODO: I have no idea about *any* security implications here.


In a caching context, you should either :

trust your local disc, in which case the checking threshold should be 0, or 1 to avoid regressions due to code changes, or

not trust the cache, in which case the appropriate threshold is INFINITY

huitseeker

I think there's a concurrency issue in the access of the CK / PPs. Happy to set some time to discuss it along with potential solutions.

huitseeker · 2023-10-25T13:52:50Z

src/cache.rs

+
+/// Cache structure that holds the root directory of all the cached files
+pub struct Cache {
+  inner: Arc<Mutex<PathBuf>>,


This hints at a path that would be edited more than once over the lifetime of the program. Is there a rationale for this, or should we simplify this to a sync::OnceCell?

huitseeker · 2023-10-25T14:03:21Z

src/cache.rs

+  /// Sets the inner root directory of the cache
+  pub fn set_inner(&self, path: &PathBuf) {
+    self.inner.lock().unwrap().clone_from(path);
+
+    create_dir_all(path).unwrap();
+    create_dir_all(path.join("commitment_keys")).unwrap();
+    create_dir_all(path.join("public_params")).unwrap();
+  }


There's no way to get a cache other than init_cache(), which makes me think this should be called reset_inner. But as mentioned above, I don't think we need the functionality at this stage.

huitseeker · 2023-10-25T14:05:49Z

src/cache.rs

+    let mut file = self.ck_file::<G>()?;
+
+    let ck_size = file.read_u64::<LittleEndian>()? as usize;
+    println!("ck_size: {}, n: {}", ck_size, n);


Please use the log crate instead.

huitseeker · 2023-10-25T14:08:37Z

src/cache.rs

+    let mut mmap = unsafe { MmapMut::map_mut(&file)? };
+    let skip = std::mem::size_of::<usize>();
+    let bytes = &mut mmap[skip..]; // skip the first `usize` bytes


Ah, so we should at least make clear that despite the use of concurrency-safe structures above, this is not concurrency-safe, right? Two calls to commitment_key (including those generated as simply as running unit tests concurrently) could easily step on each other's toes.

Have you considered something like a struct MemoryMappedMutFile(Arc<RwLock<MmapMut>>); of which the cache would create a singleton instance per relevant file (commitment key, public params)?

huitseeker · 2023-10-25T14:26:40Z

src/cache.rs

+    provider::{bn256_grumpkin::bn256, secp_secq::secq256k1},
+    traits::{commitment::CommitmentEngineTrait, Group},
+  };
+


I think the concurrency test is three threads all trying to extend the same CK, here.

huitseeker · 2023-10-25T14:27:58Z

src/provider/pedersen.rs

+
+  fn decode(bytes: &mut [u8], n: usize) -> io::Result<Self> {
+    // Note: this is unsafe but stable; it only assumes that `bytes` is any valid commitment key
+    unsafe {


I think this unsafe section can be smaller.

huitseeker · 2023-10-25T14:32:58Z

src/provider/pedersen.rs

+      let mut test_ck = Self { ck: Vec::new() };
+      test_ck.ck.set_len(n);
+      let header_size = mem::size_of::<Self>();
+      let header: &[u8] =
+        std::slice::from_raw_parts(&test_ck as *const _ as *const u8, header_size);
+      bytes[..header_size].copy_from_slice(header);


I suspect this can fit within a MaybeUninit usage: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#initializing-an-array-element-by-element

huitseeker · 2023-10-25T14:33:47Z

src/traits/commitment.rs

@@ -101,6 +115,33 @@ pub trait CommitmentEngineTrait<G: Group>: Clone + Send + Sync {
  /// Samples a new commitment key of a specified size
  fn setup(label: &'static [u8], n: usize) -> Self::CommitmentKey;

+  /// Internal implementation of [CommitmentEngineTrait::extend]
+  ///
+  /// **Do not call this directly**. This does not check the consistency of the given label.


One way to improve this is to call us extend_aux_unsafe

huitseeker

For emphasis, this is the important comment:
#79 (comment)

winston-h-zhang · 2024-02-20T07:01:28Z

Closed in favor of #335, this has bit-rot too much

winston-h-zhang self-assigned this Oct 22, 2023

winston-h-zhang requested a review from huitseeker October 22, 2023 22:37

huitseeker reviewed Oct 22, 2023

View reviewed changes

winston-h-zhang force-pushed the cache-feature branch 3 times, most recently from 245abc5 to 1a19c37 Compare October 24, 2023 17:57

huitseeker reviewed Oct 25, 2023

View reviewed changes

huitseeker requested changes Oct 26, 2023

View reviewed changes

-- wip --

52e336f

winston-h-zhang force-pushed the cache-feature branch from 1a19c37 to 52e336f Compare November 12, 2023 05:00

winston-h-zhang closed this Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A cache for saving public params and commitment keys to local disk #79

A cache for saving public params and commitment keys to local disk #79

winston-h-zhang commented Oct 22, 2023

huitseeker Oct 22, 2023

huitseeker Oct 22, 2023

huitseeker Oct 22, 2023

huitseeker left a comment

huitseeker Oct 25, 2023

huitseeker Oct 25, 2023

huitseeker Oct 25, 2023

huitseeker Oct 25, 2023

huitseeker Oct 25, 2023

huitseeker Oct 25, 2023

huitseeker Oct 25, 2023

huitseeker Oct 25, 2023

huitseeker left a comment

winston-h-zhang commented Feb 20, 2024

A cache for saving public params and commitment keys to local disk #79

A cache for saving public params and commitment keys to local disk #79

Conversation

winston-h-zhang commented Oct 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker left a 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

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

winston-h-zhang commented Feb 20, 2024