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 MPOS SSO DB Compatibility #612

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ahmedbodi
Copy link

No description provided.

@ahmedbodi
Copy link
Author

@HashUnlimited @zone117x merge?

@HashUnlimited
Copy link
Collaborator

There’s a problem with MPOS always taking the diff from pool_workers and using it for hashrate calculations. Seb is out of the game already so we need to take care.

@ahmedbodi
Copy link
Author

This PR just sticks with the current behaviour in regards to the diff. Lets work through it step by step. This would fix the SSO Support. and then we can fix MPOS's hashrate calcs in another PR

@HashUnlimited
Copy link
Collaborator

HashUnlimited commented Jul 10, 2018

So what issue do you see in SSO? Maybe I don’t get it

Edit: for my understanding you would update the shared db on worker diff change. That’d be good for single pools on pushpoold but on setups like ours just letting MPOS handle the frontend it would basically result in altered dashboard values?

@ahmedbodi
Copy link
Author

MPOS Supports SSO for accounts and workers correct?
This was a new feature developed before myself with stratum-mining and @zone117x with nomp added support for it.

So the problem lies here:

MPOS set to SSO on workers will add all the workers to the SSO db but expect shares in the normal DB. So NOMP will either submit shares to the wrong db or look for workers in the wrong db.

Another one is accounts. If NOMP is set to auto create workers it'll look in the db specified in the config which wont be the SSO db for the reasons specified above. So it'll think the user doesnt exist and wont create the account.

@@ -5,14 +5,22 @@ module.exports = function(logger, poolConfig){
var mposConfig = poolConfig.mposMode;
var coin = poolConfig.coin.name;

var connection = mysql.createPool({
var mainDBConnection = mysql.createPool({
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per Coin DB Connection details to be specified in config to be used here

port: mposConfig.port,
user: mposConfig.user,
password: mposConfig.password,
database: mposConfig.shared_database
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shared SSO DB to be specified here

@@ -26,7 +34,7 @@ module.exports = function(logger, poolConfig){
return;
}

connection.query(
sharedDBConnection.query(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look for Workers in SSO DB rather than per coin db

@@ -89,7 +97,7 @@ module.exports = function(logger, poolConfig){
typeof(shareData.error) === 'undefined' ? null : shareData.error,
shareData.blockHash ? shareData.blockHash : (shareData.blockHashInvalid ? shareData.blockHashInvalid : '')
];
connection.query(
mainDBConnection.query(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Insert Shares into Coin DB

@@ -103,14 +111,14 @@ module.exports = function(logger, poolConfig){

this.handleDifficultyUpdate = function(workerName, diff){

connection.query(
'UPDATE `pool_worker` SET `difficulty` = ' + diff + ' WHERE `username` = ' + connection.escape(workerName),
sharedDBConnection.query(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update difficulty in Share DB not worker DB.

(This is what we need to get rid of soon but until we come up with a way to handle the issues caused by this, let's not remove it yet)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now here's the issue I was referring to. For payouts, MPOS is using the share DB but for stats it's looking into the worker DB

@ahmedbodi
Copy link
Author

Check the Comments on the commit. I've described the changes there

@HashUnlimited
Copy link
Collaborator

Exactly. So for SSO to function correctly, the way is to set up the MPOS config for shared but leave out the workers table. That’s tbd by user. Was playing with the topic just yesterday, Seb isn’t totally aware. Going to update the wiki and a PR removing the workers example. Clean up and remove the code as well.

@ahmedbodi
Copy link
Author

It also means disabling the autoCreateWorkers option of NOMP

@HashUnlimited
Copy link
Collaborator

So what is the approach? I still don’t get it. Do you simply want workers getting copied across the pools?

@HashUnlimited
Copy link
Collaborator

HashUnlimited commented Jul 10, 2018

I guess we both are just thinking from different sides, you might want to find a solution so MPOS is handling the worker updates when configured as SSO and I am thinking from the MPOS side

@ahmedbodi
Copy link
Author

Okay lets step through it. What's the issue with MPOS?

@HashUnlimited
Copy link
Collaborator

I am not 100% sure about the dashboard but the admin panel and statistics are checking the worker table for update.
In general, sharing workers across pools is a bad idea at all:

  • Updating a worker on one pool will disconnect miners on any other pool where it's used
  • Statistics per worker aren't useful when the worker is shared

Is there on the other side any useful aspect in sharing workers nowadays?

@ahmedbodi
Copy link
Author

The idea behind it when it was developed was that someone could use the same worker on various different pools. a common concept was that people used the .1 worker on every pool they were using. One of the other main reasons for this is "multipools" many of which are based on MPOS. The only exceptions i know of are YIIMP based ones which seem extremely popular. and Multipool.us and IPOMiner which are based on an ancient software called simplecoin which they updated themselves. The way they work is having the central db contain the workers and then just working round robin between the stratum instances based on the profitability.

Updating a worker on one pool shouldn't disconnect a worker on any other pool.
Also you're correct re: statistics per worker. With SSO we should use the central DB for verifying the workers, while inputting the difficulty and statistics info into the coin DB.

@HashUnlimited
Copy link
Collaborator

Sorry, I have been busy recently.... how about we create a dev_dev branch and check the things through?

@ahmedbodi
Copy link
Author

sure thats fine by me

@HashUnlimited
Copy link
Collaborator

After long long time working on main net, we finally found some time to test this on a test pool :-)
As expected, the result is not how I (personally) would prefer to run my pool cluster, but on the other hand side there's no negative impact if handled correctly. A few lines of documentation would be nice, what are your thoughts @zone117x ? I'd say give it a go.

@zone117x
Copy link
Owner

Doesn't this need some checks for the new mposMode configuration property so existing setups aren't broken?
Looks good if we get those checks and a bit of documentation for the new configuration.

@HashUnlimited
Copy link
Collaborator

HashUnlimited commented Sep 11, 2018

Existing installations would need to add the shared_databaseto their config. Actually good practise would be to fall back to the standard database if this doesn't exist in the file. The example config should be updated as well, @ahmedbodi could you maybe tweak a little more?

[edit:] We might also keep in mind, that MPOS allows the configuration of the shared DB per table, it would probably be a good idea to go with that flexibility as well so it's up to the user if he wants to work with shared accounts only or workers as well.

@ahmedbodi
Copy link
Author

I'll try and find some time to get this done

@le2Ks
Copy link

le2Ks commented May 28, 2021

It doesn't work :(

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.

4 participants