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

add version to coin-generator #4007

Merged

Conversation

id-ms
Copy link
Contributor

@id-ms id-ms commented May 18, 2022

Summary

In order to support changes in the stateproof verifier, we need to add a version number to the coinChoiceSeed structure so that Fiat-Shamir could be used safely in the future.

Test Plan

@id-ms id-ms requested a review from algonautshant May 18, 2022 14:06
Copy link
Contributor

@algonautshant algonautshant left a comment

Choose a reason for hiding this comment

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

Looks Great!

@@ -48,7 +48,8 @@ func (cc *coinChoiceSeed) ToBeHashed() (protocol.HashID, []byte) {
lnProvenWtAsBytes := make([]byte, 8)
binary.LittleEndian.PutUint64(lnProvenWtAsBytes, cc.lnProvenWeight)

coinChoiceBytes := make([]byte, 0, len(cc.partCommitment)+len(lnProvenWtAsBytes)+len(cc.sigCommitment)+len(signedWtAsBytes)+len(cc.data))
coinChoiceBytes := make([]byte, 0, 1+len(cc.partCommitment)+len(lnProvenWtAsBytes)+len(cc.sigCommitment)+len(signedWtAsBytes)+len(cc.data))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use len(cc.version) instead of 1+?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately , len(cc.version) does not compile

@algonautshant
Copy link
Contributor

Can you please add a comment in the PR description?

@codecov
Copy link

codecov bot commented May 18, 2022

Codecov Report

Merging #4007 (17dc000) into feature/stateproofs (1df452a) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@                   Coverage Diff                   @@
##           feature/stateproofs    #4007      +/-   ##
=======================================================
- Coverage                49.93%   49.91%   -0.03%     
=======================================================
  Files                      412      412              
  Lines                    69231    69233       +2     
=======================================================
- Hits                     34572    34558      -14     
- Misses                   30932    30940       +8     
- Partials                  3727     3735       +8     
Impacted Files Coverage Δ
crypto/stateproof/coinGenerator.go 100.00% <100.00%> (ø)
cmd/algoh/blockWatcher.go 77.77% <0.00%> (-3.18%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
network/wsPeer.go 68.88% <0.00%> (-1.95%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
ledger/acctupdates.go 68.77% <0.00%> (-0.66%) ⬇️
catchup/service.go 69.38% <0.00%> (ø)
agreement/proposalManager.go 98.03% <0.00%> (+1.96%) ⬆️
agreement/cryptoVerifier.go 69.71% <0.00%> (+2.11%) ⬆️

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 1df452a...17dc000. Read the comment docs.

@id-ms id-ms merged commit bc6dd87 into algorand:feature/stateproofs May 18, 2022
@id-ms id-ms deleted the add-version-to-coingenerator branch May 18, 2022 16:18
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.

2 participants