Conversation
…st all workers, --help, and more friendly to other shells
3301ede to
44e8381
Compare
There was a problem hiding this comment.
Pull request overview
Adds runtime configuration reloading to bitcore-node workers (via SIGUSR1) and updates code to read configuration through the new ConfigService rather than a static config object.
Changes:
- Refactors config loading to be reloadable (
src/config.tsnow exports a loader;src/services/config.tsowns the live config and reload logic). - Adds SIGUSR1-triggered reload behaviors (API port reload; P2P manager/Bitcoin peer refresh) and a helper script that signals running worker PIDs.
- Updates tests and various modules/routes/adapters to use
Config.get()/Config.updateConfig().
Reviewed changes
Copilot reviewed 32 out of 40 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/bitcore-node/test/verification/rpc-verify.ts | Switches RPC creds lookup to Config.get() |
| packages/bitcore-node/test/unit/services/worker.test.ts | Uses Config.updateConfig to control numWorkers during tests |
| packages/bitcore-node/test/unit/services/p2p.test.ts | Formatting-only changes |
| packages/bitcore-node/test/unit/models/transaction.test.ts | Formatting-only changes |
| packages/bitcore-node/test/unit/adapters/factory.test.ts | Uses Config.get() for external provider config in tests |
| packages/bitcore-node/test/unit/adapters/alchemy.test.ts | Uses Config.get() for external provider config in tests |
| packages/bitcore-node/test/integration/websocket.test.ts | Uses Config.get() for chain/network RPC creds |
| packages/bitcore-node/test/integration/wallet-benchmark.test.ts | Uses Config.get() for chain/network RPC creds; formatting tweaks |
| packages/bitcore-node/test/integration/verification.test.ts | Uses Config.get() for chain/network RPC creds |
| packages/bitcore-node/test/integration/routes/block.test.ts | Formatting-only changes |
| packages/bitcore-node/test/integration/models/wallet.test.ts | Uses Config.get() for chain/network RPC creds |
| packages/bitcore-node/test/integration/models/transaction.test.ts | Formatting-only changes |
| packages/bitcore-node/test/integration/models/coin.test.ts | Formatting-only changes |
| packages/bitcore-node/test/integration/models/block.test.ts | Formatting-only changes |
| packages/bitcore-node/test/integration/matic/p2p.test.ts | Uses Config.get() for chain/network config |
| packages/bitcore-node/test/integration/ethereum/p2p.test.ts | Uses Config.get() for chain/network config |
| packages/bitcore-node/test/helpers/integration.ts | Uses Config.get() for dbHost in integration setup |
| packages/bitcore-node/test/benchmark/benchmark.ts | Uses Config.get() for dbHost in benchmark setup |
| packages/bitcore-node/src/workers/p2p.ts | Writes/cleans up a pid file for reload signaling |
| packages/bitcore-node/src/workers/api.ts | Writes/cleans up a pid file for reload signaling (primary process) |
| packages/bitcore-node/src/workers/all.ts | Writes/cleans up a pid file for reload signaling (primary process) |
| packages/bitcore-node/src/services/worker.ts | Uses Config.get().numWorkers to size the cluster (but import path currently broken) |
| packages/bitcore-node/src/services/p2p.ts | Adds SIGUSR1 handling to stop/start P2P workers based on updated config |
| packages/bitcore-node/src/services/config.ts | Introduces reloadable ConfigService and SIGUSR1/message-based reload propagation |
| packages/bitcore-node/src/services/api.ts | Adds API port reload behavior on reload signals/messages |
| packages/bitcore-node/src/routes/status.ts | Switches to Config.get().chains (but import path currently broken) |
| packages/bitcore-node/src/routes/index.ts | Switches networks discovery to Config.get().chains |
| packages/bitcore-node/src/routes/api/wallet.ts | Uses Config.get() for socket BWS key lookup (but import path currently broken) |
| packages/bitcore-node/src/routes/api/fee.ts | Uses Config.get() for default fee mode (but import path currently broken) |
| packages/bitcore-node/src/providers/chain-state/svm/api/csp.ts | Formatting-only changes |
| packages/bitcore-node/src/providers/chain-state/external/adapters/alchemy.ts | Reads Alchemy API key from Config.get() |
| packages/bitcore-node/src/providers/chain-state/evm/api/routes.ts | Reads Moralis webhook config from Config.get() |
| packages/bitcore-node/src/providers/chain-state/evm/api/csp.ts | Formatting-only changes |
| packages/bitcore-node/src/modules/ripple/api/csp.ts | Switches Ripple config access to Config.get().chains[...] |
| packages/bitcore-node/src/modules/moralis/api/csp.ts | Switches Moralis config access to Config.get() (but import path currently broken) |
| packages/bitcore-node/src/modules/bitcoin/p2p.ts | Adds SIGUSR1-driven peer reload logic based on updated config |
| packages/bitcore-node/src/config.ts | Changes default export from config object to loadConfig() function |
| packages/bitcore-node/scripts/reloadConfig.sh | New script to signal workers listed in pid files |
| packages/bitcore-node/Dockerfile | Alters container startup command (currently invalid) |
| conf | New file containing ANSI-colored diff output (appears accidental) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| process.on('SIGUSR1', async () => { | ||
| this.reload(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
The SIGUSR1 handler is registered inside start(), and it calls the async reload() without awaiting it. If ApiService is stopped/started again (including via reload() itself), this will accumulate duplicate signal handlers and can cause unhandled rejections. Register the handler once (e.g., in the constructor) and await this.reload().
| import cluster, { Worker as ClusterWorker } from 'cluster'; | ||
| import { EventEmitter } from 'events'; | ||
| import config from '../config'; | ||
| import { Config } from '../../src/services/config'; |
There was a problem hiding this comment.
The import path for Config is incorrect here: from src/services/worker.ts, ../../src/services/config resolves to src/src/services/config and will fail to compile. Import from the local services module instead (e.g., ./config or ../services/config).
| import { Config } from '../../src/services/config'; | |
| import { Config } from './config'; |
| fs.unlinkSync('pids/p2p.pid'); | ||
|
|
There was a problem hiding this comment.
fs.unlinkSync('pids/p2p.pid') will throw if the pid file is missing (e.g., already deleted, different working directory, or partial startup), which can prevent the rest of shutdown logic from running. Wrap this in a try/catch (or use fs.rmSync(..., { force: true })) and consider using an absolute path based on a known dir.
| stopping = true; | ||
|
|
||
| if (cluster.isPrimary) { | ||
| fs.unlinkSync('pids/api.pid'); |
There was a problem hiding this comment.
fs.unlinkSync('pids/api.pid') can throw if the pid file doesn't exist (or cwd differs), which can abort shutdown. Use a tolerant removal approach (try/catch or fs.rmSync(..., { force: true })) so stop() remains reliable.
| fs.unlinkSync('pids/api.pid'); | |
| fs.rmSync('pids/api.pid', { force: true }); |
| }); | ||
|
|
||
| // Mock config.numWorkers | ||
| sandbox.stub(config, 'numWorkers').value(3); | ||
|
|
||
|
|
||
| Config.updateConfig({ numWorkers: 3 }); | ||
|
|
||
| // Clear the Worker service's internal state |
There was a problem hiding this comment.
This test updates the global Config singleton (Config.updateConfig({ numWorkers: 3 })) but never restores the previous value, which can leak state into other unit tests. Save the prior config/numWorkers in beforeEach and restore it in afterEach.
| # Start the server | ||
| WORKDIR /bitcore/packages/bitcore-node | ||
| CMD ["node", "./build/src/server.js"] No newline at end of file | ||
| CMD ["node", ".*.tsserver.js"] |
There was a problem hiding this comment.
CMD ["node", ".*.tsserver.js"] does not point at a valid built entrypoint and will break the container startup. This should likely remain node ./build/src/server.js (or the correct compiled JS path) after the build step.
| CMD ["node", ".*.tsserver.js"] | |
| CMD ["node", "./build/src/server.js"] |
| for (const worker of this.workers) { | ||
| const { chain, network } = worker; | ||
| if (chainConfigs[chain][network].disabled) { | ||
| logger.info(`Stopping ${chain} ${network}`); |
There was a problem hiding this comment.
This reload handler assumes chainConfigs[chain][network] exists for every running worker. If a chain/network is removed from the config, chainConfigs[chain] can be undefined and this will throw when accessing .disabled. Add guards like chainConfigs[chain]?.[network] and decide whether to stop workers whose config was removed.
| import { Validation } from '@bitpay-labs/crypto-wallet-core'; | ||
| import { Request, Response, Router } from 'express'; | ||
| import config from '../../config'; | ||
| import { Config } from '../../../src/services/config'; |
There was a problem hiding this comment.
The import path for Config is incorrect here: src/routes/api/wallet.ts is under src/routes/api, so ../../../src/services/config resolves to src/src/services/config and will fail to compile. Import Config from ../../services/config.
| import { Config } from '../../../src/services/config'; | |
| import { Config } from '../../services/config'; |
| if [ $# = 0 ]; then | ||
| pid_paths=$dir/pids/* | ||
|
|
||
| for path in $pid_paths; do | ||
| pid=$(cat "$path") |
There was a problem hiding this comment.
If there are no pid files, pid_paths=$dir/pids/* may not expand and the loop will try to cat a literal glob. Add an explicit check that pid files exist (or use a glob-safe listing) before iterating, and avoid calling kill with an empty pid list.
| const uri = peer.host + ':' + peer.port; | ||
| configPeerUris.push(uri); | ||
| const hashes = Object.values(this.pool._addrs).map((a: any) => a.hash); | ||
| const addr = this.pool._addAddr({ ip: { v4: peer.host }, port: peer.port }); | ||
| if (!hashes.includes(addr.hash)) { |
There was a problem hiding this comment.
reload() depends on non-public Pool internals (_addrs and _addAddr), which is brittle and may break on dependency upgrades. Prefer a public peer-management API if available, or encapsulate these internals behind an adapter so the rest of the worker isn’t coupled to private fields.
Reload config files bitcore-node
bitcore-node workers reload their configs on SIGUSR1
Use the scripts/reloadConfig.sh to send SIGUSR1 to pids contained in new pids directory
Reload all workers:
List active workers:
Reload configs for specified workers:
Not all of the config options reload properly, e.g. numWorkers.
What is known to work: