-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Lowdiff shares (2nd update #2718) #2728
Conversation
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.
Check my change comment, other than that it looks okay. Make sure to test it a bit
include/classes/statistics.class.php
Outdated
IFNULL(SUM(difficulty) / ?, 0) AS sharerate, | ||
IFNULL(SUM(difficulty), 0) AS shares, | ||
IFNULL(AVG(difficulty), 0) AS avgsharediff | ||
IFNULL(SUM(difficulty) / ?, " . $this->coin->getShareDifficultyPrecision() . ") AS sharerate, |
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.
This should remain 0, if the column is NULL assume 0. No rounding done here. Same for the others below.
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, will change back
Once merged maybe @HashUnlimited can update their branch and re-run the DB schema migration. Don't forget to reset the DB_VERSION string again so it will run properly. Blocks should then show the proper amount of shares even with blocks < 1 diff share. |
just resetting wallet and db again... moving @r4sas remote |
@HashUnlimited @r4sas since you are both kinda doing some P2P coding and testing here, please let me know when this is ready for merge into development. I think it's close to ready if not completed. I have looked into the float/double for shares and I am not entirely sure ... I doubt think that double precision on share difficulties per user (which is what it is in that table) would not exceed that many shares. But if we want to play save we could increase it to double (but potentially waste 4 bytes per entry since it's not used). No idea what's best? What do you think? |
In that case we can set to unsigned float because here will never be negative values. |
Good compromise :) Doubles the range and should be sufficient for single-account share counting. |
sufficient for sure, the maximum diff in all bitcoin based code is 2.6959947e+67 if we are confident we aren't running into
I'm fine |
one more tiny tiny comment once we are on it already, the Totals in the block statistics are showing integers, if it's simple it could be aligned to the new global style. |
Let me check... |
Let's merge this now, looks complete and can be further tested on development. If more changes are required, we can start new PRs. |
I hope that's all :D |
agreed, clicking through there's only one place left which is the graph but we don't want to be over-accurate here. for my part, all ready to test it squeeze it and try to break it ;-) |
In Statitics -> Graphs -> sharerate? |
exactly, that's the very last place I could find :D |
Found where it happens. By the way I have opinion about that graphics:
We store that json string triple times. I think better to save it in one variable and reuse from it rather then store 3 times. php-mpos/templates/bootstrap/statistics/graphs/default.tpl Lines 3 to 46 in e79b558
|
Probably a good idea ... If you find a quick and easy way to do it, go ahead :) |
Please verify and test before merging.