Add dcap proof system
#153
Conversation
WalkthroughAdds DCAP proof-system handler and session-storage variants (DCAP/Circom/EZKL), removes the Jolt handler and aggregate_proofs requirement, centralizes circuit metadata parsing into a new module with TOML/JSON support, updates constants and pre-flight tooling (teeonnx, snarkjs), adds two deployment-layer models with TOML metadata, and ignores Changes
Sequence Diagram(s)DCAP Proof Generation and Verification FlowsequenceDiagram
participant Client
participant VerifiedModelSession
participant DCAPHandler
participant Docker_TEE
participant FileSystem
Client->>VerifiedModelSession: request proof generation
VerifiedModelSession->>DCAPHandler: gen_proof(session)
DCAPHandler->>FileSystem: write input JSON (gen_input_file)
DCAPHandler->>Docker_TEE: generate_witness(session) -> produce witness + quote
Docker_TEE->>FileSystem: write witness and quote
DCAPHandler->>FileSystem: run teeonnx to create proof, read proof
DCAPHandler-->>VerifiedModelSession: return proof and instances
Client->>VerifiedModelSession: request verification
VerifiedModelSession->>DCAPHandler: verify_proof(session, inputs, proof)
DCAPHandler->>FileSystem: persist proof, compute SHA256 hashes
DCAPHandler->>Docker_TEE: run teeonnx verify with hashes + expected MRENCLAVE
DCAPHandler-->>VerifiedModelSession: return verification result (bool)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)neurons/execution_layer/proof_handlers/dcap_handler.py✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
neurons/execution_layer/circuit.py (1)
412-417: Enum misuse will raise at runtime
self.metadata.proof_systemis now aProofSystemenum instance (converted inCircuitMetadata.from_file), yet you pass it toProofSystem[...]which expects the name of a member, not an enum value.
At runtime this line will raiseKeyError, preventing any circuit from loading.- self.proof_system = ProofSystem[self.metadata.proof_system] + self.proof_system = self.metadata.proof_system
🧹 Nitpick comments (6)
neurons/constants.py (1)
173-181: Consolidate external file requirements by roleCurrent lists diverge easily across the codebase. Suggest centralizing in a single mapping to improve maintainability and make lookups simpler.
You can add this mapping near these constants:
EXTERNAL_FILES_BY_ROLE = { Roles.MINER: ["circuit.zkey", "pk.key", "network.onnx"], Roles.VALIDATOR: ["circuit.zkey", "network.onnx"], }Then phase out direct use of MINER_EXTERNAL_FILES and VALIDATOR_EXTERNAL_FILES.
.gitignore (1)
198-198: Prefer a clearer pattern for ONNX filesUse a more conventional pattern.
*.onnxmatches in all directories;**/*.onnxis also clear. Either avoids ambiguity.Apply one of:
-**.onnx +*.onnxor
-**.onnx +**/*.onnxIf any ONNX files are intentionally tracked (e.g., small test fixtures), add exceptions like:
!path/to/fixture.onnxneurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.toml (1)
1-11: Fix description grammar & add checksum for external file integrityPlease update
neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.tomlas follows:
- Correct “advise” → “advice” in the description.
- Add an
[external_files_checksums]section with the SHA-256 of the remote ONNX to ensure integrity.Locations to update:
- Line with
description = "...advise..."- After the existing
[external_files]blockDiff:
-description = "A dishonest advisor which provides generic cryptocurrency advise (NFA)" +description = "A dishonest advisor which provides generic cryptocurrency advice (NFA)"Add immediately below:
[external_files_checksums] # SHA-256 of the file at the given URL network.onnx = "<SHA256_CHECKSUM>"Please compute the SHA-256 checksum of the ONNX file at
https://storage.omron.ai/815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/network.onnx
(e.g. viaopenssl dgst -sha256 network.onnx) and replace<SHA256_CHECKSUM>with the result.neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/metadata.toml (1)
1-11: Update description grammar and add external file checksumPlease correct the typo in the
descriptionfield and include a SHA-256 checksum for the remote ONNX file to ensure integrity:• File:
neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/metadata.toml
• Lines: 1–11-description = "An honest advisor which provides generic cryptocurrency advise (NFA)" +description = "An honest advisor which provides generic cryptocurrency advice (NFA)"Add the following section after
[external_files], replacing<sha256>with the actual checksum computed in your environment:[external_files_checksums] # sha256 of the file at the given URL network.onnx = "<sha256>"Compute the SHA-256 hash locally (e.g. with
sha256sum network.onnxor your preferred tool) and fill in the value above.neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/input.py (1)
41-44: Return-type mismatch invalidate
validate()is annotated to returnNoneyet returns aCircuitInputSchemainstance.
Either change the annotation toCircuitInputSchemaor explicitly discard the result:- def validate(data: dict[str, object]) -> None: - return CircuitInputSchema(**data) + def validate(data: dict[str, object]) -> CircuitInputSchema: + return CircuitInputSchema(**data)neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/input.py (1)
41-44: Return-type mismatch invalidateSame as the “Honest Advisor” input: adjust the return annotation or drop the return value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.gitignore(1 hunks)neurons/constants.py(1 hunks)neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/input.py(1 hunks)neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.toml(1 hunks)neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/input.py(1 hunks)neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/metadata.toml(1 hunks)neurons/execution_layer/circuit.py(6 hunks)neurons/execution_layer/proof_handlers/base_handler.py(0 hunks)neurons/execution_layer/proof_handlers/circom_handler.py(0 hunks)neurons/execution_layer/proof_handlers/dcap_handler.py(1 hunks)neurons/execution_layer/proof_handlers/ezkl_handler.py(0 hunks)neurons/execution_layer/proof_handlers/factory.py(1 hunks)neurons/execution_layer/proof_handlers/jolt_handler.py(0 hunks)neurons/execution_layer/session_storage.py(2 hunks)neurons/execution_layer/verified_model_session.py(2 hunks)neurons/utils/pre_flight.py(5 hunks)
💤 Files with no reviewable changes (4)
- neurons/execution_layer/proof_handlers/ezkl_handler.py
- neurons/execution_layer/proof_handlers/circom_handler.py
- neurons/execution_layer/proof_handlers/base_handler.py
- neurons/execution_layer/proof_handlers/jolt_handler.py
🧰 Additional context used
🪛 GitHub Check: Bandit
neurons/utils/pre_flight.py
[failure] 100-101:
subprocess call with shell=True identified, security issue.
[warning] 106-106:
Chmod setting a permissive mask 0o755 on file (LOCAL_TEEONNX_PATH).
🔇 Additional comments (2)
neurons/execution_layer/proof_handlers/factory.py (1)
4-12: DCAP handler successfully integrated in ProofSystemFactoryValidation confirms:
ProofSystem.DCAPis defined inneurons/execution_layer/circuit.pyDCAPHandlerclass exists inneurons/execution_layer/proof_handlers/dcap_handler.py- Factory mapping includes
ProofSystem.DCAP: DCAPHandleralongside the other handlersAll wiring matches the established pattern—no further changes needed.
neurons/execution_layer/circuit.py (1)
132-135: DCAP paths likely incompleteFor the
DCAPbranch onlycompiled_modelis updated;pkandvkremain set to CIRCOM defaults (circuit.zkey,verification_key.json).
Confirm if DCAP really has no proving/verification keys; if it does, add them here to avoid downstream file-not-found errors.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
neurons/execution_layer/proof_handlers/dcap_handler.py (2)
92-92: Hashing a dict causesTypeError– serialize first
158-164: Ternary returnsValueErrorinstance instead of raising
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
neurons/execution_layer/proof_handlers/dcap_handler.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
neurons/execution_layer/proof_handlers/dcap_handler.py (4)
neurons/execution_layer/proof_handlers/base_handler.py (1)
ProofSystemHandler(11-64)neurons/execution_layer/generic_input.py (1)
GenericInput(7-26)neurons/execution_layer/session_storage.py (2)
DCAPSessionStorage(68-86)witness_path(85-86)neurons/execution_layer/verified_model_session.py (5)
VerifiedModelSession(24-174)gen_input_file(66-70)gen_proof(72-94)generate_witness(151-156)verify_proof(121-137)
🪛 GitHub Check: CodeQL
neurons/execution_layer/proof_handlers/dcap_handler.py
[warning] 94-94: File is not always closed
File may not be closed if an exception is raised.
[warning] 97-97: File is not always closed
File may not be closed if an exception is raised.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL Analysis (python)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
neurons/utils/pre_flight.py (1)
100-104:subprocess.runwill fail – pass the command as a listWith
shell=False(the default), a single-stringargsis treated as the executable path, so the call currently raisesFileNotFoundError.
Switch to the vector form:- subprocess.run( - "wget https://github.com/zkonduit/teeonnx-p/releases/v23/download/teeonnx-zk-cpu-linux -O " - f"{LOCAL_TEEONNX_PATH}", - check=True, - ) + subprocess.run( + [ + "wget", + "https://github.com/zkonduit/teeonnx-p/releases/v23/download/teeonnx-zk-cpu-linux", + "-O", + LOCAL_TEEONNX_PATH, + ], + check=True, + )This eliminates the Bandit warning, avoids invoking a shell, and actually executes.
🧹 Nitpick comments (1)
neurons/utils/pre_flight.py (1)
16-24: Consolidate duplicatedconstantsimport
constantsis already imported on Line 8. Re-importing the same module adds nothing and slightly slows startup; merge these into the first import.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
neurons/utils/pre_flight.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
neurons/utils/pre_flight.py (1)
neurons/constants.py (1)
Roles(6-8)
🪛 GitHub Check: Semgrep OSS
neurons/utils/pre_flight.py
[warning] 105-105: Semgrep Finding: python.lang.security.audit.insecure-file-permissions.insecure-file-permissions
These permissions 0o700 are widely permissive and grant access to more people than may be necessary. A good default is 0o644 which gives read and write access to yourself and read access to everyone else.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL Analysis (python)
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
neurons/utils/pre_flight.py (1)
259-266: Critical: Add support for metadata.toml in pre-flight syncOur scan found two models in neurons/deployment_layer that ship only metadata.toml and would be skipped by the current JSON-only check. Please update the logic in neurons/utils/pre_flight.py (lines 259–266) to probe for both formats, preferring JSON when present:
- metadata_file = os.path.join(MODEL_DIR, model_hash, "metadata.json") - if not os.path.isfile(metadata_file): + metadata_json = os.path.join(MODEL_DIR, model_hash, "metadata.json") + metadata_toml = os.path.join(MODEL_DIR, model_hash, "metadata.toml") + # prefer JSON, but fall back to TOML if JSON is missing + metadata_file = metadata_json if os.path.isfile(metadata_json) else metadata_toml + if not os.path.isfile(metadata_file): bt.logging.error( - SYNC_LOG_PREFIX - + f"Metadata file not found at {metadata_file} for {model_hash}. Skipping sync for this model." + SYNC_LOG_PREFIX + + f"No metadata.json or metadata.toml found in {os.path.join(MODEL_DIR, model_hash)} for {model_hash}. Skipping sync." ) continueThis ensures both legacy JSON and newer TOML metadata are handled.
♻️ Duplicate comments (3)
neurons/utils/pre_flight.py (1)
157-162: Remove shell=True and avoid piping via shell for EZKL install.Shell piping is flagged by security scanners and is unnecessary here. Fetch the script and pass it to bash directly.
- # trunk-ignore(bandit/B605) - subprocess.run( - f"curl -s https://raw.githubusercontent.com/zkonduit/ezkl/main/install_ezkl_cli.sh | bash -s -- v{python_ezkl_version}", # noqa - shell=True, - check=True, - ) + # Download install script then execute with bash, avoiding shell=True and pipes + curl_res = subprocess.run( + ["curl", "-s", "https://raw.githubusercontent.com/zkonduit/ezkl/main/install_ezkl_cli.sh"], + check=True, + capture_output=True, + ) + subprocess.run( + ["bash", "-s", "--", f"v{python_ezkl_version}"], + input=curl_res.stdout, + check=True, + )neurons/execution_layer/proof_handlers/dcap_handler.py (2)
34-41: Validate presence of session.inputs.data before writing JSONGuard against None to avoid runtime errors when serializing inputs.
Apply this diff:
def gen_input_file(self, session: VerifiedModelSession): bt.logging.trace("Generating input file") - if session.inputs is None: - raise ValueError("Session inputs cannot be None when generating input file") + if session.inputs is None: + raise ValueError("Session inputs cannot be None when generating input file") + if getattr(session.inputs, "data", None) is None: + raise ValueError("Session inputs.data cannot be None when generating input file") os.makedirs(os.path.dirname(session.session_storage.input_path), exist_ok=True) with open(session.session_storage.input_path, "w", encoding="utf-8") as f: json.dump(session.inputs.data, f)
165-196: Harden docker invocation: cleanup and timeoutsAdd --rm to avoid container buildup and a timeout to prevent indefinite hangs. Also consider surfacing stderr on failure.
Apply this diff:
result = subprocess.run( [ "docker", "run", + "--rm", "--device", SGX_ENCLAVE_PATH, "--device", SGX_PROVISION_PATH, "-v", f"{session.session_storage.base_path}:{WORKSPACE_PATH}", SGX_IMAGE, "gen-output", "--input", os.path.join( WORKSPACE_PATH, os.path.basename(session.session_storage.input_path) ), "--model", os.path.join(WORKSPACE_PATH, f"network_{session.model_id}.onnx"), "--output", os.path.join( WORKSPACE_PATH, os.path.basename(session.session_storage.witness_path), ), "--quote", os.path.join( WORKSPACE_PATH, os.path.basename(session.session_storage.quote_path) ), ], check=True, capture_output=True, text=True, + timeout=600, )
🧹 Nitpick comments (12)
neurons/execution_layer/circuit_metadata.py (3)
63-73: Docstring mismatch: supports TOML and JSON, not just JSON.from_file handles both .json and .toml, but the docstring refers only to JSON and “ModelMetadata”. Update for accuracy and to reflect the class name.
- """ - Create a ModelMetadata instance from a JSON file. - - Args: - metadata_path (str): Path to the metadata JSON file. - - Returns: - ModelMetadata: An instance of ModelMetadata. - """ + """ + Load CircuitMetadata from a JSON or TOML file. + + Args: + metadata_path (str): Path to the metadata file (.json or .toml). + + Returns: + CircuitMetadata: An instance populated from the file. + """
75-81: Fail fast on unsupported extensions instead of loading empty metadata.Returning an empty dict on unknown extensions leads to a less clear TypeError later. Raise a ValueError immediately with guidance.
- with open(metadata_path, "r", encoding="utf-8") as f: - if metadata_path.endswith(".json"): + with open(metadata_path, "r", encoding="utf-8") as f: + if metadata_path.endswith(".json"): metadata = json.load(f) elif metadata_path.endswith(".toml"): metadata = toml.load(f) else: - metadata = {} + raise ValueError( + f"Unsupported metadata file extension for {metadata_path}. " + "Expected .json or .toml" + )
45-61: Consider making external_files optional with a default.If a metadata file omits external_files, from_file will raise. Making it optional reduces friction for simple circuits.
-from dataclasses import dataclass +from dataclasses import dataclass, field @@ - external_files: dict[str, str] + external_files: dict[str, str] = field(default_factory=dict)neurons/utils/pre_flight.py (1)
268-273: Use CircuitMetadata directly; avoid re-wrapping to dict.You can drop the intermediate dict and read external_files straight from the dataclass.
- circuit_metadata = CircuitMetadata.from_file(metadata_file) - metadata = {"external_files": circuit_metadata.external_files} + circuit_metadata = CircuitMetadata.from_file(metadata_file)And below:
- external_files = metadata.get("external_files", {}) + external_files = circuit_metadata.external_files or {}neurons/_validator/models/miner_response.py (1)
15-16: Import move to circuit_metadata LGTM; add DCAP-specific proof_size for better metricsShifting ProofSystem to the centralized metadata module is correct. One follow-up: DCAP proofs currently fall into the default branch for proof_size, which may skew evaluation metrics. Consider adding a DCAP branch that derives a stable size from the teeonnx proof payload.
Example (apply outside this hunk):
# Inside from_raw_response(), when computing proof_size: elif response.circuit.proof_system == ProofSystem.DCAP: try: # Prefer size of the 'proof' field if present; else length of serialized object payload = proof_content.get("proof", proof_content) proof_size = len(json.dumps(payload, separators=(",", ":"))) except Exception: proof_size = DEFAULT_PROOF_SIZEneurons/execution_layer/circuit.py (2)
94-96: DCAP compiled model path: add existence guard for clearer failuresPointing compiled_model to external network.onnx is correct for DCAP. Add a guard so missing models fail fast with a clear message (helps avoid late failures in handlers).
Apply this diff:
- elif proof_system == ProofSystem.DCAP: - self.compiled_model = os.path.join(self.external_base_path, "network.onnx") + elif proof_system == ProofSystem.DCAP: + self.compiled_model = os.path.join(self.external_base_path, "network.onnx") + if not os.path.exists(self.compiled_model): + logging.error( + f"DCAP compiled model not found at {self.compiled_model}. " + "Ensure the ONNX model is placed under the external models path." + ) + raise FileNotFoundError(self.compiled_model)
329-334: Mapping metadata.proof_system to enum: add defensive handling for invalid valuesAccessing ProofSystem[...] assumes exact enum name. Add a defensive guard to surface a helpful error if metadata is invalid or case-mismatched.
Apply this diff:
- self.proof_system = ProofSystem[self.metadata.proof_system] + try: + self.proof_system = ProofSystem[self.metadata.proof_system] + except KeyError as e: + raise ValueError( + f"Unsupported proof_system in metadata: {self.metadata.proof_system!r}" + ) from eneurons/execution_layer/proof_handlers/dcap_handler.py (1)
157-160: Return type annotation mismatchFunction may return str (stdout) or JSON content, but annotation is list | dict. Update for accuracy.
Apply this diff:
- def generate_witness( - self, session: VerifiedModelSession, return_content: bool = False - ) -> list | dict: + def generate_witness( + self, session: VerifiedModelSession, return_content: bool = False + ) -> str | list | dict:neurons/execution_layer/session_storage.py (4)
57-66: Make CircomSessionStorage a dataclass (or drop field()); current field() isn’t effectiveThe annotation
public_path: str = field(init=False)in a subclass that isn’t decorated with @DataClass is treated as a class attribute holding a dataclasses.Field, not a dataclass field. Instance behavior works because you assignself.public_pathin__post_init__, but this is misleading and breaks dataclass-based introspection.Apply this diff to make the intent explicit and correct:
+@dataclass class CircomSessionStorage(SessionStorage): public_path: str = field(init=False)Alternatively, if you prefer not to make the subclass a dataclass, remove field() usage:
-class CircomSessionStorage(SessionStorage): - public_path: str = field(init=False) +class CircomSessionStorage(SessionStorage): + public_path: str # set in __post_init__
68-89: Also decorate DCAPSessionStorage as a dataclass (field() otherwise becomes a class-level Field object)Similar to Circom,
output_path: str = field(init=False)andquote_path: str = field(init=False)in a non-dataclass subclass do not create dataclass fields. While instance assignment in__post_init__works, this is confusing and harms tooling/introspection.Apply this diff:
- class DCAPSessionStorage(SessionStorage): +@dataclass +class DCAPSessionStorage(SessionStorage): output_path: str = field(init=False) quote_path: str = field(init=False)
72-99: Optional: add a witness_path setter and call super().post_init() to DRY path initializationTo reduce duplication and stay in sync if the base changes how it computes
input_path/proof_path, you can implement a setter forwitness_paththat re-routes tooutput_path, then callsuper().__post_init__(). This keeps base logic centralized while honoring DCAP’s different witness/output semantics.Apply this diff:
- def __post_init__(self): - # Initialize base path directory (from parent's __post_init__) - if not os.path.exists(self.base_path): - os.makedirs(self.base_path) - # Initialize paths required by base class, excluding witness_path (handled by property) - self.input_path = os.path.join( - self.base_path, f"input_{self.model_id}_{self.session_uuid}.json" - ) - self.proof_path = os.path.join( - self.base_path, f"proof_{self.model_id}_{self.session_uuid}.json" - ) - # Initialize DCAP-specific paths - self.output_path = os.path.join( - self.base_path, f"output_{self.model_id}_{self.session_uuid}.json" - ) - self.quote_path = os.path.join( - self.base_path, f"quote_{self.model_id}_{self.session_uuid}.bin" - ) + def __post_init__(self): + # Use base initialization; witness_path assignment will trigger our setter below + super().__post_init__() + # Initialize DCAP-specific paths + self.quote_path = os.path.join( + self.base_path, f"quote_{self.model_id}_{self.session_uuid}.bin" + ) bt.logging.debug( f"DCAPSessionStorage initialized with model_id: {self.model_id} and session_uuid: {self.session_uuid}" ) bt.logging.trace(f"DCAPSessionStorage: Input path: {self.input_path}") - bt.logging.trace(f"DCAPSessionStorage: Output path: {self.output_path}") + bt.logging.trace(f"DCAPSessionStorage: Output path: {self.output_path}") bt.logging.trace(f"DCAPSessionStorage: Quote path: {self.quote_path}") bt.logging.trace(f"DCAPSessionStorage: Proof path: {self.proof_path}") bt.logging.trace(f"DCAPSessionStorage: Witness path: {self.witness_path}")And extend the property to include a setter:
@property def witness_path(self) -> str: return self.output_path + + @witness_path.setter + def witness_path(self, _value: str) -> None: + # Map witness_path to DCAP's output_path to keep base init DRY + self.output_path = os.path.join( + self.base_path, f"output_{self.model_id}_{self.session_uuid}.json" + )
111-129: String-to-enum coercion: trim whitespace for robustness; consider explicit typingMinor resilience improvement: when accepting strings for
proof_system, trim whitespace before uppercasing to avoid subtle input issues.Apply this diff:
- if isinstance(proof_system, str): - try: - proof_system = ProofSystem[proof_system.upper()] + if isinstance(proof_system, str): + try: + proof_system = ProofSystem[proof_system.strip().upper()]Optionally, update the type hint to reflect accepted inputs:
- Change the signature to:
def create_storage(cls, proof_system: "ProofSystem | str", model_id: str, session_uuid: str) -> SessionStorage:- Or import
Unionand annotate asUnion[ProofSystem, str].
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
neurons/_validator/core/request_pipeline.py(1 hunks)neurons/_validator/models/miner_response.py(1 hunks)neurons/_validator/models/poc_rpc_request.py(1 hunks)neurons/_validator/pow/proof_of_weights_handler.py(1 hunks)neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.toml(1 hunks)neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/metadata.toml(1 hunks)neurons/execution_layer/circuit.py(3 hunks)neurons/execution_layer/circuit_metadata.py(1 hunks)neurons/execution_layer/proof_handlers/dcap_handler.py(1 hunks)neurons/execution_layer/proof_handlers/factory.py(1 hunks)neurons/execution_layer/session_storage.py(2 hunks)neurons/incentive_mechanism.ipynb(3 hunks)neurons/protocol.py(1 hunks)neurons/utils/pre_flight.py(7 hunks)neurons/utils/system.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- neurons/incentive_mechanism.ipynb
- neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/metadata.toml
- neurons/execution_layer/proof_handlers/factory.py
🔇 Additional comments (14)
neurons/execution_layer/circuit_metadata.py (1)
31-35: Nice ergonomic enum conversion with missing.Case-insensitive string-to-enum conversion is clean and predictable. Good use of Enum.missing for UX.
neurons/_validator/models/poc_rpc_request.py (1)
4-4: Import source aligned with new centralized metadata module.Switching CircuitType import to execution_layer.circuit_metadata is consistent with the refactor. No further issues spotted.
neurons/utils/pre_flight.py (6)
15-24: Consolidated constants import looks good.Centralizing these constants reduces duplication and improves maintainability.
42-46: Docstring update is accurate and helpful.Calling out teeonnx and conditional Docker checks improves operator clarity.
57-61: Neat pattern: conditional checks via None sentinel.Using None to skip non-applicable checks keeps the loop simple and readable.
71-75: Graceful skip log is a nice touch.Clear runtime logging when a check is skipped due to role aids troubleshooting.
118-132: Docker check: robust and consistent.Covers both CalledProcessError and FileNotFoundError; concise and clear.
228-229: Early exit for SRS download on miner role is appropriate.Skipping SRS downloads for miners avoids unnecessary I/O. Good.
neurons/protocol.py (1)
7-7: Import aligned with new metadata module.Switching ProofSystem import to execution_layer.circuit_metadata matches the centralization across the codebase.
neurons/_validator/pow/proof_of_weights_handler.py (1)
3-4: Import source change LGTMImporting CircuitType from execution_layer.circuit_metadata aligns with the refactor. No functional concerns here.
neurons/_validator/core/request_pipeline.py (1)
21-23: Import split LGTMDecoupling CircuitType from Circuit via circuit_metadata is consistent with the new design. No behavioral changes introduced by this edit.
neurons/execution_layer/circuit.py (1)
67-71: Prefer TOML metadata with JSON fallback: good changePreferring metadata.toml while retaining a JSON fallback improves flexibility and keeps backward compatibility. Looks good.
neurons/execution_layer/session_storage.py (2)
51-55: LGTM: EZKL subclass initialization is correctDefers to the base paths and adds a targeted trace; no functional issues spotted.
72-101: Good fix: avoids the previous witness_path property assignment clashNot calling
super().__post_init__()here avoids assigning to the read-onlywitness_pathproperty. The explicit property mapping tooutput_pathis clear and resolves the previously reported AttributeError risk.
| model_dest = os.path.join( | ||
| session.session_storage.base_path, f"network_{session.model_id}.onnx" | ||
| ) | ||
| if not os.path.exists(model_dest): | ||
| shutil.copy2(session.model.paths.compiled_model, model_dest) | ||
| bt.logging.trace(f"Copied model to {model_dest}") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure source ONNX exists; fail fast with a clear message
Copying the compiled model assumes the source exists. Add an explicit check to surface helpful errors.
Apply this diff:
model_dest = os.path.join(
session.session_storage.base_path, f"network_{session.model_id}.onnx"
)
if not os.path.exists(model_dest):
- shutil.copy2(session.model.paths.compiled_model, model_dest)
+ if not os.path.exists(session.model.paths.compiled_model):
+ raise FileNotFoundError(
+ f"Compiled model not found: {session.model.paths.compiled_model}"
+ )
+ shutil.copy2(session.model.paths.compiled_model, model_dest)
bt.logging.trace(f"Copied model to {model_dest}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| model_dest = os.path.join( | |
| session.session_storage.base_path, f"network_{session.model_id}.onnx" | |
| ) | |
| if not os.path.exists(model_dest): | |
| shutil.copy2(session.model.paths.compiled_model, model_dest) | |
| bt.logging.trace(f"Copied model to {model_dest}") | |
| model_dest = os.path.join( | |
| session.session_storage.base_path, f"network_{session.model_id}.onnx" | |
| ) | |
| if not os.path.exists(model_dest): | |
| if not os.path.exists(session.model.paths.compiled_model): | |
| raise FileNotFoundError( | |
| f"Compiled model not found: {session.model.paths.compiled_model}" | |
| ) | |
| shutil.copy2(session.model.paths.compiled_model, model_dest) | |
| bt.logging.trace(f"Copied model to {model_dest}") |
🤖 Prompt for AI Agents
In neurons/execution_layer/proof_handlers/dcap_handler.py around lines 42 to 48,
the code copies session.model.paths.compiled_model to model_dest without
verifying the source exists; add an explicit existence check for
session.model.paths.compiled_model before calling shutil.copy2, and if the file
is missing log a clear error (including the source path and session/model
identifiers) and fail fast by raising a FileNotFoundError or otherwise aborting
the operation so the missing-source condition surfaces immediately.
| with open(session.session_storage.proof_path, "r", encoding="utf-8") as f: | ||
| try: | ||
| proof = json.load(f) | ||
| except json.JSONDecodeError: | ||
| bt.logging.error( | ||
| f"Failed to load proof from {session.session_storage.proof_path}" | ||
| ) | ||
| raise | ||
|
|
||
| return json.dumps(proof), json.dumps(proof["instances"]) | ||
|
|
There was a problem hiding this comment.
KeyError risk: proof['instances'] may be absent
Not all teeonnx proofs guarantee an "instances" key. Default gracefully to an empty list or an alternate field.
Apply this diff:
- with open(session.session_storage.proof_path, "r", encoding="utf-8") as f:
- try:
- proof = json.load(f)
- except json.JSONDecodeError:
- bt.logging.error(
- f"Failed to load proof from {session.session_storage.proof_path}"
- )
- raise
-
- return json.dumps(proof), json.dumps(proof["instances"])
+ with open(session.session_storage.proof_path, "r", encoding="utf-8") as f:
+ try:
+ proof = json.load(f)
+ except json.JSONDecodeError:
+ bt.logging.error(
+ f"Failed to load proof from {session.session_storage.proof_path}"
+ )
+ raise
+
+ instances = proof.get("instances") or proof.get("public_signals") or []
+ return json.dumps(proof, separators=(",", ":")), json.dumps(instances, separators=(",", ":"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with open(session.session_storage.proof_path, "r", encoding="utf-8") as f: | |
| try: | |
| proof = json.load(f) | |
| except json.JSONDecodeError: | |
| bt.logging.error( | |
| f"Failed to load proof from {session.session_storage.proof_path}" | |
| ) | |
| raise | |
| return json.dumps(proof), json.dumps(proof["instances"]) | |
| with open(session.session_storage.proof_path, "r", encoding="utf-8") as f: | |
| try: | |
| proof = json.load(f) | |
| except json.JSONDecodeError: | |
| bt.logging.error( | |
| f"Failed to load proof from {session.session_storage.proof_path}" | |
| ) | |
| raise | |
| instances = proof.get("instances") or proof.get("public_signals") or [] | |
| return json.dumps(proof, separators=(",", ":")), json.dumps(instances, separators=(",", ":")) |
🤖 Prompt for AI Agents
In neurons/execution_layer/proof_handlers/dcap_handler.py around lines 79 to 89,
the code assumes proof["instances"] exists which can raise KeyError for some
teeonnx proofs; change the return to safely access instances using
proof.get("instances", []) (or check for alternate field if applicable), and
ensure you JSON-dump that defaulted value (e.g.,
json.dumps(proof.get("instances", []))). Also consider logging a debug/info
message when the key is missing to aid diagnostics.
| def get_temp_folder() -> str: | ||
| if not os.path.exists(TEMP_FOLDER): | ||
| os.makedirs(TEMP_FOLDER, exist_ok=True) | ||
| os.makedirs(TEMP_FOLDER, mode=0o777, exist_ok=True) | ||
| return TEMP_FOLDER |
There was a problem hiding this comment.
Avoid world-writable temp dir; use 0o700 (or 0o1777 with sticky bit if multi-user sharing is required).
Creating TEMP_FOLDER with mode=0o777 is overly permissive and can enable unintended access or deletion by other users/processes. Prefer restrictive permissions by default.
Apply one of the following:
Option A (most secure, single-user):
- os.makedirs(TEMP_FOLDER, mode=0o777, exist_ok=True)
+ os.makedirs(TEMP_FOLDER, mode=0o700, exist_ok=True)Option B (multi-user shared, with sticky bit):
- os.makedirs(TEMP_FOLDER, mode=0o777, exist_ok=True)
+ os.makedirs(TEMP_FOLDER, mode=0o1777, exist_ok=True)Note: mode is subject to the process umask; set explicitly as needed for your deployment model.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_temp_folder() -> str: | |
| if not os.path.exists(TEMP_FOLDER): | |
| os.makedirs(TEMP_FOLDER, exist_ok=True) | |
| os.makedirs(TEMP_FOLDER, mode=0o777, exist_ok=True) | |
| return TEMP_FOLDER | |
| def get_temp_folder() -> str: | |
| if not os.path.exists(TEMP_FOLDER): | |
| os.makedirs(TEMP_FOLDER, mode=0o700, exist_ok=True) | |
| return TEMP_FOLDER |
| def get_temp_folder() -> str: | |
| if not os.path.exists(TEMP_FOLDER): | |
| os.makedirs(TEMP_FOLDER, exist_ok=True) | |
| os.makedirs(TEMP_FOLDER, mode=0o777, exist_ok=True) | |
| return TEMP_FOLDER | |
| def get_temp_folder() -> str: | |
| if not os.path.exists(TEMP_FOLDER): | |
| os.makedirs(TEMP_FOLDER, mode=0o1777, exist_ok=True) | |
| return TEMP_FOLDER |
🤖 Prompt for AI Agents
In neurons/utils/system.py around lines 98 to 101, the TEMP_FOLDER is created
with mode 0o777 which is too permissive; change it to a restrictive default (use
0o700 for single-user use) or to a shared-but-safe mode (0o1777 with the sticky
bit) depending on your deployment, and ensure the permission is enforced despite
the process umask by either setting the umask appropriately before mkdir or
calling os.chmod(TEMP_FOLDER, desired_mode) after creation; keep exist_ok=True
and preserve existing behavior when the folder already exists.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
neurons/execution_layer/proof_handlers/dcap_handler.py (6)
42-47: Fail fast if compiled model is missing before copy.Improve diagnostics when the source ONNX is absent rather than surfacing a generic FileNotFoundError from shutil.
if not os.path.exists(model_dest): - shutil.copy2(session.model.paths.compiled_model, model_dest) + if not os.path.exists(session.model.paths.compiled_model): + raise FileNotFoundError( + f"Compiled model not found: {session.model.paths.compiled_model}" + ) + shutil.copy2(session.model.paths.compiled_model, model_dest) bt.logging.trace(f"Copied model to {model_dest}")
79-89: Avoid KeyError on missing proof['instances']; return deterministic JSON.Instances may be absent or named differently (e.g., public_signals). Default gracefully and canonicalize JSON.
- return json.dumps(proof), json.dumps(proof["instances"]) + instances = proof.get("instances") or proof.get("public_signals") or [] + return json.dumps(proof, separators=(",", ":")), json.dumps( + instances, separators=(",", ":") + )
165-196: Harden docker invocation: add --rm and a timeout.Prevents container buildup and hangs.
[ "docker", "run", + "--rm", "--device", SGX_ENCLAVE_PATH, "--device", SGX_PROVISION_PATH, "-v", f"{session.session_storage.base_path}:{WORKSPACE_PATH}", SGX_IMAGE, "gen-output", @@ ], check=True, capture_output=True, text=True, + timeout=600, )
11-17: Fix incorrect import paths (missing 'neurons.' package prefix).These imports will raise ModuleNotFoundError when used outside the local package context. Use absolute package paths.
-from execution_layer.proof_handlers.base_handler import ProofSystemHandler -from execution_layer.generic_input import GenericInput -from constants import LOCAL_TEEONNX_PATH -from execution_layer.session_storage import DCAPSessionStorage +from neurons.execution_layer.proof_handlers.base_handler import ProofSystemHandler +from neurons.execution_layer.generic_input import GenericInput +from neurons.constants import LOCAL_TEEONNX_PATH +from neurons.execution_layer.session_storage import DCAPSessionStorage if TYPE_CHECKING: - from execution_layer.verified_model_session import VerifiedModelSession + from neurons.execution_layer.verified_model_session import VerifiedModelSession
36-41: Guard against None input data before writing input.json.If session.inputs.data is None, you'll write "null" and break downstream. Fail fast with a clear error.
if session.inputs is None: raise ValueError("Session inputs cannot be None when generating input file") + if session.inputs.data is None: + raise ValueError("Session inputs data cannot be None when generating input file") os.makedirs(os.path.dirname(session.session_storage.input_path), exist_ok=True) with open(session.session_storage.input_path, "w", encoding="utf-8") as f: json.dump(session.inputs.data, f)
112-115: Canonicalize validator input JSON before hashing; guard None.Using default separators introduces spaces and unstable hashes; also, hashing None-serialized "null" is dangerous.
- input_hash = hashlib.sha256( - json.dumps(validator_inputs.data, sort_keys=True).encode("utf-8") - ).hexdigest()[:64] + if validator_inputs.data is None: + raise ValueError("validator_inputs.data cannot be None for verification") + input_hash = hashlib.sha256( + json.dumps( + validator_inputs.data, sort_keys=True, separators=(",", ":") + ).encode("utf-8") + ).hexdigest()[:64]
🧹 Nitpick comments (2)
neurons/execution_layer/proof_handlers/dcap_handler.py (2)
49-49: Avoid logging full input payloads (possible PII leakage).Reduce verbosity to metadata (e.g., count) instead of full content.
- bt.logging.trace(f"Generated input.json with data: {session.inputs.data}") + bt.logging.trace( + f"Generated input.json (items={0 if session.inputs.data is None else len(session.inputs.data)})" + )
61-73: Add timeout to proof generation subprocess.Prevents indefinite hangs when teeonnx stalls.
result = subprocess.run( [ LOCAL_TEEONNX_PATH, "prove", "--quote", session.session_storage.quote_path, "--proof", session.session_storage.proof_path, ], check=True, capture_output=True, text=True, + timeout=300, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
neurons/execution_layer/proof_handlers/dcap_handler.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
neurons/execution_layer/proof_handlers/dcap_handler.py (4)
neurons/execution_layer/proof_handlers/base_handler.py (1)
ProofSystemHandler(11-64)neurons/execution_layer/generic_input.py (1)
GenericInput(7-26)neurons/execution_layer/session_storage.py (2)
DCAPSessionStorage(68-101)witness_path(100-101)neurons/execution_layer/verified_model_session.py (5)
VerifiedModelSession(24-182)gen_input_file(74-78)gen_proof(80-102)generate_witness(159-164)verify_proof(129-145)
🔇 Additional comments (1)
neurons/execution_layer/proof_handlers/dcap_handler.py (1)
116-129: LGTM: files are safely closed via context managers.Using with open(...) ensures file descriptors are closed even on exceptions, addressing prior “File is not always closed” warnings.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
neurons/utils/pre_flight.py (1)
88-116: Harden teeonnx installer: catch FileNotFoundError, restrict perms, and ensure executability
- Catch FileNotFoundError (wget missing).
- Use 0o750 to avoid world-executable, as flagged by security scanners.
- If the binary exists but is not executable, fix perms instead of early returning.
Apply this diff:
def ensure_teeonnx_installed(): @@ - try: - if os.path.exists(LOCAL_TEEONNX_PATH): - bt.logging.info("teeonnx is already installed") - return + try: + if os.path.exists(LOCAL_TEEONNX_PATH): + if os.access(LOCAL_TEEONNX_PATH, os.X_OK): + bt.logging.info("teeonnx is already installed and executable") + return + bt.logging.warning("teeonnx exists but is not executable; fixing permissions...") + os.chmod(LOCAL_TEEONNX_PATH, 0o750) + return @@ - # trunk-ignore(bandit/B605) subprocess.run( [ "wget", "https://github.com/zkonduit/teeonnx-p/releases/download/v23/teeonnx-zk-cpu-linux", "-O", - f"{LOCAL_TEEONNX_PATH}", + LOCAL_TEEONNX_PATH, ], check=True, ) - os.chmod(LOCAL_TEEONNX_PATH, 0o755) + os.chmod(LOCAL_TEEONNX_PATH, 0o750) bt.logging.info("teeonnx installed successfully") - except subprocess.CalledProcessError as e: + except (subprocess.CalledProcessError, FileNotFoundError) as e: bt.logging.error(f"Failed to install teeonnx: {e}") raise RuntimeError( "teeonnx installation failed. Please install it manually." ) from eOptional: remove the trunk-ignore since shell=True is no longer used here. Also consider replacing wget with a requests-based download and checksum verification to avoid relying on external tooling.
🧹 Nitpick comments (4)
neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.toml (1)
10-11: Optional: add integrity metadata for external fileIf your metadata model supports it, consider including a checksum (e.g., sha256) for network.onnx to harden download verification during sync.
neurons/utils/pre_flight.py (3)
72-75: Skip-log wording when role is NoneMinor: when role is None, the log reads “… for None”. Consider clarifying the message for readability.
Apply this diff to avoid printing “None”:
- if check_function is None: - bt.logging.info(f" PreFlight | Skipping {check_name} check for {role}") + if check_function is None: + suffix = f" for {role}" if role is not None else "" + bt.logging.info(f" PreFlight | Skipping {check_name} check{suffix}") continue
80-83: Improve exception logging: avoid passingeas a second arg; use exception() or exc_infoPassing
eas a second argument to logging is non-idiomatic and may format incorrectly. Use exception logging to capture stack trace once.Apply this diff:
- except Exception as e: - bt.logging.error(f"Failed {check_name.lower()}.", e) - bt.logging.debug(f" PreFlight | {check_name} error details: {str(e)}") - traceback.print_exc() - raise e + except Exception: + bt.logging.exception(f" PreFlight | {check_name} failed") + raise
262-275: Prefer TOML over JSON if both exist (align with new TOML-first workflow)Given TOML is the new canonical format, consider checking metadata.toml before metadata.json.
Apply this diff:
- for filename in ["metadata.json", "metadata.toml"]: + for filename in ["metadata.toml", "metadata.json"]: metadata_path = os.path.join(model_dir, filename) if os.path.isfile(metadata_path): try: circuit_metadata = CircuitMetadata.from_file(metadata_path) break except Exception as e:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.toml(1 hunks)neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/metadata.toml(1 hunks)neurons/execution_layer/proof_handlers/dcap_handler.py(1 hunks)neurons/utils/pre_flight.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/metadata.toml
- neurons/execution_layer/proof_handlers/dcap_handler.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
neurons/utils/pre_flight.py (2)
neurons/execution_layer/circuit_metadata.py (2)
CircuitMetadata(46-87)from_file(64-87)neurons/constants.py (1)
Roles(6-8)
🔇 Additional comments (4)
neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.toml (1)
5-6: Enums Verified: “DCAP” and “proof_of_computation” ExistBoth values are defined in the enums used by CircuitMetadata.from_file, so loading will succeed without a ValueError.
• ProofSystem enum (neurons/execution_layer/circuit_metadata.py):
– DCAP = "DCAP"
• CircuitType enum (neurons/execution_layer/circuit_metadata.py):
– PROOF_OF_COMPUTATION = "proof_of_computation"neurons/utils/pre_flight.py (3)
57-61: Good addition: role-conditional Docker check and teeonnx preflightThe preflight now conditionally verifies Docker for miners and includes a teeonnx check. This aligns with DCAP requirements.
118-131: LGTM on Docker verificationCovers both failure modes (non-zero exit and missing binary) and doesn’t over-promise an installation.
282-289: LGTM: role-scoped external file filteringThe selection logic cleanly separates downloads for validator vs miner roles.
...t_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.toml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
neurons/utils/pre_flight.py (1)
293-301: Prevent path traversal when writing external files derived from metadataKeys from
circuit_metadata.external_filesare used as relative paths. If a key includes “..” or absolute paths, the write could escape the model directory. You already haveis_safe_path; use it here.Apply this diff:
- file_path = os.path.join( - cli_parser.config.full_path_models, model_hash, key - ) - os.makedirs(os.path.dirname(file_path), exist_ok=True) + base_dir = os.path.join(cli_parser.config.full_path_models, model_hash) + file_path = os.path.normpath(os.path.join(base_dir, key)) + if not is_safe_path(base_dir, file_path): + bt.logging.error( + SYNC_LOG_PREFIX + + f"Refusing to write outside model dir for key '{key}' in {model_hash}" + ) + continue + os.makedirs(os.path.dirname(file_path), exist_ok=True) @@ - try: + try: with requests.get( url, timeout=FIVE_MINUTES * 2, stream=True ) as response: response.raise_for_status() with open(file_path, "wb") as f: for chunk in response.iter_content(chunk_size=8192): f.write(chunk)Optional hardening:
- Download to a temp file (same directory) and
os.replaceto avoid partial files if interrupted.- Enforce an allowlist of expected keys per role beyond the existing filter to prevent unexpected file placement even within the model dir.
Also applies to: 303-317
♻️ Duplicate comments (1)
neurons/utils/pre_flight.py (1)
88-117: Tighten file permissions and broaden exception handling in teeonnx installer
- 0o755 is flagged as overly permissive. Use 0o750 (or 0o700) to avoid world-executable/readable.
- If wget is missing,
subprocess.runraises FileNotFoundError; catch it to surface the intended RuntimeError.- Only chmod when needed; if the file exists and is executable, return early.
Apply this diff:
def ensure_teeonnx_installed(): @@ - try: - if os.path.exists(LOCAL_TEEONNX_PATH): - os.chmod(LOCAL_TEEONNX_PATH, 0o755) - bt.logging.info("teeonnx is already installed") - return + try: + if os.path.exists(LOCAL_TEEONNX_PATH): + if os.access(LOCAL_TEEONNX_PATH, os.X_OK): + bt.logging.info("teeonnx is already installed and executable") + return + bt.logging.warning("teeonnx exists but is not executable; fixing permissions...") + os.chmod(LOCAL_TEEONNX_PATH, 0o750) + return @@ - subprocess.run( + subprocess.run( [ "wget", "https://github.com/zkonduit/teeonnx-p/releases/download/v23/teeonnx-zk-cpu-linux", "-O", - f"{LOCAL_TEEONNX_PATH}", + LOCAL_TEEONNX_PATH, ], check=True, ) - os.chmod(LOCAL_TEEONNX_PATH, 0o755) + os.chmod(LOCAL_TEEONNX_PATH, 0o750) bt.logging.info("teeonnx installed successfully") - except subprocess.CalledProcessError as e: + except (subprocess.CalledProcessError, FileNotFoundError) as e: bt.logging.error(f"Failed to install teeonnx: {e}") raise RuntimeError( "teeonnx installation failed. Please install it manually." ) from eOptional hardening:
- Verify checksum/signature of the downloaded binary before chmod.
- Avoid external dependency on wget by streaming via requests and setting mode bits after an atomic rename.
🧹 Nitpick comments (4)
neurons/utils/pre_flight.py (4)
15-24: Import paths appear package-relative; verify they resolve in production and prefer explicit package-qualified importsIf this module is executed within the neurons package, absolute imports like
from constants import ...andfrom execution_layer...may depend onsys.pathside-effects. To avoid import ambiguity and improve maintainability, prefer package-qualified imports.Apply this diff locally within the selected range:
-from constants import ( +from neurons.constants import ( IGNORED_MODEL_HASHES, LOCAL_EZKL_PATH, LOCAL_TEEONNX_PATH, LOCAL_SNARKJS_PATH, LOCAL_SNARKJS_INSTALL_DIR, MINER_EXTERNAL_FILES, VALIDATOR_EXTERNAL_FILES, ) -from execution_layer.circuit_metadata import CircuitMetadata +from neurons.execution_layer.circuit_metadata import CircuitMetadataAdditionally update the earlier import (outside the selected range) for consistency:
# elsewhere in this file (Line 7) from neurons.constants import FIVE_MINUTES, Roles
42-46: Preflight gating additions read well and are safe to skip dynamicallyDoc updates and conditional inclusion of Docker/teeonnx checks look good. Skipping None-valued checks keeps the loop simple. Minor nit: when role is None, the skip log prints “for None,” which is fine but you could omit the suffix for cleaner logs.
Also applies to: 57-61, 72-75
119-133: Nit: error message says “install/verify” but the function only verifiesThe function doesn’t install Docker; reword the error to avoid confusion.
- except (subprocess.CalledProcessError, FileNotFoundError) as e: - bt.logging.error(f"Failed to install/verify Docker: {e}") - raise RuntimeError( - "Docker installation failed. Please install it manually." - ) from e + except (subprocess.CalledProcessError, FileNotFoundError) as e: + bt.logging.error(f"Failed to verify Docker availability: {e}") + raise RuntimeError( + "Docker not available. Please install Docker and retry." + ) from e
229-231: Minor structure nit: check miner role before entering the loopBreaking on the first iteration works but doing the role check before the loop avoids unnecessary loop setup and a branch per-iteration.
Example refactor (outside this range):
loop = asyncio.get_event_loop() if role != Roles.MINER: for logrows in range(1, 26): ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
neurons/utils/pre_flight.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
neurons/utils/pre_flight.py (2)
neurons/execution_layer/circuit_metadata.py (2)
CircuitMetadata(46-87)from_file(64-87)neurons/constants.py (1)
Roles(6-8)
🪛 GitHub Check: CodeQL
neurons/utils/pre_flight.py
[failure] 94-94: Overly permissive file permissions
Overly permissive mask in chmod sets file to world readable.
🪛 GitHub Check: Semgrep OSS
neurons/utils/pre_flight.py
[warning] 94-94: Semgrep Finding: python.lang.security.audit.insecure-file-permissions.insecure-file-permissions
These permissions 0o755 are widely permissive and grant access to more people than may be necessary. A good default is 0o644 which gives read and write access to yourself and read access to everyone else.
🪛 GitHub Check: Bandit
neurons/utils/pre_flight.py
[warning] 94-94:
Chmod setting a permissive mask 0o755 on file (LOCAL_TEEONNX_PATH).
🔇 Additional comments (1)
neurons/utils/pre_flight.py (1)
260-275: LGTM: TOML/JSON metadata resolution with graceful error handlingFalling back between metadata.json and metadata.toml and logging parse failures per-file is a good DX improvement.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
neurons/utils/pre_flight.py (2)
290-303: Sanitize metadata-derived filenames to prevent path traversal; enforce base dirKeys from
external_filesare used directly inos.path.join. A malicious or malformed key containing path separators (e.g., "../../…") could escape the intended directory.Apply this diff:
- file_path = os.path.join( - cli_parser.config.full_path_models, model_hash, key - ) - os.makedirs(os.path.dirname(file_path), exist_ok=True) + base_dir = os.path.join(cli_parser.config.full_path_models, model_hash) + safe_key = os.path.basename(key) + file_path = os.path.join(base_dir, safe_key) + # Enforce staying within base_dir + if not is_safe_path(base_dir, file_path): + bt.logging.error( + SYNC_LOG_PREFIX + + f"Refusing to write outside model directory for key={key}" + ) + continue + os.makedirs(base_dir, exist_ok=True)
310-323: Avoid corrupt partial downloads: write to a temp file and atomic-rename on successIf the process crashes mid-download, the next run will see the file present and skip re-downloading, leaving a corrupted file. Use a
.partfile andos.replaceto commit atomically.Apply this diff:
- try: - with requests.get( - url, timeout=FIVE_MINUTES * 2, stream=True - ) as response: - response.raise_for_status() - with open(file_path, "wb") as f: - for chunk in response.iter_content(chunk_size=8192): - f.write(chunk) - except requests.RequestException as e: - bt.logging.error( - SYNC_LOG_PREFIX + f"Failed to download {url} to {file_path}: {e}" - ) - continue + try: + tmp_path = f"{file_path}.part" + with requests.get(url, timeout=FIVE_MINUTES * 2, stream=True) as response: + response.raise_for_status() + with open(tmp_path, "wb") as f: + for chunk in response.iter_content(chunk_size=8192): + if chunk: + f.write(chunk) + os.replace(tmp_path, file_path) + except requests.RequestException as e: + # Cleanup temp file on failure + try: + if os.path.exists(tmp_path): + os.remove(tmp_path) + except OSError: + pass + bt.logging.error( + SYNC_LOG_PREFIX + f"Failed to download {url} to {file_path}: {e}" + ) + continueOptional hardening:
- Validate URL scheme is http(s) before requesting.
- Verify checksum if provided in metadata.
♻️ Duplicate comments (6)
neurons/execution_layer/proof_handlers/dcap_handler.py (5)
50-56: Fail fast if the compiled model is missing; improve error messageCopying without checking the source will raise a less-informative error. Add an explicit existence check and raise a clear exception.
model_dest = os.path.join( session.session_storage.base_path, f"network_{session.model.id}.onnx" ) if not os.path.exists(model_dest): - shutil.copy2(session.model.paths.compiled_model, model_dest) + if not os.path.exists(session.model.paths.compiled_model): + raise FileNotFoundError( + f"Compiled model not found: {session.model.paths.compiled_model}" + ) + shutil.copy2(session.model.paths.compiled_model, model_dest) bt.logging.trace(f"Copied model to {model_dest}")
126-139: LGTM: files are safely closed via context managersThese
with open(...):blocks address prior “file not always closed” scanner findings; files are closed even if hashing raises.
11-17: Fix import paths to the neurons package to prevent ModuleNotFoundErrorThese absolute imports miss the
neurons.package prefix (and one import should come fromneurons.constants). This will break module resolution outside this module’s directory.Apply:
-from execution_layer.proof_handlers.base_handler import ProofSystemHandler -from execution_layer.generic_input import GenericInput -from constants import LOCAL_TEEONNX_PATH -from execution_layer.session_storage import DCAPSessionStorage +from neurons.execution_layer.proof_handlers.base_handler import ProofSystemHandler +from neurons.execution_layer.generic_input import GenericInput +from neurons.constants import LOCAL_TEEONNX_PATH +from neurons.execution_layer.session_storage import DCAPSessionStorageAnd under TYPE_CHECKING:
-if TYPE_CHECKING: - from execution_layer.verified_model_session import VerifiedModelSession +if TYPE_CHECKING: + from neurons.execution_layer.verified_model_session import VerifiedModelSession
34-49: Guard against missing input data before building teeonnx payload
session.inputscan be set whilesession.inputs.datais None, which will raise when calling.get(...). Fail fast with a clear error and serialize deterministically.def gen_input_file(self, session: VerifiedModelSession): bt.logging.trace("Generating input file") if session.inputs is None: raise ValueError("Session inputs cannot be None when generating input file") os.makedirs(os.path.dirname(session.session_storage.input_path), exist_ok=True) - input_data = session.inputs.data + if session.inputs.data is None: + raise ValueError("Session inputs data is required to generate input file") + input_data = session.inputs.data teeonnx_format = { "data": input_data.get("input_ids", input_data.get("data", [])), "shapes": input_data.get("shapes", []), "variables": input_data.get("variables", []), } - with open(session.session_storage.input_path, "w", encoding="utf-8") as f: - json.dump(teeonnx_format, f) + with open(session.session_storage.input_path, "w", encoding="utf-8") as f: + json.dump(teeonnx_format, f, separators=(",", ":"))
87-96: Prevent KeyError when ‘instances’ is absent; canonicalize output JSONSome proofs may not include an “instances” field. Default gracefully and use canonical separators for consistency.
- return json.dumps(proof), json.dumps(proof["instances"]) + instances = proof.get("instances") or proof.get("public_signals") or [] + return json.dumps(proof, separators=(",", ":")), json.dumps( + instances, separators=(",", ":") + )neurons/utils/pre_flight.py (1)
101-101: Semgrep warning on 0o700 is a false positive for executables; consider 0o500 or document the choice0o700 is owner-only read/write/execute and is more restrictive than 0o644. For binaries, execute permission is required. If the binary doesn’t need to be owner-writable post-install, 0o500 is even tighter; if multiple users must execute it, prefer 0o750.
Optional tweak:
- os.chmod(LOCAL_TEEONNX_PATH, 0o700) + os.chmod(LOCAL_TEEONNX_PATH, 0o500) # owner read/execute onlyAlternatively, add an inline comment to justify the permission to appease scanners.
Also applies to: 116-116
🧹 Nitpick comments (4)
neurons/execution_layer/proof_handlers/dcap_handler.py (2)
140-160: Prefer exit code over parsing stdout to decide verification result; log both streamsBecause
check=Truealready throws on verification failure, you can return True on success and avoid brittle string matching. Also log both stdout and stderr for diagnostics.result = subprocess.run( [ LOCAL_TEEONNX_PATH, "verify", session.session_storage.proof_path, "--input-hash", input_hash, "--model-hash", model_hash, "--output-hash", witness_hash, "--mrenclave", EXPECTED_MRENCLAVE, ], check=True, capture_output=True, text=True, timeout=60, ) - return "verified: true" in result.stdout + bt.logging.debug( + f"teeonnx verify stdout: {result.stdout.strip()} | stderr: {result.stderr.strip()}" + ) + return True
167-169: Parameterreturn_contentis ignored; align behavior or docstringThis method always returns the witness JSON regardless of
return_content. Either honor the flag or adjust the docstring to reflect current behavior to avoid confusion.Also applies to: 211-227
neurons/utils/pre_flight.py (2)
57-61: Gate teeonnx installation to when DCAP circuits are presentInstalling teeonnx unconditionally increases surface and build time. Consider enabling this check only if the configured circuits include proof_system == DCAP.
Example update within preflight (conceptual):
- "Checking teeonnx installation": ensure_teeonnx_installed, + "Checking teeonnx installation": ( + ensure_teeonnx_installed if has_dcap_circuits() else None + ),Helper (outside this file’s changed lines): scan deployment_layer/*/metadata.(json|toml) for proof_system == "DCAP".
106-117: Use requests + atomic write instead of external wget; broaden exceptionsRemoving the wget dependency improves portability, reduces supply-chain surface, and lets you write atomically to avoid partially-written binaries if the download is interrupted.
Apply this diff:
- # trunk-ignore(bandit/B605) - subprocess.run( - [ - "wget", - "https://github.com/zkonduit/teeonnx-p/releases/download/v23/teeonnx-zk-cpu-linux", - "-O", - LOCAL_TEEONNX_PATH, - ], - check=True, - ) - os.chmod(LOCAL_TEEONNX_PATH, 0o700) - bt.logging.info("teeonnx installed successfully") - except (subprocess.CalledProcessError, FileNotFoundError) as e: + tmp_path = f"{LOCAL_TEEONNX_PATH}.part" + with requests.get( + "https://github.com/zkonduit/teeonnx-p/releases/download/v23/teeonnx-zk-cpu-linux", + timeout=FIVE_MINUTES * 2, + stream=True, + ) as resp: + resp.raise_for_status() + with open(tmp_path, "wb") as f: + for chunk in resp.iter_content(chunk_size=8192): + if chunk: + f.write(chunk) + os.chmod(tmp_path, 0o700) + os.replace(tmp_path, LOCAL_TEEONNX_PATH) + bt.logging.info("teeonnx installed successfully") + except (requests.RequestException, OSError) as e: bt.logging.error(f"Failed to install teeonnx: {e}") raise RuntimeError( "teeonnx installation failed. Please install it manually." ) from eOutside this range: add a cleanup guard if desired in case of exceptions before
os.replace:# ensure_teeonnx_installed(): before raising, attempt to remove tmp_path if it exists try: if os.path.exists(tmp_path): os.remove(tmp_path) except OSError: passAlso applies to: 118-123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.toml(1 hunks)neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/metadata.toml(1 hunks)neurons/execution_layer/proof_handlers/dcap_handler.py(1 hunks)neurons/utils/pre_flight.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- neurons/deployment_layer/model_c2493244e153e8dcb2bfa6e0ebb411b76069074041d95718c47a36f4ec637237/metadata.toml
- neurons/deployment_layer/model_815d49886a610c17e34204b1c544c363bb06328a6cb126181bd0b6e77164cd4b/metadata.toml
🧰 Additional context used
🧬 Code Graph Analysis (2)
neurons/execution_layer/proof_handlers/dcap_handler.py (4)
neurons/execution_layer/proof_handlers/base_handler.py (1)
ProofSystemHandler(11-64)neurons/execution_layer/generic_input.py (1)
GenericInput(7-26)neurons/execution_layer/session_storage.py (2)
DCAPSessionStorage(68-101)witness_path(100-101)neurons/execution_layer/verified_model_session.py (5)
VerifiedModelSession(24-182)gen_input_file(74-78)gen_proof(80-102)generate_witness(159-164)verify_proof(129-145)
neurons/utils/pre_flight.py (2)
neurons/execution_layer/circuit_metadata.py (2)
CircuitMetadata(46-87)from_file(64-87)neurons/constants.py (1)
Roles(6-8)
🪛 GitHub Check: Semgrep OSS
neurons/utils/pre_flight.py
[warning] 101-101: Semgrep Finding: python.lang.security.audit.insecure-file-permissions.insecure-file-permissions
These permissions 0o700 are widely permissive and grant access to more people than may be necessary. A good default is 0o644 which gives read and write access to yourself and read access to everyone else.
[warning] 116-116: Semgrep Finding: python.lang.security.audit.insecure-file-permissions.insecure-file-permissions
These permissions 0o700 are widely permissive and grant access to more people than may be necessary. A good default is 0o644 which gives read and write access to yourself and read access to everyone else.
🔇 Additional comments (2)
neurons/execution_layer/proof_handlers/dcap_handler.py (1)
120-125: LGTM: canonical, stable hashing of validator inputsUsing sort_keys with compact separators and encoding to bytes correctly stabilizes the input hash computation.
neurons/utils/pre_flight.py (1)
72-74: LGTM: clean conditional skipping patternSkipping optional checks by storing
Nonein the map and short-circuiting here is clear and maintainable.
| try: | ||
| if os.path.exists(LOCAL_TEEONNX_PATH): | ||
| if os.access(LOCAL_TEEONNX_PATH, os.X_OK): | ||
| bt.logging.info("teeonnx is already installed and executable") | ||
| return | ||
| else: | ||
| bt.logging.warning( | ||
| "teeonnx exists but is not executable; fixing permissions..." | ||
| ) | ||
| os.chmod(LOCAL_TEEONNX_PATH, 0o700) | ||
| return | ||
|
|
There was a problem hiding this comment.
Don’t trust an existing teeonnx file solely based on executability; verify it runs
If a previous download created a truncated/non-binary file (e.g., network error) without the exec bit, this branch will chmod it and return, leaving a corrupt binary in place.
Apply this diff to verify the binary executes and re-install if it doesn’t:
- try:
- if os.path.exists(LOCAL_TEEONNX_PATH):
- if os.access(LOCAL_TEEONNX_PATH, os.X_OK):
- bt.logging.info("teeonnx is already installed and executable")
- return
- else:
- bt.logging.warning(
- "teeonnx exists but is not executable; fixing permissions..."
- )
- os.chmod(LOCAL_TEEONNX_PATH, 0o700)
- return
+ try:
+ if os.path.exists(LOCAL_TEEONNX_PATH):
+ if not os.access(LOCAL_TEEONNX_PATH, os.X_OK):
+ bt.logging.warning(
+ "teeonnx exists but is not executable; fixing permissions..."
+ )
+ os.chmod(LOCAL_TEEONNX_PATH, 0o700)
+ # Verify the binary actually runs; if it fails, fall through and re-install
+ try:
+ subprocess.run(
+ [LOCAL_TEEONNX_PATH, "--help"],
+ check=True,
+ capture_output=True,
+ text=True,
+ )
+ bt.logging.info("teeonnx is already installed and executable")
+ return
+ except (subprocess.CalledProcessError, FileNotFoundError, OSError):
+ bt.logging.warning(
+ "Existing teeonnx binary failed to execute; re-installing..."
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| if os.path.exists(LOCAL_TEEONNX_PATH): | |
| if os.access(LOCAL_TEEONNX_PATH, os.X_OK): | |
| bt.logging.info("teeonnx is already installed and executable") | |
| return | |
| else: | |
| bt.logging.warning( | |
| "teeonnx exists but is not executable; fixing permissions..." | |
| ) | |
| os.chmod(LOCAL_TEEONNX_PATH, 0o700) | |
| return | |
| try: | |
| if os.path.exists(LOCAL_TEEONNX_PATH): | |
| if not os.access(LOCAL_TEEONNX_PATH, os.X_OK): | |
| bt.logging.warning( | |
| "teeonnx exists but is not executable; fixing permissions..." | |
| ) | |
| os.chmod(LOCAL_TEEONNX_PATH, 0o700) | |
| # Verify the binary actually runs; if it fails, fall through and re-install | |
| try: | |
| subprocess.run( | |
| [LOCAL_TEEONNX_PATH, "--help"], | |
| check=True, | |
| capture_output=True, | |
| text=True, | |
| ) | |
| bt.logging.info("teeonnx is already installed and executable") | |
| return | |
| except (subprocess.CalledProcessError, FileNotFoundError, OSError): | |
| bt.logging.warning( | |
| "Existing teeonnx binary failed to execute; re-installing..." | |
| ) | |
| # …rest of installation logic continues here… |
🧰 Tools
🪛 GitHub Check: Semgrep OSS
[warning] 101-101: Semgrep Finding: python.lang.security.audit.insecure-file-permissions.insecure-file-permissions
These permissions 0o700 are widely permissive and grant access to more people than may be necessary. A good default is 0o644 which gives read and write access to yourself and read access to everyone else.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
neurons/execution_layer/proof_handlers/dcap_handler.py (4)
68-71: Fail fast if compiled model is missing before copyingCopying without verifying the source path leads to a confusing FileNotFoundError later.
Apply this diff:
- if not os.path.exists(model_dest): - shutil.copy2(session.model.paths.compiled_model, model_dest) - bt.logging.trace(f"Copied model to {model_dest}") + if not os.path.exists(model_dest): + if not os.path.exists(session.model.paths.compiled_model): + raise FileNotFoundError( + f"Compiled model not found: {session.model.paths.compiled_model}" + ) + shutil.copy2(session.model.paths.compiled_model, model_dest) + bt.logging.trace(f"Copied model to {model_dest}")
11-15: Fix import paths to use the package namespace (neurons.…)These imports will raise ModuleNotFoundError when used outside the package root. Use fully-qualified package paths.
Apply this diff:
-from execution_layer.proof_handlers.base_handler import ProofSystemHandler -from execution_layer.generic_input import GenericInput -from constants import LOCAL_TEEONNX_PATH -from execution_layer.session_storage import DCAPSessionStorage +from neurons.execution_layer.proof_handlers.base_handler import ProofSystemHandler +from neurons.execution_layer.generic_input import GenericInput +from constants import LOCAL_TEEONNX_PATH +from neurons.execution_layer.session_storage import DCAPSessionStorageAnd for TYPE_CHECKING:
-from execution_layer.verified_model_session import VerifiedModelSession +from neurons.execution_layer.verified_model_session import VerifiedModelSessionNote: If
constants.pyalso lives underneurons/, adjust accordingly to:
from neurons.constants import LOCAL_TEEONNX_PATH.Also applies to: 16-17
51-56: Guard against missingsession.inputs.datato avoidAttributeErrorAccessing
.getonNonewill crash wheninputs.datais None.Apply this diff:
- if session.inputs is None: - raise ValueError("Session inputs cannot be None when generating input file") + if session.inputs is None or session.inputs.data is None: + raise ValueError("Session inputs are required to generate input file")
102-112: Avoid KeyError when accessingproof["instances"]and canonicalize JSONSome proofs may not include “instances”. Default gracefully and use compact separators.
Apply this diff:
- return json.dumps(proof), json.dumps(proof["instances"]) + instances = proof.get("instances") or proof.get("public_signals") or [] + return json.dumps(proof, separators=(",", ":")), json.dumps(instances, separators=(",", ":"))
🧹 Nitpick comments (2)
neurons/execution_layer/proof_handlers/dcap_handler.py (2)
84-97: Add a timeout toteeonnx proveto prevent indefinite hangsThe verify step has a timeout; align prove with a reasonable timeout.
Apply this diff:
result = subprocess.run( [ LOCAL_TEEONNX_PATH, "prove", "--quote", session.session_storage.quote_path, "--proof", session.session_storage.proof_path, ], check=True, capture_output=True, text=True, + timeout=600, )
182-185: Nit:return_contentparameter is unusedThe method always returns parsed JSON regardless of the flag. Either document this behavior or wire the flag to skip file/stdout parsing when False.
Would you like me to update the docstrings and interface comments to clarify this?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
neurons/execution_layer/proof_handlers/dcap_handler.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
neurons/execution_layer/proof_handlers/dcap_handler.py (4)
neurons/execution_layer/proof_handlers/base_handler.py (1)
ProofSystemHandler(11-64)neurons/execution_layer/generic_input.py (1)
GenericInput(7-26)neurons/execution_layer/session_storage.py (2)
DCAPSessionStorage(68-101)witness_path(100-101)neurons/execution_layer/verified_model_session.py (5)
VerifiedModelSession(24-182)gen_input_file(74-78)gen_proof(80-102)generate_witness(159-164)verify_proof(129-145)
🔇 Additional comments (4)
neurons/execution_layer/proof_handlers/dcap_handler.py (4)
135-139: LGTM: canonical, deterministic input hashingUsing sorted keys and compact separators stabilizes the hash across environments.
28-41: LGTM: conditional sudo and SGX device access checks are sensibleNice small helpers to decide when sudo is needed; avoids unnecessary privilege escalation.
190-231: LGTM: hardened Docker invocationGood use of
--rm, device mounts, workspace bind, and a timeout with explicit error handling.
235-251: LGTM: robust witness JSON retrieval with fallback to stdoutSolid error handling path and consistent return type.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
neurons/execution_layer/proof_handlers/dcap_handler.py (5)
190-193: Make sudo usage configurable; retain SGX-based auto-detectionSome environments require sudo for docker regardless of SGX device access. Allow override via env var while keeping the current heuristic.
- docker_cmd = ["docker", "run", "--rm"] - if _needs_sudo_for_sgx(): - docker_cmd = ["sudo"] + docker_cmd + docker_cmd = ["docker", "run", "--rm"] + use_sudo_env = os.getenv("TEEONNX_DOCKER_USE_SUDO") + if (use_sudo_env == "1") or (use_sudo_env is None and _needs_sudo_for_sgx()): + docker_cmd = ["sudo"] + docker_cmd
11-18: Fix import paths: missing 'neurons.' package prefix (prevents module resolution)These imports will fail outside the local package context. Use the fully qualified package path, including constants and TYPE_CHECKING import.
Apply this diff:
-from execution_layer.proof_handlers.base_handler import ProofSystemHandler -from execution_layer.generic_input import GenericInput -from constants import LOCAL_TEEONNX_PATH, ONE_HOUR -from execution_layer.session_storage import DCAPSessionStorage +from neurons.execution_layer.proof_handlers.base_handler import ProofSystemHandler +from neurons.execution_layer.generic_input import GenericInput +from neurons.constants import LOCAL_TEEONNX_PATH, ONE_HOUR +from neurons.execution_layer.session_storage import DCAPSessionStorage @@ -if TYPE_CHECKING: - from execution_layer.verified_model_session import VerifiedModelSession +if TYPE_CHECKING: + from neurons.execution_layer.verified_model_session import VerifiedModelSession
49-60: Guard against missing or invalid input datasession.inputs.data can be None or non-dict, which will raise on .get(). Fail fast with a clear message.
- if session.inputs is None: - raise ValueError("Session inputs cannot be None when generating input file") + if session.inputs is None or session.inputs.data is None: + raise ValueError("Session inputs are required to generate input file") + if not isinstance(session.inputs.data, dict): + raise TypeError("Session inputs.data must be a dict[str, object]") - input_data = session.inputs.data + input_data = session.inputs.data
65-71: Ensure compiled model exists before copying; fail fast with helpful errorCopying without verifying the source path can hide the root cause. Check and raise FileNotFoundError to surface configuration issues early.
if not os.path.exists(model_dest): - shutil.copy2(session.model.paths.compiled_model, model_dest) + if not os.path.exists(session.model.paths.compiled_model): + raise FileNotFoundError( + f"Compiled model not found: {session.model.paths.compiled_model}" + ) + shutil.copy2(session.model.paths.compiled_model, model_dest) bt.logging.trace(f"Copied model to {model_dest}")
102-112: Avoid KeyError for missing 'instances'; also canonicalize JSON outputNot all proofs include "instances". Default gracefully and return canonical JSON for determinism.
- return json.dumps(proof), json.dumps(proof["instances"]) + instances = proof.get("instances") or proof.get("public_signals") or [] + return json.dumps(proof, separators=(",", ":")), json.dumps( + instances, separators=(",", ":") + )
🧹 Nitpick comments (2)
neurons/execution_layer/proof_handlers/dcap_handler.py (2)
175-175: Make verification result parsing more robust (prefer JSON, fallback to substring)Relying on a substring is brittle. If teeonnx returns JSON, parse it; otherwise, fallback. This keeps behavior backward compatible.
- return "verified: true" in result.stdout + # Prefer JSON output if available; fallback to substring for legacy CLI + try: + out = json.loads(result.stdout) + return bool(out.get("verified", False)) + except Exception: + return "verified: true" in (result.stdout or "").lower()If the CLI definitely does not return JSON, keep the substring check. Otherwise this improves reliability.
223-231: Include stderr and return code in docker failure logsThis greatly improves diagnosability for SGX/Docker issues.
- except subprocess.CalledProcessError as e: - bt.logging.error(f"Docker command failed: {e}") - raise + except subprocess.CalledProcessError as e: + bt.logging.error( + f"Docker command failed (code={e.returncode}): {e}\nSTDOUT: {e.stdout}\nSTDERR: {e.stderr}" + ) + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
neurons/execution_layer/proof_handlers/dcap_handler.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
neurons/execution_layer/proof_handlers/dcap_handler.py (4)
neurons/execution_layer/proof_handlers/base_handler.py (1)
ProofSystemHandler(11-64)neurons/execution_layer/generic_input.py (1)
GenericInput(7-26)neurons/execution_layer/session_storage.py (2)
DCAPSessionStorage(68-101)witness_path(100-101)neurons/execution_layer/verified_model_session.py (5)
VerifiedModelSession(24-182)gen_input_file(74-78)gen_proof(80-102)generate_witness(159-164)verify_proof(129-145)
🔇 Additional comments (1)
neurons/execution_layer/proof_handlers/dcap_handler.py (1)
135-139: LGTM: Stable, deterministic input hashingGood use of sort_keys and compact separators to stabilize the hash across processes.
…on-subnet into dcap
Summary by CodeRabbit
New Features
Improvements
Removals
Chores