-
Notifications
You must be signed in to change notification settings - Fork 260
Kaspa Stratum Bridge binary #793
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Kaspa Cargo and FMT
…ulty in share_handler
Kaspa Cargo and FMT
…ulty in share_handler
bridge/src/main.rs
Outdated
| Inprocess, | ||
| } | ||
|
|
||
| fn split_shell_words(input: &str) -> Result<Vec<String>, anyhow::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for? I'm not clear what it's actually doing, but it's strange that it is trying to parse things char by char
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used to parse the --node-args "" CLI option into a proper argv vector for kaspad_args::Args::parse. It implements minimal shell-style tokenization: splits on whitespace, preserves quoted segments ("..." / '...') as a single argument, and errors on unterminated quotes. The char-by-char scan is just to track “inside quotes” state; .split_whitespace() would break quoted args.
Since we did not add args to the kaspad, section directly as requested not to edit the kaspad area.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is it possible to pass through arguments from command line to the inprocess node without using --node-args?
Something like:
./bridge --appdir=/dir/blah --disable-upnp
And these would work equivalent to as if you directly called:
./kaspad --appdir=/dir/blah --disable-upnp
Even an approach where these arguments are copied as needed to struct Cli could work better than doing --node-args this way.
What do you think of this approach?
bridge/src/main.rs
Outdated
| tracing::info!("----------------------------------"); | ||
| tracing::info!("initializing bridge ({} instance{})", instance_count, if instance_count > 1 { "s" } else { "" }); | ||
| tracing::info!("\tkaspad: {} (shared)", config.global.kaspad_address); | ||
| tracing::info!("\tblock wait: {:?}", config.global.block_wait_time); | ||
| tracing::info!("\tprint stats: {}", config.global.print_stats); | ||
| tracing::info!("\tvar diff: {}", config.global.var_diff); | ||
| tracing::info!("\tshares per min: {}", config.global.shares_per_min); | ||
| tracing::info!("\tvar diff stats: {}", config.global.var_diff_stats); | ||
| tracing::info!("\tpow2 clamp: {}", config.global.pow2_clamp); | ||
| tracing::info!("\textranonce: auto-detected per client"); | ||
| tracing::info!("\thealth check: {}", config.global.health_check_port); | ||
|
|
||
| for (idx, instance) in config.instances.iter().enumerate() { | ||
| tracing::info!("\t--- Instance {} ---", idx + 1); | ||
| tracing::info!("\t stratum: {}", instance.stratum_port); | ||
| tracing::info!("\t min diff: {}", instance.min_share_diff); | ||
| if let Some(ref prom_port) = instance.prom_port { | ||
| tracing::info!("\t prom: {}", prom_port); | ||
| } | ||
| if let Some(log_to_file) = instance.log_to_file { | ||
| tracing::info!("\t log to file: {}", log_to_file); | ||
| } | ||
| } | ||
| tracing::info!("----------------------------------"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor this initial log into it's own function in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactored
bridge/src/main.rs
Outdated
| let exe_base = std::env::current_exe().ok().and_then(|p| p.parent().map(|p| p.to_path_buf())); | ||
| let exe_root = exe_base.as_ref().and_then(|p| p.parent()).and_then(|p| p.parent()).map(|p| p.to_path_buf()); | ||
|
|
||
| let mut candidates: Vec<std::path::PathBuf> = vec![config_path.to_path_buf(), fallback_path.clone()]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these candidates for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
candidates vector contains multiple paths where the bridge will look for the config.yaml file. This ensures the bridge can find its configuration in different deployment scenarios:
Development: Running from different directories in the repo
Deployment: Different installation layouts inprocess versus external
Flexibility: Users can place config in various logical locations
Robustness: Bridge finds config regardless of how it's launched
The bridge iterates through these candidates and uses the first one that exists, making it resilient to different directory structures and deployment scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, can you refactor these into a fn initialize_config() -> BridgeConfig (Currently L152 - L184) so that the remaining code in main is something like:
let config = initialize_config();
ulti-instance duplicate block mitigation (multi-port). Observed occasional duplicate block logs when ASICs were split across multiple stratum ports, not reproducible when all ASICs shared a single port. Root cause is that extranonce allocation was per ClientHandler (per instance), so miners on different ports could receive identical extranonces and mine overlapping nonce space for the same template. Update: extranonce allocation is now process-global while keeping existing miner expectations intact (Bitmain still gets extranonce_size=0; IceRiver/BzMiner/Goldshell still get 2-byte extranonce).
359fabc to
9c0f357
Compare
bridge/docs/README.md
Outdated
| - **External** node (you run `kaspad` yourself) | ||
| - **In-process** node (the bridge starts `kaspad` in the same process) | ||
|
|
||
| The bridge no longer supports spawning `kaspad` as a subprocess. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line. It's not relevant since there was never a release that spawned it as a subprocess.
Make sure to go through this README and check that everything makes sense from a first release POV.
bridge/src/main.rs
Outdated
| Inprocess, | ||
| } | ||
|
|
||
| fn split_shell_words(input: &str) -> Result<Vec<String>, anyhow::Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is it possible to pass through arguments from command line to the inprocess node without using --node-args?
Something like:
./bridge --appdir=/dir/blah --disable-upnp
And these would work equivalent to as if you directly called:
./kaspad --appdir=/dir/blah --disable-upnp
Even an approach where these arguments are copied as needed to struct Cli could work better than doing --node-args this way.
What do you think of this approach?
bridge/src/main.rs
Outdated
| node_args: Option<String>, | ||
|
|
||
| #[arg(long, action = clap::ArgAction::Append)] | ||
| node_arg: Vec<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a node_args and node_arg? Although, I think with the suggestion to copy relevant arguments to cli, you can just get rid of both of these.
bridge/src/main.rs
Outdated
| let inferred_mode = NodeMode::Inprocess; | ||
| let node_mode = cli.node_mode.unwrap_or(inferred_mode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let inferred_mode = NodeMode::Inprocess; | |
| let node_mode = cli.node_mode.unwrap_or(inferred_mode); | |
| let node_mode = cli.node_mode.unwrap_or(NodeMode::Inprocess); |
bridge/src/main.rs
Outdated
| let exe_base = std::env::current_exe().ok().and_then(|p| p.parent().map(|p| p.to_path_buf())); | ||
| let exe_root = exe_base.as_ref().and_then(|p| p.parent()).and_then(|p| p.parent()).map(|p| p.to_path_buf()); | ||
|
|
||
| let mut candidates: Vec<std::path::PathBuf> = vec![config_path.to_path_buf(), fallback_path.clone()]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, can you refactor these into a fn initialize_config() -> BridgeConfig (Currently L152 - L184) so that the remaining code in main is something like:
let config = initialize_config();
bridge/src/stratum_listener.rs
Outdated
| let listener = | ||
| TcpListener::bind(&addr_str).await.map_err(|e| format!("failed listening to socket {}: {}", self.config.port, e))?; | ||
|
|
||
| tracing::debug!("Stratum listener started on {}", self.config.port); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is tracing debug used directly, while others are in use tracing::{error, info, warn};? Please make it consistent, and please make sure to apply this to all files where such inconsistency lies. Just add debug to the use tracing::{error, info, warn}; line in this case.
Clarified that the bridge no longer supports spawning 'kaspad' as a subprocess.
core/src/log/mod.rs
Outdated
| .unwrap(); | ||
|
|
||
| let _handle = log4rs::init_config(config).unwrap(); | ||
| let _handle = log4rs::init_config(config).ok(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is needed because when running the bridge in inprocess mode, when this is set to unwrap it will panic at this call (or is it somewhere else that panics?)
But why does it? What is happening on the bridge side that makes this call panic? When the node is ran on its own, it will unwrap cleanly.
|
|
||
| ## Stratum Bridge | ||
|
|
||
| Check out the [README.md](bridge/docs/README.md) for instructions on how to run the stratum bridge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should have a beta warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stratum Bridge changed to ## Stratum Bridge Beta
| @@ -0,0 +1,73 @@ | |||
| ## Stratum Bridge | |||
|
|
|||
| This repository contains a standalone Stratum bridge binary at: | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doc should have a beta warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kaspa Stratum Bridge (BridgeBinary) is a standalone Stratum bridge for Kaspa built on top of the [rusty-kaspa] stack.
The bridge supports two node modes:
External mode – connects to an already running [kaspad] node.
In‑process mode – starts an embedded [kaspad] instance inside the bridge process, then talks to it via gRPC.
Both modes share the same mining logic and stats; only node lifecycle is different.
Configuration
The bridge is configured via a YAML file, typically: