-
Notifications
You must be signed in to change notification settings - Fork 141
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
Fix JDC fallback to solo-mining #1055
Fix JDC fallback to solo-mining #1055
Conversation
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
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.
utack. Few nits. Can we remove these lock files also.
jd.safe_lock(|jd| jd.coinbase_tx_prefix = job.coinbase_tx_prefix.clone()) | ||
.unwrap(); | ||
jd.safe_lock(|jd| jd.coinbase_tx_suffix = job.coinbase_tx_suffix.clone()) | ||
.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.
jd.safe_lock(|jd| jd.coinbase_tx_prefix = job.coinbase_tx_prefix.clone()) | |
.unwrap(); | |
jd.safe_lock(|jd| jd.coinbase_tx_suffix = job.coinbase_tx_suffix.clone()) | |
.unwrap(); | |
jd.safe_lock(|jd| { | |
jd.coinbase_tx_prefix = job.coinbase_tx_prefix.clone(); | |
jd.coinbase_tx_suffix = job.coinbase_tx_suffix.clone(); | |
}).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.
I don't commit this suggestion because of the policies on how nice commit history must be, if you need credits for this mod I can add it in the commit message. Let me know.
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.
No credit needed. I see you included the suggestion in the latest push, and it works perfectly.
54fb3e8
to
a3cf8ce
Compare
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
f42eec5
to
1993eaf
Compare
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.
Utack. A few more nits. The rest looks decent code-wise. I will test the functionality and get back to you soon. Can you delete rest of the lock files.
test/config/change-upstream/jdc-config-local-example-change-upstream-solo-fallback.toml
Outdated
Show resolved
Hide resolved
70a17d7
to
341d1e5
Compare
9067613
to
58c70a4
Compare
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.
tACK 👏
I left just one comment, if you could clarify it it would be great, thanks @lorbax
5f1c25d
to
2d0f169
Compare
fixed the fallback to solo-mining in the case that the upstream sends a `SubmitShareError` on a valid share AND there are no other available upstreams in the JDC config.
2d0f169
to
cd493ee
Compare
If the JDC switches to solo_mining, then it initializes as solo mining (using
initialize_jd_as_solo_miner
).This function initialize a
DownstreamMiningNode
withjd
field set toNone
. This is the right behaviour, but makes panic this unwrap.Since we are doing solo mining, it should be care of the downstream to change the coinbase downstream, instead of wrapping a None, the
coinbase_tx_prefix
andsuffix
are ignored.Perhaps this is not the best way to do that, feedback needed.
In the commit message is writtend how to test this PR.
This PR is on top of PR1001.
This PR closes issue 844
How to test this PR:
The testing environment is very similar to the one described on PR1001. The only difference is that here we do not have a "good" pool to fallback into. So apply suggestions on PR1001 with the only difference that the jdc config is
(you can avoid running a "good" pool then)
EDIT: the MG test is removed