microsoft / microsoft/CCF

Dead code removal

Open
#8,326 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C++
Stars
876
Forks
260
Avg merge
1d 11h
Merged PRs (30d)
157

Description

Audit and clean up dead code following BFT removal


Disclaimer: push back on things that you think should stay.


Follow-up to #8312 (dead BFT execution code) and #8303 (unused KV read serialization). Earlier related cleanup: #4760 and #5176.

Findings are based on static repository-wide reference searches and caller tracing, not a whole-program reachability proof. Reconfirm against current main before removing anything. Not every finding is necessarily BFT-related.

Dead functions

No in-repository callers found:

KV and consensus (#8328)
  • Remove Store::has_map_internal().
  • Remove the zero-argument Store::next_version() and its abstract declaration. Keep the parameterized overload and next_version_unsafe().
  • Remove Store::unset_flag() and its abstract declaration. Keep the production-used unset_flag_unsafe().
  • Remove Map::HandleCommitter::set_commit_version().
  • Remove Aft::is_retired_completed().
Signatures and utilities (#8332)
  • Remove unused NodeSignature::get_serialized_size(), serialize(), and deserialize(). Signature tables use JSON serialization through ServiceValue; retain that path.
  • Remove the orphan ccf::ds::system::exec() utility/header.
  • Remove unused http::Message::get_headers(), clear_headers(), and get_content_data().
  • Remove unused http::Request::get_method() and get_path(). Same-named methods on RPC contexts, curl requests, and Merkle proofs are unrelated and live.
  • Remove unused NotifyingWriterFactory::get_outbound_work_beacon(); investigate its associated notification work below.
Python infrastructure (#8345)
  • Remove ccall_with_pipe().
  • Remove probably_free_remote_port(). Keep the live local-port finder.
  • Remove two_different().
  • Remove cert_bytes() and quote_bytes().
  • Remove mk_new() and its recursively dead helper mk().

Dead variables, arguments, and recursively dead work

  • Remove the unused "last new map" tracking chain:

    • StoreState::last_new_map
    • Store::next_version()'s previous_last_new_map result
    • Forwarding through CommittableTx
    • apply_changes::version_last_new_map, which is assigned but never read
    • Associated VersionLastNewMap alias, tuple component, updates, and commit_new_map argument
    • Preserve version allocation, rollback-count tracking, and actual new-map conflict checks.
  • Remove unused per-key read-version metadata:

    • VersionV::read_version
    • Its copies into the second component of Read
    • LastReadVersion
    • Conflict checking consumes only component zero.
    • Keep ChangeSet::read_version: it implements live whole-map conflict tracking.
  • Simplify the unreachable strict-version checking path:

    • The sole caller of Store::fill_maps() always passes ignore_strict_versions=true.
    • Review removal of that argument, the unreachable branch, and the strict_versions member/constructor option.
    • Preserve the remaining deserialization, rollback, and expected-TxID checks.
  • Remove or simplify HandleCommitter::committed_writes:

    • Its only observation through has_writes() occurs before commit() sets it.
    • The true state is not observed in the current lifecycle.
  • Remove the ignored force_become_leader argument from Aft::become_leader() and its callers.

  • Remove Consensus::periodic_end() and its NodeState call:

    • Empty base implementation.
    • No overrides.
    • Same pattern as the unused hooks removed by #8312.
  • Eliminate unused outbound notification work:

    • outbound_work_beacon is allocated and notified, but no caller retrieves it to wait.
    • Outbound writes still perform locking, counter updates, and notifications.
    • Keep inbound notifications and shared functionality used elsewhere.
  • Remove PIPE and Popen imports from the process-helper module when removing ccall_with_pipe(); it is their only user there.

Nearly dead: test-/benchmark-only production code

Decide whether to retain, move into test support, narrow the interface, or remove. Test-only use is not itself a reason to delete useful coverage.

History and proofs
  • Review TxHistory / HashedTxHistory::get_proof() and verify_proof(); only history unit tests call these virtual APIs.
  • Review the recursively test-/benchmark-only proof serialization and verification path:
    • Proof(vector)
    • Proof::to_v()
    • Proof::verify()
    • MerkleTreeHistory::verify()
  • Review TxHistory::append() / HashedTxHistory::append(); the raw-byte API is used only by history benchmarks. Production uses append_entry().
  • Move or otherwise isolate the test-only NullTxHistory -> NullTxHistoryPendingTx implementation chain.

Keep: MerkleTreeHistory::get_proof(), Proof::get_root(), Proof::get_path(), and TxHistory::get_raw_leaf() have production callers.

Location: History implementation

KV and consensus interfaces
  • Review test-driver-only Aft::is_candidate(), is_backup(), and is_active().
  • Review test/simulation accessors Aft::get_last_idx(), get_committed_seqno(), and get_state_representation().
  • Review test-only Store::commit_view(); keep the underlying live commit-term state.
  • Review test-only locked wrapper Store::flag_enabled(); production uses flag_enabled_unsafe().
  • Review the test-only chain Store::unset_global_hook() -> Map::unset_global_hook().
  • Review test-only CommittableTx::commit_term(). commit_version() has production callers.
  • Review execution-wrapper get_index() plumbing:
    • Production forwards its result to LedgerEnclave::put_entry(), which ignores it.
    • The test ledger consumes it.
    • put_entry()'s term parameter is likewise test-consumed.
    • Keep execution-wrapper get_term(), which has other live uses.
HTTP and other utilities
  • Review test-/benchmark-only free HTTP builders:

    • build_header()
    • build_request()
    • build_post_header()
    • build_post_request()
    • build_header() is reached through build_post_header().
    • Keep the production-used Request::build_request() member.
  • Review test-only http::Request::set_method() and set_query_param().

  • Review test-only SimpleResponseProcessor; SimpleRequestProcessor has production use.

  • Review test-only CBOR mutation helpers ccf::cbor::with_element() and with_entry().

  • Review test-only WorkerShutdownGate::is_shutting_down(); retain the live shutdown mechanism.

  • Review test-/benchmark-only untimed WorkBeacon::wait_for_work(); timed waiting remains live.

Public indexing APIs: external-use caveat

No in-repository production instantiations found. External applications may legitimately use these APIs.

  • Assess retention/deprecation of SeqnosByKey_InMemory and SeqnosByKey_InMemory_Untyped.
  • Assess retention/deprecation of SeqnosForValue_Bucketed.

Compatibility-sensitive BFT and legacy remnants

These are review items, not unconditional deletion candidates.

  • Document or review AppendEntries::contains_new_view; explicitly retained for wire compatibility.
  • Review raft_append_entries_signed_response; no producer or implementation remains. Preserve numeric values of subsequent wire message types.
  • Review ConsensusType::BFT and ServiceConfiguration::consensus; serialized configuration/API remnants rather than an active consensus-selection mechanism.
  • Review NodeSignature::hashed_nonce; emitted empty but still part of signature JSON.
  • Review deprecated PrimarySignature::commit_seqno and commit_view; emitted as zero, serialized, and checked by governance-history tests.
  • Retain or explicitly version legacy read-set headers and the max_conflict_version placeholder. Removing dead in-memory metadata does not justify deleting legacy decoders.
  • Assess the unused public ConsensusTypeMismatch error constant; no emission sites found.
  • Record an explicit retain/remove/deprecate decision for each compatibility-sensitive item.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reconfirming the unchecked findings against current main with repository-wide reference searches and caller tracing. The history review points to src/node/history.h; inspect the named KV, consensus, HTTP, and Python entry points and their tests. Done means each item has a justified remove, retain, move, deprecate, or compatibility decision, with relevant tests still passing.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, python
Domain
databases, distributed-systems
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.