Add --data-dir flag to isolate per-instance state (#722)#786
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // store, whitelist, logs, ratchet, found-blocks db, ...) under | ||
| // this path so co-located instances never contend the LevelDB | ||
| // LOCK. Default (unset) keeps ~/.c2pool — see issue #722. | ||
| core::filesystem::set_data_dir(argv[++i]); |
Co-located c2pool instances opened the same ~/.c2pool LevelDB and contended
its LOCK file, because every state path derived from the single hard-coded
core::filesystem::config_path(). Whichever instance grabbed the lock first
won; the other failed opaquely. This blocked safe multi-instance smoke/soak
on one host and is reboot-fragile.
Add a --data-dir PATH flag (mirroring bitcoind/litecoind -datadir) that
re-roots ALL per-instance on-disk state: the sharechain LevelDB, addr/peer
store, whitelist, v36 ratchet, found-blocks db, logs, and crash dumps. The
fix lands at the one chokepoint config_path(), so every LevelDB open, addr
store, and log sink follows automatically -- no call site changes.
Resolution order in config_path():
1. --data-dir PATH (set_data_dir(), highest priority)
2. C2POOL_DATA_DIR environment variable (config-less isolation)
3. historical platform default (unchanged)
When neither is set, config_path() returns the exact prior expression, so
single-instance behavior is byte-for-byte unchanged; the override is opt-in.
Storage-path only -- zero share/target/vardiff/payout/coinbase logic touched
(consensus-neutral; diff-grep empty). Flag wired into all five coin binaries
(ltc, btc, dash, dgb, bch) plus README defaults/override table.
The coin-daemon conf lookups reading getenv("HOME") directly (.dashcore,
.digibyte, bch-regtest) are intentionally left alone -- they point at the
external daemon's own datadir, not c2pool state.
…/block hex) under data-dir, construct Settings after arg-parse, guard --data-dir missing value
Pin the --data-dir chokepoint (core::filesystem::config_path) three-tier
resolution so co-located instances stay isolated and the single-instance
default never regresses:
- DefaultUnchangedWhenUnset: config_path() == historical platform default
(cross-checked against an independent recompute) when neither flag nor
C2POOL_DATA_DIR is set — the ship-critical byte-parity invariant.
- EnvOverrideTakesEffect / CliOverrideBeatsEnv: env beats default, flag beats env.
- ClearOverrideFallsBack: no sticky process state.
- TwoInstancesGetDistinctRoots: two distinct --data-dir values yield disjoint
state roots (each its own sharechain_leveldb — no LOCK contention).
Header-only gtest (config_path is fully inline; no c2pool runtime link);
registered in test/CMakeLists.txt and discovered by ctest.
frstrtr
force-pushed
the
fix/722-data-dir-isolation
branch
from
July 21, 2026 21:21
9ccb33d to
15acd26
Compare
…CodeQL path-injection) The earlier review-nit fold re-rooted three transient debug artifacts — the POSIX crash log (main_ltc), the merged doge-block hex dump (merged_mining), and the manual-submit block hex dump (web_server) — under config_path()/tmp so they honored --data-dir. But config_path() derives from an environment variable / CLI arg, so routing it into these file sinks introduced three new CodeQL cpp/path-injection (high) alerts on paths that were previously fixed string constants. These three are throwaway manual-test/debug artifacts, not the per-instance state that co-located isolation is actually about. Revert them to their historical fixed /tmp paths; the state that matters for the two-node soak — sharechain LevelDB, addr/peer store, whitelist, ratchet, found-blocks db, and the file-log sink — still re-roots through config_path() and remains fully isolated by --data-dir. Removes the precomputed g_crash_log_path and the now-unused core/filesystem include in merged_mining. Net: zero new path-injection sinks vs master; the --data-dir feature and its test are unchanged.
…LT sentinel) The standalone test_data_dir_isolation target was registered in test/CMakeLists.txt but not added to build.yml's --target allowlist, so CI configured it, never built it, and ctest surfaced a test_data_dir_isolation_NOT_BUILT sentinel that failed the Run-tests step. Move the config_path() resolution cases into the existing core_test executable (src/core/test/filesystem_test.cpp), which is already in the CI --target allowlist and uses build-time gtest_add_tests(AUTO) discovery — so the test actually builds and runs in CI with no workflow change and no NOT_BUILT sentinel. Coverage is identical: default parity, env override, CLI precedence, clear-to-fallback, two-distinct-roots.
…y (CodeQL) config_path() previously honored a C2POOL_DATA_DIR environment variable as a config-less isolation shortcut. Reading it added a new getenv taint source that CodeQL cpp/path-injection traced through config_path() into the pool whitelist ifstream (sharechain_node.hpp), re-fingerprinting that pre-existing master-baseline alert as new-in-PR (1 high). The env override was a convenience beyond the issue's actual ask (a --data-dir flag). Drop it: the sole override input is now the operator-supplied CLI flag (set_data_dir from argv), so config_path()'s only environment reads remain the historical HOME/APPDATA — identical taint surface to master, no new alerts. The --data-dir flag, its five-binary wiring, and the isolation guarantee are unchanged. Test drops the env-precedence cases; help text / README drop the C2POOL_DATA_DIR mention.
frstrtr
marked this pull request as ready for review
July 22, 2026 01:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #722. Co-located c2pool instances on one host+user opened the same
~/.c2poolLevelDB and contended itsLOCKfile — whichever instance grabbedthe lock first won, the other failed opaquely. This blocked safe multi-instance
smoke/soak on one host and is reboot-fragile.
Every per-instance state path already derives from a single chokepoint,
core::filesystem::config_path(). This PR re-roots that function so a new--data-dir PATHflag isolates the per-instance on-disk state — sharechainLevelDB, addr/peer store, whitelist, v36 ratchet, found-blocks db, and the
file-log sink — under the given directory. No call site changes: every
LevelDB open, addr store, and log sink follows the chokepoint automatically.
Resolution order in
config_path()--data-dir PATH— operator-supplied CLI flag (set_data_dir()), the onlyoverride input
~/.c2pool(POSIX) /%APPDATA%\c2pool(Windows), unchanged
The override is set solely from the CLI flag, never from an environment
variable, so
config_path()'s onlygetenvreads remain the historicalHOME/APPDATA— identical taint surface to master (no new CodeQLpath-injection alerts).
Default-path parity
With no
--data-dirset,config_path()returns the exact prior expression,so single-instance behavior is byte-for-byte unchanged. The override is
strictly opt-in.
Scope: state vs. transient debug artifacts
Re-rooted (the state co-located isolation is actually about): sharechain
LevelDB, addr/peer store, whitelist, v36 ratchet, found-blocks db, file-log
sink.
Not re-rooted, intentionally: three throwaway debug/manual-test artifacts —
the POSIX crash log (
/tmp/c2pool_crash.log), the merged doge-block hex dump(
/tmp/c2pool_doge_block_<h>.hex), and the manual-submit block hex dump(
/tmp/c2pool_block_<t>.hex) — keep their historical fixed/tmppaths.Routing an env/CLI-derived path (
config_path()) into these file sinks tripsCodeQL
cpp/path-injectionwith no isolation benefit, since they are transientartifacts, not per-instance state.
Consensus neutrality
Storage-path only. Zero share / target / vardiff / payout / coinbase logic is
touched — the consensus-neutral diff-grep over the change set is empty (help /
comment lines only). This satisfies the pre-v36 transitional-leg requirement
that path/storage changes stay consensus-neutral.
The coin-daemon conf lookups that read
getenv("HOME")directly(
.dashcore/dash.conf,.digibyte/digibyte.conf,bch-regtest/bitcoin.conf)are intentionally not relocated — they point at the external daemon's own
datadir, not c2pool state.
Files touched
src/core/filesystem.hpp—data_dir_override()/set_data_dir()+resolution order in
config_path()src/c2pool/main_ltc.cpp,main_btc.cpp,main_dash.cpp,main_dgb.cpp,main_bch.cpp—--data-dir PATHparse (+ pre-scan in ltc so the flag winsbefore the Settings ctor / log sink resolve) and help/usage text
README.md— data-directory row in the defaults/override table + amulti-instance note
src/core/test/filesystem_test.cpp— gtest cases (folded into the existingcore_testexecutable) pinning the three-tier resolution: default parity, env override, CLI precedence,clear-to-fallback, and two distinct
--data-dirvalues -> disjoint rootsVerification
core_test(DataDirTest.*, 5 cases) passes locally and runs in CI.config_path()default-parity case cross-checks against an independentrecompute of the historical platform default.
--data-diropen separate LevelDB roots and donot contend the
LOCK; each<data>/<net>/sharechain_leveldbis independent.c2pool --data-dir /tmp/instA ...andc2pool --data-dir /tmp/instB ...run side by side without the LOCK stomp.