Skip to content

Dead code removal #8326

Description

@maxtropets

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:

  • 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions