Refactor(jd-client): replace monolithic start loop with JdcRuntime typestate machine by nulllpc · Pull Request #633 · stratum-mining/sv2-apps

nulllpc · 2026-07-20T20:03:42Z

Refactors the monolithic start function inside the Pool (miner-apps/jd-client) into a structured, typestate-based state machine (JdcRuntime) to manage bootstrap, component lifecycles, and graceful shutdown.

This PR also properly handles errors instead of calling unwrap or expect

Fixes #526

nulllpc · 2026-07-20T20:29:14Z

A note: in the initial sketch, ChannelManagerReady was put after SoloMiningReady and UpstreamReady but then I realized we need the ChannelManager to start first so we could send CoinbaseOutputConstraints to the template provider so I changed the order. Conceptually, I think there's no major problem there, just want to highlight this just in case.

plebhash · 2026-07-30T22:44:24Z

@nulllpc looks like fmt CI is broken, likely due to the fact that #651 was just merged

plebhash

LGTM

@nulllpc can you please squash all commits into 1?

bit-aloo

Some suggestions, we are pretty close.

bit-aloo · 2026-08-02T15:35:00Z

+        if let Some(monitoring_addr) = self.jd_client.config.monitoring_address() {
+            info!("Initializing monitoring server on http://{monitoring_addr}");
+            if let Err(e) = self.start_monitoring_tasks(&channel_manager, monitoring_addr) {
+                error!("Failed to initialize monitoring tasks: {e}");
+                self.jd_client.cancellation_token.cancel();
+            }
+        }


Why are we not propagating error here, in case the monitoring services doesn't spawn?

Nice, we can also remove the cancellation token call here because we already call mark_stopped on fail state

I did some checks and found a related issue in connect_to_bitcoin_core and I tagged you in the issue

#679

thanks for reporting

for now I think we should treat #679 as orthogonal and deal with it later

plebhash · 2026-08-05T13:16:58Z

trying to expedite this PR, here's a few commit suggestions to be cherry-picked (on this specific order, otherwise won't apply) and squashed:

a2e2482: addressing this suggestion: Refactor(jd-client): replace monolithic start loop with JdcRuntime typestate machine #633 (comment)
eeb1aec: addresses ChannelManager::start return signature deviating from pattern established on #582 plus state transition gap on failure of CoinbaseOutputConstraints
858dc34: moved the start call out of bootstrap_channel_manager into start_services_inner, ahead of start_downstream_server, restoring Pool's "bootstrap constructs, start_services activates" split pattern established on #582
06e82b4
- restored #[must_use] + private fields + into_parts() on BootstrapError (following #582)
- normalized pub → pub(super) (following #582)
- dropped the mark_stopped() calls that duplicated what shutdown() already does

moreover, #685 is aligning Pool with some patterns that are being established here, and also addressing some bugs found here

plebhash · 2026-08-05T13:21:38Z

also needs rebasing against main

nulllpc · 2026-08-05T19:55:39Z

Cherry picked the commits from @plebhash
Squashed into 1 commit and edited the commit message
Rebased main

@plebhash @bit-aloo please review again. Just a reminder that I'm contributing on my free time so things might not move very fast. I'll try my best though. Thanks guys!

nulllpc · 2026-08-05T20:12:13Z

trying to expedite this PR, here's a few commit suggestions to be cherry-picked (on this specific order, otherwise won't apply) and squashed:

a2e2482: addressing this suggestion: Refactor(jd-client): replace monolithic start loop with JdcRuntime typestate machine #633 (comment)

eeb1aec: addresses ChannelManager::start return signature deviating from pattern established on #582 plus state transition gap on failure of CoinbaseOutputConstraints

858dc34: moved the start call out of bootstrap_channel_manager into start_services_inner, ahead of start_downstream_server, restoring Pool's "bootstrap constructs, start_services activates" split pattern established on #582

06e82b4

restored #[must_use] + private fields + into_parts() on BootstrapError (following #582)

normalized pub → pub(super) (following #582)

dropped the mark_stopped() calls that duplicated what shutdown() already does

moreover, #685 is aligning Pool with some patterns that are being established here, and also addressing some bugs found here

I'll apply the same rationale here to this PR #644 for the translator so we have consistent pattern

plebhash · 2026-08-05T20:12:13Z

Just a reminder that I'm contributing on my free time so things might not move very fast. I'll try my best though.

no worries, your contributions are much appreciated!

whenever you have time, please have a look at #685

along with the commits suggested above, it will probably bring some important insights to #644

plebhash · 2026-08-05T20:12:40Z

oh oops, I guess we posted at the same time

nulllpc · 2026-08-06T20:42:08Z

I just added a tiny Rust doc change for mark_stopped so that it's consistent with the changes you made to PoolRuntime @plebhash

bit-aloo

Looks good to me. Thanks for working on this.

…pestate machine Overhauls the JDC initialization sequence to align its API and behavior with PoolRuntime. - Typestate Machine: Replaces the monolithic startup logic with strict Init -> TemplateProviderReady -> UpstreamReady -> Running state transitions. - Separation of Concerns: Constructs ChannelManager and core services during bootstrap, but explicitly activates them later in start_services_inner. - Error Propagation: Ensures all startup errors cascade cleanly via Result signatures instead of failing silently or panicking. - Unblocking Bootstrap: Spawns the downstream IBD template wait loop into the background to prevent the JDC from freezing during Bitcoin Core syncs, keeping Ctrl+C functional. TcpListener::bind is preserved as a synchronous check for immediate port-in-use feedback. - Cleanups: Normalizes visibilities to pub(super), restores must_use attributes, and removes redundant mark_stopped() calls.

`PoolSv2::start` propagated a `PoolRuntime::<Init>::new` failure with `?`, leaving `is_alive` true and never calling `notify_waiters()`. Since `PoolSv2` is `Clone`, a concurrent holder calling `PoolSv2::shutdown` would pass the `is_alive` check and then block forever on `notified`, because the only `notify_waiters()` lives in `PoolRuntime::shutdown` — and on that path no runtime was ever built. Extract `mark_stopped()` (mirroring `JobDeclaratorClient::mark_stopped`, per bit-aloo's review on stratum-mining#633), route `PoolRuntime::shutdown` through it, and call it directly on the `new()` failure path.

`PoolRuntime` startup and shutdown gaps, aligned with #633

nulllpc force-pushed the npc/refactor-jdc-start branch 3 times, most recently from 27713eb to 45f79e5 Compare July 20, 2026 20:22

nulllpc force-pushed the npc/refactor-jdc-start branch 2 times, most recently from 7dcc7ee to 5dd1782 Compare July 20, 2026 20:35

nulllpc commented Jul 20, 2026

View reviewed changes

Comment thread miner-apps/jd-client/src/lib/jdc_runtime.rs Outdated

nulllpc force-pushed the npc/refactor-jdc-start branch from 5dd1782 to ec4544a Compare July 22, 2026 17:15