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

dRNG integration test #408

Merged
merged 15 commits into from May 8, 2020
Merged

dRNG integration test #408

merged 15 commits into from May 8, 2020

Conversation

capossele
Copy link
Contributor

Description of change

This PR adds the integration test for the dRNG. It also refactors the creation of a peer docker container by adding a GoShimmerConfig struct.

@capossele capossele requested a review from jonastheis May 6, 2020 10:53
@capossele capossele marked this pull request as draft May 6, 2020 11:15
@capossele capossele marked this pull request as ready for review May 6, 2020 12:23
Copy link
Collaborator

@jonastheis jonastheis left a comment

Choose a reason for hiding this comment

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

Mostly minor stuff. Looks good overall :)

@@ -15,6 +15,9 @@ jobs:
- name: Build GoShimmer image
run: docker build -t iotaledger/goshimmer .

- name: Pull drand image
Copy link
Collaborator

Choose a reason for hiding this comment

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

The image should also be pulled in runTests.sh

@@ -69,25 +69,49 @@ func (d *DockerContainer) CreateGoShimmerEntryNode(name string, seed string) err
}

// CreateGoShimmerPeer creates a new container with the GoShimmer peer's configuration.
func (d *DockerContainer) CreateGoShimmerPeer(name string, seed string, entryNodeHost string, entryNodePublicKey string, bootstrap bool) error {
func (d *DockerContainer) CreateGoShimmerPeer(conf GoShimmerConfig) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename to config

distKey []byte
}

// newDRNGNetwork returns a Network instance, creates its underlying Docker network and adds the tester container to the network.
Copy link
Collaborator

Choose a reason for hiding this comment

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

DRNGNetwork instance

}
}

conf := GoShimmerConfig{
Copy link
Collaborator

Choose a reason for hiding this comment

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

config

func (n *DRNGNetwork) CreatePeer(c GoShimmerConfig, publicKey hive_ed25519.PublicKey) (*Peer, error) {
name := n.network.namePrefix(fmt.Sprintf("%s%d", containerNameReplica, len(n.network.peers)))

conf := c
Copy link
Collaborator

Choose a reason for hiding this comment

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

config


for i := dkgMaxTries; i > 0; i-- {

//var dkey *drand.DistKeyResponse
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the comment and newlines to make it more readable

}

// wait until containers are fully started
time.Sleep(60 * time.Second)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe say why these additional 60 seconds waiting are necessary? It also waits already in WaitForDKG so more explanation would be nice :)

if drngCommittee != "" {
drngCommittee += fmt.Sprintf(",")
}
drngCommittee += fmt.Sprintf("%s", base58.Encode(pubKeys[i][:]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

simplify to

Suggested change
drngCommittee += fmt.Sprintf("%s", base58.Encode(pubKeys[i][:]))
drngCommittee += pubKeys[i].String()


// wait until peers are fully started and connected
time.Sleep(1 * time.Second)
err = drng.network.WaitForAutopeering(3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why set minimumNeighbors here hardcoded? I think it should be a parameter to the function

for _, peer := range drng.Peers() {
resp, err := peer.GetRandomness()
require.NoError(t, err)
log.Println(resp)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use t.Log and use the String() function or extract the parameters you want/need to see to avoid a cryptic output like this:
2020/05/06 13:49:11 &{7 2020-05-06 13:49:11.030803713 +0000 UTC [191 58 172 186 178 90 43 238 162 122 248 194 214 41 24 132 189 41 200 84 8 42 129 210 168 182 144 176 17 14 80 185 176 171 79 130 188 245 43 195 168 221 210 24 213 131 127 110 79 115 190 192 233 139 136 152 214 66 25 89 85 145 56 24] }

@capossele capossele requested a review from jonastheis May 7, 2020 18:16
Copy link
Collaborator

@jonastheis jonastheis left a comment

Choose a reason for hiding this comment

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

lgtm!

@capossele capossele merged commit 2bda49c into develop May 8, 2020
@capossele capossele deleted the feat/drng-integration-test branch May 8, 2020 13:29
@capossele capossele restored the feat/drng-integration-test branch May 8, 2020 13:33
@capossele capossele deleted the feat/drng-integration-test branch May 8, 2020 13:35
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.

None yet

3 participants