Add support for stratumd - #4821
Open
sanlee42 wants to merge 11 commits into
Open
Conversation
added 11 commits
February 25, 2026 04:11
…r cleanup - cap inbound JSON size and idle read timeout - require login before submit/keepalive/logout; reject unknown methods - bind worker_id to connection; disconnect on mismatch or submit rejection - add unsubscribe event to drop worker on connection close - switch to bounded channels for outbound/job delivery
- hash job ids from minting blob to avoid collisions - verify network-target seals before forwarding to miner service - log disconnect reasons and add protocol error window - expand recent job cache and relax stale/job-miss defaults - speed up vardiff updates, lower min diff, add no-share decay - make miner client sink handling resilient to disconnects
…erage
- normalize stratum server logging to `stratum_server` and add clearer disconnect reasons
- refactor connection handling with explicit read timeout / writer-drain flow and protocol error window
- switch server-side framing to newline-delimited JSON-RPC handling
- keep and refresh current upstream mint job to reduce stale state during submit
- enforce stricter submit validation and accounting:
- worker/job/nonce format checks
- duplicate-share detection by (job_id, nonce, extra)
- stale/job_miss/invalid counters with threshold-based disconnect
- share rate limiting and stats logging
- update forwarding path for network-solved shares with solved-job
- suppression and candidate forwarding trace logs
- tune vardiff manager to Instant-based timing and add no-share decay behavior
- adjust default stratum limits in config (stale window and error thresholds)
- align miner client sink error mapping and request loop behavior
- update stratum protocol docs with real miner-compatible request/response examples
- extend stratum tests:
- protocol login/submit/keepalive
- invalid/duplicate/stale/job_miss/rate-limit paths
- protocol-error threshold disconnect checks
- vardiff update/skip/decay cases
- churn regression on long-lived connection
- move stratum protocol/runtime modules into cmd/stratumd (codec/rpc/diff/pplns_store) - remove legacy in-process stratum crate and its tests - switch miner client imports to starcoin-stratumd - keep node as standalone-stratum mode (no in-process registration) - add standalone protocol/compat tests against stratumd process - update workspace deps/layout and lockfile
- drop legacy node-side stratum config module and CLI wiring (delete config/src/stratum_config.rs) - stop exposing/merging stratum options in StarcoinOpt and NodeConfig runtime config - keep backward compatibility for existing config files by accepting an optional raw [stratum] table in NodeConfig - move StratumLimits and StratumPplnsConfig definitions into cmd/stratumd so standalone stratumd owns its config types - update stratumd pplns_store imports to the new local config types - remove [stratum] sections from example network config files and related config tests - simplify node startup warning to explicitly point users to standalone starcoin_stratumd
- rename cmd/stratumd/src/rpc.rs -> cmd/stratumd/src/stratum_rpc.rs - add cmd/stratumd/src/node_rpc.rs for upstream node RPC connection helpers - extract PPLNS runtime and settlement flow from main into cmd/stratumd/src/pplns.rs - update module exports in cmd/stratumd/src/lib.rs - switch miner client and protocol tests to starcoin_stratumd::stratum_rpc imports - keep runtime behavior unchanged while reducing main.rs responsibilities
- remove legacy ServiceRequest/event glue and SubmitSealRequest conversion from stratum rpc models - move miner client login ServiceRequest to local LoginServiceRequest wrapper - drop unused starcoin-miner and starcoin-service-registry deps from starcoin-stratumd
- Rework VarDiff from per-share interval estimation to windowed work-rate + EMA: - target share time: 3s, update period: 10s, min samples: 6 - bounded step changes: up 1.25x / down 0.8x, deadband: +/-15% - add desired_diff_ema and work_since_last_update state - reset update window on decay and no-work paths - extend unit tests to cover up/down/stable/capped behavior - Fix share validation consistency with dispatched job difficulty: - store per-session assigned_diff (actual job difficulty sent to miner) - validate submitted shares against assigned_diff instead of transient worker diff - feed assigned_diff + network_diff into VarDiff update to avoid drift - Normalize downstream difficulty application: - always dispatch with applied = min(worker_diff, network_diff) - refresh assigned_diff on every job broadcast - log worker/network/applied triplet for observability - Harden connection behavior without breaking mining throughput: - exempt submit from generic request-rate disconnect path - keep submit throttling under share-rate limiter - keep stale-share accounting but avoid immediate disconnect by default - downgrade noisy stale upstream submit logs to debug - Change difficulty state ownership from account-level to worker-level: - keep difficulty state across reconnects for the same worker login - prevent cross-worker contamination under the same account - Decouple miner_client from stratumd crate internals: - introduce local protocol compatibility layer cmd/miner_client/src/stratum_compat.rs - remove direct starcoin-stratumd dependency from miner_client - update miner client code paths and compatibility test wiring - Refactor stratumd entrypoint layout for maintainability: - split monolithic main into focused gateway/server/verify_settlement modules - move shared stratum constants into lib.rs for cross-module reuse - keep runtime behavior and protocol handling unchanged
- remove `cmd/stratumd` from root workspace members/default-members and mark it excluded - convert `cmd/stratumd/Cargo.toml` to a standalone manifest with pinned git+rev deps - add standalone `cmd/stratumd/Cargo.lock` and keep root lock consistent after workspace removal - introduce `NodeRpc` / `NodeRpcSync` traits in `node_rpc.rs` - implement rpc adapters for `AsyncRpcClient` and `RpcClient` - route gateway/pplns/verify-settlement through trait-based node rpc interfaces - keep stratum runtime behavior unchanged while reducing direct rpc-client coupling
📝 WalkthroughWalkthroughThis PR restructures the Stratum mining protocol implementation by removing the standalone Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Miner Client
participant StratumCompat as Stratum Compat Layer<br/>(stratum_compat.rs)
participant StratumClientService as Stratum Client Service
participant JsonCodec as JSON Codec<br/>(with validation)
participant External as External Stratumd<br/>(cmd/stratumd)
Client->>StratumCompat: Import LoginRequest, ShareRequest<br/>types and utilities
Client->>JsonCodec: Initialize decoder with<br/>MAX_INBOUND_BYTES validation
External->>JsonCodec: Send JSON RPC messages
JsonCodec->>StratumCompat: Deserialize to LoginRequest/<br/>StratumJob/etc.
StratumCompat->>StratumClientService: Forward deserialized types
StratumClientService->>StratumClientService: Wrap LoginRequest in<br/>LoginServiceRequest
StratumClientService->>External: Process requests, send responses
External->>JsonCodec: Return StratumJobResponse
JsonCodec->>Client: Deliver decoded job to miner client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
node/src/node.rs (1)
391-391: Consider downgrading this startup log toinfo!.
warn!implies an unexpected or actionable problem each time the node boots. Since in-process stratum removal is permanent and expected, this message will appear as a spurious warning in perpetuity for all operators — including those who never used stratum.info!carries the same visibility in startup logs without inflating the warning noise floor.♻️ Proposed change
- warn!("In-process stratum is removed from node; use standalone `starcoin_stratumd`."); + info!("In-process stratum is removed from node; use standalone `starcoin_stratumd`.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/src/node.rs` at line 391, The startup log currently uses warn! ("In-process stratum is removed from node; use standalone `starcoin_stratumd`.") which incorrectly signals an actionable problem; change that warn! macro invocation to info! so the message remains visible at startup without raising the warning noise floor—locate the warn! call in node.rs (the line emitting the in-process stratum removal message) and replace it with info! (no additional code changes required since the logging macro is already in scope).cmd/miner_client/src/stratum_client_service.rs (1)
248-249: Pre-existing: misleadingexpectmessage on submit request builder.Line 249's expect says "build stratum login request failed never happen" but this is building a submit request. Appears to be a copy-paste from line 240.
📝 Suggested fix
- let message = build_request_string("submit", &seal_req, request_id).expect("build stratum login request failed never happen"); + let message = build_request_string("submit", &seal_req, request_id).expect("build stratum submit request failed: should never happen");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/miner_client/src/stratum_client_service.rs` around lines 248 - 249, The expect() message on the Request::SubmitSealRequest branch is misleading (it mentions "login" while building a submit request); update the call to build_request_string in the Request::SubmitSealRequest arm (where message is assigned) to use a corrected panic message that references "submit" (e.g., "build stratum submit request failed") and keep the same symbols: Request::SubmitSealRequest, build_request_string, message, and request_id so the intent and diagnostics match the actual operation.cmd/miner_client/tests/stratum_compat.rs (1)
96-102: Intentional server leak inDrop— consider adding a comment.
std::mem::forget(server)intentionally leaks theWsServerto avoid shutdown panics, which is a known workaround forjsonrpc-ws-server. A brief comment explaining the intent would help future readers.📝 Suggested comment
impl Drop for MockMiningRpc { fn drop(&mut self) { if let Some(server) = self.server.take() { + // Intentionally leak: WsServer::close() can panic in test teardown. std::mem::forget(server); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/miner_client/tests/stratum_compat.rs` around lines 96 - 102, Add a brief explanatory comment inside the Drop impl for MockMiningRpc where std::mem::forget(server) is called: state that the WsServer is intentionally leaked to avoid shutdown panics caused by jsonrpc-ws-server, reference that this is a deliberate workaround for the library's shutdown behavior, and mention the alternative (proper shutdown) is unsafe here so future maintainers understand the intent. Ensure the comment sits immediately above or beside the std::mem::forget(server) call in the Drop implementation.code_layout.md (1)
36-36: Table ordering:cmd/stratumdis out of alphabetical position.The header on line 6 states entries are "in alphabetical order." The new
cmd/stratumdentry sits betweenstorageandsync, but alphabetically it should be near the othercmdentry (around line 15). This was likely carried over from the oldstratumrow position.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_layout.md` at line 36, The table row for "| [cmd/stratumd](cmd/stratumd) | standalone stratum gateway | [Stratum Mining Protocol](cmd/stratumd/stratum_mining_protocol.md) |" is out of alphabetical order; move this row so it appears with other "cmd/..." entries in the table (i.e., place it alphabetically among the other cmd entries) to honor the file header that the list is in alphabetical order.cmd/miner_client/src/stratum_compat.rs (2)
27-33: Nit:ShareRequestis missingPartialEq, Equnlike all other structs in this module.All other structs (
LoginRequest,Status,StratumJobResponse,StratumJob) derivePartialEq, Eq. If this omission is intentional, ignore; otherwise, adding them would keep the API consistent and enable equality checks in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/miner_client/src/stratum_compat.rs` around lines 27 - 33, The ShareRequest struct currently derives Debug, Clone, Serialize, Deserialize but not PartialEq and Eq like the other types; update the derive on the ShareRequest declaration to include PartialEq and Eq (i.e., add PartialEq, Eq to the #[derive(...)] list for the ShareRequest struct) so it matches LoginRequest/Status/StratumJobResponse/StratumJob and enables equality comparisons in tests.
59-66: Nit: include actual blob length in the error message for easier debugging.When the blob length is not 76, the current error doesn't indicate the actual size received.
Suggested improvement
- if blob.len() != 76 { - return Err(anyhow::anyhow!("Invalid stratum job")); + if blob.len() != 76 { + return Err(anyhow::anyhow!( + "Invalid stratum job: expected blob of 76 bytes, got {}", + blob.len() + )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/miner_client/src/stratum_compat.rs` around lines 59 - 66, In get_extra, the error for invalid blob length should include the actual blob length for debugging: after decoding into blob (variable blob) check blob.len() and return an anyhow error that contains the expected length (76) and the actual blob.len(); keep the rest of the function (including converting blob[35..39] and calling BlockHeaderExtra::new) unchanged.simnet/src/lib.rs (1)
185-194:RngCoreis the minimal sufficient bound for these helpers.
next_unit_intervalonly callsrng.next_u64(), which is a required method onRngCore.next_u32,next_u64andfill_bytesare part ofRngCore, notRng. UsingR: Rng + ?Sizedis a wider-than-necessary constraint; any type that implementsRngCorebut notRngwould be excluded unnecessarily. Tightening toR: RngCore + ?Sizedwould be the minimal, most general bound for both helpers.♻️ Suggested tighter bounds
-fn exp_sample<R: Rng + ?Sized>(rng: &mut R, mean_interval: u64) -> u64 { +fn exp_sample<R: RngCore + ?Sized>(rng: &mut R, mean_interval: u64) -> u64 { let u = next_unit_interval(rng); (-u.ln() * mean_interval as f64) as u64 } -fn next_unit_interval<R: Rng + ?Sized>(rng: &mut R) -> f64 { +fn next_unit_interval<R: RngCore + ?Sized>(rng: &mut R) -> f64 { const SCALE: f64 = 1.0 / (u64::MAX as f64 + 1.0); (rng.next_u64() as f64 * SCALE).clamp(f64::EPSILON, 1.0) }The import would also revert to the previous
RngCore:use rand_chacha::{ - rand_core::{Rng, SeedableRng}, + rand_core::{RngCore, SeedableRng}, ChaCha8Rng, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@simnet/src/lib.rs` around lines 185 - 194, The generic bounds on exp_sample and next_unit_interval are too wide (R: Rng + ?Sized); change them to the minimal RngCore bound (R: RngCore + ?Sized) so any type implementing RngCore (which provides next_u64) is accepted; update the imports to use RngCore instead of Rng and ensure both function signatures (exp_sample and next_unit_interval) and any call sites compile with the tightened bound.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 470: The workspace-level dependency entry postgres = "0.19.12" is unused
by any active workspace member; either remove that line from Cargo.toml or, if
the cmd/stratumd crate (which is excluded from the workspace) needs it, move the
postgres dependency into that crate's own Cargo.toml; verify by searching for
"postgres" in workspace member Cargo.toml files and ensuring no member relies on
the workspace declaration before deleting it.
In `@cmd/miner_client/src/stratum_compat.rs`:
- Around line 10-16: The function target_hex_to_difficulty currently divides by
the parsed U256 value and will panic if the decoded target is zero; update
target_hex_to_difficulty to detect when the parsed U256 (the variable created
from temp after hex::encode and U256::from_str_radix) is zero and return an Err
via anyhow::bail (or an appropriate anyhow::Result error) instead of performing
the division; reference the decoded/parsed temp U256 variable and add a guard
like "if parsed == U256::zero() { bail!(...); }" before computing
U256::from(u64::MAX) / parsed so callers get a clear error instead of a runtime
panic.
---
Nitpick comments:
In `@cmd/miner_client/src/stratum_client_service.rs`:
- Around line 248-249: The expect() message on the Request::SubmitSealRequest
branch is misleading (it mentions "login" while building a submit request);
update the call to build_request_string in the Request::SubmitSealRequest arm
(where message is assigned) to use a corrected panic message that references
"submit" (e.g., "build stratum submit request failed") and keep the same
symbols: Request::SubmitSealRequest, build_request_string, message, and
request_id so the intent and diagnostics match the actual operation.
In `@cmd/miner_client/src/stratum_compat.rs`:
- Around line 27-33: The ShareRequest struct currently derives Debug, Clone,
Serialize, Deserialize but not PartialEq and Eq like the other types; update the
derive on the ShareRequest declaration to include PartialEq and Eq (i.e., add
PartialEq, Eq to the #[derive(...)] list for the ShareRequest struct) so it
matches LoginRequest/Status/StratumJobResponse/StratumJob and enables equality
comparisons in tests.
- Around line 59-66: In get_extra, the error for invalid blob length should
include the actual blob length for debugging: after decoding into blob (variable
blob) check blob.len() and return an anyhow error that contains the expected
length (76) and the actual blob.len(); keep the rest of the function (including
converting blob[35..39] and calling BlockHeaderExtra::new) unchanged.
In `@cmd/miner_client/tests/stratum_compat.rs`:
- Around line 96-102: Add a brief explanatory comment inside the Drop impl for
MockMiningRpc where std::mem::forget(server) is called: state that the WsServer
is intentionally leaked to avoid shutdown panics caused by jsonrpc-ws-server,
reference that this is a deliberate workaround for the library's shutdown
behavior, and mention the alternative (proper shutdown) is unsafe here so future
maintainers understand the intent. Ensure the comment sits immediately above or
beside the std::mem::forget(server) call in the Drop implementation.
In `@code_layout.md`:
- Line 36: The table row for "| [cmd/stratumd](cmd/stratumd) |
standalone stratum gateway
| [Stratum Mining Protocol](cmd/stratumd/stratum_mining_protocol.md) |" is out
of alphabetical order; move this row so it appears with other "cmd/..." entries
in the table (i.e., place it alphabetically among the other cmd entries) to
honor the file header that the list is in alphabetical order.
In `@node/src/node.rs`:
- Line 391: The startup log currently uses warn! ("In-process stratum is removed
from node; use standalone `starcoin_stratumd`.") which incorrectly signals an
actionable problem; change that warn! macro invocation to info! so the message
remains visible at startup without raising the warning noise floor—locate the
warn! call in node.rs (the line emitting the in-process stratum removal message)
and replace it with info! (no additional code changes required since the logging
macro is already in scope).
In `@simnet/src/lib.rs`:
- Around line 185-194: The generic bounds on exp_sample and next_unit_interval
are too wide (R: Rng + ?Sized); change them to the minimal RngCore bound (R:
RngCore + ?Sized) so any type implementing RngCore (which provides next_u64) is
accepted; update the imports to use RngCore instead of Rng and ensure both
function signatures (exp_sample and next_unit_interval) and any call sites
compile with the tightened bound.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
Cargo.tomlcmd/miner_client/Cargo.tomlcmd/miner_client/src/lib.rscmd/miner_client/src/main.rscmd/miner_client/src/stratum_client.rscmd/miner_client/src/stratum_client_service.rscmd/miner_client/src/stratum_compat.rscmd/miner_client/tests/stratum_compat.rscode_layout.mdconfig/example/barnard/config.tomlconfig/example/main/config.tomlconfig/example/proxima/config.tomlconfig/src/lib.rsconfig/src/stratum_config.rsconfig/src/tests.rsnode/Cargo.tomlnode/src/node.rsrpc/client/src/async_client.rsrpc/client/src/lib.rssimnet/src/lib.rsstratum/Cargo.tomlstratum/src/diff_manager.rsstratum/src/lib.rsstratum/src/rpc.rsstratum/src/service.rsstratum/src/stratum.rsstratum/stratum_mining_protocol.mdstratum/tests/stratum_mining_protocol.rsvm2/vm-runtime/src/starcoin_vm.rs
💤 Files with no reviewable changes (14)
- config/example/main/config.toml
- config/src/tests.rs
- node/Cargo.toml
- stratum/stratum_mining_protocol.md
- stratum/tests/stratum_mining_protocol.rs
- config/example/barnard/config.toml
- stratum/src/diff_manager.rs
- stratum/src/stratum.rs
- stratum/src/rpc.rs
- config/src/stratum_config.rs
- config/example/proxima/config.toml
- stratum/src/service.rs
- stratum/src/lib.rs
- stratum/Cargo.toml
| pretty = "0.12.5" | ||
| proc-macro2 = "1.0" | ||
| prometheus = "0.13.0" | ||
| postgres = "0.19.12" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if postgres 0.19.12 exists on crates.io
curl -s "https://crates.io/api/v1/crates/postgres/0.19.12" | python3 -c "
import sys, json
try:
data = json.load(sys.stdin)
if 'version' in data:
print(f'✓ Found: postgres {data[\"version\"][\"num\"]}')
elif 'errors' in data:
print(f'✗ Not found: {data[\"errors\"]}')
else:
print(json.dumps(data, indent=2))
except json.JSONDecodeError as e:
print(f'Error parsing response: {e}')
"Repository: starcoinorg/starcoin
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Check if any workspace member (excluding cmd/stratumd) depends on 'postgres'
echo "=== Checking for postgres references in workspace Cargo.toml files ==="
rg 'postgres' --glob '*/Cargo.toml' -A 2 -B 2Repository: starcoinorg/starcoin
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Get context around line 470 in Cargo.toml to see the workspace.dependencies section
echo "=== Context around line 470 (postgres dependency) ==="
sed -n '465,475p' Cargo.tomlRepository: starcoinorg/starcoin
Length of output: 408
Remove unused postgres workspace dependency or verify its necessity.
The postgres crate version 0.19.12 exists on crates.io, but no active workspace member references it in their dependencies. Since cmd/stratumd is excluded from the workspace, it manages its own dependencies and does not use the workspace-level declaration. Remove this unused entry unless it serves a specific purpose for the workspace.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` at line 470, The workspace-level dependency entry postgres =
"0.19.12" is unused by any active workspace member; either remove that line from
Cargo.toml or, if the cmd/stratumd crate (which is excluded from the workspace)
needs it, move the postgres dependency into that crate's own Cargo.toml; verify
by searching for "postgres" in workspace member Cargo.toml files and ensuring no
member relies on the workspace declaration before deleting it.
Comment on lines
+10
to
+16
| pub fn target_hex_to_difficulty(target: &str) -> anyhow::Result<U256> { | ||
| let mut temp = hex::decode(target)?; | ||
| temp.reverse(); | ||
| let temp = hex::encode(temp); | ||
| let temp = U256::from_str_radix(&temp, 16)?; | ||
| Ok(U256::from(u64::MAX) / temp) | ||
| } |
There was a problem hiding this comment.
Division by zero if target decodes to all zeros.
If the stratum server sends a zero target (e.g., "0000000000000000"), temp will be U256::zero() and the division on line 15 will panic at runtime, crashing the miner client.
🛡️ Proposed fix: guard against zero target
pub fn target_hex_to_difficulty(target: &str) -> anyhow::Result<U256> {
let mut temp = hex::decode(target)?;
temp.reverse();
let temp = hex::encode(temp);
let temp = U256::from_str_radix(&temp, 16)?;
+ if temp.is_zero() {
+ return Err(anyhow::anyhow!("target is zero, cannot compute difficulty"));
+ }
Ok(U256::from(u64::MAX) / temp)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn target_hex_to_difficulty(target: &str) -> anyhow::Result<U256> { | |
| let mut temp = hex::decode(target)?; | |
| temp.reverse(); | |
| let temp = hex::encode(temp); | |
| let temp = U256::from_str_radix(&temp, 16)?; | |
| Ok(U256::from(u64::MAX) / temp) | |
| } | |
| pub fn target_hex_to_difficulty(target: &str) -> anyhow::Result<U256> { | |
| let mut temp = hex::decode(target)?; | |
| temp.reverse(); | |
| let temp = hex::encode(temp); | |
| let temp = U256::from_str_radix(&temp, 16)?; | |
| if temp.is_zero() { | |
| return Err(anyhow::anyhow!("target is zero, cannot compute difficulty")); | |
| } | |
| Ok(U256::from(u64::MAX) / temp) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/miner_client/src/stratum_compat.rs` around lines 10 - 16, The function
target_hex_to_difficulty currently divides by the parsed U256 value and will
panic if the decoded target is zero; update target_hex_to_difficulty to detect
when the parsed U256 (the variable created from temp after hex::encode and
U256::from_str_radix) is zero and return an Err via anyhow::bail (or an
appropriate anyhow::Result error) instead of performing the division; reference
the decoded/parsed temp U256 variable and add a guard like "if parsed ==
U256::zero() { bail!(...); }" before computing U256::from(u64::MAX) / parsed so
callers get a clear error instead of a runtime panic.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c49e41f174
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
223
to
226
| #[clap(flatten)] | ||
| pub vault: AccountVaultConfig, | ||
| #[serde(default)] | ||
| #[clap(flatten)] | ||
| pub stratum: StratumConfig, | ||
| #[clap(flatten)] | ||
| pub account_provider: AccountProviderConfig, |
There was a problem hiding this comment.
Removing StratumConfig from StarcoinOpt means clap now rejects previously supported flags like --stratum-port, --stratum-address, and --disable-stratum, so existing node startup scripts that still pass them will fail before startup. This is a runtime regression for operators upgrading from older configs, especially since the new NodeConfig shim explicitly preserves legacy [stratum] file entries; keeping deprecated CLI flags as no-ops (with warnings) would avoid hard breakage during migration.
Useful? React with 👍 / 👎.
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
1 participant
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.
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit
Breaking Changes
stratumdtool required for Stratum mining support.New Features
chain_info,chain_get_block_txn_infos,chain_get_events_by_txn_hash,miner_get_job.Architecture