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

maxusers: Update TODOs #466

Merged
merged 1 commit into from
Sep 24, 2019
Merged

maxusers: Update TODOs #466

merged 1 commit into from
Sep 24, 2019

Conversation

JoeGruffins
Copy link
Member

@JoeGruffins JoeGruffins commented Aug 10, 2019

-make max users a configurable variable
-default 10000
-stakepoold start with default 10000
-when starting dcrstakepool grpc commands are used to set the max on all servers and re-derive pool fees if there was a change

ahh, meant to do the draft thing, adding WIP to title

This pr took a different direction mid-stream. I now think that maxusers is an artificial ceiling and it should be removed. If my understanding is incorrect, please comment.

@JoeGruffins JoeGruffins reopened this Aug 10, 2019
@JoeGruffins JoeGruffins changed the title multi: SetMaxUsers from config [WIP] multi: SetMaxUsers from config Aug 10, 2019
sample-dcrstakepool.conf Outdated Show resolved Hide resolved
@JoeGruffins
Copy link
Member Author

I think this is finished. It will change a bit after #463 if that goes through first.

@JoeGruffins JoeGruffins changed the title [WIP] multi: SetMaxUsers from config multi: SetMaxUsers from config Aug 13, 2019
@JoeGruffins JoeGruffins force-pushed the maxusers branch 3 times, most recently from 0df42ec to a085af3 Compare August 16, 2019 01:04
@JoeGruffins
Copy link
Member Author

Will rebase after release.

@JoeGruffins
Copy link
Member Author

JoeGruffins commented Aug 22, 2019

@jholdstock Let me know your thoughts:
Q. What is max users there for?
My A. To decide the number of fee addresses that should be derived.
Q. Should max users be settable?
A. Yes. As dcrstakepool is now, a pool that is approaching max users must be able to increase this to allow more users.
Q. Should it be set in one place?
A. No preference from me. But I believe you think this could lead to bugs later on. In which case the appropriate path to take is add maxusers to the stakepoold conf and check that they are all the same on startup. If not let the operator change his/her conf.
Q. What's the future of max users?
A. Lets remove it. I believe there was talk yesterday on setting all of the fee addresses inside dcrwallet. It shouldn't be done there. Having a limit on users because of the feeaddresses (if that is the reason for this config setting) is silly.

Edit: I just remembered this is also set in the dcrwallet conf.

@JoeGruffins
Copy link
Member Author

This pr won't work. stakepoolcoldextkey must also be set on wallet start-up and there's no way to query/set the value afterwards. As I think this entire concept will be changed in the end it's probably not worth making this value settable through rpc commands. As for this pr and the issue, either we should make it settable also in stakepoold conf or make it a constant for now. currently max users can't exceed 10000, even if set above, so making it a constant should not have an affect on any current pools.

@JoeGruffins JoeGruffins changed the title multi: SetMaxUsers from config [WIP] multi: SetMaxUsers from config Aug 22, 2019
@JoeGruffins
Copy link
Member Author

If there's no good way to check that dcrwallet.conf value, I think making this settable is a bad idea. I'm going to make maxusers a constant unless there is an objection.

@JoeGruffins JoeGruffins reopened this Aug 23, 2019
@JoeGruffins JoeGruffins changed the title [WIP] multi: SetMaxUsers from config maxusers: Update TODOs Aug 23, 2019
@JoeGruffins
Copy link
Member Author

Ok. I think #310 should be closed in order to progress in the direction of #504.

@dajohi dajohi added this to the v1.3.0 milestone Sep 18, 2019
Copy link
Member

@jholdstock jholdstock left a comment

Choose a reason for hiding this comment

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

Sorry this one got ignored for so long. I've read your braindumps above and I agree with the gist of it all, so lets just merge this one and look back at this once we have privacy implemented.

@dajohi dajohi merged commit e6e3418 into decred:master Sep 24, 2019
jyap808 pushed a commit to ubiq/dcrstakepool that referenced this pull request Dec 16, 2019
ljk662 pushed a commit to bisontrails/dcrstakepool that referenced this pull request Mar 4, 2020
ljk662 pushed a commit to bisontrails/dcrstakepool that referenced this pull request Mar 4, 2020
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.

3 participants