mulit: remove voting public key from config and check fee public key on start up #422

JoeGruffins · 2019-06-25T23:24:18Z

After stakepools are confirmed running, loop through them sending a grpc request to perform rpc requests on their respective dcrd to get maps of master pubkeys to account. found pairs are printed and then the user defined key values are compared to the found keys. The voting key must be found to continue, the fee key must not be found.

Still want to write some tests for the grpc and rpc methods.

closes #407

JoeGruffins · 2019-06-27T04:55:50Z

Hmm. rpc tests are a little more than I bargained for. It's on my todo list... I will test a little more manually.

jholdstock

A couple of smal

stakepooldclient/stakepooldclient.go

backend/stakepoold/rpc/rpcserver/context.go

JoeGruffins · 2019-07-18T13:03:26Z

Ok. The coldwalletextpub is checked to not belong to the voting wallets. The votingwalletextpub is found automatically. votingwalletextpub is deprecated and there is a warning if set. It has been removed from the readme and a warning is in the config file.

JoeGruffins · 2019-07-18T23:45:07Z

I'm not sure that this travis build error is my fault. Google says I can close and reopen the issue to trigger a rebuild. Trying.

README.md

backend/stakepoold/rpc/rpcserver/context.go

stakepooldclient/stakepooldclient.go

jholdstock · 2019-07-22T09:27:48Z

I'm not sure that this travis build error is my fault. Google says I can close and reopen the issue to trigger a rebuild. Trying.

I didnt know you could trigger a rebuild like that. Alternatively you can just log into travis with your github ID and it should allow you to press a button which rebuilds. Not sure if you need any repo permissions to do that though...

JoeGruffins · 2019-07-22T23:03:32Z

I didnt know you could trigger a rebuild like that. Alternatively you can just log into travis with your github ID and it should allow you to press a button which rebuilds. Not sure if you need any repo permissions to do that though...

I just do what google tells me to do. I should start a new religion. Would you too like to become a Goober?

JoeGruffins · 2019-07-23T22:36:01Z

Should this increment the api version major, since it is a breaking change?

jholdstock · 2019-07-25T11:06:16Z

Should this increment the api version major, since it is a breaking change?

Yes please.

jholdstock · 2019-07-25T12:14:05Z

The latest changes look good, just the API version to increment and one outstanding comment from me.

Needs to be rebased also, some conflicts have popped up.

JoeGruffins · 2019-07-25T23:06:17Z

Rebased and breaking this pr into its parts. The final check will also be broken into two methods.

JoeGruffins · 2019-07-27T01:59:42Z

@jholdstock what do you think?

README.md

backend/stakepoold/rpc/rpcserver/context.go

config.go

sample-dcrstakepool.conf

server.go

stakepooldclient/stakepooldclient.go

JoeGruffins · 2019-08-01T03:08:32Z

@chappjc thank you for the review and sorry about the basic formatting mistakes. They won't happen again.

chappjc · 2019-08-01T19:44:38Z

Oh, no worries. It's small stuff.

backend/stakepoold/rpc/rpcserver/server.go

jholdstock

Two more minor things noted. Otherwise I am happy with this. @chappjc are you satisfied all of your points were addressed?

config.go

JoeGruffins · 2019-08-18T23:05:06Z

Will rebase after release.

stakepooldclient/stakepooldclient.go

itswisdomagain · 2019-08-21T01:45:26Z

stakepooldclient/stakepooldclient.go

+// error if pubkeys for the "default" account are not the same accross all
+// stakepoold wallets or any gRPC commands fail.
+func (s *StakepooldManager) DefaultAccountPubKey() (string, error) {
+	var pubkey string


Could rename to validDefaultAccountPubKey to signify that

the pubkey of interest is for the default account (should be obvious but could become confusing if my previous suggestion is applied)

the value stored here at the end of the day ought to be the same for all stakepoold instances (hence the valid prefix)

Could probably just use named return variables as well.

jholdstock isn't a fan of naked returns.

@dajohi isn't either

Could just rename the variable then

Named return variables (as suggested by itswisdomagain) are fine. I actually kind of wish naming return values was enforced and not optional.

It's just naked returns which are icky.

I was thinking of it as a package deal, but you mean naming them, and then placing their names in the return I guess. Never even thought about that.

@itswisdomagain sorry, my naked comments on your pr are in the wrong place.

itswisdomagain · 2019-08-21T01:54:10Z

stakepooldclient/stakepooldclient.go

+		}
+		for _, v := range resp.MasterPubKeys {
+			if fn(v.Account, v.PubKey) {
+				return true, nil


This return statement in this loop implies that the grpc request may likely not be made to ALL stakepoold instances. Not sure if this was intended, PR description says After stakepools are confirmed running, loop through them sending a grpc request to perform rpc requests on their respective dcrd to get maps of master pubkeys to account; which I read to mean perform the grpc request on ALL stakepoold instances.

It's also important to check all instances to confirm/verify that the wallets on each backend server were all restored from the same seed; i.e. checking that the master pub key is the same across all the backend server wallets.

The first true is sufficient for what I use it for. Return true if a default xpub is different and return true if xpub is found. Once a true if found there is no need to continue, and if no trues then all accounts on all wallets have been iterated.

It is checking that the master pub key for the default account is the same across all servers.

I wrote it like this to avoid iterating the map twice. Perhaps the complexity is not worth it?

Don't really quite get you. What I mean to confirm is this:
Does this code retrieve the voting wallet xpub key from ALL stakepoold instances and confirm that the xpub key is the same across ALL stakepoold instances?

Yes.

It should. Does it not? 😬

itswisdomagain · 2019-08-21T01:58:58Z

stakepooldclient/stakepooldclient.go

+
+// WalletsHavePubKey sends gRPC commands to all stakepoold and returns whether
+// pubkey exists on any wallets. Returns an error if any gRPC commands fail.
+func (s *StakepooldManager) WalletsHavePubKey(pubkey string) (bool, error) {


I see that this is only used to confirm that the coldwalletextpub does not belong to the voting wallets. I think this check can be done at the same time as the master pub key for the default account is read to prevent performing the stakepoold WalletMasterPubKeys grpc request more than once.

1 idea is to invoke that grpc method once for ALL instances, ensuring that all the stakepoold instances return the same result (equal number of account-key pairs, with the same key value for each account on each backend server). Then the vetted account-key map is parsed to retrieve the default account pub key and also checked to ensure it does not contain the coldwalletextpub.

I would rather keep the functions separate, for easier testing and such, but you are right that we only need to call the grpc once. Because this operation only ever happens on start-up, I didn't think it was necessary, but we could cache the results, or use a clever closure scheme.

If it's all the same, I've already had numerous reviews on this pr, and it's working ok. Unless you think the gRPC commands will need to change, I'd rather further changes be made in another pr. That alright @itswisdomagain ?

Sure, haven't tested myself but if others have tested and it works ok, further changes can be made in a subsequent PR.

- set votingWalletVoteKey - check that coldwalletextpub does not belong to the pool

jholdstock · 2019-09-26T11:39:46Z

backend/stakepoold/rpc/rpcserver/server.go

@@ -33,8 +33,8 @@ const (
 	// collection cycle to also trigger a timeout but the current allocation
 	// pattern of stakepoold is not known to cause such conditions at this time.
 	GRPCCommandTimeout = time.Millisecond * 100
-	semverString       = "8.0.0"
-	semverMajor        = 8
+	semverString       = "9.0.0"


Needs to be updated to 9.1.0. We incremented to 9.0.0 in #526, and this PR adds a new method without breaking backwards compatability.

JoeGruffins · 2019-10-03T11:29:26Z

I think I will close this because I think this change will ultimately become pointless. It's not worth the time to rebase and review.

JoeGruffins force-pushed the checkpubs branch from fdc54a6 to 776a474 Compare June 27, 2019 04:53

jholdstock reviewed Jun 27, 2019

View reviewed changes

stakepooldclient/stakepooldclient.go Outdated Show resolved Hide resolved

backend/stakepoold/rpc/rpcserver/context.go Outdated Show resolved Hide resolved

backend/stakepoold/rpc/rpcserver/context.go Outdated Show resolved Hide resolved

JoeGruffins force-pushed the checkpubs branch 3 times, most recently from 2c90725 to da980ec Compare July 18, 2019 12:53

JoeGruffins changed the title ~~[WIP]Check pubkeys on start up~~ mulit: remove voting public key from config and check fee public key on start up Jul 18, 2019

JoeGruffins force-pushed the checkpubs branch from da980ec to 1790045 Compare July 18, 2019 22:28

JoeGruffins closed this Jul 18, 2019

JoeGruffins reopened this Jul 18, 2019

jholdstock requested changes Jul 22, 2019

View reviewed changes

README.md Show resolved Hide resolved

backend/stakepoold/rpc/rpcserver/context.go Outdated Show resolved Hide resolved

backend/stakepoold/rpc/rpcserver/context.go Outdated Show resolved Hide resolved

stakepooldclient/stakepooldclient.go Outdated Show resolved Hide resolved

JoeGruffins force-pushed the checkpubs branch from 1790045 to 3d926d1 Compare July 23, 2019 03:38

JoeGruffins force-pushed the checkpubs branch from 4fe687e to f663675 Compare July 25, 2019 23:04

JoeGruffins changed the title ~~mulit: remove voting public key from config and check fee public key on start up~~ [WIP]mulit: remove voting public key from config and check fee public key on start up Jul 25, 2019

JoeGruffins force-pushed the checkpubs branch 2 times, most recently from 1edecf6 to 8e2d812 Compare July 27, 2019 01:37

chappjc mentioned this pull request Jul 28, 2019

Config: Check that cold wallet key and voting wallet key are not the same #407

Open

chappjc requested changes Jul 30, 2019

View reviewed changes

JoeGruffins force-pushed the checkpubs branch 3 times, most recently from d57e816 to 55b3d77 Compare August 1, 2019 02:56

JoeGruffins changed the title ~~[WIP]mulit: remove voting public key from config and check fee public key on start up~~ mulit: remove voting public key from config and check fee public key on start up Aug 1, 2019

JoeGruffins commented Aug 2, 2019

View reviewed changes

backend/stakepoold/rpc/rpcserver/server.go Outdated Show resolved Hide resolved

JoeGruffins force-pushed the checkpubs branch 3 times, most recently from 9aae095 to 1748d12 Compare August 8, 2019 23:23

JoeGruffins force-pushed the checkpubs branch from 1748d12 to 82f875d Compare August 14, 2019 00:31

jholdstock requested changes Aug 14, 2019

View reviewed changes

config.go Outdated Show resolved Hide resolved

config.go Outdated Show resolved Hide resolved

itswisdomagain suggested changes Aug 21, 2019

View reviewed changes

JoeGruffins mentioned this pull request Aug 21, 2019

deprecate votingwalletextpub dcrstakepool config option #496

Open

JoeGruffins force-pushed the checkpubs branch from 986c298 to 144fdeb Compare August 25, 2019 08:23

dajohi added this to the v1.3.0 milestone Sep 15, 2019

JoeGruffins added 10 commits September 19, 2019 03:07

stakepoold: add MyMasterPubkeys gRPC command

57c7c30

config: remove votingWalletVoteKey

4443cde

readme: update votingwalletextpub instructions

15bfb46

sample-dcrstakepool.conf: deprecate votingwalletextpub

50a3735

stakepooldclient: add DefaultAccountPubKey and WalletsHavePubkey methods

38a3333

server.go: get public key from stakepoold

65c07e7

- set votingWalletVoteKey - check that coldwalletextpub does not belong to the pool

stakepoold: increment semver

cbfec92

config: replace tab with space

f593940

stakepooldclient: Make func names more descriptive

ff6333e

stakepoold: increment semver

1e6210f

JoeGruffins force-pushed the checkpubs branch from 144fdeb to 1e6210f Compare September 19, 2019 03:08

jholdstock reviewed Sep 26, 2019

View reviewed changes

JoeGruffins closed this Oct 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mulit: remove voting public key from config and check fee public key on start up #422

mulit: remove voting public key from config and check fee public key on start up #422

JoeGruffins commented Jun 25, 2019 •

edited

Loading

JoeGruffins commented Jun 27, 2019

jholdstock left a comment

JoeGruffins commented Jul 18, 2019

JoeGruffins commented Jul 18, 2019

jholdstock commented Jul 22, 2019

JoeGruffins commented Jul 22, 2019 •

edited

Loading

JoeGruffins commented Jul 23, 2019

jholdstock commented Jul 25, 2019

jholdstock commented Jul 25, 2019

JoeGruffins commented Jul 25, 2019

JoeGruffins commented Jul 27, 2019

JoeGruffins commented Aug 1, 2019

chappjc commented Aug 1, 2019

jholdstock left a comment

JoeGruffins commented Aug 18, 2019

itswisdomagain Aug 21, 2019

JoeGruffins Aug 23, 2019

dajohi Aug 23, 2019

itswisdomagain Aug 23, 2019

jholdstock Sep 23, 2019

JoeGruffins Sep 23, 2019

itswisdomagain Aug 21, 2019

JoeGruffins Aug 23, 2019

JoeGruffins Aug 23, 2019

itswisdomagain Aug 23, 2019

JoeGruffins Aug 24, 2019

itswisdomagain Aug 21, 2019

JoeGruffins Aug 21, 2019

JoeGruffins Aug 21, 2019

itswisdomagain Aug 21, 2019

jholdstock Sep 26, 2019

JoeGruffins commented Oct 3, 2019

mulit: remove voting public key from config and check fee public key on start up #422

mulit: remove voting public key from config and check fee public key on start up #422

Conversation

JoeGruffins commented Jun 25, 2019 • edited Loading

JoeGruffins commented Jun 27, 2019

jholdstock left a comment

Choose a reason for hiding this comment

JoeGruffins commented Jul 18, 2019

JoeGruffins commented Jul 18, 2019

jholdstock commented Jul 22, 2019

JoeGruffins commented Jul 22, 2019 • edited Loading

JoeGruffins commented Jul 23, 2019

jholdstock commented Jul 25, 2019

jholdstock commented Jul 25, 2019

JoeGruffins commented Jul 25, 2019

JoeGruffins commented Jul 27, 2019

JoeGruffins commented Aug 1, 2019

chappjc commented Aug 1, 2019

jholdstock left a comment

Choose a reason for hiding this comment

JoeGruffins commented Aug 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins commented Oct 3, 2019

JoeGruffins commented Jun 25, 2019 •

edited

Loading

JoeGruffins commented Jul 22, 2019 •

edited

Loading