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

ledger: do final validation in endOfBlock() #3132

Merged
merged 2 commits into from
Oct 29, 2021

Conversation

tolikzinovyev
Copy link
Contributor

Summary

  1. It doesn't make sense to calculate new totals in finalValidation()
  2. endOfBlock() already does some validation

Moving the content of finalValidation() to endOfBlock() makes things more reasonable.

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2021

Codecov Report

Merging #3132 (7a1957a) into master (17429d5) will decrease coverage by 0.00%.
The diff coverage is 30.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3132      +/-   ##
==========================================
- Coverage   43.70%   43.69%   -0.01%     
==========================================
  Files         390      390              
  Lines       86689    86681       -8     
==========================================
- Hits        37887    37879       -8     
+ Misses      42780    42777       -3     
- Partials     6022     6025       +3     
Impacted Files Coverage Δ
ledger/internal/evalindexer.go 0.00% <ø> (ø)
ledger/internal/eval.go 70.05% <30.76%> (+0.10%) ⬆️
ledger/blockqueue.go 82.18% <0.00%> (-2.88%) ⬇️
agreement/cryptoVerifier.go 75.73% <0.00%> (-2.21%) ⬇️
agreement/proposalManager.go 96.07% <0.00%> (-1.97%) ⬇️
ledger/acctupdates.go 64.25% <0.00%> (-0.60%) ⬇️
network/requestTracker.go 71.12% <0.00%> (+0.86%) ⬆️
network/wsPeer.go 72.00% <0.00%> (+2.13%) ⬆️

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 17429d5...7a1957a. Read the comment docs.

@tolikzinovyev tolikzinovyev marked this pull request as ready for review October 22, 2021 20:45
@tolikzinovyev tolikzinovyev self-assigned this Oct 22, 2021
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

This change looks pretty good. It does change the semantics of eval.finalValidation during Eval, as it would now be performing the signature validation in parallel to the eval.finalValidation. That being said - I can't find a reason this would cause an issue.

Separately, the eval.state.mods.OptimizeAllocatedMemory call was intended to be called once we have the state delta changes ready. At their new location, this is not the case, since the eval.resetExpiredOnlineAccountsParticipationKeys could modify their content.

To resolve that, just move the eval.state.mods.OptimizeAllocatedMemory right before the if eval.validate predicate.

@tolikzinovyev
Copy link
Contributor Author

done

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

looks good. thanks for the changes.

@tsachiherman tsachiherman merged commit 5eb09af into algorand:master Oct 29, 2021
cce pushed a commit to cce/go-algorand that referenced this pull request Nov 3, 2021
## Summary

1. It doesn't make sense to calculate new totals in `finalValidation()`
2. `endOfBlock()` already does some validation

Moving the content of `finalValidation()` to `endOfBlock()` makes things more reasonable.
@egieseke egieseke mentioned this pull request Nov 23, 2021
@tolikzinovyev tolikzinovyev deleted the end-of-block-validation branch January 6, 2022 21:37
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.

3 participants