Remove attestation module (removed from saorsa-core)#7
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the attestation module and all related functionality from saorsa-node, aligning with the removal of attestation features from the upstream saorsa-core library.
Changes:
- Removed the entire
attestationmodule includingVerificationLeveland related types - Removed attestation-related configuration types (
AttestationMode,AttestationNodeConfig) and theattestationfield fromNodeConfig - Removed attestation validation and configuration building methods from
NodeBuilder
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/attestation.rs | Complete removal of the attestation security level detection module (159 lines) |
| src/config.rs | Removed attestation configuration types and field from NodeConfig |
| src/node.rs | Removed attestation validation, configuration building, and related imports |
| src/lib.rs | Removed attestation module from public API |
| src/probe.rs | Updated comment to remove reference to attestation |
| build.rs | Removed attestation warning emissions from build script |
| Cargo.toml | Removed zkvm feature flags and switched to local path dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile OverviewGreptile Summaryremoved attestation module and all related configuration in response to Key Changes:
Issues:
Testing: Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| Cargo.toml | Removed attestation features and switched to local path dependency (path doesn't exist in repo) |
| src/attestation.rs | Entire module deleted (159 lines removed) |
| src/config.rs | Removed AttestationMode, AttestationNodeConfig types and attestation field from NodeConfig |
| src/node.rs | Removed validate_attestation_security() and build_attestation_config() methods |
Sequence Diagram
sequenceDiagram
participant Dev as Developer
participant PR as This PR
participant Node as saorsa-node
participant Core as saorsa-core
Note over Core: saorsa-core removed<br/>AttestationConfig &<br/>EnforcementMode
Dev->>PR: Remove attestation module
PR->>Node: Delete src/attestation.rs
PR->>Node: Remove from src/lib.rs exports
Dev->>PR: Update config
PR->>Node: Remove AttestationNodeConfig
PR->>Node: Remove AttestationMode enum
PR->>Node: Remove attestation field from NodeConfig
Dev->>PR: Update node builder
PR->>Node: Remove validate_attestation_security()
PR->>Node: Remove build_attestation_config()
Dev->>PR: Clean up features & build
PR->>Node: Remove zkvm-prover feature
PR->>Node: Remove zkvm-verifier-groth16 feature
PR->>Node: Strip attestation warnings from build.rs
Note over PR,Node: ⚠️ Cargo.toml now uses<br/>local path dependency<br/>that doesn't exist
PR-->>Core: Expects updated saorsa-core API
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
mickvandijke
force-pushed
the
fix/remove-attestation
branch
from
25a4635 to
82154c6
Compare
saorsa-core no longer exposes AttestationConfig or EnforcementMode. Remove all attestation-related code, config, docs, CI, Grafana dashboards, Prometheus alerts, and testnet plan references. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
mickvandijke
force-pushed
the
fix/remove-attestation
branch
from
82154c6 to
fba0238
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@claude please do a final review |
|
Claude finished @mickvandijke's task —— View job Final PR ReviewReview complete! This is a clean and thorough removal of the attestation module. Todo List
SummaryThis PR successfully removes the attestation module and all related functionality in response to Stats: 14 files changed, +13/-1,040 lines ✅ What's Done Well1. Complete RemovalAll attestation-related code has been properly removed:
2. Thorough CleanupThe PR goes beyond just code removal:
3. No Remaining ReferencesGrep search confirms zero remaining attestation references in source code (only git metadata, which is expected). 4. Proper Comment UpdatesMinor but important - // Probe CoreConfig for attestation_config fieldto: // Probe CoreConfig fields🔍 Critical Issue: Cargo.toml DependencyLine 27 in saorsa-core = "0.10.2"Problem: The PR description states:
But looking at the current Questions:
Recommendation: If the dependency was already corrected to
📋 Minor Observations1. CI Workflow is Minimal but CompleteThe
No attestation-specific steps remain. 2. Build Script Now Minimal
fn main() {
println!("cargo:rerun-if-changed=build.rs");
}This is fine - it just triggers rebuild when the build script changes. 3. Node Configuration CleanThe
All well-organized and properly documented. 🧪 Testing StatusPer PR description:
Note: I cannot verify the build locally without approval to run 📊 Code QualityStrengths:
Architecture:The changes maintain the thin wrapper design philosophy:
🎯 VerdictStatus: ✅ APPROVE (pending dependency verification) This PR is ready to merge IF:
What's Done Right:
Confidence Level: High - this is a well-executed removal with proper attention to detail. 📝 Recommendations for FutureNone required - this PR accomplishes its goal cleanly. The attestation module removal is complete and consistent across the entire codebase. Final NotesGreat work on this PR! The removal is:
The only concern is the dependency version verification, which CI should confirm. Once CI passes, this is ready to merge. Review completed: 2026-02-05 Job run • Branch: |
|
Wait with merging this until the new |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #[serde(default)] | ||
| pub payment: PaymentConfig, | ||
|
|
||
| /// Attestation configuration for software integrity verification. | ||
| #[serde(default)] | ||
| pub attestation: AttestationNodeConfig, | ||
|
|
||
| /// Bootstrap cache configuration for persistent peer storage. | ||
| #[serde(default)] | ||
| pub bootstrap_cache: BootstrapCacheConfig, |
There was a problem hiding this comment.
Removing the attestation field from NodeConfig will cause deserialization to fail for any existing config files that still include an [attestation] section (Serde errors on unknown fields by default). If you want to preserve backward compatibility, consider adding a deprecated/ignored placeholder field (e.g., rename = "attestation" + skip_* + default) so older configs continue to load while effectively disabling attestation.
| # sp1-sdk (zkvm-prover feature) requires C++20 which isn't available on Windows CI | ||
| - name: Run tests | ||
| run: cargo test | ||
|
|
There was a problem hiding this comment.
CI no longer runs tests with --all-features. Now that the attestation feature flags were removed (and the previous Windows limitation was tied to those), it would be safer to restore an --all-features job (at least on Linux) to avoid silently breaking non-default feature combinations in the future.
| test-all-features: | |
| name: Test all features (ubuntu-latest) | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - uses: dtolnay/rust-toolchain@stable | |
| - uses: Swatinem/rust-cache@v2 | |
| - name: Run tests with all features | |
| run: cargo test --all-features |
mickvandijke
force-pushed
the
fix/remove-attestation
branch
from
3c271bf to
96fd3aa
Compare
Add unit and e2e tests covering the remaining Section 18 scenarios: Unit tests (32 new): - Quorum: #4 fail→abandoned, #16 timeout→inconclusive, #27 single-round dual-evidence, #28 dynamic threshold undersized, #33 batched per-key, #34 partial response unresolved, #42 quorum-derived paid-list auth - Admission: #5 unauthorized peer, #7 out-of-range rejected - Config: #18 invalid config rejected, #26 dynamic paid threshold - Scheduling: #8 dedup safety, #8 replica/paid collapse - Neighbor sync: #35 round-robin cooldown skip, #36 cycle completion, #38 snapshot stability mid-join, #39 unreachable removal + slot fill, #40 cooldown peer removed, #41 cycle termination guarantee, consecutive rounds, cycle preserves sync times - Pruning: #50 hysteresis prevents premature delete, #51 timestamp reset on heal, #52 paid/record timestamps independent, #23 entry removal - Audit: #19/#53 partial failure mixed responsibility, #54 all pass, #55 empty failure discard, #56 repair opportunity filter, response count validation, digest uses full record bytes - Types: #13 bootstrap drain, repair opportunity edge cases, terminal state variants - Bootstrap claims: #46 first-seen recorded, #49 cleared on normal E2e tests (4 new): - #2 fresh offer with empty PoP rejected - #5/#37 neighbor sync request returns response - #11 audit challenge multi-key (present + absent) - Fetch not-found for non-existent key Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
attestationmodule (src/attestation.rs) and all references to it — saorsa-core no longer exposesAttestationConfigorEnforcementModezkvm-proverandzkvm-verifier-groth16feature definitions fromCargo.toml(they forwarded to saorsa-core features that no longer exist)AttestationMode,AttestationNodeConfigtypes and theattestationfield fromNodeConfiginsrc/config.rsvalidate_attestation_security()andbuild_attestation_config()methods fromNodeBuilderinsrc/node.rsbuild.rs7 files changed, 453 lines removed. All 173 tests pass, clippy and fmt clean.
Test plan
cargo clippy --all-targets -- -D warnings— zero warningscargo fmt --all -- --check— cleancargo test— 130 unit + 42 e2e + 1 doctest pass🤖 Generated with Claude Code