Conversation
e9cb2a8 to
a285ecf
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate unresolved findings remain in the downloader, registry validation, and aggregator tooling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a feature-gated circuit verification key registry crate with retrieval, caching, signing, and aggregator management tools.
Changes:
- Extracts registry types, certifiers, retrievers, and bounded HTTP downloading.
- Adds aggregator export, whitelist, revoke, sign, and bootstrap commands.
- Updates documentation, workspace metadata, tests, and CI/release workflows.
File summaries
| File | Reviewed changes |
|---|---|
README.md |
Documents the new crate. |
mithril-common/src/test/double/mod.rs |
Removes the moved retriever double. |
mithril-common/src/crypto_helper/mod.rs |
Re-exports circuit digest types. |
mithril-common/src/crypto_helper/circuit_key_registry/mod.rs |
Removes the legacy registry module. |
mithril-common/src/certificate_chain/mod.rs |
Exposes the certifier trait. |
mithril-common/src/certificate_chain/circuit_verification_key_certifier.rs |
Defines the certifier interface. |
mithril-aggregator/src/tools/mod.rs |
Registers registry tools. |
mithril-aggregator/src/tools/circuit_key_registry.rs |
Implements registry operations. L203: Moderate, 3 votes — guard maximum-version increment overflow. L328: Moderate, 2 votes — apply inverted-range validation only to allowed entries. |
mithril-aggregator/src/commands/mod.rs |
Registers the registry command. |
mithril-aggregator/src/commands/circuit_key_registry_command.rs |
Implements CLI subcommands. |
mithril-aggregator/Cargo.toml |
Adds feature-gated crate wiring. |
Makefile |
Includes the new component. |
internal/mithril-circuit-key-registry/src/test/mod.rs |
Adds test utilities. |
internal/mithril-circuit-key-registry/src/test/double/mod.rs |
Exposes retriever test doubles. |
internal/mithril-circuit-key-registry/src/test/double/circuit_key_registry_retriever.rs |
L7: Nit, 1 vote — add the standard Dummy implementation. |
internal/mithril-circuit-key-registry/src/retriever.rs |
Adds file, HTTP, and unconfigured retrievers. |
internal/mithril-circuit-key-registry/src/registry.rs |
Defines registry format and verification. L161: Critical, 3 votes — validate duplicate digests or ensure revocation wins. |
internal/mithril-circuit-key-registry/src/lib.rs |
Defines crate modules and exports. |
internal/mithril-circuit-key-registry/src/http_downloader.rs |
Adds bounded HTTP downloading and retries. L83: Critical, 3 votes — stream bodies incrementally to enforce the size cap. L33: Moderate, 2 votes — propagate client construction errors instead of falling back to a client without the timeout. |
internal/mithril-circuit-key-registry/src/certifier.rs |
Adds verification and caching. |
internal/mithril-circuit-key-registry/README.md |
Documents the crate. |
internal/mithril-circuit-key-registry/Makefile |
Adds crate build and test targets. |
internal/mithril-circuit-key-registry/Cargo.toml |
Defines crate metadata and features. |
docs/website/root/manual/develop/nodes/mithril-aggregator.md |
Documents the CLI commands. |
Cargo.toml |
Adds the crate to the workspace. |
Cargo.lock |
Locks new dependencies. |
.github/workflows/test-rust.yml |
Tests the new crate. |
.github/workflows/release.yml |
Publishes the crate. |
.github/workflows/pre-release.yml |
Includes pre-release publishing. |
.github/workflows/nightly-dispatcher.yml |
Includes nightly publishing. |
.github/workflows/manual-publish-crates.yml |
Adds manual publication support. |
.github/workflows/ci.yml |
Updates publication and documentation coverage. |
Review details
Suppressed comments (2)
internal/mithril-circuit-key-registry/src/registry.rs:165
- This first-match lookup silently ignores later entries with the same digest. The single-entry invariant is checked only by the aggregator tools, while
SignedCircuitVerificationKeyRegistry::verifyaccepts a signed document with duplicates; an allowed first entry followed by a revoked entry would therefore certify the revoked key. Validate uniqueness when parsing/verifying, or retain revocation-wins behavior for malformed duplicates.
internal/mithril-circuit-key-registry/src/test/double/circuit_key_registry_retriever.rs:10 - The moved public fake remains a test double without implementing
mithril_common::test::double::Dummy, contrary to the repository's dummy test-double guideline. Add aDummyimplementation next to the type so shared tests can construct it through the standard test-double API.
- Files reviewed: 31/32 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Test Results 5 files ± 0 222 suites +1 59m 41s ⏱️ + 29m 29s Results for commit e00d62b. ± Comparison against base commit 928b3f6. This pull request removes 29 and adds 74 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
a285ecf to
bceb3fd
Compare
bceb3fd to
5fd7281
Compare
The crate holds the registry format, certifiers and retrievers, mithril-common keeps only the certifier trait used by the certificate verifier.
The genesis signature and the version rollback rejection of the cached certifier already cover the registry, a compiled floor would need a release per publication.
…verification key A revoked circuit verification key is rejected for every epoch, so the epoch a certificate claims cannot bypass a revocation.
The retriever downloads the signed registry from a URL with a bounded downloader, so a node refreshes the registry itself.
… older refresh A failed refresh, or one yielding a lower registry version, no longer fails the check: the previously verified registry is kept until the next refresh.
Export the circuit key digests, whitelist or revoke a key with a genesis signed registry update, sign a registry and bootstrap one for tests.
Generated with the hidden documentation subcommand of the aggregator.
5fd7281 to
e00d62b
Compare
Content
This PR includes the circuit verification key registry crate and the tools to manage a registry, prepared for its enforcement in #3514:
mithril-circuit-key-registryinternal crate, behind itsfuture_snarkfeature (registry format, certifiers and retrievers),mithril-commonkeeping only theCircuitVerificationKeyCertifiertrait used by the certificate verifierallowedover an epoch range orrevoked, a revoked key being rejected for every epochcircuit-key-registrycommand to the aggregator:exportthe circuit key digests for the network protocol parameters,whitelista key (creating the registry of a Mithril network when missing),revokean allowed key with its revocation epoch and comment,signa hand-authored registry, and the test-onlybootstrap, the registry being updated in place and genesis-signedPre-submit checklist
Issue(s)
Relates to #3148