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

dkg: add frost command test #482

Merged
merged 4 commits into from
May 5, 2022
Merged

dkg: add frost command test #482

merged 4 commits into from
May 5, 2022

Conversation

corverroos
Copy link
Contributor

Adds frost algo to the dkg command test.

  • Add public shares to cluster lock file.

category: test
ticket: #478
feature_set: alpha

@codecov
Copy link

codecov bot commented May 5, 2022

Codecov Report

Merging #482 (74413f9) into main (44ce408) will increase coverage by 2.78%.
The diff coverage is 70.68%.

@@            Coverage Diff             @@
##             main     #482      +/-   ##
==========================================
+ Coverage   53.67%   56.46%   +2.78%     
==========================================
  Files          81       81              
  Lines        7385     7424      +39     
==========================================
+ Hits         3964     4192     +228     
+ Misses       2868     2656     -212     
- Partials      553      576      +23     
Impacted Files Coverage Δ
cluster/distvalidator.go 88.23% <ø> (ø)
tbls/tblsconv/tblsconv.go 43.37% <0.00%> (-3.38%) ⬇️
dkg/keycast.go 65.03% <70.27%> (-0.69%) ⬇️
dkg/disk.go 40.54% <100.00%> (ø)
dkg/dkg.go 54.09% <100.00%> (+8.64%) ⬆️
dkg/frost.go 76.07% <100.00%> (+1.39%) ⬆️
tbls/tss.go 49.68% <100.00%> (ø)
app/app.go 64.20% <0.00%> (+0.88%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44ce408...74413f9. Read the comment docs.

Verifiers [][]byte `json:"threshold_verifiers"`
// PublicShares are the public keys of each node's secret key share.
// It can be used to verify a partial signature created by any node in the cluster.
PublicShares [][]byte `json:"public_shares,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can name this PubShares

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok cool

@@ -22,8 +22,13 @@ type DistValidator struct {
// PubKey is the root distributed public key.
PubKey string `json:"distributed_public_key"`

// Verifiers are the public shares.
Verifiers [][]byte `json:"threshold_verifiers"`
// PublicShares are the public keys of each node's secret key share.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// PublicShares are the public keys of each node's secret key share.
// PubShares are the public keys corresponding to each node's secret key share.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok cool


// Verifiers are the threshold verifier commitments.
// Deprecated: Use PublicShares.
Verifiers [][]byte `json:"threshold_verifiers,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

discussion: why are we keeping this around then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

still used, will refactor in another PR

shares = make([][]*bls_sig.SecretKey, vals)
locks []cluster.Lock
secretShares = make([][]*bls_sig.SecretKey, def.NumValidators)
locks []cluster.Lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
locks []cluster.Lock
locks []cluster.Lock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was gofmt

@@ -26,6 +26,16 @@ import (
"github.com/obolnetwork/charon/core"
)

// KeyFromBytes unmarshalls the bytes into a kryptology bls public key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// KeyFromBytes unmarshalls the bytes into a kryptology bls public key.
// KeyFromBytes unmarshals the bytes into a kryptology bls public key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool

})
}

func testDKG(t *testing.T, def cluster.Definition, p2pKeys []*ecdsa.PrivateKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we can also have another test which checks the negative cases, like what happens when we have a malformed or incomplete definition/lock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are welcome to add these in another PR

@xenowits xenowits self-requested a review May 5, 2022 10:19
@corverroos corverroos added the merge when ready Indicates bulldozer bot may merge when all checks pass label May 5, 2022
@obol-bulldozer obol-bulldozer bot merged commit c815250 into main May 5, 2022
@obol-bulldozer obol-bulldozer bot deleted the corver/frosttest branch May 5, 2022 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants