introduce an async version of stratum server. #2468
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks, this is awesome and looks great from a first glance at the code and the graph! I'll give it a run at grin-pool.org tomorrow, I just have to port some changes I've made to grin's stratum server over to Tokio first. |
|
@hendi I've fixed a bug, please update. I hope it won't affect your ported changes. BTP, this PR might not solve all the problems you experience with stratum server. It definitely improves amount time and resources stratum server spends on handling network connections but there is another place which might be an issue - block checking. I have another PR for this, will try to test it tomorrow. |
Thanks for pinging me, your second commit fixed an issue I was having while porting my changes. |
|
I'm running a setup that passes data to the old stratum server and to the new one in parallel, and so far the results are:
I've removed some of my stratum's improvements, after that:
So all in all I'm happy! Only issue so far is the following (that happend with ~22 active workers, ~200 inactive connections): |
|
@hendi Is this stacktrace from the version with your modifications? It looks like the cause for it should be calling |
|
Some time ago having 2 stratum impls seemed a good idea, now it just adds extra complexity. Should we aim to replace the current version? |
No, my code has no
I'm all in favor for the a single implementation. |
|
The error occurred again using a debug build:
|
|
Thanks Hendrik! This is very interesting. It seems that mutex didn't work properly in this function. I've pushed a fix, please check it out. And I'll check if there is the same error in the current implementation. |
|
Yep, there is. I'll create a separate PR. |
|
Thanks, giving it a spin now! |
|
It seems that the same problem in the master branch was just resolved by #2446 |
|
FYI, working fine now, no issues in the last ~23 hours. |
|
So, if it's ok for everybody, I am going to get rid of feature |
|
It's done. You don't need |
hashmap
reviewed
Jan 28, 2019
servers/src/mining/stratumserver.rs
Outdated
| self.error = true; | ||
| return; | ||
| // Package the reply as RpcResponse json | ||
| match response { |
There was a problem hiding this comment.
I'd suggest
let resp = match response{// create RpcRepsonses here};
serde_json::to_string(&resp).unwrap();
servers/src/mining/stratumserver.rs
Outdated
| .clone() | ||
| .unwrap() | ||
| .parse() | ||
| .expect("Incorrect address"); |
There was a problem hiding this comment.
Can we make this message more specific?
hashmap
approved these changes
Jan 29, 2019
|
I agree with @hashmap that having 2 stratum implementations for too long makes no sense. However it'd likely be safer to keep both for a bit, and remove the old one when we're comfortable with this implementation? I'll try to find some time for a review over the next few days. |
|
Thanks, Igno. I'll revert the commit which replaces the current server with the async one. But I would like to notice that the changes are not so big. It is not a completely new server. In order to simplify migration, I tried to keep as much of old code as I could. |
|
@e-max never mind my last, I hadn't realized how much was changed in the first place. So feel free to make async the default and sorry for the back-and-forth. I do think we should only merge this on 1.1.0 given the amount of changes, and try to get a few more pools to try it out before then. Any objection? |
It seems that current approach stops workring after amount of parallel connection exceeds ~500. This PR introduces an 'async' feature which enable asyncronius, tokio based implementation of stratum server.
I passed in Handler current_difficulty as an u64, so it was copied and all later changes were ignored. In this commit I pass referece on RwLock with current_difficulty.
|
rebased on top of current master. |
|
@ignopeverell Were the "duplicate coinbase create" problems resolved prior to merge? |
|
We don't see it in our tests, perhaps we don't have enough real workers to reproduce it. However a bug was found so to use async stratum #2556 is needed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It seems that the current approach to handle incoming connections stops working after the amount of parallel connection exceeds ~300.
This PR introduces an 'async' feature which
enable asynchronous, tokio based implementation of stratum server.
I made a quick test. I run stratum server on a node with 8 CPU and 32Gb of memory and started to call 'KeepAlive' command 100Rps/per connection and slowly increasing the number of connections from 20 to 1000. This is, for example, 95% response time.
Tokio version doesn't show any sign of deterioration up until 1000 connections.
In order to enable tokio you need to build server with
--features async