Chores grooming #11

pavelkrolevets · 2023-09-14T12:05:02Z

No description provided.

y0sher · 2023-09-14T14:34:11Z

README.md

 ```

-Its also possible to use yaml configuration file `./config/initiator.yaml` for parameters. `dkgcli` will be looking for this file at `./config/` folder.
+Its also possible to use yaml configuration file `./config/initiator.yaml` for parameters. `dkgcli` will be looking for this file at `./config/` folder at a same root as the binary.


dkgcli -> ssv-dkg

y0sher · 2023-09-14T14:34:32Z

README.md

 ```

 When using configuration file, run:

 ```sh
-dkgcli init-dkg
+ssv-dkg init-dkg


can be just ssv-dkg init no need to repeat dkg

y0sher · 2023-09-14T14:34:54Z

README.md

 port: 3030
 storeShare: true
 ```

 When using configuration file, run:

 ```sh
-dkgcli start-dkg-server
+ssv-dkg start-dkg-operator


start-dkg-operator -> start-operator

y0sher · 2023-09-14T14:43:48Z

integration_test/integration_test.go

+	// t.Run("test 7 operators happy flow", func(t *testing.T) {
+	// 	ops := make(map[uint64]initiator.Operator)
+	// 	srv1 := CreateOperator(t, 1)
+	// 	ops[1] = initiator.Operator{srv1.srv.URL, 1, &srv1.privKey.PublicKey}
+	// 	srv2 := CreateOperator(t, 2)
+	// 	ops[2] = initiator.Operator{srv2.srv.URL, 2, &srv2.privKey.PublicKey}
+	// 	srv3 := CreateOperator(t, 3)
+	// 	ops[3] = initiator.Operator{srv3.srv.URL, 3, &srv3.privKey.PublicKey}
+	// 	srv4 := CreateOperator(t, 4)
+	// 	ops[4] = initiator.Operator{srv4.srv.URL, 4, &srv4.privKey.PublicKey}
+	// 	srv5 := CreateOperator(t, 5)
+	// 	ops[5] = initiator.Operator{srv5.srv.URL, 5, &srv5.privKey.PublicKey}
+	// 	srv6 := CreateOperator(t, 6)
+	// 	ops[6] = initiator.Operator{srv6.srv.URL, 6, &srv6.privKey.PublicKey}


why is this commented? anyway it should be pretty easy to write the functionality to run 4/7/13 tests without copy pasting so much.

Yeah, some leftover I forgot about. Should be uncommented.

y0sher

Looks good! please see my comments, also lets fix tests.

liorrutenberg · 2023-09-18T12:18:48Z

Lets please remove unused functions such as:

https://github.com/bloxapp/ssv-dkg/blob/ac06e5464f055fb39b5cdd854deba17b81308266/pkgs/load/operators.go
Operators(path string)

OperatorsPubkeys(path string)

liorrutenberg · 2023-09-18T12:19:29Z

@pavelkrolevets can we please add lint command to Makefile

liorrutenberg · 2023-09-18T12:28:29Z

Makefile

 	@echo "Done building."
-	@echo "Run dkgcli to launch the tool."
+	@echo "Run ssv-dkg to launch the tool."


remove tool

Suggested change

@echo "Run ssv-dkg to launch the tool."

@echo "ssv-dkg installed successfully."

liorrutenberg · 2023-09-18T12:29:15Z

README.md

@@ -1,77 +1,112 @@
 # ssv-dkg-tool


remove tool

Suggested change

# ssv-dkg-tool

# ssv-dkg

liorrutenberg · 2023-09-18T12:32:32Z

incorporate logs to file similar to ssv

liorrutenberg · 2023-09-18T14:24:31Z

Additional chores:

Remove plain text support for operator
IMO we should add config example files in one place for both operator & initiator
and rename files to xx.example.yaml
move imgs folder to docs folder
Should we restrict operators to use SSL only?
add GPL license

liorrutenberg · 2023-09-18T14:27:08Z

make docker-build fails after make build (add .dockerignore to exclude bin folder)

liorrutenberg

see conversion

liorrutenberg · 2023-09-18T14:52:04Z

Remove CSV example files, we choose to support only JSON
in example there are 4 servers and each has config file, yet for docker we use env params, so IMO it's redundant

liorrutenberg · 2023-09-18T14:56:59Z

Failed to save output

liorrutenberg · 2023-09-18T15:25:27Z

deposit and payload files should have a prefix (validator pk)

pkgs/dkg/drand.go

y0sher

see my comments

y0sher

Great job!
@moshe-blox @andrew-blox please take final look so we can merge

andrew-blox · 2023-10-03T12:46:06Z

pkgs/crypto/crypto.go

+		return nil, err
+	}
+	block, _ := pem.Decode(operatorKeyByte)
+	// TODO: resolve deprecation https://github.com/golang/go/issues/8860


Should we resolve this?

andrew-blox · 2023-10-03T12:46:08Z

pkgs/crypto/crypto.go

+	b := block.Bytes
+	if enc {
+		var err error
+		// TODO: resolve deprecation https://github.com/golang/go/issues/8860


Should we resolve this?

Well we have the same thing in our node, @MatusKysel mentioned something about the way we do RSA, maybe we should revisit it entirely, for sure it's not the place to do here.

moshe-blox

Looks good 👍

Left a few comments, although did not finish reviewing yet.

moshe-blox · 2023-10-04T16:26:04Z

cli/initiator/initiator.go

 		}
 		nonce := viper.GetUint64("nonce")
-		withdrawPubKey, err := hex.DecodeString(withdrawAddr)
+		withdrawPubKey := common.HexToAddress(withdrawAddr).Bytes()


HexToAddress is dangerous for user input because it fails silently, so you can pass just an "m" (for example) and it would continue without notifying the user of his mistake.

WDYT about making our own address parsing func that would return an error if input is too long, too short or not a valid hex?

added HexToAddress with error handling to utils + unit tests for it

moshe-blox · 2023-10-04T16:33:31Z

pkgs/operator/operator.go

@@ -132,10 +134,14 @@ func New(key *rsa.PrivateKey) *Server {
 }

 func (s *Server) Start(port uint16) error {
-	s.Logger.Infof("server listening for incoming requests on port %d", port)
 	srv := &http.Server{Addr: fmt.Sprintf(":%v", port), Handler: s.Router}


Not part of this PR, but it'd be nice if we can add read/write timeouts to the `http.Server to prevent flooding attacks.

We have Chi limiting requests to routes, to prevent DDOS, if I understood you correctly

moshe-blox · 2023-10-04T16:41:28Z

pkgs/operator/state.go

+	logger.Info("🚀 Initializing DKG instance")
+	init := &wire.Init{}
+	if err := init.UnmarshalSSZ(initMsg.Data); err != nil {
+		return nil, err


We should return error wrappings (as you did below in VerifyRSA) here and in the rest of the returns, so we can tell where exactly things failed in retrospect.

added error wrapps at state. Should we have wrapps everywhere?

- change deposit and keyshares results param names

liorrutenberg · 2023-10-10T13:42:03Z

Merged as part 1 of grooming

pavelkrolevets added 10 commits September 13, 2023 16:03

change sever to operator and clint to initiator

46a4190

update readme about operator`s priv key

8f83e7a

change plaintext password to file

64ee75f

add logs about message signature verification

a5b52c5

change fork hex to name

44e019f

add identity to initiator

a6d122f

add security notes to readme

9ddfa77

update flags + docker

aa1c0a7

rename project

ba097fb

readme update

8935dad

y0sher reviewed Sep 14, 2023

View reviewed changes

y0sher requested changes Sep 14, 2023

View reviewed changes

pavelkrolevets added 3 commits September 15, 2023 17:07

fix tests + work on comments

ecf4e91

add test of initiator identity

4ad85a0

update test operator

ac06e54

liorrutenberg reviewed Sep 18, 2023

View reviewed changes

liorrutenberg requested changes Sep 18, 2023

View reviewed changes

add linter + fixes

a71153e

y0sher reviewed Oct 2, 2023

View reviewed changes

pkgs/dkg/drand.go Outdated Show resolved Hide resolved

y0sher reviewed Oct 2, 2023

View reviewed changes

pkgs/dkg/drand.go Outdated Show resolved Hide resolved

y0sher reviewed Oct 2, 2023

View reviewed changes

pkgs/dkg/drand.go Outdated Show resolved Hide resolved

y0sher requested changes Oct 2, 2023

View reviewed changes

pavelkrolevets added 4 commits October 2, 2023 18:53

move NewID to crypto

d7a439c

remove VerifyFunc

7949342

move VerifyInitiatorMessage to state

8765162

removed rsa priv from local owner

c3c1dcd

y0sher approved these changes Oct 3, 2023

View reviewed changes

andrew-blox approved these changes Oct 3, 2023

View reviewed changes

y0sher added 9 commits October 3, 2023 23:24

logs path

e1caae3

change payload strcuture

e6fd291

fix payload to v4

8c9050c

correct help for flag

8704e08

set default nonce to 0

2fad7d7

fixed integration tests to new format

8a21497

added single operator/initiator to Makefile

28c4c0a

change withdrawl address to be ETH1

e79c08b

fix test to look for the ETH1 addr

b0a1ae6

y0sher approved these changes Oct 4, 2023

View reviewed changes

moshe-blox requested changes Oct 4, 2023

View reviewed changes

pavelkrolevets and others added 5 commits October 5, 2023 13:46

add hexToAddress function with error handling

f09a35a

add error wrapper at state

62077ed

fix operator test

b623cbc

- change ssvpayload name to keyshares

a2b65cf

- change deposit and keyshares results param names

change error to show bad sigs

695177e

y0sher approved these changes Oct 10, 2023

View reviewed changes

liorrutenberg merged commit 214ec16 into main Oct 10, 2023
1 check passed

pavelkrolevets deleted the chores_grooming branch November 23, 2023 06:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chores grooming #11

Chores grooming #11

pavelkrolevets commented Sep 14, 2023

y0sher Sep 14, 2023

y0sher Sep 14, 2023

y0sher Sep 14, 2023

y0sher Sep 14, 2023

pavelkrolevets Sep 15, 2023 •

edited

Loading

y0sher left a comment •

edited

Loading

liorrutenberg commented Sep 18, 2023

liorrutenberg commented Sep 18, 2023

liorrutenberg Sep 18, 2023

y0sher Sep 19, 2023 •

edited

Loading

liorrutenberg Sep 18, 2023

y0sher Sep 19, 2023

liorrutenberg commented Sep 18, 2023

liorrutenberg commented Sep 18, 2023

liorrutenberg commented Sep 18, 2023

liorrutenberg left a comment

liorrutenberg commented Sep 18, 2023

liorrutenberg commented Sep 18, 2023 •

edited

Loading

liorrutenberg commented Sep 18, 2023

y0sher left a comment

y0sher left a comment

andrew-blox Oct 3, 2023

andrew-blox Oct 3, 2023

y0sher Oct 3, 2023

moshe-blox left a comment

moshe-blox Oct 4, 2023 •

edited

Loading

pavelkrolevets Oct 5, 2023

moshe-blox Oct 4, 2023 •

edited

Loading

pavelkrolevets Oct 5, 2023

moshe-blox Oct 4, 2023 •

edited

Loading

pavelkrolevets Oct 5, 2023

liorrutenberg commented Oct 10, 2023

	@echo "Run ssv-dkg to launch the tool."
	@echo "ssv-dkg installed successfully."

Chores grooming #11

Chores grooming #11

Conversation

pavelkrolevets commented Sep 14, 2023

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

pavelkrolevets Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

y0sher left a comment • edited Loading

Choose a reason for hiding this comment

liorrutenberg commented Sep 18, 2023

liorrutenberg commented Sep 18, 2023

Choose a reason for hiding this comment

y0sher Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liorrutenberg commented Sep 18, 2023

liorrutenberg commented Sep 18, 2023

liorrutenberg commented Sep 18, 2023

liorrutenberg left a comment

Choose a reason for hiding this comment

liorrutenberg commented Sep 18, 2023

liorrutenberg commented Sep 18, 2023 • edited Loading

liorrutenberg commented Sep 18, 2023

y0sher left a comment

Choose a reason for hiding this comment

y0sher left a 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

moshe-blox left a comment

Choose a reason for hiding this comment

moshe-blox Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshe-blox Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moshe-blox Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liorrutenberg commented Oct 10, 2023

pavelkrolevets Sep 15, 2023 •

edited

Loading

y0sher left a comment •

edited

Loading

y0sher Sep 19, 2023 •

edited

Loading

liorrutenberg commented Sep 18, 2023 •

edited

Loading

moshe-blox Oct 4, 2023 •

edited

Loading

moshe-blox Oct 4, 2023 •

edited

Loading

moshe-blox Oct 4, 2023 •

edited

Loading