016 audit remediation

.github/copilot-instructions.md

@@ -20,6 +20,8 @@ Auto-generated from all feature plans. Last updated: 2026-04-12
 - N/A (no persistence changes) (014-documentation-developer-guide)
 - C++20 (`-std=c++20`) + Boost (Asio, Serialization, Program Options), OpenSSL, nlohmann/json (vendored), Catch2 (test) (015-compile-time-optimization)
 - 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, nlohmann/json (vendored `src/json.hpp`), Catch2 (test only) (001-code-constitution-audit)
 
@@ -47,9 +49,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
+- 016-audit-remediation: Added C++20 (`-std=c++20`) + Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`)
 - 015-compile-time-optimization: Added C++20 (`-std=c++20`) + Boost (Asio, Serialization, Program Options), OpenSSL, nlohmann/json (vendored), Catch2 (test)
 - 014-documentation-developer-guide: Added GitHub-Flavored Markdown (documentation-only feature; no C++ changes) + N/A (Markdown files only; Mermaid diagrams rendered natively by GitHub)
-- 013-ci-cd-pipeline: Added YAML (GitHub Actions workflow), C++20 (project under test) + GitHub Actions, MSYS2 (Windows), apt (Linux), Homebrew (macOS)
 
 
 <!-- MANUAL ADDITIONS START -->

.specify/feature.json

@@ -1,3 +1,3 @@
 {
-  "feature_directory": "specs/015-compile-time-optimization"
+  "feature_directory": "specs/016-audit-remediation"
 }

docs/AUDIT.md

@@ -401,3 +401,32 @@ Ordered by impact and effort:
 | 8 | Add `chunkFilename()` utility (§3.5) | Single source of truth for paths | Trivial |
 | 9 | Split `Blockchain.cpp` into focused modules (§6.2) | Maintainability at scale | High |
 | 10 | Narrow `IBlockchain` into reader/writer interfaces (§6.4) | Reduces coupling | Medium |
+
+---
+
+## 9. Remediation Status (016-audit-remediation)
+
+**Date:** 2026-04-12
+
+| # | Issue | Status | Details |
+|---|-------|--------|---------|
+| 1 | `getChainBlockCount()` ignores `totalBlockCount_` (§2.1) | **Fixed** | Returns `totalBlockCount_` directly |
+| 2 | `dirty_` flag cleared prematurely in `publish()` (§2.4) | **Fixed** | Removed premature `dirty_ = false` in chunk rotation |
+| 3 | Merkle root recomputed on deserialized blocks (§2.2) | **Fixed** | Added verify-then-cache Block constructor; used in `appendReceivedBlock()` |
+| 4 | `sender_key` IPv6 parsing fragile (§2.3) | **Fixed** | New `parsePeerKey()` utility with bracket-aware IPv6 support |
+| 5 | Single-threaded enforcement (§5) | **Fixed** | `std::atomic<int>` guard in `main.cpp` aborts if `io_context.run()` entered twice |
+| 6 | Pending pool O(n) linear scan (§4.3) | **Fixed** | Replaced with `unordered_map` + `deque` for O(1) operations |
+| 7 | Difficulty cache missing (§4.2) | **Fixed** | `difficultyCache_` + `ChunkRetainGuard` RAII; invalidated on `replaceChain()`/`recoverChain()` |
+| 8 | Chunk filename duplication (§3.5) | **Fixed** | `chunkFilename()` in `utils.hpp`; all 8 inline sites replaced |
+| 9 | RPC response boilerplate (§3.1) | **Fixed** | Shared `errorMessage()`/`resultJsonMessage()` helpers; 13 → 3 inline sites |
+| 10 | Broadcast/relay duplication (§3.3) | **Fixed** | `send_to_peers()` in `PeerManager`; `broadcast_block()`/`relay_block()` delegate |
+| 11 | Stream entry lookup duplication (§3.4) | **Fixed** | Extracted shared `collectEntries` lambda in `getStreamEntries()` |
+| 12 | Test setup duplication (§7.4) | **Fixed** | `TestHelpers.hpp` with shared utilities; 9 test files updated |
+
+### Not Addressed (deferred)
+
+| # | Issue | Reason |
+|---|-------|--------|
+| 9 | Split `Blockchain.cpp` into modules (§6.2) | High effort; architecture change outside audit scope |
+| 10 | Narrow `IBlockchain` interfaces (§6.4) | Medium effort; deferred to future refactoring |
+| — | RPC dispatch table (§4.6, §6.1) | Deferred; helper extraction sufficient for now |

docs/ROADMAP.md

@@ -21,6 +21,7 @@ Last updated: 2026-04-12
 | 013  | CI/CD Pipeline | GitHub Actions workflow with matrix build (Linux GCC/Clang, macOS Clang, Windows MSYS2), per-binary test execution, MSYS2 caching, Catch2-from-source on Windows |
 | 014  | Documentation & Developer Guide | Expanded README with cross-platform build instructions and two-node quickstart, configuration reference (9 CLI flags, 30 config.json fields, TLS setup), RPC API reference (20 methods with curl examples), architecture overview with Mermaid diagrams, contributing guide |
 | 015  | Compile-Time Optimization | Shared static archive (`libblockchain_core.a`) eliminates redundant compilation of 11 core source files across 14 build targets, reducing compilation units from ~177 to ~34 (~81% reduction), clean build time from ~16 min to ~2.5 min |
+| 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 |
 
 ## Suggested Specs
 

specs/016-audit-remediation/checklists/requirements.md

@@ -0,0 +1,36 @@
+# Specification Quality Checklist: Code Audit Remediation
+
+**Purpose**: Validate specification completeness and quality before proceeding to planning
+**Created**: 2026-04-12
+**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 validation. The spec is ready for `/speckit.clarify` or `/speckit.plan`.
+- Scope explicitly excludes large architectural refactors (§6.2–6.4) and streaming chain replacement (§4.8) to keep the feature focused on targeted fixes.
+- Threading fix assumes single-threaded enforcement as the pragmatic approach, documented in Assumptions.

specs/016-audit-remediation/data-model.md

@@ -0,0 +1,156 @@
+# Data Model: Code Audit Remediation
+
+**Feature**: 016-audit-remediation  
+**Date**: 2026-04-12
+
+---
+
+## New Entities
+
+### DifficultyCache (in-memory, not persisted)
+
+Added to `Blockchain` class as a member.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| `difficultyCache_` | `std::unordered_map<size_t, uint32_t>` | Maps adjustment boundary height → difficulty at that boundary |
+
+**Lifecycle**:
+- Populated lazily in `getDifficultyForHeight()` as boundaries are computed
+- Cleared entirely on `replaceChain()` (chain history changed)
+- Cleared on `recoverChain()` (chain reloaded from disk)
+- Not invalidated by `publish()`/`appendBlock()` (new boundaries are additive)
+
+**Validation**: None — values are computed from validated chain data.
+
+---
+
+### ChunkRetainSet (in-memory, transient)
+
+Added to `Blockchain` class as a member.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| `retainedChunks_` | `std::set<size_t>` | Set of chunk indices that must not be freed |
+
+**Lifecycle**:
+- Populated by RAII guard (`ChunkRetainGuard`) at the start of multi-access operations
+- Cleared by guard destructor, which also frees all retained chunks that are no longer needed
+- Never persisted
+
+**Validation**: None — indices are always valid chunk positions.
+
+---
+
+### PendingPool (modified structure in BlockPropagation)
+
+Replaces current `std::unordered_map<std::string, PendingEntry> pending_pool_`.
+
+| Field | Type | Description |
+|-------|------|-------------|
+| `pending_map_` | `std::unordered_map<std::string, PendingEntry>` | Block hash → pending entry for O(1) lookup |
+| `pending_order_` | `std::deque<std::string>` | Block hashes in insertion order for O(1) eviction |
+
+**Lifecycle**:
+- Insert: push hash to `pending_order_` back, insert into `pending_map_`
+- Evict oldest: pop from `pending_order_` front, erase from `pending_map_`
+- Expire: walk `pending_order_` from front while expired, erase both
+- Lookup: `pending_map_.contains(hash)`
+
+**Validation**: Both structures must stay in sync — every hash in the deque must exist in the map.
+
+---
+
+## Modified Entities
+
+### Block (modified constructor)
+
+New constructor overload added for received/synced blocks:
+
+```
+Block(index, time, prev_hash, entries, nonce, difficulty, merkleRoot, hash)
+```
+
+| Parameter | Type | Description |
+|-----------|------|-------------|
+| `merkleRoot` | `std::string` | Pre-computed merkle root from sender |
+| `hash` | `std::string` | Pre-computed block hash from sender |
+
+**Validation**: Constructor verifies merkle root against entries. Throws `std::invalid_argument` if verification fails. Hash is verified against block contents including the verified merkle root.
+
+**State transition**: None — blocks are immutable after construction.
+
+---
+
+### Blockchain (modified members)
+
+| Member | Change | Description |
+|--------|--------|-------------|
+| `difficultyCache_` | Added | See DifficultyCache entity above |
+| `retainedChunks_` | Added | See ChunkRetainSet entity above |
+
+---
+
+### BlockPropagation (modified members)
+
+| Member | Change | Description |
+|--------|--------|-------------|
+| `pending_pool_` | Replaced by `pending_map_` + `pending_order_` | See PendingPool entity above |
+
+---
+
+## Utility Functions (new)
+
+### utils.hpp / utils.cpp
+
+| Function | Signature | Description |
+|----------|-----------|-------------|
+| `chunkFilename` | `std::string chunkFilename(size_t index)` | Returns `"chunk_NNNNNN.dat"` with zero-padded 6-digit index |
+| `parsePeerKey` | `std::pair<std::string, uint16_t> parsePeerKey(const std::string& key)` | Parses `host:port` or `[ipv6]:port` into host string and port number |
+
+### RpcServer (new static methods)
+
+| Function | Signature | Description |
+|----------|-----------|-------------|
+| `makeJsonRpcError` | `json makeJsonRpcError(json id, int code, const std::string& message, json data = nullptr)` | Constructs JSON-RPC error response |
+| `makeJsonRpcResult` | `json makeJsonRpcResult(json id, json result)` | Constructs JSON-RPC success response |
+
+### PeerManager (modified methods)
+
+| Method | Change | Description |
+|--------|--------|-------------|
+| `broadcast_block` | Removed | Replaced by `send_to_peers` |
+| `relay_block` | Removed | Replaced by `send_to_peers` |
+| `send_to_peers` | Added | `void send_to_peers(const Block& block, const std::string& exclude_key = "")` |
+
+### TestHelpers.hpp (new, test-only)
+
+| Function | Signature | Description |
+|----------|-----------|-------------|
+| `createTestDir` | `std::filesystem::path createTestDir(const std::string& name)` | Creates temp directory for test isolation |
+| `cleanupTestDir` | `void cleanupTestDir(const std::filesystem::path& dir)` | Removes temp directory |
+| `defaultConsensusConfig` | `ConsensusConfig defaultConsensusConfig()` | Returns config with difficulty=0 |
+| `mineTestBlock` | `Block mineTestBlock(...)` | Mines a block with given parameters |
+| `buildValidChain` | `std::vector<Block> buildValidChain(size_t length, ...)` | Builds a valid chain of N blocks |
+
+---
+
+## Relationships
+
+```
+Blockchain
+├── has-a DifficultyCache (1:1, in-memory)
+├── has-a ChunkRetainSet (1:1, transient per operation)
+├── has-many Chunk (1:N, persisted)
+└── has-many Block (via Chunks)
+
+BlockPropagation
+├── has-a PendingPool (1:1, map + deque)
+└── uses parsePeerKey() for sender key parsing
+
+PeerManager
+└── uses send_to_peers() (unified from broadcast/relay)
+
+RpcServer
+└── uses makeJsonRpcError/makeJsonRpcResult (shared helpers)
+```

specs/016-audit-remediation/plan.md

@@ -0,0 +1,88 @@
+# Implementation Plan: Code Audit Remediation
+
+**Branch**: `016-audit-remediation` | **Date**: 2026-04-12 | **Spec**: [spec.md](spec.md)
+**Input**: Feature specification from `/specs/016-audit-remediation/spec.md`
+
+## Summary
+
+Remediate 5 bugs, 6 duplication clusters, 4 performance issues, and thread-safety gaps identified in the code audit (docs/AUDIT.md). The approach is targeted fixes with shared utilities rather than architectural refactoring: fix `getChainBlockCount()` to use cached total, enforce single-threaded `io_context` with a runtime assertion, add difficulty caching per adjustment boundary, retain chunks during multi-access operations, extract shared helpers for chunk filenames/RPC responses/peer sending/test setup, fix IPv6 sender-key parsing, add verify-then-cache for received merkle roots, replace pending-pool linear scan with `unordered_map` + `deque`, and fix the premature `dirty_` flag clear.
+
+## Technical Context
+
+**Language/Version**: C++20 (`-std=c++20`)
+**Primary Dependencies**: Boost (Asio, Serialization), OpenSSL (EVP SHA-256), nlohmann/json (vendored `src/json.hpp`)
+**Storage**: Boost.Serialization binary chunk files (`chunk_NNNNNN.dat`), keys (`keys.dat`), streams (`streams.dat`, `stream_index.dat`)
+**Testing**: Catch2
+**Target Platform**: Linux, macOS, Windows (cross-platform per constitution §VII)
+**Project Type**: CLI / daemon (blockchain node)
+**Performance Goals**: Low-latency query/response per constitution §X. Difficulty calculation must not scale with chain length.
+**Constraints**: Must use GNU Autotools (constitution §II). Must use `make -j8`. Dependencies limited to approved set (constitution §V).
+**Scale/Scope**: ~5,900 lines in `src/`, ~6,750 lines in `tests/`. 13 functional requirements, 6 user stories.
+
+## Constitution Check
+
+*GATE: Must pass before Phase 0 research. Re-check after Phase 1 design.*
+
+| Principle | Status | Notes |
+|-----------|--------|-------|
+| I. Language Standard | PASS | All changes are C++20 compatible |
+| II. Build System | PASS | GNU Autotools unchanged. All `make` with `-j8` |
+| III. Full Test Coverage | PASS | New tests for each bug fix. Catch2 + mocks. Individual binary execution |
+| IV. Code Style | PASS | Follow existing codebase conventions |
+| V. Minimal Dependencies | PASS | No new dependencies. All within approved set |
+| VI. Mandatory TLS | PASS | No TLS changes |
+| VII. Cross-Platform | PASS | No platform-specific code introduced |
+| VIII. Feature Branches | PASS | On `016-audit-remediation` branch |
+| IX. Pre-1.0 API Stability | PASS | Internal refactoring only. No protocol changes |
+| X. Low-Latency Performance | PASS | Performance improvements (difficulty cache, chunk retention, O(1) eviction) |
+| XI. MIT License | PASS | No new third-party code |
+| XII. .gitignore | PASS | No new build targets |
+| XIII. Roadmap Currency | PASS | Will update ROADMAP.md on completion |
+
+**Gate result**: ALL PASS — proceed to Phase 0.
+
+## Project Structure
+
+### Documentation (this feature)
+
+```text
+specs/016-audit-remediation/
+├── plan.md              # This file
+├── research.md          # Phase 0 output
+├── data-model.md        # Phase 1 output
+├── quickstart.md        # Phase 1 output
+├── contracts/           # Phase 1 output (N/A — internal refactoring)
+└── tasks.md             # Phase 2 output (not created by /speckit.plan)
+```
+
+### Source Code (repository root)
+
+```text
+src/
+├── Block.cpp            # FR-009: verify-then-cache merkle root
+├── Block.hpp            # FR-009: new constructor overload
+├── Blockchain.cpp       # FR-001, FR-003, FR-004, FR-011, FR-012: block count, difficulty cache, chunk retention, dirty flag, stream entries
+├── Blockchain.hpp       # FR-003: difficulty cache member
+├── BlockPropagation.cpp # FR-008, FR-013: IPv6 parsing, O(1) pending pool
+├── BlockPropagation.hpp # FR-013: data structure change
+├── PeerManager.cpp      # FR-007: unified send_to_peers()
+├── main.cpp             # FR-002: single-thread assertion + documentation
+├── utils.cpp            # FR-005, FR-008: chunkFilename(), parsePeerKey()
+├── utils.hpp            # FR-005, FR-008: utility declarations
+├── network/
+│   └── RpcServer.cpp    # FR-006: shared makeErrorResponse()
+
+tests/
+├── TestHelpers.hpp      # FR-010: shared test utilities (NEW)
+├── block_tests.cpp      # Updated to use TestHelpers
+├── consensus_tests.cpp  # Updated to use TestHelpers
+├── chunk_persistence_tests.cpp  # Updated to use TestHelpers
+├── lifecycle_tests.cpp  # Updated to use TestHelpers
+└── ... (all test files updated for shared helpers)
+```
+
+**Structure Decision**: All changes are in existing `src/` and `tests/` directories. One new file: `tests/TestHelpers.hpp`. No structural changes to project layout.
+
+## Complexity Tracking
+
+No constitution violations to justify.