fix(storage,node): path-ancestor & shared-stamp fixes + node hardening batch#3208
Conversation
…al Shared stamp authority is_ancestor_of iterated only the offsets between segments, so it never compared the ancestor candidate's deepest segment: `::a::x` reported as an ancestor of `::a::y::z`, and a single-segment path matched any deeper path regardless of content. is_descendant_of inherited both. Compare all of self's segments (depth()+1) as a true prefix instead, and add regression tests for the differing-final-segment and differing-single-segment cases. The local-write authorization for Shared entities lived in two hand-rolled copies (save_raw used stored-union-claimed; the delete path used stored only), which could silently drift on the auth boundary. Extract one authorize_local_shared_stamp helper and route both through it. Behavior is preserved: in the delete path metadata is loaded fresh from the index, so its writers already are the stored set and the union collapses to the same stored-membership check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 MeroReviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 66.3s
✅ No Issues Found
All agents reviewed the code and found no issues. LGTM! 🎉
🤖 Generated by MeroReviewer | Review ID: review-cd59540c
Documentation ReviewThe following documentation may need updates based on the changes in this PR:
|
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 1 agents | Quality score: 85% | Review time: 240.0s
✅ No Issues Found
All agents reviewed the code and found no issues. LGTM! 🎉
🤖 Generated by AI Code Reviewer | Review ID: review-22acd7ad
…, sync debounce, map TTLs, log throttle, config knobs Address a batch of node-crate review findings: - utils: choose_stream consumed the first item before enumerating, biasing Algorithm R toward later elements (and a single-item stream matched anything). Enumerate the whole stream so replacement prob is 1/(idx+1). - blobs: verify a downloaded blob's content hash equals the requested id before reporting success; discard mismatched/tampered content. - namespace backfill: enforce MAX_BACKFILL_OPS on the receiver, not just the responder, so a misbehaving peer can't drive unbounded apply work. Shared const in sync::mod. - subscriptions: stop swallowing has_context store errors via unwrap_or_default(); log and bail instead of treating them as 'no context'. - namespace parent-pull: add bounded exponential backoff between peer attempts. - network_event_channel: lock-free time throttle on the capacity/backpressure warnings so sustained overload can't flood the log. - network_event_processor: bound NodeManager's mailbox and forward via try_send with bounded retry + a drop counter instead of do_send (which ignores capacity and grew the mailbox without limit). - heartbeat: per-context debounce on the 'peer is ahead' recovery sync so N ahead peers no longer spawn N syncs per 30s cycle. - heartbeat/readiness: TTL-evict the peer-keyed divergence_streak, behind_sync, and probe-response maps so they can't grow on peer churn. - run/constants: name the channel capacities in constants.rs instead of scattering magic numbers. - namespace: decompose the ~200-line governance-delta spawned future into a NamespaceDeltaApply context with run()/on_applied() methods. cargo test -p calimero-node: 431 passed. clippy + fmt clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 85% | Review time: 481.7s
💡 1 suggestions. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-ac6dc130
| // in the stored writer set to authorize the delete. | ||
| // If this is a local shared action by a writer, set the nonce. Same | ||
| // authority rule as save_raw, via the shared helper. Here `metadata` | ||
| // was just loaded from the index above, so its writers already are the |
There was a problem hiding this comment.
💡 Unifying save/delete Shared-stamp authorization adds a redundant index read on the delete path
remove_child_from_inner loads metadata (and therefore the current stored writer set) from the index just a few lines above, then calls Self::authorize_local_shared_stamp(child_id, claimed) where claimed is literally that same stored writer set. But authorize_local_shared_stamp (interface.rs:3548) unconditionally re-fetches <Index<S>>::get_metadata(id) to compute stored_has_executor. On the delete path this second fetch is provably redundant — claimed already is the stored set, so stored_has_executor == claimed.contains_key(&executor) always. This adds an extra index/DB lookup to every local delete of a Shared entity that the pre-refactor code did not have (it only did stored.contains_key(&executor) inline).
Suggested fix:
Add a variant (or an `Option<&BTreeMap<..>>` pre-fetched-stored parameter) to `authorize_local_shared_stamp` that lets the delete-path caller pass the already-loaded stored writer set directly instead of triggering another `Index::get_metadata` call, e.g. `authorize_local_shared_stamp_from_stored(claimed, claimed)` or an internal helper that skips the fetch when the caller already knows stored == claimed.
There was a problem hiding this comment.
Addressed in ae0b7b9: added a stored: Option<&BTreeMap<..>> param to authorize_local_shared_stamp. The delete path passes its already-loaded writers (Some(claimed)) so it skips the second Index::get_metadata; the save path passes None and still looks stored up (its claimed is the incoming action's set, not stored). Single authority rule preserved, redundant read gone.
Address PR review: remove_child_from_inner already loads metadata (and thus the stored writer set) from the index right before authorizing the stamp, so authorize_local_shared_stamp re-fetching it via Index::get_metadata was a provably redundant read on every local Shared delete. Add a `stored` param: the delete path passes its already-loaded writers, the save path passes None (its `claimed` is the incoming action's set, not stored) and still looks up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 95% | Review time: 573.5s
✅ No Issues Found
All agents reviewed the code and found no issues. LGTM! 🎉
🤖 Generated by AI Code Reviewer | Review ID: review-719e016b
cargo-deny advisories fails on RUSTSEC-2026-0204 (invalid pointer deref in crossbeam-epoch's fmt::Display for Atomic/Shared), pulled transitively via rayon -> wasmer. Lockfile-only patch bump to the fixed 0.9.20; no source or API change. Affects master too (Cargo.lock was identical) — unblocks CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 95% | Review time: 199.9s
✅ No Issues Found
All agents reviewed the code and found no issues. LGTM! 🎉
🤖 Generated by AI Code Reviewer | Review ID: review-37d4dfc9
Two storage-crate correctness/hardening fixes plus a batch of node-crate review findings. Two independent commits — split easily if a reviewer prefers separate PRs.
Storage (commit
f10467f)Path::is_ancestor_ofnever compared the final segment (correctness)is_ancestor_ofiterated only the byte offsets between segments, so it never compared the ancestor candidate's deepest segment:Second latent hole: a single-segment path has an empty
offsets, so the loop never ran and it returnedtruefor any deeper path.is_descendant_ofinherited both. Fixed by comparing all ofself'sdepth() + 1segments as a true prefix viasegments().zip(...), with regression tests. (No production callers today — a latent landmine in a public API.)Unify the local
Sharedstamp authority rule (hardening)Local-write authorization for
Sharedentities lived in two hand-rolled copies with divergent rules (save_raw: stored ∪ claimed; delete: stored only). Extracted a singleauthorize_local_shared_stamphelper so the rule can't drift. Behavior-preserving: in the delete pathmetadatais loaded fresh from the index, so its writers already are the stored set and the union collapses to the same check.Node review-finding batch (commit
df1a289)Medium
utils.rs):choose_streamconsumed the first item beforeenumerate(), so Algorithm R over-weighted later elements (and a single-item stream matched anything). Now enumerates the whole stream — replacement prob1/(idx+1).network_event_processor.rs):do_sendignored NodeManager's mailbox capacity, so draining the bounded source channel grew the mailbox without limit. Now bounds the mailbox (set_mailbox_capacity) and forwards viatry_send+ bounded retry, dropping with a counter only under sustained overload (shutdown still flushes best-effort viado_send). Highest-impact change in the batch — the intended tradeoff is dropping-with-visibility over unbounded growth; periodic sync reconciles anything dropped.heartbeat.rs): the "peer is ahead" branch spawned a sync every heartbeat, so N ahead peers → N syncs/30s. Added a per-context debounce (a context-wide sync serves every peer).heartbeat.rs,readiness.rs):divergence_streak, the newbehind_sync, andlast_probe_response_atnow TTL-evict during their ticks so they can't grow on peer churn.namespace.rs): the ~200-line governance-delta spawned future is decomposed into aNamespaceDeltaApplycontext withrun()/on_applied().Low
blobs.rs): verifystored_blob_id == requested blob_id(content hash) before reporting success; discard mismatched content.namespace.rs/sync/mod.rs): enforceMAX_BACKFILL_OPSon the receiver, not just the responder (shared const), so a misbehaving peer can't drive unbounded apply work.subscriptions.rs):has_context(...).unwrap_or_default()treated store errors as "no context"; now logs and bails.namespace.rs): bounded exponential backoff between namespace parent-pull peer attempts.network_event_channel.rs): lock-free time throttle on the capacity/backpressure warnings.run.rs/constants.rs): channel capacities named inconstants.rsinstead of scattered magic numbers. (Named constants, not runtime-tunable knobs — runtime config would be a larger follow-up.)Testing
cargo test -p calimero-storage --lib— 669 passed.cargo test -p calimero-node --lib— 431 passed.-D warnings+ rustfmt clean on both crates.🤖 Generated with Claude Code