Merge pull request #8 from metaverse-systems/019-perf-dedup-cleanup

019 perf dedup cleanup

Author: timschwartz

.github/copilot-instructions.md

@@ -22,6 +22,8 @@ Auto-generated from all feature plans. Last updated: 2026-04-13
 - N/A (build-system-only change) (015-compile-time-optimization)
 - C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`) (016-audit-remediation)
 - Boost.Serialization binary chunk files (`chunk_NNNNNN.dat`), keys (`keys.dat`), streams (`streams.dat`, `stream_index.dat`) (016-audit-remediation)
+- C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`), Catch2 (test only) (019-perf-dedup-cleanup)
+- Boost.Serialization binary chunk files (`chunk_NNNNNN.dat`), JSON files (`peers.json`, `config.json`) (019-perf-dedup-cleanup)
 
 - C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL, nlohmann/json (vendored `src/json.hpp`), Catch2 (test only) (001-code-constitution-audit)
 
@@ -49,9 +51,9 @@ C++20 (`-std=c++20`): Follow standard conventions
 - Do not include task numbers (e.g. T001, T010) in code comments. Comments should describe *what* or *why*, not reference planning artifacts.
 
 ## Recent Changes
+- 019-perf-dedup-cleanup: Added C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`), Catch2 (test only)
 - 018-audit-bug-security-fixes: Added C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`)
 - 017-blockchain-module-split: Added C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`)
-- 016-audit-remediation: Added C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`)
 
 
 <!-- MANUAL ADDITIONS START -->

.specify/feature.json

@@ -1,3 +1,3 @@
 {
-  "feature_directory": "specs/018-audit-bug-security-fixes"
+  "feature_directory": "specs/019-perf-dedup-cleanup"
 }

docs/AUDIT.md

@@ -33,16 +33,18 @@ This audit found **4 bugs**, **2 security issues**, **5 performance issues**,
 test-quality weaknesses** that reduce confidence in the test suite.
 
 All 4 bugs, both security issues, and 1 performance issue were resolved in
-**018-audit-bug-security-fixes**. Remaining items are tracked below.
+**018-audit-bug-security-fixes**. Three more performance issues, both
+duplication clusters, and 1 test-quality issue were resolved in
+**019-perf-dedup-cleanup**. Remaining items are tracked below.
 
 | Category              | Found | Resolved | Remaining | Highest Open Severity |
 |-----------------------|------:|---------:|----------:|----------------------:|
 | Bugs                  |     4 |        4 |         0 | —                     |
 | Security issues       |     2 |        2 |         0 | —                     |
-| Performance issues    |     5 |        1 |         4 | Medium                |
-| Code duplication      |     2 |        0 |         2 | Medium                |
+| Performance issues    |     5 |        4 |         1 | Low                   |
+| Code duplication      |     2 |        2 |         0 | —                     |
 | Architecture concerns |     3 |        0 |         3 | Medium                |
-| Test quality issues   |     6 |        0 |         6 | High                  |
+| Test quality issues   |     6 |        1 |         5 | High                  |
 
 ---
 
@@ -91,39 +93,29 @@ returns JSON-RPC error -32001 "Block not found" for out-of-range indices.
 
 ## 4. Performance Issues
 
-### 4.1 O(n) peer lookups — MEDIUM
+### 4.1 ~~O(n) peer lookups — MEDIUM~~ ✅ RESOLVED (019)
 
-`find_peer()`, `add_peer()`, `remove_peer()`, `is_banned()`, and
-`get_non_banned_peer_addresses()` all perform linear scans over
-`std::vector<PeerEntry>` and `std::vector<BanRecord>`.
+`peers_` and `bans_` are now `std::unordered_map<std::string, PeerEntry/BanRecord>`
+keyed by `host:port`, giving O(1) lookups in `find_peer()`, `add_peer()`,
+`remove_peer()`, `is_banned()`, `ban_peer()`, `unban_peer()`, and
+`get_non_banned_peer_addresses()`.
 
-With a configured maximum of 256 stored peers and `is_banned()` called on
-every peer exchange, connection attempt, and block reception, switching to
-`std::unordered_map<std::string, PeerEntry>` keyed by `host:port` would drop
-lookups from O(n) to O(1).
+### 4.2 ~~RPC dispatch is a 21-branch `if`/`else` chain — MEDIUM~~ ✅ RESOLVED (019)
 
-### 4.2 RPC dispatch is a 21-branch `if`/`else` chain — MEDIUM
-
-[RpcServer.cpp](../src/network/RpcServer.cpp#L74-L704)
-
-Every request walks up to 21 string comparisons. A
-`std::unordered_map<std::string, Handler>` dispatch table would be O(1) and
-reduce the 700-line `do_read()` callback into individually testable handler
-functions.
+`do_read()` now performs a single `dispatch_.find(method)` lookup into an
+`std::unordered_map<std::string, RpcHandler>` initialized in `init_dispatch()`.
+All 20 handlers are private methods returning `nlohmann::json`.
 
 ### 4.3 ~~`recoverChain()` loads each chunk multiple times — MEDIUM~~ ✅ RESOLVED (018)
 
 Resolved together with §2.2. Single-pass recovery loads each chunk once.
 
-### 4.4 String construction in log calls — LOW
-
-Throughout the codebase, `logMessage("INFO", "Block #" + std::to_string(...) + ...)`
-constructs the string even when the log level would suppress it. The
-`logMessage()` function already filters by level, but the string allocation
-happens at the call site.
+### 4.4 ~~String construction in log calls — LOW~~ ✅ RESOLVED (019)
 
-**Fix:** A level-check macro or a lazy-evaluation wrapper would eliminate
-unnecessary allocations.
+Hot-path `logMessage()` calls in `PeerManager.cpp`, `BlockPropagation.cpp`,
+`PeerClient.cpp`, and `PeerServer.cpp` have been replaced with lazy
+`LOG_INFO`/`LOG_WARN`/`LOG_ERROR`/`LOG_DEBUG` macros that check `getLogLevel()`
+before evaluating the message expression.
 
 ### 4.5 `replaceChain()` loads entire candidate into memory — LOW
 
@@ -137,19 +129,18 @@ simultaneously. A streaming/chunked replacement would bound memory usage.
 
 ## 5. Code Duplication
 
-### 5.1 Packet serialization in PeerClient and PeerServer — MEDIUM
+### 5.1 ~~Packet serialization in PeerClient and PeerServer — MEDIUM~~ ✅ RESOLVED (019)
 
-`PeerClient::send<T>()` ([PeerClient.cpp](../src/network/PeerClient.cpp#L352-L375)) and
-`PeerServer::send_packet<T>()` ([PeerServer.cpp](../src/network/PeerServer.cpp#L271-L300))
-share the same serialize → `PacketHeader` → `memcpy` → `async_write` pattern.
-This could live in a shared utility or base class.
+Both `PeerClient::send<T>()` and `PeerServer::send_packet<T>()` now call the
+shared `serialize_packet<T>()` template from `PacketSerializer.hpp`, which
+returns header bytes + serialized payload. Each caller retains its own
+`async_write` logic.
 
-### 5.2 Test files still duplicate `mineTestBlock()` / `buildValidChain()` — LOW
+### 5.2 ~~Test files still duplicate `mineTestBlock()` / `buildValidChain()` — LOW~~ ✅ RESOLVED (019)
 
-Despite `TestHelpers.hpp` existing, `sync_tests.cpp`,
-`block_propagation_tests.cpp`, `consensus_tests.cpp`, and
-`chunk_persistence_tests.cpp` still define their own local versions of
-`mineTestBlock()`, `buildValidChain()`, and temporary-directory helpers.
+Local helper definitions in `sync_tests.cpp`, `consensus_tests.cpp`, and
+`chunk_persistence_tests.cpp` have been removed. All test files now use the
+shared `TestHelpers::` namespace.
 
 ---
 
@@ -263,13 +254,12 @@ The following behaviors have no test coverage:
 | Block propagation relay excludes sender correctly | Medium | Open |
 | `recoverChain()` with corrupted index files (fallback to chunk rebuild) | Medium | Open |
 
-### 7.6 Duplicated test setup persists in 4 files — LOW
+### 7.6 ~~Duplicated test setup persists in 4 files — LOW~~ ✅ RESOLVED (019)
 
-Despite `TestHelpers.hpp` existing, `sync_tests.cpp`,
-`block_propagation_tests.cpp`, `consensus_tests.cpp`, and
-`chunk_persistence_tests.cpp` still define local `mineTestBlock()` /
-`buildValidChain()` / temp directory helpers instead of using the shared
-utilities.
+All local test helper definitions have been removed. Test files now use
+`TestHelpers::mineTestBlock()`, `TestHelpers::buildValidChain()`,
+`TestHelpers::createTestDir()`, `TestHelpers::cleanupTestDir()`, and
+`TestHelpers::make_block()` exclusively.
 
 ---
 
@@ -288,8 +278,8 @@ Ordered by impact and effort:
 | 5 | Rewrite `rpc_expansion_tests.cpp` to test real RPC handlers (§7.3) | False confidence → real coverage | Medium | Open |
 | 6 | Replace trivial assertions with meaningful ones (§7.1, §7.2) | Catches actual regressions | Medium | Open |
 | 7 | Cache chunk during `recoverChain()` validation (§2.2, §4.3) | 3× faster startup | Low | ✅ Done (018) |
-| 8 | Replace O(n) peer lookups with `unordered_map` (§4.1) | O(1) peer operations | Medium | Open |
-| 9 | Extract RPC dispatch table from `do_read()` (§4.2) | Maintainability, testability | Medium | Open |
+| 8 | Replace O(n) peer lookups with `unordered_map` (§4.1) | O(1) peer operations | Medium | ✅ Done (019) |
+| 9 | Extract RPC dispatch table from `do_read()` (§4.2) | Maintainability, testability | Medium | ✅ Done (019) |
 | 10 | Narrow `IBlockchain` into reader/writer interfaces (§6.1) | Reduces coupling | Medium | Open |
-| 11 | Remove local test helpers in favor of `TestHelpers.hpp` (§7.6) | Consistency | Low | Open |
+| 11 | Remove local test helpers in favor of `TestHelpers.hpp` (§7.6) | Consistency | Low | ✅ Done (019) |
 | 12 | Make integration tests deterministic (§7.4) | Reduces CI flakiness | Medium | Open |

docs/ROADMAP.md

@@ -24,6 +24,7 @@ Last updated: 2026-04-13
 | 016  | Code Audit Remediation | Fixed 5 bugs (block count, dirty flag, merkle root, IPv6 parsing, pending pool), cached difficulty per boundary with ChunkRetainGuard, extracted shared utilities (chunkFilename, parsePeerKey, RPC helpers, send_to_peers), added TestHelpers.hpp, single-threaded io_context enforcement |
 | 017  | Blockchain Module Split | Split monolithic Blockchain.cpp (1,019 lines) into four focused modules: ChainPersistence (379 lines), DifficultyEngine (95 lines), MerkleProofService (60 lines), and slimmed Blockchain core (624 lines); composition-based ownership; zero API changes; 3 new focused test suites |
 | 018  | Audit Bug & Security Fixes | Fixed 4 bugs (sync response block append, RPC getBlockByIndex bounds check, getBlockByIndex resize chunk IDs, recovery triple-load) and 2 security issues (port range validation in parsePeerKey, seed node input validation in main); single-pass recovery optimization |
+| 019  | Performance & Deduplication Cleanup | O(1) peer lookups via `unordered_map`, RPC dispatch table replacing 21-branch `if`/`else`, shared `serialize_packet<T>()` template, consolidated `TestHelpers`, lazy `LOG_*` macros |
 
 ## Suggested Specs
 

specs/019-perf-dedup-cleanup/checklists/requirements.md

@@ -0,0 +1,35 @@
+# Specification Quality Checklist: Performance & Deduplication Cleanup
+
+**Purpose**: Validate specification completeness and quality before proceeding to planning
+**Created**: 2026-04-13
+**Feature**: [spec.md](../spec.md)
+
+## Content Quality
+
+- [x] No implementation details (languages, frameworks, APIs)
+- [x] Focused on user value and business needs
+- [x] Written for non-technical stakeholders
+- [x] All mandatory sections completed
+
+## Requirement Completeness
+
+- [x] No [NEEDS CLARIFICATION] markers remain
+- [x] Requirements are testable and unambiguous
+- [x] Success criteria are measurable
+- [x] Success criteria are technology-agnostic (no implementation details)
+- [x] All acceptance scenarios are defined
+- [x] Edge cases are identified
+- [x] Scope is clearly bounded
+- [x] Dependencies and assumptions identified
+
+## Feature Readiness
+
+- [x] All functional requirements have clear acceptance criteria
+- [x] User scenarios cover primary flows
+- [x] Feature meets measurable outcomes defined in Success Criteria
+- [x] No implementation details leak into specification
+
+## Notes
+
+- All items pass after one revision (removed C++ type names and code-level patterns from FRs and Key Entities).
+- Spec is ready for `/speckit.clarify` or `/speckit.plan`.

specs/019-perf-dedup-cleanup/contracts/rpc-dispatch.md

@@ -0,0 +1,69 @@
+# JSON-RPC Contract: 019-perf-dedup-cleanup
+
+**Date**: 2026-04-13
+
+## Overview
+
+This feature refactors the RPC dispatch mechanism. The JSON-RPC interface
+exposed to external clients does **not change** — all 20 methods retain
+identical request/response formats. This contract documents the existing
+interface for verification that the refactor is behavior-preserving.
+
+## Method Registry
+
+All methods below must be present in the dispatch table after refactoring.
+Each method returns a JSON-RPC 2.0 response.
+
+| Method | Params Required | Success Response | Error Codes |
+|--------|----------------|------------------|-------------|
+| `publish` | `stream`, `key`; optional: `data`, `keys` | Block JSON | -32602 (invalid params), -32000 (mining timeout), -32001 (syncing), -32003 (stream not permitted) |
+| `createStream` | `name` | Stream name | -32602, -32000, -32001 |
+| `listStreams` | none | JSON array of streams | — |
+| `getStreamEntries` | `stream` | JSON array of entries | -32602 |
+| `getStreamEntry` | `stream`, `key` | Entry JSON | -32602 |
+| `requestSync` | none | `"sync_started"` | -32002 (already syncing), -32003 (no peer) |
+| `getBlockByIndex` | `index` | Block JSON | -32602, -32001 (not found) |
+| `getBlocksByKeys` | `keys` | JSON array of blocks | -32602 |
+| `addPeer` | `host`, `port` | Success message | -32602 |
+| `removePeer` | `host`, `port` | Success message | -32602 |
+| `listPeers` | none | JSON array of peers | — |
+| `banPeer` | `host`, `port`; optional: `reason`, `duration` | Success message | -32602 |
+| `unbanPeer` | `host`, `port` | Success message | -32602 |
+| `getInclusionProof` | `blockIndex`, `entryIndex` | Proof JSON | -32602 |
+| `verifyInclusionProof` | `proof` | Boolean result | -32602 |
+| `getBlockHeader` | `index` | Header JSON | -32602 |
+| `getNodeStatus` | none | Status JSON | — |
+| `getBlockRange` | `start`, `end` | JSON array of blocks | -32602 |
+| `getChainLength` | none | Length string | — |
+| `getChunkCount` | none | Count string | — |
+
+## Error Response Format
+
+All error responses use the existing `errorMessage()` helper:
+
+```json
+{
+  "jsonrpc": "2.0",
+  "error": {
+    "code": -32601,
+    "message": "Invalid method: unknownMethod"
+  },
+  "id": 1
+}
+```
+
+Standard error codes:
+- `-32600`: Invalid JSON-RPC message
+- `-32601`: Method not found
+- `-32602`: Invalid parameters
+- `-32000`: Mining timeout / internal error
+- `-32001`: Block not found / sync in progress
+- `-32002`: Sync already in progress
+- `-32003`: No peer connected / stream not permitted
+
+## Verification
+
+After refactoring, every method in this table must produce byte-identical
+JSON responses for the same input. The existing `rpc_integration_tests`
+exercise the live RPC interface over SSL sockets and serve as the primary
+regression gate.

specs/019-perf-dedup-cleanup/data-model.md

@@ -0,0 +1,80 @@
+# Data Model: Performance & Deduplication Cleanup
+
+**Date**: 2026-04-13  
+**Feature**: 019-perf-dedup-cleanup  
+
+## Modified Entities
+
+### PeerEntry (existing — container change only)
+
+No field changes. The storage container changes from `std::vector<PeerEntry>` to `std::unordered_map<std::string, PeerEntry>` keyed by `peer_key(host, port)`.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| host | string | Peer hostname/IP (normalized) |
+| port | uint16_t | Peer listen port |
+| node_uuid | string | Remote node UUID |
+| last_seen | uint64_t | Unix timestamp of last contact |
+| error_count | uint32_t | Consecutive error count |
+
+**Key**: `peer_key(host, port)` → `"host:port"` string
+**Serialization**: JSON (to/from `peers.json`) — unchanged format
+**Validation**: Host is normalized via `normalize_address()` before key construction
+
+### BanRecord (existing — container change only)
+
+No field changes. The storage container changes from `std::vector<BanRecord>` to `std::unordered_map<std::string, BanRecord>` keyed by `peer_key(host, port)`.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| host | string | Banned peer hostname/IP |
+| port | uint16_t | Banned peer port |
+| reason | string | Ban reason |
+| expires | uint64_t | Unix timestamp; 0 = permanent |
+
+**Key**: `peer_key(host, port)` → `"host:port"` string
+**Serialization**: JSON (to/from `peers.json`) — unchanged format
+**State transition**: `expires == 0` → permanent ban; `expires > 0 && expires <= now` → expired (removed by `purge_expired_bans()`)
+
+## New Entities
+
+### RPC Dispatch Table
+
+A mapping from JSON-RPC method names to handler functions, stored as a private member of `RpcServer`.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| method_name | string (key) | JSON-RPC method name (e.g., "publish") |
+| handler | function | Callable returning JSON response |
+
+**Type**: `std::unordered_map<std::string, std::function<nlohmann::json(const nlohmann::json&)>>`
+**Lifecycle**: Initialized once in `RpcServer` constructor; immutable thereafter
+**Cardinality**: Exactly 20 entries (one per existing RPC method)
+
+### PacketSerializer (utility — no persistent state)
+
+A header-only template function that serializes an object with Boost.Serialization and prepends a `PacketHeader`. No stored state — pure function.
+
+**Input**: Serializable object of type T, packet type enum value
+**Output**: Pair of (header bytes, serialized payload string)
+**Wire format**: `[PacketHeader: 16 bytes][serialized payload: N bytes]` — identical to current format
+
+## Relationships
+
+```
+PeerManager
+  ├── peers_: unordered_map<string, PeerEntry>   (was: vector<PeerEntry>)
+  ├── bans_: unordered_map<string, BanRecord>     (was: vector<BanRecord>)
+  └── peer_key(host, port) → string key            (existing static helper)
+
+RpcServer
+  └── dispatch_: unordered_map<string, RpcHandler> (new)
+       └── 20 handler methods (extracted from do_read)
+
+PeerClient::send<T>()  ──uses──▶  serialize_packet<T>()  (PacketSerializer.hpp)
+PeerServer::send_packet<T>()  ──uses──▶  serialize_packet<T>()  (PacketSerializer.hpp)
+```
+
+## On-Disk Format
+
+No changes. `peers.json` continues to store peers as a JSON array and bans as a JSON array. The internal container type is transparent to the serialized format.