Take kv and service out of the source dependency cycle by achamayou · Pull Request #8463 · microsoft/CCF

Amaury Chamayou (achamayou) · 2026-09-28T22:08:35Z

Partially addresses #3517: this implements steps 0 and 1 from the latest analysis.

Result

Against current main, the source component graph drops from 86 to 84 edges between 25 components. The strongly connected core shrinks from {consensus, endpoints, js, kv, node, service} to {consensus, endpoints, js, node}. kv and service are no longer part of any cycle. kv now depends directly only on ccf-api, crypto and ds, and service only on those and kv.

The diagram shows every direct dependency among the six components that formed the cycle. Each edge is labelled with the number of #include directives behind it.

graph LR
  subgraph core["remaining cycle"]
    consensus -- 5 --> node
    node -- 9 --> consensus
    endpoints -- 6 --> node
    node -- 4 --> endpoints
    js -- 9 --> node
    node -- 14 --> js
    js -- 7 --> endpoints
  end
  consensus -- 4 --> service
  endpoints -- 12 --> service
  js -- 4 --> service
  node -- 105 --> service
  consensus -- 5 --> kv
  endpoints -- 1 --> kv
  js -- 4 --> kv
  node -- 26 --> kv
  service -- 9 --> kv

Scope of the check

check-source-dependencies.py only counts direct includes from src/. It attributes public headers under include/ccf/<dir> to <dir>, but does not follow their own includes. Following includes transitively, two older paths through public headers remain:

kv → ccf/crypto/sha256_hash.h → ccf/service/map.h. sha256_hash.h only needs map.h for its BlitSerialiser<Sha256Hash> specialisation.
service/network_tables.h → ccf/endpoint.h → ccf/endpoint_context.h → ccf/endpoints/authentication/authentication_types.h.

Neither path reaches any file in consensus, js, node or src/endpoints. The transitive includes of src/kv drop from 109 files, including 4 in node and 17 in service, to 88 files, with none in node and 1 in service. Those of src/service used to reach 4 files in node, and now reach none. Removing the two remaining paths, and making the check follow public headers, are follow-ups for #3517.

Changes

Step 0: housekeeping

Add scripts/source-dependencies.json policies for kv, service and rust. check-source-dependencies.py will now reject any new upward edge from them. Each allowed dependency is necessary: removing any one of them makes the check fail. The checker itself is unchanged.
Remove stale includes:
- consensus/aft/raft_types.h no longer includes node/rpc/rpc_handler.h.
- kv/kv_types.h no longer includes ccf/node/configuration.h or ccf/service/consensus_type.h. It now includes what it uses directly.
Move src/kv/test/null_encryptor.h to src/kv/null_encryptor.h. It is not only used by tests: node_state.h includes it when CCF is built with the debug-only USE_NULL_ENCRYPTOR option.

Step 1: make kv a leaf of the core

Move node/rpc/claims.h to kv/claims.h (static becomes inline). Includers now include it directly instead of relying on transitive includes.
Move the four table names the KV inspects on deserialisation to kv/internal_table_names.h:
- SIGNATURES
- COSE_SIGNATURES
- SERIALISED_MERKLE_TREE
- ENCRYPTED_PAST_LEDGER_SECRET
service/tables/{signatures,shares}.h include it, so ccf::Tables::* is unchanged for all users.
Move ccf::split_net_address() and ccf::make_net_address() from ccf/service/node_info_network.h to the new public header ccf/ds/net_address.h.
Move ccf::COSESignaturesConfig and ccf::ReconfigurationType to the new public headers ccf/cose_signatures_config.h and ccf/reconfiguration_type.h.

Public API impact

No breaking API changes:

ccf/service/node_info_network.h still includes ccf/ds/net_address.h, so the functions stay available to existing includers. Their signatures are equivalent, since NodeInfoNetwork::NetAddress is std::string.
ccf/node/cose_signatures_config.h and ccf/service/reconfiguration_type.h remain as deprecated forwarding shims, to be removed in 8.0.
- They emit a #warning, following the precedent of ccf/pal/locking.h (Break the crypto, ds, and pal dependency cycle #8265). As with that header, builds using -Werror will fail on the warning unless they add -Wno-error=#warnings or switch to the new headers.
- They are excluded from headers-are-included.sh in the same way.
- There are no in-tree includers.
CHANGELOG.md has entries under Changed and a new Deprecated section.

Validation

Local, on WSL with clang 21, RelWithDebInfo, before main was merged in:

Full build: no warnings or failures.
ctest -L unit: 63/63 passed, plus raft_scenario_test.
-fsyntax-only of src/enclave/main.cpp with -DUSE_NULL_ENCRYPTOR.
e2e: schema_test (which validates the OpenAPI for ReconfigurationType and related types) and e2e_logging (which includes test_cose_config) pass.
A translation unit including both the old and new headers builds with CCF's flags, warns once for each deprecated header, and runs successfully. It round-trips both types through JSON and calls the net address helpers.
clang-format 18 and shellcheck pass.
I did not run clang-tidy at the CI version: the locally available clang-tidy 21 is too noisy to be useful, so CI's -DCLANG_TIDY=ON build is the authoritative check.

After merging main (#8459 and #8454; only CHANGELOG.md conflicted), these checks pass on the merged tree, and the graph numbers above are measured on it:

check-source-dependencies.py, includes-checks.sh, headers-are-included.sh
ascii-checks.sh, copyright-checks.sh, todo-checks.sh, extract-release-notes.py
prettier

Steps 0 and 1 from the dependency analysis on #3517, reducing the cyclic core of the source component graph from {consensus, endpoints, js, kv, node, service} to {consensus, endpoints, js, node}. - Move the helpers and types kv needs out of node and service: - claims.h moves from node/rpc to kv. - The internal table names the KV inspects on deserialisation move to kv/internal_table_names.h. - split_net_address() and make_net_address() move to the new public header ccf/ds/net_address.h, still included by ccf/service/node_info_network.h. - COSESignaturesConfig and ReconfigurationType move to the new public headers ccf/cose_signatures_config.h and ccf/reconfiguration_type.h. The old headers are kept as deprecated shims, to be removed in 8.0. - Drop stale includes from kv/kv_types.h and consensus/aft/raft_types.h. - Move null_encryptor.h out of kv/test, since node_state.h includes it under USE_NULL_ENCRYPTOR and test directories are not installed. - Add source dependency policies for kv, service and rust so that new upward edges are caught by check-source-dependencies.py. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot review overview

🟡 Changes recommended

The closing reference would prematurely close #3517 despite the remaining dependency-cycle steps.

Review effort: Balanced
Findings: 1

Open (1)

Avoid prematurely closing #3517 before all DAG steps are complete · New

What changed in this PR

Refactors dependencies so kv and service leave the cyclic source core while preserving public API compatibility.

Changes:

Relocates KV helpers, table names, and the null encryptor to appropriate KV-owned headers.
Moves shared public types and network-address helpers to lower-level headers with deprecated forwarding shims.
Adds dependency policies and updates direct includes, tests, and release notes.

Custom instructions used

.github/copilot-instructions.md
.github/instructions/reviewing.instructions.md
.github/instructions/changelog.instructions.md

File	Description
`CHANGELOG.md`	Documents moved and deprecated APIs.
`include/ccf/cose_signatures_config.h`	Adds the relocated COSE configuration type.
`include/ccf/cose_signatures_config_interface.h`	Uses the new COSE header.
`include/ccf/ds/net_address.h`	Adds relocated network-address helpers.
`include/ccf/node/configuration.h`	Uses the new COSE header.
`include/ccf/node/cose_signatures_config.h`	Becomes a deprecated forwarding shim.
`include/ccf/reconfiguration_type.h`	Adds the relocated reconfiguration enum.
`include/ccf/service/node_info_network.h`	Forwards network helpers through the DS header.
`include/ccf/service/reconfiguration_type.h`	Becomes a deprecated forwarding shim.
`include/ccf/service/service_config.h`	Uses the new reconfiguration header.
`scripts/headers-are-included.sh`	Excludes compatibility-only shims.
`scripts/source-dependencies.json`	Adds KV, service, and Rust policies.
`src/consensus/aft/raft.h`	Uses the lower-level reconfiguration header.
`src/consensus/aft/raft_types.h`	Removes a stale node dependency.
`src/consensus/aft/test/view_straddling_common.h`	Updates the null-encryptor include.
`src/endpoints/endpoint_registry.cpp`	Removes the obsolete claims include.
`src/indexing/test/common.h`	Updates the null-encryptor include.
`src/js/test/js.cpp`	Updates the null-encryptor include.
`src/kv/claims.h`	Relocates and makes claims helpers inline.
`src/kv/committable_tx.h`	Removes the node claims dependency.
`src/kv/deserialise.h`	Uses KV-owned claims and table names.
`src/kv/generic_serialise_wrapper.h`	Uses the relocated claims header.
`src/kv/internal_table_names.h`	Centralizes KV-inspected table names.
`src/kv/kv_types.h`	Replaces upward dependencies with direct includes.
`src/kv/null_encryptor.h`	Relocates the debug encryptor outside tests.
`src/kv/snapshot.h`	Includes KV claims directly.
`src/kv/store.h`	Includes KV claims directly.
`src/kv/test/kv_contention.cpp`	Updates the null-encryptor include.
`src/kv/test/kv_dynamic_tables.cpp`	Updates the null-encryptor include.
`src/kv/test/kv_serialisation.cpp`	Updates the null-encryptor include.
`src/kv/test/kv_snapshot.cpp`	Updates the null-encryptor include.
`src/kv/test/kv_test.cpp`	Updates relocated KV helper includes.
`src/kv/test/null_tx_history.h`	Uses the new COSE header.
`src/node/hooks.h`	Uses the new reconfiguration header.
`src/node/identity.h`	Uses the new COSE header.
`src/node/internal_tables_access.h`	Adds its direct host-data dependency.
`src/node/node_state.h`	Uses relocated public and KV headers.
`src/node/rpc/node_call_types.h`	Adds direct configuration dependencies.
`src/node/rpc/node_frontend.h`	Uses the new reconfiguration header.
`src/node/rpc/node_operation_interface.h`	Uses the new COSE header.
`src/node/rpc/test/frontend_test.cpp`	Updates the null-encryptor include.
`src/node/rpc/test/frontend_test_infra.h`	Updates the null-encryptor include.
`src/node/rpc/test/internal_tables_access_test.cpp`	Updates the null-encryptor include.
`src/node/rpc/test/node_frontend_test.cpp`	Updates the null-encryptor include.
`src/node/test/historical_queries.cpp`	Updates the null-encryptor include.
`src/node/test/history.cpp`	Updates the null-encryptor include.
`src/node/test/jwt_key_auto_refresh.cpp`	Updates the null-encryptor include.
`src/node/test/ledger_secrets.cpp`	Updates the null-encryptor include.
`src/node/test/network_identity_subsystem.cpp`	Updates the null-encryptor include.
`src/node/test/node_info_json.cpp`	Includes network helpers directly.
`src/node/test/open_service.cpp`	Updates the null-encryptor include.
`src/node/test/snapshot.cpp`	Updates relocated KV helper includes.
`src/node/test/snapshotter.cpp`	Updates the null-encryptor include.
`src/node/tx_receipt_impl.h`	Includes KV claims directly.
`src/service/tables/config.h`	Uses the new reconfiguration header.
`src/service/tables/shares.h`	Reuses KV-owned table names.
`src/service/tables/signatures.h`	Reuses KV-owned table names.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…encies

Resolving the CHANGELOG conflict when merging main left the #8459 entry at the end of the new Deprecated section. It describes a behaviour change, so restore it to its position under Changed, as on main. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…encies

Copilot AI balanced review requested due to automatic review settings September 28, 2026 22:08

Amaury Chamayou (achamayou) requested a review from a team as a code owner September 28, 2026 22:08

Add PR number to CHANGELOG entries

536c9fc

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot started reviewing on behalf of Amaury Chamayou (achamayou) September 28, 2026 22:09 View session

Copilot AI reviewed Sep 28, 2026

View reviewed changes

Comment thread scripts/source-dependencies.json

Amaury Chamayou (achamayou) and others added 2 commits September 29, 2026 07:55

Merge branch 'main' into achamayou-clarify-framework-component-depend…

07263d6

…encies

Max (maxtropets) approved these changes Sep 29, 2026

View reviewed changes

Amaury Chamayou (achamayou) added 2 commits September 29, 2026 11:21

Merge branch 'main' into achamayou-clarify-framework-component-depend…

d7b3428

…encies

Merge branch 'main' into achamayou-clarify-framework-component-depend…

9ee9aa9

…encies

Amaury Chamayou (achamayou) merged commit 3341f1d into main Sep 29, 2026
14 checks passed

Amaury Chamayou (achamayou) deleted the achamayou-clarify-framework-component-dependencies branch September 29, 2026 13:14

This was referenced Sep 30, 2026

Clarify dependencies between framework source components #3517

Closed

Take consensus out of the source dependency cycle #8472

Merged

Navigation Menu

Repository navigation

Take kv and service out of the source dependency cycle - #8463

Take kv and service out of the source dependency cycle#8463
Amaury Chamayou (achamayou) merged 6 commits into
mainfrom
achamayou-clarify-framework-component-dependencies

Amaury Chamayou (achamayou) commented Sep 28, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Amaury Chamayou (achamayou) commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Result

Scope of the check

Changes

Step 0: housekeeping

Step 1: make kv a leaf of the core

Public API impact

Validation

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Custom instructions used

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Amaury Chamayou (achamayou) commented Sep 28, 2026 •

edited

Loading