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

deprecate votingwalletextpub dcrstakepool config option #496

Open
itswisdomagain opened this issue Aug 21, 2019 · 9 comments
Open

deprecate votingwalletextpub dcrstakepool config option #496

itswisdomagain opened this issue Aug 21, 2019 · 9 comments

Comments

@itswisdomagain
Copy link
Member

itswisdomagain commented Aug 21, 2019

The votingwalletextpub is only used for generating a ticket address to pair with a user's wallet address to generate a multisig. #451 moved the multisig creation process to stakepoold but the ticket address is still generated by dcrstakepool using the votingwalletextpub config value. I believe the entire op can be moved to stakepoold (generate the per-user ticket address in stakepoold) and thus retire the votingwalletextpub config option.

This would render part of the work done in #422 obsolete.

Also, with reference to #451, there would be no need to perform the CreateMultisig rpc call on all stakepoold servers if a check is done at dcrstakepool startup to ensure that the votingwalletextpub is the same across all stakepoold instances. This check can be performed without needing to save the votingwalletextpub value somewhere; but rather allow the value to be retrieved for use in stakepoold when the CreateMultisig rpc method is invoked.

With the above note in mind, #422 could then focus on ensuring that the dcrwallet instances running on all stakepoold backends are properly configured (by checking that the votingwalletextpub value is the same across all stakepoold instances). #422 would also continue to check that the coldwalletextpubkey does not belong to any of the stakepoold dcrwallet instances.

@JoeGruffins
Copy link
Member

JoeGruffins commented Aug 21, 2019

I may be reading this wrong, but #422 will deprecate votingwalletextpub.

@itswisdomagain
Copy link
Member Author

itswisdomagain commented Aug 21, 2019

I may be reading this wrong, but #422 will deprecate votingwalletextpub.

Oops, I meant #422 in my issue desc actually, not #407. Will correct.

Yes, it does deprecate the option from dcrstakepool config but does not completely retire it from dcrstakepool. The approach in #422 so far is to issue rpc calls to stakepoold to retrieve the voting wallet pub key. I'm suggssting we shouldn't even need to do that.

#422 is already a work in the desired direction, hence I meant to reference it as what could be modified to apply the suggestion above. I mis-referenced #407.

@JoeGruffins
Copy link
Member

I feel like there was a reason @jholdstock wanted to do the multisig on all the wallets. So the voting wallets watched addresses are incremented I think. The xpub is also used when a new user signs up, before they make the multisig our address for them is saved in the db. I guess that could be changed tho.

@itswisdomagain
Copy link
Member Author

itswisdomagain commented Aug 21, 2019

Ah, I see. What you say about the wallet watched addresses increment makes some sense. I'll look at the code again though to confirm if that's necessary.

I'm not sure the ticket address from the voting wallet is saved before the multisig is created. In any case, we can/should modify the CreateMultisig rpc method implementation to return the ticket address for the user as well, so it can be saved in the database after the multisig is created. Also, the address generation, saving to db and multisig creation is really only done when the user submits his wallet address, not necessarily when a user signs up. When a user does submit his wallet address, the intent is to generate and save the multisig.

@JoeGruffins
Copy link
Member

Yeah, I was wrong about the xpub being used on sign up. I don't think it matters until the multisig is made.

@JoeGruffins
Copy link
Member

What would be the benefit of not saving the xpub?

@itswisdomagain
Copy link
Member Author

itswisdomagain commented Aug 21, 2019

If there's no need for it, why save it? And I don't see a need for it as it can easily be included in the CreateMultisig stakepoold rpc method. If your point about incrementing watched addresses on all backend wallets holds true, then generating the ticket address when CreateMultisig is invoked on all backend stakepoold instances is even more profitable.

@jholdstock
Copy link
Member

https://matrix.to/#/!HEeJkbPRpAqgAwhXWO:decred.org/$156561569214852DcEGd:decred.org?via=decred.org&via=matrix.org&via=zettaport.com

Calling CreateMultisig on all wallets is useful because it ensures all wallets register all of the multi-sig pubkeys

@itswisdomagain
Copy link
Member Author

That makes sense @jholdstock. The goal of this issue then would be to have all stakepoold instances generate the ticket address that is paired with the user's address for creating the multisig, instead of generating it on dcrstakepool then passing the generated address to stakepoold CreateMultisig rpc. Thus, we can retire the votingwalletextpub config option in dcrstakepool.

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

No branches or pull requests

3 participants