Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

winston-h-zhang
Copy link
Contributor

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.

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

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

src/traits/mod.rs Show resolved Hide resolved
src/constants.rs Outdated
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

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.

src/cache.rs Outdated

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

Choose a reason for hiding this comment

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

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?

src/cache.rs Outdated
Comment on lines 62 to 69
/// 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();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

src/cache.rs Outdated
let mut file = self.ck_file::<G>()?;

let ck_size = file.read_u64::<LittleEndian>()? as usize;
println!("ck_size: {}, n: {}", ck_size, n);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the log crate instead.

src/cache.rs Outdated
Comment on lines 125 to 127
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this unsafe section can be smaller.

Comment on lines +58 to +63
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

One way to improve this is to call us extend_aux_unsafe

Copy link
Contributor

@huitseeker huitseeker left a comment

Choose a reason for hiding this comment

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

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

@winston-h-zhang
Copy link
Contributor Author

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

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.

2 participants