Add network field to GlobalInfo (GET /api/v1/global)#367
Add network field to GlobalInfo (GET /api/v1/global)#367vnprc wants to merge 4 commits intostratum-mining:mainfrom
network field to GlobalInfo (GET /api/v1/global)#367Conversation
|
Overall this looks clean and well-structured, especially how the changes are split across components and gated behind the monitoring feature. A few things I think we should address before merging: Must fix before merge 1. Even though It would be better to make this dependency conditional on the 2. Missing TLS support and no URL validation Right now: reqwest = { version = "0.12", default-features = false, features = ["json"] }With default features disabled and no TLS backend enabled ( The config docs say Two reasonable fixes here:
3. network value isn’t validated Right now any string from the TOML config is accepted. That makes it easy for typos like It would be safer to either:
Suggestions (non-blocking) Lock poisoning handling
Startup retry behavior If the first fetch fails, the code waits the full 60 seconds before retrying. That means Integration test timing The current 10-second wait could be a bit tight on slower CI machines. Bumping it to 20–30 seconds or documenting why 10 is safe would make this more robust. Test coverage The test covers the happy path, but it’d be good to also check:
Questions
Verdict Needs changes. The unconditional |
fd8c1a3 to
fc7e0ce
Compare
Add `network: Option<String>` to `GlobalInfo` so the Bitcoin network is machine-readable from `GET /api/v1/global` without consulting Prometheus or config files. - `GlobalInfo` gets `pub network: Option<String>` - `ServerState.network`: `Option<String>` → `Arc<RwLock<Option<String>>>` - `MonitoringServer::with_network(self, Option<String>)` writes into the existing Arc so any handle obtained via `network_handle()` remains valid after the call - `MonitoringServer::network_handle()` returns a clone of the Arc for background tasks that need to update network after `run()` consumes self - `handle_global` reads via `.read().unwrap().clone()` - Tests: `global_endpoint_with_no_sources` asserts network is null; new `global_endpoint_network_field` asserts null and populated cases
Pool infers the Bitcoin network from the sv2-tp port in tp_address using well-known default ports (8442=mainnet, 18442=testnet3, 48442=testnet4, 38442=signet, 18447=regtest). An explicit `network` config field remains as an optional override for non-standard port setups. Translator fetches network from pool's /api/v1/global once per upstream connection instead of polling every 60 seconds. Retries are handled by the existing reconnect logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove reqwest from translator entirely; replace with a plain hyper
HTTP/1.1 client inside stratum-apps/monitoring. hyper is already
compiled transitively via axum, so no new Cargo.lock entries are
introduced.
- MonitoringServer gains with_upstream_monitoring_url() which validates
the http:// scheme, stores the URL, and spawns a one-shot fetch in
run(). The old network_handle() method (which leaked internal Arc state
into application code) is removed.
- Translator chains .with_upstream_monitoring_url() onto the
MonitoringServer builder; fetch_network_from_pool() private function
removed from translator/mod.rs.
- Fix network_from_tp_port to use bitcoin-cli / getblockchaininfo names:
"main" (was "mainnet"), "test" (was "testnet3"). Add VALID_NETWORKS
constant; effective_network() validates explicit overrides against it.
Add valid_networks_covers_known_port_outputs unit test.
- Change all RwLock .unwrap() to .expect("network lock poisoned").
- Rename integration test to global_info_network_from_config_override;
bump polling deadline 10s -> 30s; add
global_info_network_unreachable_upstream test.
…tum-apps Move network_from_tp_port() and VALID_NETWORKS from pool-apps (private) into stratum-apps/src/tp_type.rs as public API. Add BitcoinNetwork::as_network_str() and TemplateProviderType::infer_network() so any application using TemplateProviderType can derive the Bitcoin network without duplicating the port-mapping logic. Pool config is updated to delegate to infer_network() instead of carrying its own copy of the helper function. JobDeclaratorClientConfig gains the same effective_network() / with_network() pattern as PoolConfig. For Sv2Tp the network is inferred from the sv2-tp port; for BitcoinCoreIpc it is taken directly from the BitcoinNetwork enum value. An explicit network override field (serde default) is provided for non-standard port setups. Both the initial startup and reconnect MonitoringServer paths are updated to call .with_network(config.effective_network()). Integration test lib adds start_jdc_with_network_override(); new test global_info_network_jdc_from_config_override verifies JDC GlobalInfo exposes the network field. Unit tests added to stratum-apps/tp_type, pool config, and JDC config.
dfa8ab3 to
5e3bb6f
Compare
|
Thanks for the detailed review @Alkamal01! I pushed three commits today and rebased onto current upstream main (95 commits, no conflicts). I believe this PR is ready for another look. feat: wire network field through pool, translator, and integration test
fix(monitoring): address PR review feedback
feat: extend network inference to JDC; move
|
Summary
network: Option<String>toGlobalInfoso the Bitcoin network is machine-readable from the monitoring REST API without inspecting config files or Prometheus metrics[pool]TOML config (network = "regtest")GET <upstream_monitoring_url>/api/v1/globalevery 60 seconds in a background taskMotivation
Downstream consumers of the monitoring API (dashboards, alerting tools, and same for translator proxy) need to know which Bitcoin network the pool is operating on. Today this requires reading config files out-of-band. Exposing it in
GlobalInfomakes it a first-class, machine-readable API field.Changes
stratum-appsGlobalInfo.network: Option<String>(serializes tonullwhen not configured)ServerState.network: Arc<RwLock<Option<String>>>for runtime updatesMonitoringServer::with_network()— builder method for static configurationMonitoringServer::network_handle()— returns anArcclone for background tasks that need to update the field afterrun()consumesselfpool-apps/poolPoolConfig.network: Option<String>with#[serde(default)](backward-compatible).with_network(config.network())on startupminer-apps/translatorTranslatorConfig.upstream_monitoring_url: Option<String>with#[serde(default)]monitoring_server.network_handle()then spawnspoll_network_from_pool(): fetches pool's/api/v1/globalimmediately, then every 60 seconds; exits cleanly on cancellationreqwestadded as a dependency (default-features = false, features = ["json"])integration-testsstart_pool_with_networkandstart_sv2_translator_with_upstream_monitoringhelpers added (existing helpers unchanged, delegate to new ones)global_info_exposes_networktest: starts pool withnetwork = "regtest", starts translator pointing at pool's monitoring server, asserts pool exposes"regtest"immediately and translator propagates it within 10 secondsNetwork value convention
Values follow
bitcoin-cli -getinfo/bitcoin-cli getblockchaininfoconvention:"main","test","testnet4","regtest","signet". The field isnullif not configured.Test plan
cargo clippy --manifest-path stratum-apps/Cargo.toml -- -D warnings -A dead-code— cleancargo test --manifest-path stratum-apps/Cargo.toml— 29/29 passcargo fmt --manifest-path stratum-apps/Cargo.toml -- --check— cleanreqwestaddedglobal_info_exposes_networkintegration test passes end-to-end