Dead code removal
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 andnext_version_unsafe(). - Remove
Store::unset_flag()and its abstract declaration. Keep the production-usedunset_flag_unsafe(). - Remove
Map::HandleCommitter::set_commit_version(). - Remove
Aft::is_retired_completed().
Signatures and utilities (#8332)
- Remove unused
NodeSignature::get_serialized_size(),serialize(), anddeserialize(). Signature tables use JSON serialization throughServiceValue; retain that path. - Remove the orphan
ccf::ds::system::exec()utility/header. - Remove unused
http::Message::get_headers(),clear_headers(), andget_content_data(). - Remove unused
http::Request::get_method()andget_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()andquote_bytes(). - Remove
mk_new()and its recursively dead helpermk().
Dead variables, arguments, and recursively dead work
-
Remove the unused "last new map" tracking chain:
StoreState::last_new_mapStore::next_version()'sprevious_last_new_mapresult- Forwarding through
CommittableTx apply_changes::version_last_new_map, which is assigned but never read- Associated
VersionLastNewMapalias, tuple component, updates, andcommit_new_mapargument - 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 passesignore_strict_versions=true. - Review removal of that argument, the unreachable branch, and the
strict_versionsmember/constructor option. - Preserve the remaining deserialization, rollback, and expected-TxID checks.
- The sole caller of
-
Remove or simplify
HandleCommitter::committed_writes:- Its only observation through
has_writes()occurs beforecommit()sets it. - The
truestate is not observed in the current lifecycle.
- Its only observation through
-
Remove the ignored
force_become_leaderargument fromAft::become_leader()and its callers. -
Remove
Consensus::periodic_end()and itsNodeStatecall:- Empty base implementation.
- No overrides.
- Same pattern as the unused hooks removed by #8312.
-
Eliminate unused outbound notification work:
outbound_work_beaconis 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
PIPEandPopenimports from the process-helper module when removingccall_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()andverify_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 usesappend_entry(). - Move or otherwise isolate the test-only
NullTxHistory->NullTxHistoryPendingTximplementation 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(), andis_active(). - Review test/simulation accessors
Aft::get_last_idx(),get_committed_seqno(), andget_state_representation(). - Review test-only
Store::commit_view(); keep the underlying live commit-term state. - Review test-only locked wrapper
Store::flag_enabled(); production usesflag_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()'stermparameter is likewise test-consumed.- Keep execution-wrapper
get_term(), which has other live uses.
- Production forwards its result to
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 throughbuild_post_header().- Keep the production-used
Request::build_request()member.
-
Review test-only
http::Request::set_method()andset_query_param(). -
Review test-only
SimpleResponseProcessor;SimpleRequestProcessorhas production use. -
Review test-only CBOR mutation helpers
ccf::cbor::with_element()andwith_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_InMemoryandSeqnosByKey_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::BFTandServiceConfiguration::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_seqnoandcommit_view; emitted as zero, serialized, and checked by governance-history tests. - Retain or explicitly version legacy read-set headers and the
max_conflict_versionplaceholder. Removing dead in-memory metadata does not justify deleting legacy decoders. - Assess the unused public
ConsensusTypeMismatcherror constant; no emission sites found. - Record an explicit retain/remove/deprecate decision for each compatibility-sensitive item.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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