refactor(stm): cache the circuit test fixtures on disk - #3481
Conversation
hjeljeli32
added
the
run-slow-tests
damrobi
left a comment
damrobi
left a comment
There was a problem hiding this comment.
Some none blocking comments but otherwise it looks good 👍
| if degree == shared_srs_degree { | ||
| universal_kzg_parameters.clone() | ||
| } else { | ||
| build_deterministic_params(degree) |
There was a problem hiding this comment.
Should we use the load_shared_unsafe_srs function here too?
| /// File holding the cached recursive verifying key inside its fingerprinted cache directory. | ||
| const RECURSIVE_VERIFYING_KEY_CACHE_FILE: &str = "recursive-verifying-key"; | ||
|
|
||
| /// Where the expensive recursive verifying key comes from. |
There was a problem hiding this comment.
Maybe add to the comment that Derived is only for asset generation (at least for now) and Cached is for general use in the test functions? I think it would make this easier to understand
| Cached, | ||
| } | ||
|
|
||
| /// Derives the recursive verifying key for the default IVC circuit shape (about 8.9 s). |
There was a problem hiding this comment.
Can you use an order of magnitude (seconds or tens of seconds) instead fixed timings? Same below with the ms
| unsafe_srs_seed: u64, | ||
| ) -> FileMutex { | ||
| FileMutex::for_shared_cache( | ||
| "ivc-recursive-verifying-key-v1", |
There was a problem hiding this comment.
Is there a reason to use the -v1 suffix here and in the function below?
| /// Reads `cache_file`, or builds the value and publishes it there on a miss. | ||
| /// | ||
| /// Rebuilds from exactly five conditions: an absent file, an unreadable one, a decode failure, bytes | ||
| /// that are not the canonical encoding of what they decode to (which covers truncation and trailing | ||
| /// bytes, since the verifying-key codec stops at the end of the key and ignores whatever follows), | ||
| /// and a rejection by `is_valid`. **It makes no wider claim: a canonically encoded value that | ||
| /// satisfies `is_valid` is trusted.** Canonical encoding shows the bytes are self-consistent, not | ||
| /// who wrote them, so an entry that is well-formed but semantically wrong is accepted — acceptable | ||
| /// only because this cache is disposable, is not exposed to a hostile writer, and is never read by | ||
| /// anything that produces committed assets. | ||
| /// | ||
| /// Rebuilding rather than reporting is deliberate, and unlike | ||
| /// [`KeyProvider`](crate::circuits::key_provider::KeyProvider), which propagates deserialization | ||
| /// errors: a spoiled test cache must not fail a test run. | ||
| /// | ||
| /// `is_valid` carries any invariant the bytes alone cannot express. Values whose encoding is | ||
| /// self-contained pass `|_| true`. |
There was a problem hiding this comment.
This comment is a bit long and hard to read, can you make it shorter? maybe with less details
| /// Asset generators must use this: they write committed assets, and a stale cached key would | ||
| /// silently produce assets derived from it. Behavior tests that only read should call | ||
| /// [`build_shared_recursive_context_from_cache`]. | ||
| pub(crate) fn build_shared_recursive_context( |
There was a problem hiding this comment.
Can you rename that function to make it explicit it always derives? something like build_shared_recursive_context_from_scratch
| /// The cache address folds in the freshly derived certificate verifying key, the committed | ||
| /// production recursive key, both circuit degrees, and the SRS seed, so a change to the certificate | ||
| /// circuit or a regenerated production key resolves to a different entry. **Never call this from an | ||
| /// asset generator** — see [`build_shared_recursive_context`]. |
There was a problem hiding this comment.
Maybe this can be an explanation of build_shared_recursive_context_with instead
| /// Every input that changes the derived keys is a parameter, so the address is a pure function of | ||
| /// them and a test can vary each one: the committed production verifying key as a circuit-version | ||
| /// salt, the protocol parameters, the Merkle-tree depth, the circuit degree, and the seed pinning | ||
| /// the unsafe SRS. Distinct configurations therefore never share a directory, which is what lets | ||
| /// [`KeyProvider`] be built with no expected verifying key. |
There was a problem hiding this comment.
I'm not sure this comment is necessary, the function reads well even without it
| /// The cache is shared across processes, unlike the in-process map this replaced, which amortized | ||
| /// nothing under the nextest process-per-test model. The lock is taken before the lookup so that | ||
| /// parallel processes racing a cold miss derive the pair once rather than once each — `KeyProvider` | ||
| /// does not serialize its writers. | ||
| fn get_or_build_circuit_keys( |
There was a problem hiding this comment.
Not sure this comment is needed
| /// one: the freshly derived certificate verifying key (which tracks the certificate circuit), the | ||
| /// committed production recursive key as a circuit-version salt, both circuit degrees, and the seed | ||
| /// pinning the unsafe SRS. | ||
| fn recursive_verifying_key_cache( |
There was a problem hiding this comment.
It looks like we have two caches for the recursive VK, is it possible to re-use the cache of IvcSnarkProverSetup::build_for_test_degree to not regenerate the VK if it exists?
2 participants
Content
This PR extends the content-keyed
FileMutexcache introduced by #3433 to both circuit test trees, which had none. Under the nextest process-per-test model an in-process cache amortizes nothing, so the SRS, the derived recursive verifying key, the certificate circuit keys and the deterministic 3000-signer fixture are now computed once per fingerprint and shared across test processes and runs.Measured locally:
circuits::halo2_ivcslow tier 435 s → 381 s,circuits::halo2slow tier 50 s → 44 s (−12 % each, ±8 s run to run). Numbers per ticket item are in #3462.Changes
halo2_ivc/tests/common/generators/setup.rs):build_shared_recursive_contextreads the degree-19 SRS from the same"unsafe-srs"entryIvcSnarkProverSetup::build_for_testwrites, both deriving from the same seed. A compile-time assertion tiesASSET_SEEDtoUNSAFE_SRS_SEED, so a change to either cannot silently alter the committed assets.halo2/tests/golden/helpers.rs): the process-localLazyLockmap is replaced by the on-diskKeyProvider, behind the cache lock so parallel processes racing a cold miss derive the pair once. The SRS asset filename now carries the seed that produced it, so a seed change cannot pair an old SRS with keys fingerprinted for a new one.AssetGenerationSetupis a pure function of them and is recomputed on both paths, so the two cannot disagree. Validated on load — vector lengths, plus the genesis signature verified against the message derived from the cached tree, which transitively covers tampering with the tree, the key or the signature.Pre-submit checklist
Issue(s)
Closes #3462