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

Add Device Label (#1) #395

Closed
wants to merge 1 commit into from
Closed

Conversation

ryan-rowland
Copy link
Contributor

@ryan-rowland ryan-rowland commented May 10, 2018

Pull request checklist

  • All tests pass. Demo project builds and runs.
  • I have resolved any merge conflicts.

What's in this pull request?

Uses --extra-data flag or extraData config option to set deviceLabel in the case of connecting to a mining pool (where before it was discarded until reverting to solo mining).

See:
nimiq/mining-pool#12
nimiq-network/developer-reference#23

@ryan-rowland
Copy link
Contributor Author

Failures look like random timeouts on 2 of the suites.

@mar-v-in
Copy link
Collaborator

Extra data is not intended to be the device name when solo mining, so it shouldn't magically get this meaning when mining for a pool. I'd prefer to just make it a separate setting in the config.

@mar-v-in
Copy link
Collaborator

This thing should also be signed somehow, else everyone that knows your address and device id (both may become public through the block extra data) can rename your devices.

@codecov
Copy link

codecov bot commented May 10, 2018

Codecov Report

Merging #395 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
- Coverage   71.49%   71.45%   -0.05%     
==========================================
  Files         188      188              
  Lines       11710    11711       +1     
==========================================
- Hits         8372     8368       -4     
- Misses       3338     3343       +5
Impacted Files Coverage Δ
src/main/generic/miner/SmartPoolMiner.js 20% <0%> (ø) ⬆️
src/main/generic/miner/BasePoolMiner.js 4.71% <0%> (-0.05%) ⬇️
src/main/generic/consensus/InvRequestManager.js 72.22% <0%> (-11.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97efc1a...ce18688. Read the comment docs.

@ryan-rowland
Copy link
Contributor Author

@mar-v-in Originally, I had this as --device-label but reverted to --extra-data since I believe that's what the existing pools have been doing. I'm +1 for --device-label and pool.deviceLabel in config, if that's the route we want to go. 👍

As for signing: couldn't a malicious actor that knows both your deviceId and address already get you repeatedly banned from the pool agent by connecting at the same time as you and submitting duplicate shares? That would seem more serious than renaming the device.

@mar-v-in
Copy link
Collaborator

The reference pool implementation does banning by ip address, not device id.

@ryan-rowland
Copy link
Contributor Author

Interesting -- I've been banned when connecting to a pool server with images behind different IPs, but that shared a peer-key folder due to neglecting to remove it when creating the image. But perhaps that's not due to the device ID?

@ryan-rowland
Copy link
Contributor Author

See #401

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