fix: clock skew tolerance 30s -> 5min (stacks on #67 -> #66)#68
fix: clock skew tolerance 30s -> 5min (stacks on #67 -> #66)#68grumbach wants to merge 3 commits intoWithAutonomi:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR is intended to increase future clock-skew tolerance (30s → 5m) to prevent rejecting slightly future-dated messages/uploads from devices with drift.
Changes:
- Adds extensive architecture and planning/review documentation under
crates/saorsa-core/and.planning/. - Introduces/updates local tooling and lint configuration files for
saorsa-core(e.g.,.clippy.toml,.actrc,.gitignore). - Updates the root
Cargo.tomlto define a workspace and switchsaorsa-corefrom a crates.io dependency to a local path dependency.
Reviewed changes
Copilot reviewed 75 out of 632 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/saorsa-core/ARCHITECTURE.md | Adds architecture overview documentation for saorsa-core. |
| crates/saorsa-core/.planning/reviews/workflow-configuration.md | Adds workflow review notes/documentation. |
| crates/saorsa-core/.planning/reviews/type-safety.md | Adds type-safety review report. |
| crates/saorsa-core/.planning/reviews/type-safety-review-validation.md | Adds validation report (includes build/checklist details). |
| crates/saorsa-core/.planning/reviews/test-coverage.md | Adds test coverage review document. |
| crates/saorsa-core/.planning/reviews/task-spec.md | Adds task-spec review document. |
| crates/saorsa-core/.planning/reviews/review-iteration-1-complete.md | Adds review iteration completion report. |
| crates/saorsa-core/.planning/reviews/phase-1-review-summary.md | Adds phase-1 review summary document. |
| crates/saorsa-core/.planning/reviews/minimax.md | Adds external review notes (MiniMax). |
| crates/saorsa-core/.planning/reviews/kimi.md | Adds external review notes (Kimi). |
| crates/saorsa-core/.planning/reviews/iteration-2-summary.md | Adds iteration-2 summary document. |
| crates/saorsa-core/.planning/reviews/iteration-2-fixes-applied.md | Adds iteration-2 fixes-applied report. |
| crates/saorsa-core/.planning/reviews/glm.md | Adds external review placeholder/notes (GLM). |
| crates/saorsa-core/.planning/reviews/fixes-applied-phase2-task2.md | Adds fixes-applied writeup for phase2 task2. |
| crates/saorsa-core/.planning/reviews/fixes-applied-phase2-task1.md | Adds fixes-applied writeup for phase2 task1. |
| crates/saorsa-core/.planning/reviews/fixes-applied-encoding.md | Adds encoding module fixes summary. |
| crates/saorsa-core/.planning/reviews/error-handling.md | Adds error-handling audit report. |
| crates/saorsa-core/.planning/reviews/documentation.md | Adds documentation coverage audit report. |
| crates/saorsa-core/.planning/reviews/consensus-type-safety-20260129.md | Adds consensus report for type-safety review. |
| crates/saorsa-core/.planning/reviews/consensus-encoding-review-20260129.md | Adds consensus report for encoding review. |
| crates/saorsa-core/.planning/reviews/consensus-20260204-183500.md | Adds consensus report for refocus/cleanup effort. |
| crates/saorsa-core/.planning/reviews/consensus-20260202-143500.md | Adds consensus report for v0.10.2 work. |
| crates/saorsa-core/.planning/reviews/consensus-20260129-210600.md | Adds consensus report for uncommitted-changes review. |
| crates/saorsa-core/.planning/reviews/consensus-20260129-200500.md | Adds consensus report for Phase 5 Task 3. |
| crates/saorsa-core/.planning/reviews/consensus-20260129-155118.md | Adds post-commit consensus note for Task 3. |
| crates/saorsa-core/.planning/reviews/consensus-20260129-154812.md | Adds consensus note for Task 2. |
| crates/saorsa-core/.planning/reviews/consensus-20260129-153828.md | Adds consensus note for Task 1. |
| crates/saorsa-core/.planning/reviews/build.md | Adds build validation report (includes noted fixes). |
| crates/saorsa-core/.planning/reviews/build-iteration-2.md | Adds build verification report for iteration 2. |
| crates/saorsa-core/.planning/reviews/EXTERNAL_REVIEWS_COMPLETE.md | Adds summary that external reviews are complete. |
| crates/saorsa-core/.planning/reviews/CODEX_REVIEW_SUMMARY.md | Adds Codex external review summary for Phase 1. |
| crates/saorsa-core/.planning/progress.md | Adds progress log for message encoding optimization. |
| crates/saorsa-core/.planning/architecture-analysis/06-forward-secrecy.md | Adds forward secrecy architecture analysis doc. |
| crates/saorsa-core/.planning/architecture-analysis/05-message-persistence.md | Adds message persistence architecture analysis doc. |
| crates/saorsa-core/.planning/architecture-analysis/04-routing-strategy.md | Adds routing strategy architecture analysis doc. |
| crates/saorsa-core/.planning/architecture-analysis/03-offline-delivery.md | Adds offline delivery architecture analysis doc. |
| crates/saorsa-core/.planning/STATE.json | Adds planning state tracking JSON for the project. |
| crates/saorsa-core/.planning/REVIEW_ITERATION_1_COMPLETE.md | Adds review-iteration completion report. |
| crates/saorsa-core/.planning/REVIEW_CYCLE_COMPLETE.md | Adds external review cycle completion report. |
| crates/saorsa-core/.planning/REVIEW_CYCLE_1_COMPLETE.md | Adds review cycle completion report for encryption review. |
| crates/saorsa-core/.planning/PROJECT.md | Adds project-level planning doc for encoding optimization. |
| crates/saorsa-core/.planning/PLAN-phase-5.md | Adds plan doc for binary encoding migration. |
| crates/saorsa-core/.planning/PLAN-phase-3.md | Adds plan doc for solution design phase. |
| crates/saorsa-core/.planning/PLAN-phase-1.md | Adds plan doc for baseline measurement phase. |
| crates/saorsa-core/.planning/PHASE_2_TASK_1_REVIEW_ITERATION_1.md | Adds review iteration doc for Phase 2 Task 1. |
| crates/saorsa-core/.planning/PHASE_2_TASK_1_ITERATION_1_STATUS.md | Adds status report for Phase 2 Task 1 iteration 1. |
| crates/saorsa-core/.planning/PHASE_2_TASK_1_FIXES_APPLIED.md | Adds fixes-applied doc for Phase 2 Task 1. |
| crates/saorsa-core/.planning/MILESTONES.md | Adds milestone definitions for the project. |
| crates/saorsa-core/.planning/CODE_QUALITY_REVIEW_EXECUTIVE_SUMMARY.md | Adds executive summary for code quality review. |
| crates/saorsa-core/.gitignore | Adds crate-level ignore rules (target, IDE, tool state, etc.). |
| crates/saorsa-core/.cursorules | Adds Cursor rules pointer to CLAUDE.md. |
| crates/saorsa-core/.cursorrules | Adds Cursor rules pointer to CLAUDE.md (alternate filename). |
| crates/saorsa-core/.clippy.toml | Adds clippy configuration (test unwrap/expect/panic allowances + thresholds). |
| crates/saorsa-core/.actrc | Adds act runner image mappings (including macos-latest mapping). |
| Cargo.toml | Adds workspace members and switches saorsa-core dependency to local path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| [workspace] | ||
| members = [ | ||
| ".", | ||
| "crates/saorsa-core", | ||
| "crates/saorsa-transport", | ||
| "crates/saorsa-transport/saorsa-transport-workspace-hack", | ||
| ] | ||
| resolver = "2" |
There was a problem hiding this comment.
The PR title/description claims a single constant change (MAX_FUTURE_SECS 30→300), but this diff introduces a workspace and additional members. Either update the PR title/description to reflect these broader changes, or split the workspace/config/docs changes into separate PR(s) so the clock-skew tolerance change can be reviewed in isolation.
| ## Review Summary | ||
|
|
||
| ### File Analyzed | ||
| - **Path**: `/Users/davidirvine/Desktop/Devel/projects/saorsa-core/src/messaging/encoding.rs` |
There was a problem hiding this comment.
This document embeds an absolute local filesystem path (/Users/...), which can leak developer-machine details and is brittle for other contributors. Prefer repo-relative paths (e.g., src/messaging/encoding.rs) or omit the local path entirely.
| - **Path**: `/Users/davidirvine/Desktop/Devel/projects/saorsa-core/src/messaging/encoding.rs` | |
| - **Path**: `src/messaging/encoding.rs` |
| "repository": "saorsa-labs/saorsa-core", | ||
| "branch": "feature/encoding-optimization", | ||
| "created_by": "Claude Sonnet 4.5", | ||
| "last_updated": "2026-01-29T11:30:00Z", |
There was a problem hiding this comment.
The state file records last_updated twice with conflicting formats/values (ISO-8601 string vs epoch float) that don't appear to represent the same timestamp. If tooling depends on this field, this inconsistency can lead to incorrect ordering or stale-state behavior; consider keeping a single canonical last_updated field (or ensure both are derived from the same source of truth and match).
| }, | ||
| "session_note": "Paused - work continuing in new session. Task 3 complete and merged to main (commit 3abbc59)" | ||
| }, | ||
| "last_updated": 1738702200.000000, |
There was a problem hiding this comment.
The state file records last_updated twice with conflicting formats/values (ISO-8601 string vs epoch float) that don't appear to represent the same timestamp. If tooling depends on this field, this inconsistency can lead to incorrect ordering or stale-state behavior; consider keeping a single canonical last_updated field (or ensure both are derived from the same source of truth and match).
| "last_updated": 1738702200.000000, | |
| "last_updated": 1769686200.000000, |
grumbach
force-pushed
the
feat/clock-skew-fix
branch
2 times, most recently
from
412a26d to
8f68954
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 486 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let elapsed = now.duration_since(self.last_update).as_secs_f64(); | ||
| let refill_rate = cfg.max_requests as f64 / cfg.window.as_secs_f64(); | ||
| self.tokens += elapsed * refill_rate; |
There was a problem hiding this comment.
cfg.window.as_secs_f64() will be 0.0 for a zero-duration window, which would cause a division-by-zero and produce inf/NaN token behavior. Since EngineConfig is a public struct, consider validating window > Duration::ZERO at construction time (e.g., in Engine::new) or handling the zero case explicitly in try_consume.
| impl Default for JoinRateLimiterConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| max_joins_per_64_per_hour: 10_000, | ||
| max_joins_per_48_per_hour: 10_000, | ||
| max_joins_per_24_per_hour: 10_000, | ||
| max_global_joins_per_minute: 10_000, | ||
| global_burst_size: 10_000, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The JoinRateLimiterConfig field docs state strict defaults (e.g., “/64 per hour (default: 1)”, global 100/min), but Default currently sets everything to 10_000, effectively disabling Sybil protections in default configurations. Either (a) change Default to match the documented secure defaults, or (b) update the doc comments to reflect that defaults are permissive (ideally also providing a JoinRateLimiterConfig::production_defaults() / ::development_defaults() to avoid insecure-by-default behavior).
| async fn process_message(&self, message: DhtMessage) -> LinkResult<DhtResponse> { | ||
| match message { | ||
| DhtMessage::FindNode { target, count } => { | ||
| let engine = self.dht_engine.read().await; |
There was a problem hiding this comment.
A tokio::sync::RwLock read guard is held across an .await (find_nodes(...).await). Holding async locks across await points can lead to writer starvation and, in some designs, deadlocks (especially if find_nodes internally needs write access or awaits on work that depends on writers). Consider restructuring to avoid awaiting while the lock is held (e.g., clone/copy required state out of the guard, or move concurrency control inside DhtCoreEngine).
| let engine = self.dht_engine.read().await; | |
| let engine = { | |
| let engine = self.dht_engine.read().await; | |
| engine.clone() | |
| }; |
| //! Enables nodes to detect when their identity doesn't "fit" a DHT close group | ||
| //! and automatically regenerate with a new identity. | ||
|
|
||
| pub mod node_identity; |
There was a problem hiding this comment.
src/identity/node_identity_extensions.rs is added in this PR but is not declared here (or elsewhere) as a module, so it will not be compiled or available to consumers. If these extension methods are intended to exist, add pub mod node_identity_extensions; and re-export what should be public; otherwise, remove the file to avoid dead code drift.
| pub mod node_identity; | |
| pub mod node_identity; | |
| pub mod node_identity_extensions; |
| // Re-export saorsa-transport PQC functions for convenience | ||
| pub use self::saorsa_transport_integration::{generate_ml_dsa_keypair, ml_dsa_sign, ml_dsa_verify}; | ||
|
|
||
| // Primary post-quantum cryptography types from saorsa-pqc 0.3.0 |
There was a problem hiding this comment.
The comment says saorsa-pqc 0.3.0, but crates/saorsa-core/Cargo.toml depends on saorsa-pqc = \"0.5\". Please update the comment to match the actual dependency version to avoid misleading readers.
| // Primary post-quantum cryptography types from saorsa-pqc 0.3.0 | |
| // Primary post-quantum cryptography types from saorsa-pqc 0.5 |
|
|
||
| ## Quick Start | ||
|
|
||
| Add this to your `Cargo.toml`: |
There was a problem hiding this comment.
The README quick-start pins saorsa-core = \"0.16.0\", but crates/saorsa-core/Cargo.toml declares version = \"0.22.0\". Update the README to reference the current crate version (or use a broader version requirement policy) so consumers copy/paste a valid, up-to-date dependency.
grumbach
force-pushed
the
feat/clock-skew-fix
branch
from
8f68954 to
2fb339c
Compare
grumbach
force-pushed
the
feat/clock-skew-fix
branch
from
2fb339c to
9240dc1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 57 out of 488 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl NodeIdentity { | ||
| /// Save identity to file | ||
| pub async fn save_to_file(&self, path: &Path) -> Result<()> { |
There was a problem hiding this comment.
NodeIdentity::save_to_file and NodeIdentity::load_from_file are also defined in crates/saorsa-core/src/identity/node_identity.rs (same inherent impl target). Rust does not allow duplicate inherent methods on the same type, so this will fail to compile. Remove one set of methods, or consolidate into a single module (or rename the extension methods to avoid collisions).
| /// Load identity from file | ||
| pub async fn load_from_file(path: &Path) -> Result<Self> { |
There was a problem hiding this comment.
NodeIdentity::save_to_file and NodeIdentity::load_from_file are also defined in crates/saorsa-core/src/identity/node_identity.rs (same inherent impl target). Rust does not allow duplicate inherent methods on the same type, so this will fail to compile. Remove one set of methods, or consolidate into a single module (or rename the extension methods to avoid collisions).
| use super::node_identity::{PeerId, NodeIdentity}; | ||
| use crate::{P2PError, Result}; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::time::Duration; |
There was a problem hiding this comment.
These imports are unused in this file (PeerId and Duration are never referenced), which will trigger unused-import warnings and fail CI when warnings are denied. Remove the unused imports (or use them if intended).
| use super::node_identity::{PeerId, NodeIdentity}; | |
| use crate::{P2PError, Result}; | |
| use std::path::{Path, PathBuf}; | |
| use std::time::Duration; | |
| use super::node_identity::NodeIdentity; | |
| use crate::{P2PError, Result}; | |
| use std::path::{Path, PathBuf}; |
| max_joins_per_64_per_hour: 10_000, | ||
| max_joins_per_48_per_hour: 10_000, | ||
| max_joins_per_24_per_hour: 10_000, | ||
| max_global_joins_per_minute: 10_000, | ||
| global_burst_size: 10_000, |
There was a problem hiding this comment.
The JoinRateLimiterConfig field docs describe much lower 'default' limits (e.g., /64 per hour default 1, global joins per minute default 100), but Default sets everything to 10,000. This effectively disables Sybil-protection rate limiting by default and conflicts with the documentation. Align the Default values with the documented/desired policy (or update the docs to match the intended permissive defaults).
| max_joins_per_64_per_hour: 10_000, | |
| max_joins_per_48_per_hour: 10_000, | |
| max_joins_per_24_per_hour: 10_000, | |
| max_global_joins_per_minute: 10_000, | |
| global_burst_size: 10_000, | |
| max_joins_per_64_per_hour: 1, | |
| max_joins_per_48_per_hour: 5, | |
| max_joins_per_24_per_hour: 3, | |
| max_global_joins_per_minute: 100, | |
| global_burst_size: 10, |
| pub fn try_consume_key(&self, key: &K) -> bool { | ||
| let mut map = self.keyed.write(); | ||
| // Get or insert with LRU cache (automatically evicts oldest if at capacity) | ||
| if let Some(bucket) = map.get_mut(key) { | ||
| bucket.try_consume(&self.cfg) | ||
| } else { | ||
| let mut bucket = Bucket::new(self.cfg.burst_size as f64); | ||
| let result = bucket.try_consume(&self.cfg); | ||
| map.put(key.clone(), bucket); | ||
| result | ||
| } | ||
| } |
There was a problem hiding this comment.
Every keyed rate-limit check takes an exclusive RwLock write guard, even when just consuming from an existing bucket. Under high concurrency, this can become a contention bottleneck. Consider using a concurrent map (e.g., sharded map) or a design that narrows lock duration (e.g., store per-key buckets behind their own locks), while keeping LRU eviction semantics.
| | Node | Provider | IP Address | Region | Purpose | Status | | ||
| |------|----------|------------|--------|---------|--------| | ||
| | saorsa-1 | Hetzner | 77.42.75.115 | Helsinki | Dashboard & Website | Active | | ||
| | saorsa-2 | DigitalOcean | 142.93.199.50 | NYC1 | Bootstrap Node | Active | | ||
| | saorsa-3 | DigitalOcean | 147.182.234.192 | SFO3 | Bootstrap Node | Active | |
There was a problem hiding this comment.
This doc embeds live infrastructure details (public IPs, provider inventory, and later includes SSH key IDs and operational commands). If this repository is public or shared broadly, this materially increases attack surface (reconnaissance). Consider moving this document to a private ops repo, redacting sensitive fields (IPs/key IDs), or replacing them with placeholders/examples.
Move saorsa-core and saorsa-transport into crates/ as workspace members. All dependencies now use path references instead of fragile git URLs, branch names, or commit hashes. Cleaned up ~100 non-source files: AI planning artifacts, saved test outputs, Go legacy files, orphaned scripts. 492 ant-node tests pass.
grumbach
force-pushed
the
feat/clock-skew-fix
branch
from
9240dc1 to
3d2d5a8
Compare
Stacked on #67 (Mick's NAT fixes) -> #66 (workspace consolidation).
One constant change:
MAX_FUTURE_SECS: 30 -> 300.Without this: 0/3 uploads from Mac (31s clock skew caused all messages to be rejected as future-dated). With this: 3/3 uploads succeeded.
A decentralized network cannot partition when a consumer device's clock drifts by 31 seconds. The past window was already 5 minutes; the future window was asymmetrically tight at 30 seconds.
492 ant-node tests pass.