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

Integrate State Proof keys #2990

Merged
merged 345 commits into from
Feb 10, 2022
Merged

Conversation

id-ms
Copy link
Contributor

@id-ms id-ms commented Oct 4, 2021

Summary

This PR adds the support for the State Proof Keys.

Test Plan

Unit tests added.

@id-ms id-ms changed the title [WIP] State proof integration [WIP] dilithium-scheme-integration Oct 4, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2021

Codecov Report

Merging #2990 (617f69f) into master (c8c6fb7) will increase coverage by 0.31%.
The diff coverage is 52.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2990      +/-   ##
==========================================
+ Coverage   47.74%   48.06%   +0.31%     
==========================================
  Files         370      381      +11     
  Lines       60262    62061    +1799     
==========================================
+ Hits        28774    29829    +1055     
- Misses      28180    28815     +635     
- Partials     3308     3417     +109     
Impacted Files Coverage Δ
agreement/abstractions.go 50.00% <ø> (ø)
cmd/goal/account.go 12.88% <0.00%> (-0.24%) ⬇️
compactcert/db.go 73.68% <ø> (ø)
compactcert/worker.go 90.00% <ø> (ø)
crypto/digest.go 0.00% <0.00%> (ø)
crypto/merklearray/worker.go 100.00% <ø> (ø)
daemon/algod/api/server/v1/handlers/handlers.go 0.62% <0.00%> (ø)
daemon/algod/api/server/v2/account.go 77.55% <0.00%> (-0.80%) ⬇️
daemon/algod/api/server/v2/handlers.go 0.00% <0.00%> (ø)
data/accountManager.go 0.00% <0.00%> (ø)
... and 78 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 c8c6fb7...617f69f. Read the comment docs.

@@ -121,6 +130,11 @@ func (part Participation) GenerateRegistrationTransaction(fee basics.MicroAlgos,
SelectionPK: part.VRF.PK,
},
}
if cert := part.BlockProofSigner(); cert != nil {
if cparams.EnableBlockProofKeyregCheck {
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens when !cparams.EnableBlockProofKeyregCheck and cert != nil?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing I guess. The BlockProof key will not be registered in the transaction since it's not yet supported on this protocol version.

if err != nil {
return nil, err
}
}

// Confirm that we got the same root hash
if len(pl) != 1 {
return nil, fmt.Errorf("internal error: partial layer produced %d hashes", len(pl))
return nil, fmt.Errorf("internal error: partial Layer produced %d hashes", len(pl))
}

if validateProof {
Copy link
Contributor

Choose a reason for hiding this comment

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

validateProof is a constant set as false at line 135, when will this block if executed?

Copy link
Contributor

Choose a reason for hiding this comment

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

It will be executed when the feature is enabled. This should definitely change though, you are right.
Probably the a global configuration value needs to replace it, or perhaps some flag from the protocol params.
Will check on that.

@@ -28,7 +28,7 @@ import (
// instance (because we have no ctors...)
var DefaultGenesis = GenesisData{
FirstPartKeyRound: 0,
LastPartKeyRound: 3000000,
LastPartKeyRound: 3000,
Copy link
Contributor

Choose a reason for hiding this comment

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

reason for this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Temporary for testing reasons (3,000,000 fails in the CICD tests and it seems excessive).

@@ -48,26 +47,29 @@ func (v *Verifier) Verify(c *Cert) error {
return fmt.Errorf("cert signed weight %d <= proven weight %d", c.SignedWeight, v.ProvenWeight)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

is there a unit test for this method?

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be in the future (when the compactcert feature is ready to be released), but as for now it is tested by compactcert.TestWorkerAllSigs() and compactcert.TestWorkerPartialSigs().

@jasonpaulos jasonpaulos self-requested a review October 12, 2021 15:01
SecKQ: proto.CompactCertSecKQ,
Msg: hdr,
ProvenWeight: provenWeight,
SigRound: hdr.Round,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the sigRound param changed? Does it impact the existing implementation?

Copy link
Contributor

@Aharonee Aharonee Oct 27, 2021

Choose a reason for hiding this comment

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

Yes, in the old implementation the voting key was used to sign the compact cert (and the commitment was to the next round key , since the ephemeral voting key is disposed after use). Now we use completely different keys and scheme so this hack is no longer needed.

@algonautshant algonautshant force-pushed the feature/dilithium-scheme-integration branch from 04dd916 to c96eb57 Compare October 13, 2021 21:46
…orand/go-algorand into feature/dilithium-scheme-integration
@tsachiherman tsachiherman force-pushed the feature/dilithium-scheme-integration branch from 0d35440 to bd7fcc8 Compare February 9, 2022 19:22
tsachiherman and others added 10 commits February 9, 2022 15:00
…gression

## Summary

My previous PR was causing a minor regression. Thanks to @jannotti comment, I think that I was able to fix the regression, and added a proper testing for that.

## Test Plan

New benchmark result ( no inner method anymore )
```
goos: darwin
goarch: amd64
pkg: github.com/algorand/go-algorand/data/bookkeeping
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkTxnMerkleToRaw/copy-8         	1000000000	         1.129 ns/op	       0 B/op	       0 allocs/op
BenchmarkTxnMerkleToRaw/append-8       	243079730	         4.886 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/algorand/go-algorand/data/bookkeeping	3.183s
```
## Summary

Instead of performing a check in `SignerContext.GetVerifier`, I've added a unit test to verify that the output of the merklearray root has the same length as the Verifier object.

## Test Plan

Unit test.
@tsachiherman tsachiherman dismissed algojack’s stale review February 10, 2022 15:16

regression was rolled back.

tsachiherman
tsachiherman previously approved these changes Feb 10, 2022
@tsachiherman tsachiherman merged commit 1504f5f into master Feb 10, 2022
tsachiherman added a commit that referenced this pull request Feb 10, 2022
## Summary

Create an update path that would enable the following features:
1. Batch Verification (#3031)
2. State Proof Keys (#2990)
3. TEAL 6 (#3397)
4. Expired Participation Keys (#2924)
5. Fix rewards calculation bug (#3403)

## Test Plan

Unit tests added.
@algorandskiy algorandskiy deleted the feature/dilithium-scheme-integration branch November 3, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.