-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add stratum style mining protocol #444
Conversation
stratum/src/lib.rs
Outdated
fn can_respond_to_submition() { | ||
let addr = SocketAddr::from_str("127.0.0.1:19990").unwrap(); | ||
let _stratum = | ||
Stratum::start(&addr, Arc::new(DummyManager::build().of_initial(r#"["dummy autorize payload"]"#)), None) |
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.
autorize -> authorize
core/src/miner/stratum.rs
Outdated
impl Default for Config { | ||
fn default() -> Self { | ||
Config { | ||
listen_addr: "0.0.0.0:0".parse().unwrap(), |
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 :0
here. We already have port number in Config
.
core/src/miner/stratum.rs
Outdated
|
||
/// Start STRATUM job dispatcher and register it in the miner | ||
pub fn register(cfg: &Config, miner: Arc<Miner>, client: Arc<Client>) -> Result<(), Error> { | ||
let stratum = Stratum::start(cfg, miner.clone(), client.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.
You don't have to clone client
here.
core/src/miner/stratum.rs
Outdated
use std::sync::Arc; | ||
|
||
use cstratum::PushWorkHandler; | ||
use cstratum::{Error as StratumServiceError, JobDispatcher, Stratum as StratumService}; |
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.
Merge these two lines.
core/src/miner/stratum.rs
Outdated
} | ||
|
||
/// Start STRATUM job dispatcher and register it in the miner | ||
pub fn register(cfg: &Config, miner: Arc<Miner>, client: Arc<Client>) -> Result<(), 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.
Is there any reason for implementing this here?
This kind of registering logics are usually implemented in main.rs
.
core/src/miner/stratum.rs
Outdated
self.job() | ||
} | ||
|
||
fn job(&self) -> Option<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.
It seems this function always return None
.
What is expected behavior of this function?
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 not needed at the moment, so it may be good to delete, but it is needed to operate a minor (reward, ...). 0c69701
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.
Could you expand your comment a bit more?
I can't understand what it means that it's safe to delete, but miner wouldn't operate with that.
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.
@Kais-DkM I think this commit is the basic of stratum protocol for codechain. so I aim to provide the same degree as getwork and submit JSON-RPC.
As you know that, stratum protocols for bitcoin and ethereum provide additional information in this part to a miner.
Could you discuss it with me at the developer meeting?
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.
Oh, I was confused with miner
module in codechain.
I guess it's okay for now, but it's worth a discussion. Let's talk about this later again.
@@ -496,4 +486,44 @@ mod tests { | |||
response | |||
); | |||
} | |||
|
|||
#[test] | |||
fn can_respond_to_submition() { |
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 doubt if we can call this test as "unit test".
Usually scenarios included in source code is for unit testing, but this one seems to test operations on real world environment(e.g. opening tcp stream).
I think it'd be better test this kind of operations on integration tests.
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 have added a new one to existing test.
But I agree with you, so I will apply this to an integrated test.
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 actually meant that we should remove this test from this file..... but it seems there already exists similar test cases here.
I think we should file an issue about this problem, and revisit this issue later.
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.
Filed an issue at #457
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.
@Kais-DkM Thank you, I checked #457 out
Please merge your commits into one commit. |
8509cc3
to
b49c7db
Compare
@redongjun Why was this PR closed? |
@Kais-DkM Sorry, I think I made a mistake. |
b49c7db
to
60c17cd
Compare
@Kais-DkM I merged the latest changes from master. |
Fix #352
I have tested as below: