Break the crypto, ds, and pal dependency cycle - #8265
Merged
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Amaury Chamayou (achamayou)
September 2, 2026 13:07
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The changelog entry needs a PR reference per repository guidance, and a modified shell script should enable set -o pipefail when using pipelines.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR restructures core locking and memory helper dependencies to break a crypto/ds/pal source-layering cycle, making ds a clearer dependency-free foundation and adding enforcement checks to prevent regressions.
Changes:
- Introduces
include/ccf/ds/locking.hand migrates in-tree uses fromccf::pal::*toccf::ds::*, keeping deprecatedccf::palaliases for compatibility. - Replaces the remaining
ccf::pal::safe_memcpyusage withstd::memcpyand deprecatessafe_memcpyinccf/pal/mem.h. - Adds CI scripting to enforce that production
src/dscode does not include other CCF source components.
Custom instructions used:
.github/copilot-instructions.md.github/instructions/changelog.instructions.md.github/instructions/reviewing.instructions.md
File summaries
| File | Description |
|---|---|
| src/tasks/thread_manager.cpp | Switch task threading locks to ccf::ds locking primitives. |
| src/tasks/test/ordered_tasks.cpp | Switch test-only logging lock to ccf::ds types. |
| src/tasks/test/demo/session.h | Migrate demo session manager mutex/guard to ccf::ds. |
| src/tasks/test/demo/main.cpp | Migrate demo code lock guard to ccf::ds. |
| src/tasks/test/demo/locking_mpmc_queue.h | Migrate demo queue locking to ccf::ds. |
| src/tasks/test/demo/dispatcher.h | Migrate dispatcher lock guard to ccf::ds. |
| src/tasks/test/basic_tasks.cpp | Migrate test logger mutex/guards to ccf::ds. |
| src/tasks/sub_task_queue.h | Migrate queue mutex/guards to ccf::ds. |
| src/tasks/job_board.cpp | Migrate mutex/CV/guards to ccf::ds in job board. |
| src/tasks/fan_in_tasks.cpp | Migrate fan-in task mutex/guards to ccf::ds. |
| src/node/test/network_identity_subsystem.cpp | Migrate fake scheduler locking to ccf::ds. |
| src/node/test/history.cpp | Add ccf/ds/locking.h and migrate test CV/mutex to ccf::ds. |
| src/node/test/historical_queries.cpp | Migrate mutex type used in test lock_guard template to ccf::ds. |
| src/node/snapshotter.h | Migrate snapshotter internal locking to ccf::ds. |
| src/node/signature_cache_subsystem.h | Migrate signature cache mutex/guards to ccf::ds. |
| src/node/rpc/test/frontend_test.cpp | Add ccf/ds/locking.h and migrate WaitPoint locking to ccf::ds. |
| src/node/rpc/node_frontend.h | Replace PAL locking include; migrate JWT refresh metrics lock to ccf::ds. |
| src/node/rpc/frontend.h | Replace PAL locking include; migrate open_lock to ccf::ds. |
| src/node/rpc/forwarder.h | Migrate timeout task mutex usage to ccf::ds. |
| src/node/recovery_decision_protocol.h | Replace PAL locking include and migrate protocol lock to ccf::ds. |
| src/node/recovery_decision_protocol.cpp | Replace PAL locking include and migrate lock guards to ccf::ds. |
| src/node/quote_endorsements_client.h | Replace PAL locking include and migrate client lock to ccf::ds. |
| src/node/node_to_node_channel_manager.h | Replace PAL locking include and migrate channel map lock to ccf::ds. |
| src/node/node_state.h | Replace PAL locking include and migrate multiple node state locks to ccf::ds. |
| src/node/ledger_secrets.h | Replace PAL locking include and migrate ledger secrets lock to ccf::ds. |
| src/node/history.h | Replace PAL locking include and migrate history state/signature locks to ccf::ds. |
| src/node/historical_queries.h | Replace PAL locking include and migrate requests lock to ccf::ds. |
| src/node/commit_callback_subsystem.h | Replace PAL locking include and migrate callbacks mutex/guards to ccf::ds. |
| src/node/channels.h | Replace PAL locking include and migrate channel lock to ccf::ds. |
| src/kv/untyped_map.h | Replace PAL locking include and migrate map lock to ccf::ds. |
| src/kv/store.h | Replace PAL locking include and migrate store locks to ccf::ds. |
| src/kv/ledger_chunker.h | Replace PAL locking include and migrate chunker lock/guards to ccf::ds. |
| src/js/registry.cpp | Migrate JS context lock guard to ccf::ds. |
| src/js/interpreter_cache.h | Replace PAL locking include and migrate cache lock/guards to ccf::ds. |
| src/indexing/strategies/visit_each_entry_in_map.cpp | Migrate txid lock guards to ccf::ds. |
| src/indexing/strategies/seqnos_by_key_in_memory.cpp | Replace PAL locking include and migrate strategy lock guards to ccf::ds. |
| src/indexing/strategies/seqnos_by_key_bucketed.cpp | Replace PAL locking include and migrate locks to ccf::ds. |
| src/indexing/indexer.h | Migrate indexer lock guard to ccf::ds. |
| src/indexing/enclave_lfs_access.h | Replace PAL locking include and migrate pending-access lock to ccf::ds. |
| src/http/curl.h | Replace PAL locking include and migrate request queue lock to ccf::ds. |
| src/host/udp.h | Replace PAL locking include and migrate DNS resolve lock to ccf::ds. |
| src/host/tcp.h | Replace PAL locking include and migrate DNS resolve lock to ccf::ds. |
| src/host/ledger.h | Replace PAL locking include and migrate ledger file/state/cache locks to ccf::ds. |
| src/host/dns.h | Replace PAL locking include and migrate global resolve-request mutex to ccf::ds. |
| src/endpoints/endpoint_registry.cpp | Replace PAL locking include for endpoint registry compilation unit. |
| src/endpoints/authentication/jwt_auth.cpp | Replace PAL locking include and migrate JWT cache lock to ccf::ds. |
| src/endpoints/authentication/cert_auth.cpp | Replace PAL locking include and migrate validity-periods cache lock to ccf::ds. |
| src/enclave/rpc_sessions.h | Replace PAL locking include and migrate session lock usage to ccf::ds. |
| src/enclave/main.cpp | Replace PAL locking include and migrate enclave create lock to ccf::ds. |
| src/enclave/enclave.h | Drop PAL mem include as safe_memcpy is no longer used here. |
| src/ds/worker_shutdown_gate.h | Replace PAL locking include and use ccf::ds primitives within ds. |
| src/ds/work_beacon.h | Replace PAL locking include and use ccf::ds primitives within ds. |
| src/ds/test/work_beacon.cpp | Replace PAL locking include and migrate test lock usage to ccf::ds. |
| src/ds/test/stub_writer.h | Replace PAL locking include and migrate writer lock usage to ccf::ds. |
| src/ds/ring_buffer.h | Replace safe_memcpy with std::memcpy and remove PAL mem dependency. |
| src/consensus/aft/test/logging_stub.h | Replace PAL locking include and migrate stub lock usage to ccf::ds. |
| src/consensus/aft/raft.h | Replace PAL locking include and migrate raft state locks to ccf::ds. |
| src/consensus/aft/impl/state.h | Replace PAL locking include and migrate raft state mutex type to ccf::ds. |
| scripts/includes-checks.sh | Add enforcement check: src/ds production code must not include other components. |
| scripts/headers-are-included.sh | Update header-inclusion list to ignore deprecated compatibility shims. |
| samples/apps/logging/logging.cpp | Replace PAL locking include and migrate sample lock usage to ccf::ds. |
| include/ccf/pal/mem.h | Deprecate safe_memcpy in favor of std::memcpy. |
| include/ccf/pal/locking.h | Convert to deprecated compatibility aliases pointing to ccf::ds locking. |
| include/ccf/js/core/context.h | Replace PAL locking include and migrate context mutex to ccf::ds. |
| include/ccf/indexing/strategy.h | Replace PAL locking include and migrate base strategy lock to ccf::ds. |
| include/ccf/indexing/strategies/visit_each_entry_in_map.h | Replace PAL locking include and migrate txid lock to ccf::ds. |
| include/ccf/indexing/strategies/seqnos_by_key_in_memory.h | Replace PAL locking include and migrate strategy lock to ccf::ds. |
| include/ccf/indexing/indexer_interface.h | Replace PAL locking include and migrate strategies set lock to ccf::ds. |
| include/ccf/ds/locking.h | New public home for generic locking primitives (Mutex, MutexGuard, ConditionVariable). |
| CHANGELOG.md | Document the public API move/deprecations and rationale. |
Review details
- Files reviewed: 70/70 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: eddyashton <6000239+eddyashton@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Eddy Ashton (eddyashton)
approved these changes
Sep 3, 2026
Eddy Ashton (eddyashton)
added a commit
that referenced
this pull request
Sep 4, 2026
…currency-fuzz-testing-main Conflicts in include/ccf/pal/locking.h, src/kv/store.h, src/consensus/aft/raft.h, and src/node/history.h, from upstream's "Break the crypto, ds, and pal dependency cycle" (#8265): it moved Mutex/MutexGuard/ConditionVariable from ccf::pal to ccf::ds (leaving ccf::pal as a deprecated compatibility alias), and mechanically reverted store.h/raft.h/history.h's few dozen lock call sites from our own label-carrying ccf::pal::unique_lock back to plain std::lock_guard/ std::unique_lock (since our custom wrapper never existed upstream). Resolved by taking upstream's version of all 4 files wholesale (their functional changes preserved verbatim, including a genuine, unrelated fix to Store::rollback()'s chunker/snapshot ordering), then moving our own label-and-hook mechanism to its new home: ccf::ds::Mutex's own lock()/try_lock()/unlock() now take an optional label (as ccf::pal::Mutex used to), and ccf::ds::unique_lock<LockType> (as ccf::pal::unique_lock used to) is the labeled replacement for std::unique_lock, both now living in include/ccf/ds/locking.h. Only the 8 call sites that carried a genuinely chosen label (4 in store.h, 2 in raft.h, 2 in history.h) were converted back to ccf::ds::unique_lock; every other call site keeps upstream's plain std::lock_guard/std::unique_lock exactly as-is, since a label was never functionally required for the pthread_mutex_wrap.cpp interception to work - only for scheduler describe() output to be more readable at the handful of sites where that mattered enough to name. Updated src/commit_concurrency/scheduled/ to reference ccf::ds instead of ccf::pal throughout for the same reason. Validated: full project build succeeds; all 5 concurrency-adjacent suites (kv_test, raft_test, history_test, commit_concurrency_test, commit_concurrency_scheduled_test) pass in both the normal and TSAN configurations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Towards, but does not close #3517.
Before
dsdepended onpalforccf::pal::{Mutex, MutexGuard, ConditionVariable}(ccf/pal/locking.h, used bysrc/ds/work_beacon.handsrc/ds/worker_shutdown_gate.h) and forccf::pal::safe_memcpy(ccf/pal/mem.h, used bysrc/ds/ring_buffer.h). Combined withpal -> cryptoandcrypto -> ds, this formed a cycle.After
dsno longer has any outgoing edge to another CCF source component. The generic locking primitives moved to a new public header,ccf/ds/locking.h, in theccf::dsnamespace.src/ds/ring_buffer.hnow usesstd::memcpydirectly instead ofccf::pal::safe_memcpy.Changes
include/ccf/ds/locking.h:ccf::ds::Mutex,ccf::ds::MutexGuard,ccf::ds::ConditionVariable, moved fromccf::palwith behavior and thread-safety annotations preserved.include/ccf/pal/locking.his now a compatibility shim:usingaliases forMutex/MutexGuard/ConditionVariable, each markedCCF_DEPRECATEDpointing to theccf::dsequivalent.ccf/pal/locking.h/ccf::pal::{Mutex,MutexGuard,ConditionVariable}migrated toccf/ds/locking.h/ccf::ds::*.src/ds/ring_buffer.husesstd::memcpyinstead ofccf::pal::safe_memcpy, and no longer includesccf/pal/mem.h.src/enclave/enclave.hno longer includesccf/pal/mem.h.include/ccf/pal/mem.hretained as a source-compatible shim, withsafe_memcpynow markedCCF_DEPRECATED("Use std::memcpy instead"). It has been an exact wrapper around::memcpysince Open Enclave removal.scripts/includes-checks.sh: new check that fails if a non-test production file undersrc/dsdirectly includes another CCF source component.CHANGELOG.mdentry under[7.0.14]/Changed.