-
Notifications
You must be signed in to change notification settings - Fork 122
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
Separate lib
code from bin
in mining-device
#1163
base: main
Are you sure you want to change the base?
Separate lib
code from bin
in mining-device
#1163
Conversation
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
@@ -24,3 +29,6 @@ tracing = { version = "0.1" } | |||
tracing-subscriber = "0.3" | |||
sha2 = "0.10.6" | |||
tokio = "^1.38.0" | |||
|
|||
[features] | |||
abort_mining = [] |
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 the purpose of this feature?
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 added this to replicate exactly the MG test, which says that the mining-device
should not start mining https://github.com/stratum-mining/stratum/pull/1163/files#diff-cc06b104e73cde20ec61e683c5b4eb74e5422641e422652485c1026dd72a7a98R255
In practice I do not think its needed tbh because anyway the mining-device wont be able to open a channel
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 so why do we have it? can't we get away with not having this?
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.
Removed the redundant flag
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.
Removed the redundant flag
I don't understand, abort_mining
seems to be still there
a7ffeff
to
36ea0a2
Compare
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.
Looks good!
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.
Need to understand why we removed the info
log containing the req_id
and then fix the cargo clippy
error in the CI.
) -> Result<SendTo<()>, Error> { | ||
self.channel_opened = true; | ||
self.channel_id = Some(m.channel_id); | ||
let req_id = m.get_request_id_as_u32(); |
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 cargo clippy
CI fails on this line with:
warning: unused variable: `req_id`
--> test-utils/mining-device/src/lib/mod.rs:389:13
|
389 | let req_id = m.get_request_id_as_u32();
| ^^^^^^ help: if this is intentional, prefix it with an underscore: `_req_id`
|
= note: `#[warn(unused_variables)]` on by default
The reason it fails is because the main
branch immediately uses this req_id
in an info!
log on L438 with:
info!(
"MINING DEVICE: channel opened with: group id {}, channel id {}, request id {}",
m.group_channel_id, m.channel_id, req_id
);
Why did we remove this log? Are there any other logs that got removed? Are there any other changes in the code in this PR beyond breaking up the logic into the mod.rs
file?
36ea0a2
to
7c27cc7
Compare
Isolating the library code into a `lib/mod.rs` and consume it in `main.rs`.
7c27cc7
to
b3b55e7
Compare
..cargo does not allow hyphens in library target name
b3b55e7
to
19a6968
Compare
Bencher Report
Click to view all benchmark results
|
Resolving
MiningDevice
in the list here #1093