Skip to content

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.

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?

tmc pushed a commit to tmc/go-algorand that referenced this pull request Mar 7, 2025
Always use the new (batch) verification system, having confirmed no incompatibilities with existing blockchain.
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