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

crypto: remove batch verification flag #3781

Merged
merged 8 commits into from
Jun 21, 2022

Conversation

id-ms
Copy link
Contributor

@id-ms id-ms commented Mar 16, 2022

Summary

On V32 we introduced the batch verification for ed25519 signatures. In order to sync all nodes on the new algorithm, we added a new consensus param field.
Since the network accepted the V32 version, we can use the new algorithm on historical signatures.

Test Plan

Run full catchup test on Testnet, betanet and main net while configuring the node the verify signatures for old blocks.

@id-ms id-ms force-pushed the remove-batchverification-flag branch from 1dd6801 to ac8eb28 Compare March 22, 2022 15:40
@id-ms id-ms force-pushed the remove-batchverification-flag branch from ac8eb28 to 1a11785 Compare March 22, 2022 15:41
@id-ms id-ms marked this pull request as ready for review May 3, 2022 12:42
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2022

Codecov Report

Merging #3781 (00abc08) into master (5925aff) will increase coverage by 4.68%.
The diff coverage is 88.46%.

@@            Coverage Diff             @@
##           master    #3781      +/-   ##
==========================================
+ Coverage   49.80%   54.49%   +4.68%     
==========================================
  Files         409      390      -19     
  Lines       68929    48411   -20518     
==========================================
- Hits        34332    26382    -7950     
+ Misses      30891    19803   -11088     
+ Partials     3706     2226    -1480     
Impacted Files Coverage Δ
config/consensus.go 85.62% <ø> (-0.05%) ⬇️
node/netprio.go 0.00% <0.00%> (ø)
data/transactions/verify/txn.go 44.88% <50.00%> (+0.73%) ⬆️
agreement/vote.go 81.81% <100.00%> (ø)
crypto/batchverifier.go 100.00% <100.00%> (+9.37%) ⬆️
crypto/curve25519.go 61.81% <100.00%> (+0.80%) ⬆️
crypto/multisig.go 45.34% <100.00%> (ø)
crypto/onetimesig.go 75.86% <100.00%> (+4.08%) ⬆️
data/transactions/logic/eval.go 89.55% <100.00%> (ø)
network/wsPeer.go 65.83% <0.00%> (-2.23%) ⬇️
... and 25 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 5925aff...00abc08. Read the comment docs.

@gmalouf gmalouf requested review from algorandskiy and jannotti and removed request for tsachiherman May 31, 2022 18:07
jannotti
jannotti previously approved these changes Jun 1, 2022
Copy link
Contributor

@jannotti jannotti left a comment

Choose a reason for hiding this comment

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

I'm assuming that someone else (maybe @cce ?) has reviewed the work that went into making sure this PR is safe (where we looked at all past verifies). But judging this just for removing the enable flag, this looks good.

crypto/batchverifier.go Outdated Show resolved Hide resolved
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

Given the node was caught up from block 1 to the latest under extended verification settings, looks good to go.

Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

LGTM, I guess I should close #3783 since it is asserting both cases of EnableBatchVerification when true and false?

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.

5 participants