Skip to content
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

[WIP] - FIX few bugs - TODO: split in proper commits #48

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DanielWarloch
Copy link

Work in Progress.

@georgesFoundation georgesFoundation self-assigned this Nov 25, 2024
@georgesFoundation
Copy link
Collaborator

Thx a lot Daniel for your contribution, I will review this carefully asap.

Copy link
Collaborator

@georgesFoundation georgesFoundation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall, very good PR

  • SuggestDifficulty was on my todo list
  • adding trace/debug is always good
  • 2 interesting fixes

still need to test the new framing refactor proposal before final approval...

@@ -32,6 +32,7 @@ defmt-03 = [
"heapless/defmt-03",
"serde-json-core/defmt",
]
log = ["dep:log"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should not be necessary, an optional dep, create implicilty a featrue with same name.

Does it break something for you ?

"Public-Pool" => SocketAddr::new(Ipv4Addr::new(68, 235, 52, 36).into(), 21496),
"Braiins" => SocketAddr::new(Ipv4Addr::new(64, 225, 5, 77).into(), 3333),
// public-pool.io = 172.234.17.37:21496
"Public-Pool" => SocketAddr::new(Ipv4Addr::new(172, 234, 17, 37).into(), 21496),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx to have fixed the public-pool IP address

"NerdMiners.org",
"NerdMiner.io",
"PyBlock",
"SethForPrivacy",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more pool, why not, but can you keep braiins, I need it for my tests

c.send_connect(Some(String::<32>::from_str("demo").unwrap()))
println!("Configured start connecting");
// c.send_connect(None).await.unwrap();
c.send_connect(Some(String::<32>::from_str("esp-miner-rs").unwrap()))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please get this str from a cli parameter, esp-miner-rs has nothing to do here in this crate (proof is you are using it in your hash-hammer app)

c.send_authorize(
match pool {
"Public-Pool" => String::<64>::from_str(
"1HLQGxzAQWnLore3fWHc2W8UP1CgMv1GKQ.miner1",
"bc1qgaq3nk8yvd8294n6t27j8zwjcft9rm448f9tet",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replacing my address by your in the source code is not fair, but you can add a cli parameter for that, to allow you using your own.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I was still playing with this example while I am getting the hash-hammer up. As you wanned discus the code in the PR so I pushed it right away. I need to clean up code from the changes like that which I did for the testing purposes. I will clean up this code in some time together with applying rest of your comments :)

},
Err(e) => {
error!("Client receive_message error: {:?}", e);
panic!("Client receive_message error: {:?}", e);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to panic!() here, better remove the error!() above

}))
let h = tokio::runtime::Handle::current();

tokio::task::block_in_place(|| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for fixing this

@@ -128,6 +135,10 @@ impl<C: Read + ReadReady + Write, const RX_BUF_SIZE: usize, const TX_BUF_SIZE: u
msg = Some(Message::Authorized);
}
}
Some(ReqKind::SuggestDifficulty) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for adding support for SuggestDifficulty

@@ -234,6 +258,15 @@ impl<C: Read + ReadReady + Write, const RX_BUF_SIZE: usize, const TX_BUF_SIZE: u
debug!("Send Configure: {} bytes, id = {}", n, self.req_id);
self.send_req(n).await
}
pub async fn send_suggest_difficulty(&mut self, difficulty: u32) -> Result<()> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add the proper rust doc above ?

.iter()
.position(|&c| c == b'\n')
{
stop += start;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you 100% confident with your framing refactor (start/stop/...) ?
I did spend extensive time testing and debugging it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants