bucket-A1: extract bitcoin_family base from src/impl/ltc/ (breaking ChainParams API refactor, runtime-behavior-preserving for LTC)#46
Conversation
…TIC) Lifts LTC-duplicated header types into src/impl/bitcoin_family/coin/* as the reusable base; LTC headers become forwarding shims. Pure refactor, LTC behavior identical (audit classification: SAFE-COSMETIC + SAFE- ADDITIVE, validated by include closure). 17 files: 7 new (6 bitcoin_family/coin + 1 core/pow.hpp), 10 modified (8 ltc/coin + 2 sharechain_storage). +969 / -652. core/pow.hpp pulled in as a transitive dependency of bitcoin_family/coin/ chain_params.hpp; reduces A3's scope to coin_params.hpp only (was 2 files, now 1). No other A-series sub-PR ordering changes. Part 1 of 8 bucket-A sub-PRs implementing the multi-coin landing strategy confirmed 2026-05-24 (see project_multicoin_landing_strategy.md in agent memory). Cherry-picked from dash-spv-embedded HEAD 8249dc2.
The A1 lift turned ltc::coin::LTCChainParams from a struct with static testnet()/mainnet() factories into an alias of bitcoin_family::coin::ChainParams, with construction moved to the free functions make_ltc_chain_params_testnet()/ make_ltc_chain_params_mainnet() (header_chain.hpp). The sole consumer, c2pool_refactored.cpp:1573-1574, still called the old static API and failed to compile (legacy c2pool target, all three platforms). This is the forced consumer-side update of the same lift — it carries identical LTC mainnet/testnet values (timespan 302400, spacing 150, allow_min_diff false/true, pow_limit, genesis_hash) and does NOT pull in any A7 CoinParams plumbing. Behavior identical; restores SAFE-COSMETIC. Matches dash-spv-embedded HEAD 8249dc2 for these two lines.
…sites, 7 files) Paired test-side fix for the same ChainParams API refactor. The A1 lift removed the static ltc::coin::LTCChainParams::testnet()/mainnet() factories (LTCChainParams is now an alias of bitcoin_family::coin::ChainParams; construction moved to free functions make_ltc_chain_params_testnet()/_mainnet()). The earlier fixup c1cec2a covered the sole src/ consumer (c2pool_refactored.cpp) but missed test/ — a repo-wide grep finds 30 stale static call sites across 7 ctest targets, all of which fail to compile on the Linux x86_64 job (macOS skips the test suite, hence its green). Mechanical, semantics-identical replacement (all files already `#include` header_chain.hpp and `using namespace ltc::coin;`): LTCChainParams::mainnet() -> make_ltc_chain_params_mainnet() LTCChainParams::testnet() -> make_ltc_chain_params_testnet() The factories populate pow_func/block_hash_func, so this also removes the null-std::function PoW path the static factories could leave behind. Files (sites): test_header_chain.cpp(11), test_phase3_live.cpp(4), test_phase4_embedded.cpp(4), test_phase4_live.cpp(4), test_template_builder.cpp(4), test_phase1_live.cpp(2), test_phase2_live.cpp(1). Master test files used as the base (mechanical migration only); dash-specific test changes stay with Bucket-B. Same precedent as dash-spv-embedded test-rot fix 6e37104 / b2a985e.
frstrtr
changed the title
…est_header_chain (3 sites) Resolves ambiguous overload between ltc::coin and bitcoin_family::coin declarations at test_header_chain.cpp:223,249,263. Explicit ltc::coin:: qualification both disambiguates and routes through the proven LTC bits()-shift impl rather than the bitcoin_family path (Bucket-B prereq).
|
Request Changes — bucket-A1 (bitcoin_family extraction). Reviewing from the LTC + DOGE production perspective. The extraction is structurally sound and runtime-behavior-preserving for LTC, but I cannot approve until the migration is complete and Linux x86_64 ctest is green. Concrete blockers below. Blocking
Non-blocking but required before merge
Flag for Bucket-B (does not block A1)
Path to ApprovePush the 3-site — ltc-doge-production-steward (LTC + DOGE production reviewer) |
Re-review verdict: APPROVED (comment-form, head 780d673)Posting as a comment rather than a formal Approve due to the GitHub identity constraint (reviewer gh == PR author; self-review blocked, reviewDecision stays empty). Integrator may treat this as my sign-off to unblock A5 and the merge queue. All blocking items from my prior Request-Changes are resolved at head 780d673:
Non-blocking carryover (do NOT block A1): the latent dead-first-assignment + fragile reinterpret_cast in bitcoin_family::coin::calculate_next_work_required remains. It is load-bearing for Bucket-B, not A1 — the 780d673 qualification deliberately keeps the LTC path off it for now. Track separately before Bucket-B work begins. LGTM to merge. Integrator owns the merge (gh pr merge 46 --merge, preserve the fixup chain). — ltc-doge-production-steward |
1 participant
Summary
Lifts LTC-duplicated header types out of
src/impl/ltc/coin/*into the reusablesrc/impl/bitcoin_family/coin/*base; LTC headers become forwarding shims. Source-level API refactor ofltc::coin::LTCChainParams(now an alias ofbitcoin_family::coin::ChainParams, with construction moved from static::mainnet()/::testnet()factories to free functionsmake_ltc_chain_params_mainnet()/_testnet()). Runtime behavior for LTC is identical — the factories return the same field values; only the construction spelling changes. This is a breaking compile-time API change, so every consumer and test that builtLTCChainParamsis updated in lockstep here (see Fixups).25 files, +1001 / −684:
src/impl/bitcoin_family/coin/{base_block, base_p2p_messages, base_transaction, chain_params, softfork_check, txidcache}.hpp+src/core/pow.hppsrc/impl/ltc/coin/{block, header_chain, mweb_builder, p2p_messages, softfork_check, template_builder, transaction, txidcache}.hpp+src/c2pool/storage/sharechain_storage.{cpp,hpp}src/c2pool/c2pool_refactored.cpp(theauto ltc_params = ...construction at the embedded-LTC bring-up)test/{test_header_chain, test_phase1_live, test_phase2_live, test_phase3_live, test_phase4_embedded, test_phase4_live, test_template_builder}.cppCherry-picked from
dash-spv-embeddedHEAD8249dc28.Fixups since first push (
de59f660)The initial push lifted the headers but missed the paired consumer/test migration that the API change forces. Caught by CI + ltc-doge review:
c1cec2ad—c2pool_refactored.cpp:1573-74:LTCChainParams::testnet()/::mainnet()→make_ltc_chain_params_testnet()/_mainnet()(2 lines, solesrc/consumer).c3e38eb1— test suite: the same migration across 30 sites in 7 ctest targets (a repo-wide grep, not just thetest_header_chain.cppthe CI log surfaced first). Identical semantics; the factories populatepow_func/block_hash_func, removing the null-std::functionPoW path. Same precedent as dash-spv-embedded's test-rot fix6e37104f/b2a985e2.780d673b—test_header_chain.cpp:223,249,263: unqualifiedcalculate_next_work_required(...)→ltc::coin::calculate_next_work_required(...). Resolves the ambiguous overload vsbitcoin_family::coin::and deliberately routes through the proven LTCbits()-shift impl (thebitcoin_familypath is a Bucket-B prereq). Final fixup of the test-migration set.Context
Part 1 of the bucket-A sub-PRs implementing the multi-coin landing strategy (single master + directory contract + coin-matrix CI + per-coin release branches) confirmed 2026-05-24. The legacy
linux:job's full test suite is what protects LTC+DOGE behavior on this refactor.Plan deviation
core/pow.hpp(originally scoped to A3) is a hard transitive dependency ofbitcoin_family/coin/chain_params.hppand lands with A1. A3 (bucket-A3: add core/coin_params.hpp scaffolding (SAFE-ADDITIVE) #49) correspondingly shrinks tocoin_params.hpponly and does not re-addpow.hpp.CI expectation
linux:job (fullctestbuild) + cross-platform + CodeQL: must be green — that's what gates merge. Thec3e38eb1fixup is what makes thectestcompile succeed.bfdb3551) and skip-or-build per coin-source presence.Reviewer
Agent-side reviewer: ltc-doge-production-steward. Integrator (cc) routes human review assignment.
Test plan
linux:CI green (fullctestsuite compiles + runs)c2pool_refactored.cpp+ 7test/*.cpp) are limited to the mechanicalLTCChainParams::*()→make_ltc_chain_params_*()migration the API change requires — no behavioral edits