-
Notifications
You must be signed in to change notification settings - Fork 36
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
Conversation
@@ -0,0 +1,300 @@ | |||
//! This module provides caching utils |
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.
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
src/traits/circuit_supernova.rs
Outdated
@@ -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 |
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.
This feels like it's halfway towards a confirmation of Strehmel's results. Pray tell us more.
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. |
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.
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
245abc5
to
1a19c37
Compare
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.
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>>, |
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.
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
/// 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 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.
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); |
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.
Please use the log
crate instead.
src/cache.rs
Outdated
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 |
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.
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}, | ||
}; | ||
|
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.
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 { |
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.
I think this unsafe section can be smaller.
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); |
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.
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
@@ -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. |
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.
One way to improve this is to call us extend_aux_unsafe
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.
For emphasis, this is the important comment:
#79 (comment)
1a19c37
to
52e336f
Compare
Closed in favor of #335, this has bit-rot too much |
Addresses #67.
This PR is largely a port of the already existing infrastructure from
lurk-rs
. We implementCache
as an interface to read, write, and update public parameters and commitment keys as the user needs. We also declare a globalARECIBO_CACHE
for any downstreamarecibo
users to access, although users can also just declare a cache themselves.Group
trait by adding a way to extend commitment keys without having to recompute the entire string.CommitmentKeyTrait
to define the behavior of commitment keys.