Repository navigation
node: package envs into NodeEnv, reduce test flakes from envs, remove env locks - #1211
Merged
Merged
Conversation
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
absorb_inbound_env read RBITCOIN_P2P_MAX_INBOUND from the process env, deep inside operator_config_from_args. Any test that assembled a config could see a value another test had set, which is how trust_peer_addr read 91 instead of the default. main now reads the env once with NodeEnv::from_env and passes it to cli_main, which threads it through operator_config_from_args and finish_operator_config. The journey-node harness main does the same. Nothing below the process shell reads the var. inbound_env_is_below_cli_and_conf drives the shipped parser with an explicit env and pins the precedence: CLI and conf win, and zero or an unparsable value keeps the default. It replaces the two tests that set the var on the process.
OPERATOR_ENV_TEST_LOCK serialized the node tests that set RBITCOIN_P2P_MAX_INBOUND against the tests that read it. Those tests now pass NodeEnv instead, and no node test writes the process env, so the lock guards nothing.
cli_main read RBITCOIN_TEST_DROP_STORE from the process env on the --smoke path. The scenario that pins the shutdown error path set it with set_var under a lock that only that block could take, so any other test running cli_main --smoke at the same moment would also lose its store. NodeEnv now carries the flag as test_drop_store. It is on only for 1 or true (any case); before, any value turned it on, including an empty one. The scenario passes it through NodeEnv::from_vars and no longer writes the process env.
…as NodeEnv IbdConfig::default() read the block-queue assign-stop from the process env through rbitcoin_query::bq_assign_stop_bytes, so every IbdConfig built anywhere, tests included, saw whatever another test had set. NodeEnv now resolves the cap with the same rules: RBITCOIN_BLOCK_QUEUE_BYTES wins over RBITCOIN_BLOCK_QUEUE_GB, 0 is unlimited, and an unparsable value falls through. finish_operator_config copies it onto NodeConfig::bq_assign_stop_bytes, and run_p2p passes it into both IBD configs it builds (catch-up and the stale-tip retry). IbdConfig::default() is the plain 1 GiB constant, and bq_assign_stop_bytes is gone along with the two soft_densify tests that set the vars on the process.
Several docs and comments named an env knob by its suffix (`_GB`, `_BYTES`) or dropped the RBITCOIN_ prefix (`PIN_IO`, `SPEND_ANN`, ...). A grep for the full name missed them. They now use the full names. The BQ_ASSIGN_STOP_BYTES comment also says the node applies the override, since rbitcoin-query no longer reads the env itself.
rbitcoin-query read RBITCOIN_SH_FORCE_REBUILD from the process env on every tip SH finalize, through sh_builder::sh_force_rebuild. Any query test that ran finalize_sh_runs while the force-rebuild test had the var set took the full Class A rebuild path instead. NodeEnv now carries the flag as sh_force_rebuild, on only for 1 or true (any case), the same rule as before. finish_operator_config copies it onto NodeConfig, and apply_startup_index_mode sets it on the Query with set_sh_force_rebuild, beside set_sh_index_enabled. catch-up reads the Query field. The env var stays sticky: while it is set, every start rebuilds. sh_force_rebuild in sh_builder is gone, and the force-rebuild test sets the flag on its Query instead of the env. The 1-or-true rule is now one helper, flag_on, shared with RBITCOIN_TEST_DROP_STORE.
FORCE_ENV_LOCK serialized the query catch-up tests against the one test that set RBITCOIN_SH_FORCE_REBUILD. That test now sets the flag on its Query, and no query test writes the process env, so the lock guards nothing.
submitpackage read RBITCOIN_RPC_PACKAGE_DIALECT from the process env on every call. No test could turn the gate on without racing every other package RPC in the process, so it was only pinned on the helper. NodeEnv now carries the flag as rpc_package_dialect, on only for 1 or true (any case), as before. finish_operator_config copies it onto NodeConfig::rpc, the node passes it in RpcConfig, and the server puts it on RpcContext beside blocksonly. submitpackage reads the context. package_dialect_from_env and rpc_package_dialect are gone; the node's flag test covers the parse. submitpackage_topology_gate_follows_the_context drives the RPC with the flag on and off.
tip_accept::wait_for_rpc read RBITCOIN_RPC_WAIT_TIP_IDLE from the process env on every getblockcount, getbestblockhash, and wait_height. NodeEnv now carries the flag as rpc_wait_tip_idle, on only for 1 or true (any case), as before. finish_operator_config copies it onto NodeConfig::rpc, and run_p2p sets it on the ChainHub with set_rpc_wait_tip_idle, beside set_prefill_compact. wait_tip_stable_for_rpc picks wait_idle or wait_current_job from the hub. wait_for_rpc and wait_tip_idle_from_env are gone; the node's flag test covers the parse. The new net test pins the default and runs both branches on an idle lane. It does not time the two waits against each other: begin_job runs when a job starts, not when it is queued, so wait_idle can return in the gap between one job ending and the next queued job starting, and no in-process test can order that.
NodeEnv::from_vars dropped a value it could not parse and kept the default, so a typo such as RBITCOIN_SH_FORCE_REBUILD=on or RBITCOIN_BLOCK_QUEUE_GB=1.5 started the node as if the var were unset. from_vars and from_env now return Result<NodeEnv, NodeError>. A set value that is not UTF-8 or does not parse is a NodeError::Config naming the var and the value. An empty value is the same as unset. Flags take 1 or true (on) and 0 or false (off), true and false in any case. RBITCOIN_P2P_MAX_INBOUND=0 now means no inbound slots, as --max-inbound 0 does, where it used to keep the default. Both process shells call the new process_main, which prints the error and exits 2 before anything starts, the same as a bad argument. cli_main still takes a parsed NodeEnv.
NodeEnv::from_vars walked std::env::vars_os and compared each name with ==. std::env::var, which it replaced, ignores case on Windows, so a lower-case rbitcoin_sh_force_rebuild=1 that used to wipe and recollect there was skipped with no error, and the same for every other knob. NodeEnv::from_lookup now asks for each knob by name, and from_env passes std::env::var_os, so the platform matches names as it did before NodeEnv. from_vars is gone. Tests read literal pairs through from_lookup, and env_lookup_owns_the_name_match passes a lookup that ignores case and checks that a lower-case name sets the flag.
phlip9-github-agent
Bot
force-pushed
the
phlip9/node-env
branch
from
October 25, 2026 18:07
3ca781f to
cc16207
Compare
Contributor
There was a problem hiding this comment.
APPROVE
The Windows name match is fixed. from_env asks std::env::var_os for each knob, so the platform case rule is the one std::env::var already used, and env_lookup_owns_the_name_match covers a case-insensitive lookup. Nothing new.
0 bugs, 0 suggestions, 0 nits.
phlip9-github-agent Bot
pushed a commit
to phlip9/rbitcoin
that referenced
this pull request
Oct 25, 2026
reardencode#1204 built an RpcContext literal in the health metrics test. reardencode#1211 added package_dialect to RpcContext and updated the other test literals to false. The two pulls merged cleanly and left this initializer missing the field, so the node test crate does not compile. package_dialect is false here, the off default those other literals use. The rest of reardencode#1204 already matches the NodeEnv, NodeConfig, and RpcContext fields reardencode#1211 added, and it does not call the helpers that pull removed. Co-authored-by: Brandon Black <reardencode@users.noreply.github.com>
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
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.
Follow-up to PR #1205. Fix env-related test flakes in
rbitcoin-nodemoresystematically.
IMO, code should try to only read envs at the outer "shell" of a program,
like we already do for CLI args. Code should absolutely avoid ever calling
env::set_varorenv::remove_var; these are in factunsafe(and properlymarked as-so in edition 2024), since they mutate shared global state w/o a
lock. Also don't silently ignore bad
RBITCOIN_*envs.This PR mostly covers
rbitcoin-node. Other crates likerbitcoin-storewhichalso touch a bunch of envs will be covered in a follow-up.
(AI dump)
Follow-up to #1205. Library code read
RBITCOIN_*knobs from the processenv at the point of use, and tests changed them with
set_varunder a lockaround the writer. A lock around the writer does not cover the reader: any
test that reached
operator_config_from_args, anIbdConfig::default(), aQuerycatch-up, orsubmitpackagesaw whatever another test had set.cli::trust_peer_addrreading91was one case.mainnow reads these knobs once (process_main→NodeEnv::from_env) andpasses the
NodeEnvtocli_main.journey-nodecalls the sameprocess_main. Nothing below the process shell reads them, so no test inrbitcoin-node,rbitcoin-query,rbitcoin-rpc, orrbitcoin-testwritesthe process env any more.
RBITCOIN_P2P_MAX_INBOUNDNodeConfig::absorb_inbound_env(&NodeEnv)absorb_inbound_envRBITCOIN_TEST_DROP_STORENodeEnv::test_drop_store, checked bycli_main --smokecli_mainRBITCOIN_BLOCK_QUEUE_BYTES/RBITCOIN_BLOCK_QUEUE_GBNodeConfig::bq_assign_stop_bytes→ both IBD configsrun_p2pbuildsIbdConfig::default()RBITCOIN_SH_FORCE_REBUILDNodeConfig::sh_force_rebuild→Query::set_sh_force_rebuildinapply_startup_index_modeQuerycatch-upRBITCOIN_RPC_PACKAGE_DIALECTRpcOpts::package_dialect→RpcConfig→RpcContext::package_dialectsubmitpackageRBITCOIN_RPC_WAIT_TIP_IDLERpcOpts::wait_tip_idle→ChainHub::set_rpc_wait_tip_idletip_accept::wait_for_rpcThe last commit makes the parse strict. A set value that does not parse
used to be dropped, so
RBITCOIN_SH_FORCE_REBUILD=onorRBITCOIN_BLOCK_QUEUE_GB=1.5ran as if unset.NodeEnv::from_envnowreturns a
Result, and both shells callprocess_main, which printserror: configuration error: RBITCOIN_…and exits 2 before anythingstarts, the same as a bad argument. An empty value is the same as unset.
Flags take
1/trueor0/false.RBITCOIN_P2P_MAX_INBOUND=0now meansno inbound slots, as
--max-inbound 0does, where it used to keep thedefault. Before that commit the refactor keeps the old parse, except that
RBITCOIN_TEST_DROP_STORE(test-only) no longer turns on for any value.The deleted
*_from_envhelpers move intoNodeEnv::from_lookup, whichasks for each knob by name.
from_envpassesstd::env::var_os, so namesmatch as they did before (ignoring case on Windows), and tests pass literal
pairs.
Deleted with their last writer:
OPERATOR_ENV_TEST_LOCK(including thetrust_peer_addruse from #1205), the query catch-upFORCE_ENV_LOCK, thesoft_densifytestENV_LOCK, and theDROP_STORE_ENVlock inscenarios.rs.submitpackage's topology gate is now pinned throughdispatchwith the flag on and off, which a test could not do while theflag was process env.
docs/env-knobs.mdlists the knobsmainreads. A docs commit spells outenv names that were written as a suffix (
_GB,_BYTES) or without theRBITCOIN_prefix, so a grep for the full name finds them.Found on the way
tip_accept::wait_idlecounts a job as in flight frombegin_job, whichruns when the job starts, not when it is queued. It can return between one
job ending and the next queued job starting, so "waits until the tip-accept
lane is empty" is stronger than what it does. This PR does not change it,
and the new
ChainHubtest only pins the default and runs both branches onan idle lane.
Not in this PR
The store crate's knobs (
RBITCOIN_IO,RBITCOIN_CLASS_C_INRAM_MAX_MB,RBITCOIN_TX_HEAD_BITS,RBITCOIN_BULK_IO_WORKERS, and the rest) andRBITCOIN_LOG. The store has no options struct to carry them yet. Afterthose move, a clippy
disallowed-methodsban onstd::env::var*,set_var, andremove_varoutside the process shells keeps this fromcoming back.
changelog.d/node-env-malformed.mdcovers the strict parse for operators.