search

decred / dcrstakepool

Stakepool for Decred.
Other
72 stars 75 forks source link

multi: Move script syncing to stakepoold #618

Closed JoeGruffins closed 3 years ago

JoeGruffins commented 4 years ago

closes #613

Remove grpc methods ImportMissingScripts and ListScripts. Stakepoold now require a connection to a mysql database in order obtain user preferences which include a list of redeem scripts and heights registered. Upon start-up, all scripts found in the database but not in dcrwallet are imported. Remove user config save file as all data will come from the database.

This is an alternative to ~#606~ #619

chappjc commented 4 years ago

I always thought we wanted to get rid of the stakepoold-mysql connection. stakepoold already requires a mysql connection, no? I'm confused by the comment in https://github.com/decred/dcrstakepool/pull/606#issuecomment-597964050 because the last release of stakepoold definitely connects to mysql, and I never liked that.

From https://github.com/decred/dcrstakepool/blob/master/README.md, a bullet point below the Architecture diagram says:

The architecture is subject to change in the future to lessen the dependence on MySQL.

chappjc commented 4 years ago

Is a stakepoold-mysql connection only used presently when --norpclisten is enabled?

EDIT: No it was required already it seems: https://github.com/decred/dcrstakepool/blob/d9f62149ea4efe45129dfe85627630ad34f191bd/backend/stakepoold/server.go#L192-L204 Could it really work without a db connection somehow?

chappjc commented 4 years ago

I'm strongly in favor of https://github.com/decred/dcrstakepool/pull/606 if it works too. What are the pros and cons of each solution (this PR vs. https://github.com/decred/dcrstakepool/pull/606)?

chappjc commented 4 years ago

See also https://github.com/decred/dcrstakepool/issues/495, which I think makes a very strong case, and https://github.com/decred/dcrstakepool/issues/134 for historical context.

JoeGruffins commented 4 years ago

I left my thoughts about connecting mysql and stakepoold in #495

You can see in this code that cutting out the middle man for this one instance has also cut the needed code in half.

However, if this indeed causes a security issue, I will work on another implementation using addressesbyaccount. Security is most important.

itswisdomagain commented 4 years ago

However, if this indeed causes a security issue, I will work on another implementation using addressesbyaccount. Security is most important.

I made an implementation using getaddressesbyaccount a while ago. I think I'll just create a PR for it and throw it into the mix for consideration.

UPDATE: PR is up: #619.

1 advantage of that PR is the minimal change set introduced while maintaining existing functionality.

We can probably get that reviewed and merged while we continue to discuss whether we should or shouldn't allow stakepoold run independent of dcrstakepool.

If we do decide to continue supporting stakepoold's dcrstakepool-independent usages, then we can come up with a means for stakepoold to get the desired user data on startup without connecting directly to the database. I completely agree with @chappjc's comment here.