Skip to content

refactor(storage): remove the dead state-based comparisons sync path#3215

Closed
chefsale wants to merge 1 commit into
masterfrom
chore/remove-comparisons-state-sync
Closed

refactor(storage): remove the dead state-based comparisons sync path#3215
chefsale wants to merge 1 commit into
masterfrom
chore/remove-comparisons-state-sync

Conversation

@chefsale

Copy link
Copy Markdown
Member

What

Removes the legacy state-based Merkle comparison sync (compare_trees / StorageDelta::Comparisons), which is superseded by the HashComparison / level-wise / DAG-CausalActions protocols and is unreachable in production. Net −680 lines.

Why it's safe (evidence, not just static analysis)

Follow-up to the probe PR (#3209). Static analysis showed the path was dead; to confirm it independent of log capture, that PR made all three of its entry points hard-abort:

  • emit — commit_root producing a Comparisons artifact,
  • receive — Root::syncapply_comparisons,
  • trigger — the Action::Compare receive-arm in apply_actions.

CI ran the full multi-node divergence e2e suite (partition, peer-restart, peer-pause, 8-node mesh-soak, concurrent-rotation-converge, HashComparison-reconcile, member-removed-catchup). None of the aborts fired → the path is exercised by neither production sync nor the test suite.

Removed

  • Interface::{compare_trees, compare_affective, generate_comparison_data}
  • StorageDelta::Comparisons variant; Comparison (integration.rs — file deleted); ComparisonData (action.rs)
  • Root::apply_comparisons + the Comparisons arm of Root::sync
  • delta::push_comparison, DeltaContext.comparisons, and the Comparisons branch of commit_root / commit_causal_delta
  • Index::get_collection_names_for (only the removed path used it)
  • the inert Action::Compare pushes in apply_action (they only fed the removed protocol) and the now-false "ships in the next outbound delta" comment

Kept / compatibility

  • The Action::Compare enum variant stays — it's referenced defensively across op-adapter / node delta_store / signing / runtime. Its Root::sync and apply_action arms are now inert (no-op / reject).
  • Wire tags preserved: StorageDelta gains a hand-rolled BorshSerialize so CausalActions keeps tag 2 (tag 1 — the removed variant — is now rejected). Every persisted and in-flight delta stays decodable; this is wire-stability, not a migration shim. The zero-byte no-op sentinel now decodes to Actions(vec![]).

Verification

  • cargo clippy --workspace --all-targets --features calimero-storage/testing -- -D warnings — clean
  • cargo test -p calimero-storage --features testing — 667 passed, 0 failed (incl. rewritten StorageDelta wire-format roundtrip tests)

🤖 Generated with Claude Code

The legacy state-based Merkle comparison sync (`compare_trees` /
`StorageDelta::Comparisons`) is superseded by the HashComparison / level-wise /
DAG-CausalActions protocols and is unreachable in production. This was
confirmed by hard-abort probes at all three of its entry points (emit via
`commit_root`, receive via `apply_comparisons`, and the `Action::Compare`
receive-arm) riding the real multi-node divergence e2e suite
(partition / peer-restart / peer-pause / 8-node mesh-soak / concurrent-rotation /
HashComparison-reconcile / member-removed-catchup): none fired.

Removed:
- `Interface::{compare_trees, compare_affective, generate_comparison_data}`
- `StorageDelta::Comparisons` variant, `Comparison` (integration.rs, file
  deleted), `ComparisonData` (action.rs)
- `Root::apply_comparisons` + the `Comparisons` arm of `Root::sync`
- `delta::push_comparison`, the `DeltaContext.comparisons` field, and the
  Comparisons branch of `commit_root` / `commit_causal_delta`
- `Index::get_collection_names_for` (only used by the removed path)
- the inert `Action::Compare` pushes in `apply_action` (they only fed the
  removed protocol; Merkle convergence is handled by HashComparison) and the
  stale "ships in the next outbound delta" comment

Kept:
- the `Action::Compare` enum variant (still referenced defensively across
  op-adapter / node delta_store / signing / runtime); its `Root::sync` and
  `apply_action` arms are now inert no-ops / rejections
- wire tags: `StorageDelta` gains a hand-rolled `BorshSerialize` so
  `CausalActions` keeps tag 2 (tag 1, the removed variant, is now rejected) —
  every persisted and in-flight delta stays decodable. The zero-byte no-op
  sentinel now decodes to `Actions(vec![])`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@meroreviewer meroreviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 85% | Review time: 527.2s


✅ No Issues Found

All agents reviewed the code and found no issues. LGTM! 🎉


🤖 Generated by MeroReviewer | Review ID: review-3fa9af2b

@meroreviewer

meroreviewer Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Documentation Review

The following documentation may need updates based on the changes in this PR:

  • 🟡 AGENTS.md: AGENTS.md exists but was not updated — consider updating it to reflect the architecture changes in this PR.
  • 🟡 CONTRIBUTING.md: CONTRIBUTING.md exists but was not updated — consider updating it to reflect the architecture changes in this PR.
  • 🟡 architecture/migrations.html: Files matching crates/storage/** were deleted but architecture/migrations.html was not updated — references to removed code may now be stale (per source_to_docs_mapping).
  • 🟡 architecture/storage-schema.html: Files matching crates/storage/** were deleted but architecture/storage-schema.html was not updated — references to removed code may now be stale (per source_to_docs_mapping).
  • 🟡 docs/: Static HTML docs in docs/ may need updating — architecture-impacting changes detected. On merge, update-docs will scan this directory and open a PR if any pages need to change.

@meroreviewer meroreviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Code Reviewer

Reviewed by 3 agents | Quality score: 95% | Review time: 1180.4s


⚠️ Review Incomplete

1 of 3 agent(s) did not finish (security-reviewer) and no findings were produced. Treat this PR as not yet reviewed, not as approved.


🤖 Generated by AI Code Reviewer | Review ID: review-d135c8bf

@chefsale

Copy link
Copy Markdown
Member Author

Superseded by #3216, which merged the identical removal to master (same hand-rolled StorageDelta BorshSerialize keeping CausalActions at tag 2, same get_collection_names_for/IndexMap cleanup, same retained Action::Compare variant). This branch is based on pre-#3208 master and adds nothing over what's already merged. Closing as duplicate.

@chefsale chefsale closed this Jul 13, 2026
@chefsale chefsale deleted the chore/remove-comparisons-state-sync branch July 13, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant