-
Notifications
You must be signed in to change notification settings - Fork 127
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
sv1-mining-device
lib
#1249
base: main
Are you sure you want to change the base?
sv1-mining-device
lib
#1249
Conversation
c33c014
to
9546fe3
Compare
Bencher Report
Click to view all benchmark results
|
Bencher Report
🚨 5 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
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.
Minor nits and a few follow-ups. Everything else looks good.
@@ -1,5 +1,5 @@ | |||
[package] | |||
name = "sv1-mining-device" | |||
name = "sv1_mining_device" |
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 makes sense to keep this consistent with other crates in similar roles. I'm not sure if we've published this crate, but if we have, we should deprecate the previous version.
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 crate has not yet been published to crates.io
, so it's still not too late to change things
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.
+1 on consistency for naming patterns
@@ -13,6 +13,10 @@ keywords = ["stratum", "mining", "bitcoin", "protocol"] | |||
|
|||
# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html | |||
|
|||
[lib] | |||
name = "sv1_mining_device" |
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.
The name
field can be removed since it's already defined in the [package]
section. However, if we want to keep the original package name and use a different library name, including the name
field in [lib]
makes sense.
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 is true, however we have been following this same pattern with every roles
in case we decide to change this, IMO it should be a separate PR doing this in a consistent way across all roles
arguably cosmetic and therefore low prio IMHO
@@ -69,8 +67,8 @@ impl Client { | |||
/// the information from `sender_share`, it is formatted as a `v1::client_to_server::Submit` | |||
/// and then serialized into a json message that is sent to the Upstream via | |||
/// `sender_outgoing`. | |||
pub(crate) async fn connect(client_id: u32) { | |||
let stream = std::sync::Arc::new(TcpStream::connect(ADDR).await.unwrap()); | |||
pub async fn connect(client_id: u32, upstream_addr: std::net::SocketAddr) { |
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.
pub async fn connect(client_id: u32, upstream_addr: std::net::SocketAddr) { | |
pub async fn connect(client_id: u32, upstream_addr: SocketAddr) { |
std::net::SocketAddr
can be added to the import statement above.
const ADDR: &str = "127.0.0.1:34255"; | ||
Client::connect( | ||
80, | ||
std::net::SocketAddr::from_str(ADDR).expect("Invalid upstream address"), |
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.
std::net::SocketAddr::from_str(ADDR).expect("Invalid upstream address"), | |
SocketAddr::from_str(ADDR).expect("Invalid upstream address"), |
std::net::SocketAddr
can be added to the import statement above.
@@ -1,12 +1,12 @@ | |||
# This file is automatically @generated by Cargo. | |||
# It is not intended for manual editing. | |||
version = 3 | |||
version = 4 |
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 breaks MSRV
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 highly recommend locking SRI development setup to Rust 1.75.0
that's what I did with my setup when the same thing happened to me
pub async fn start_sv2_mining_device(pool_address: SocketAddr) { | ||
tokio::spawn(async move { | ||
mining_device::connect( | ||
pool_address.to_string(), | ||
Some( | ||
Secp256k1PublicKey::from_str("9auqWEzQDVyd2oe1JVGFLMLHZtCo2FFqZwtKA5gd9xbuEu7PH72") | ||
.unwrap(), | ||
), | ||
None, | ||
None, | ||
0, | ||
Some(0.1), | ||
) | ||
.await; | ||
}); | ||
tokio::time::sleep(std::time::Duration::from_secs(3)).await; | ||
} |
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.
looks like this is leaking from #1252
@@ -23,6 +23,8 @@ once_cell = "1.19.0" | |||
network_helpers_sv2 = { path = "../roles-utils/network-helpers", features =["with_tokio","with_buffer_pool"] } | |||
pool_sv2 = { path = "../pool" } | |||
roles_logic_sv2 = { path = "../../protocols/v2/roles-logic-sv2" } | |||
sv1_mining_device = { path = "../test-utils/sv1-mining-device" } | |||
mining_device = { path = "../test-utils/mining-device" } |
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.
looks like this is leaking from #1252
@@ -282,3 +282,28 @@ pub async fn start_template_provider(tp_port: u16) -> TemplateProvider { | |||
template_provider.generate_blocks(16); | |||
template_provider | |||
} | |||
|
|||
pub async fn start_sv1_mining_device(pool_address: SocketAddr) { |
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's try to be consistent with #1234
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.
also, pool_address
is a legacy naming convention that's a bit misleading IMO
I would call this upstream_address
Resolves #1245