Added Participation Key Expiration Check #2924

AlgoStephenAkiki · 2021-09-20T15:40:51Z

Summary

Resolves #2738

Adds the following:

consensus.go: Adds the default MaxExpiredAccountsToProcess value for
the new consensus protocol
block.go: Adds necessary block header entries
eval.go: Scans for expired accounts, modifies them to be offline and adds
validation for this use case
eval_test.go: Basic unit tests
participationExpiration_test.go: Added e2e tests that verify that
different consensus protocols behave differently

Test Plan

Unit test and e2e tests added

tsachiherman · 2021-09-20T16:17:16Z

Please update the PR's Summary and Test Plan.

data/bookkeeping/block.go

ledger/eval.go

codecov-commenter · 2021-09-20T16:26:11Z

Codecov Report

Merging #2924 (6eef8b5) into master (f17f73e) will decrease coverage by 0.07%.
The diff coverage is 49.84%.

@@            Coverage Diff             @@
##           master    #2924      +/-   ##
==========================================
- Coverage   47.63%   47.55%   -0.08%     
==========================================
  Files         359      359              
  Lines       57964    58406     +442     
==========================================
+ Hits        27611    27775     +164     
- Misses      27231    27458     +227     
- Partials     3122     3173      +51

Impacted Files	Coverage Δ
data/basics/userBalance.go	`19.54% <0.00%> (-1.45%)`	⬇️
data/bookkeeping/block.go	`50.18% <ø> (ø)`
data/bookkeeping/msgp_gen.go	`42.55% <45.63%> (-1.60%)`	⬇️
agreement/msgp_gen.go	`41.23% <49.85%> (-0.62%)`	⬇️
ledger/eval.go	`78.98% <89.65%> (+1.17%)`	⬆️
config/consensus.go	`84.64% <100.00%> (+0.10%)`	⬆️
agreement/cryptoVerifier.go	`75.73% <0.00%> (-2.21%)`	⬇️
agreement/proposalManager.go	`96.07% <0.00%> (-1.97%)`	⬇️
network/requestTracker.go	`70.25% <0.00%> (-0.87%)`	⬇️
network/wsPeer.go	`75.20% <0.00%> (ø)`
... and 4 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 f17f73e...6eef8b5. Read the comment docs.

ledger/eval.go

tsachiherman

Overall looks good - I've had few requests, some are just code-style, some are more around compatibility with other PRs.

ledger/eval.go

tsachiherman

given that you've modified the encoding of the block header, you'll need to regenerate the msgp in order to make it persistent; use make msgp to regenerate all the auto-gen msgp encoders/decoders.

ledger/eval.go

ledger/eval_test.go

tsachiherman

The test you've added at TestExpiredAccountGeneration is good - but we would need more extensive unit testing. In practice, you change would affect the GenerateBlock(), eval() and EvalForIndexer. For each one of these, we need to make sure that the code changes produce their expected output;
as a generic guideline, each of the if-statements that you've included in your PR need to be tested for both success and failure.

ledger/eval_test.go

Resolves #2738 Adds the following: - consensus.go: Adds the default MaxExpiredAccountsToProcess value for the new consensus protocol - block.go: Adds necessary block header entries - eval.go: Scans for expired accounts, modifies them to be offline and adds validation for this use case - eval_test.go: Basic unit tests - participationExpiration_test.go: Added e2e tests that verify that different consensus protocols behave differently

The evaluator shouldn't ask the ledger who is the creator of an asset multiple times. This PR adds a cache for ledger's responses in cow base.

## Summary Resolves algorand#2738 Adds the following: - consensus.go: Adds the default MaxExpiredAccountsToProcess value for the new consensus protocol - block.go: Adds necessary block header entries - eval.go: Scans for expired accounts, modifies them to be offline and adds validation for this use case - eval_test.go: Basic unit tests - participationExpiration_test.go: Added e2e tests that verify that different consensus protocols behave differently ## Test Plan Unit test and e2e tests added

## 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.

## Summary Clear out AccountData.StateProofID when ClearOnlineState() is called, by resetExpiredOnlineAccountsParticipationKeys (as part of #2924) ## Test Plan Updated TestExpiredAccountGeneration to generate random online account data, so that it fails without this change.

AlgoStephenAkiki added the Team Carbon-11 label Sep 20, 2021

AlgoStephenAkiki self-assigned this Sep 20, 2021

tsachiherman reviewed Sep 20, 2021

View reviewed changes

data/bookkeeping/block.go Outdated Show resolved Hide resolved

tsachiherman reviewed Sep 20, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman reviewed Sep 20, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman reviewed Sep 20, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman reviewed Sep 20, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman reviewed Sep 20, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman reviewed Sep 20, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman reviewed Sep 20, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman reviewed Sep 20, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman suggested changes Sep 20, 2021

View reviewed changes

tsachiherman reviewed Sep 20, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman reviewed Sep 20, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman suggested changes Sep 20, 2021

View reviewed changes

tsachiherman mentioned this pull request Sep 20, 2021

ledger: move account totals calculation to StateDelta #2922

Merged

AlgoStephenAkiki requested a review from brianolson September 21, 2021 20:37

AlgoStephenAkiki marked this pull request as ready for review September 21, 2021 20:49

AlgoStephenAkiki requested a review from tsachiherman September 21, 2021 23:20

tsachiherman reviewed Sep 22, 2021

View reviewed changes

ledger/eval.go Outdated Show resolved Hide resolved

tsachiherman reviewed Sep 22, 2021

View reviewed changes

ledger/eval_test.go Outdated Show resolved Hide resolved

tsachiherman suggested changes Sep 22, 2021

View reviewed changes

tsachiherman reviewed Sep 30, 2021

View reviewed changes

ledger/eval_test.go Outdated Show resolved Hide resolved

AlgoStephenAkiki added 5 commits October 4, 2021 09:59

Pre-fix some nits

121ebd9

More nits

db3069a

Lint check

9078c2b

Addressed PR comments

56fcf00

AlgoStephenAkiki and others added 12 commits October 4, 2021 09:59

Updated EvalForIndexer function and re-created msgp

5c20fad

Fixed failing test

61c54fc

Formatting

4aec10a

Reordered modifyOfflineAccounts and CalculateTotals

9bae84a

Cache creators in cow base. (#2939)

3d10929

The evaluator shouldn't ask the ledger who is the creator of an asset multiple times. This PR adds a cache for ledger's responses in cow base.

Removed archival requirement

0c57c7f

Removed redundant test

2904e26

Added newline

3978220

Added Test Eval Function for error cases

262d8d5

Added EvalForIndexer coverage

59c2bbe

Moved final validation into end of block

d3d1df6

Added remaining edge cases

a833a2f

AlgoStephenAkiki requested a review from tsachiherman October 4, 2021 20:09

few final touches.

6eef8b5

tsachiherman approved these changes Oct 7, 2021

View reviewed changes

tsachiherman merged commit c59326f into algorand:master Oct 7, 2021

egieseke mentioned this pull request Nov 23, 2021

go-algorand 3.2.0-beta #3231

Merged

algobarb mentioned this pull request Dec 7, 2021

go-algorand 3.2.1-stable #3279

Merged

tsachiherman mentioned this pull request Feb 10, 2022

V31 consensus upgrade #3553

Merged

cce mentioned this pull request Feb 16, 2022

ledger: add StateProofID to AccountData.ClearOnlineState() #3643

Merged

jasonpaulos mentioned this pull request Jun 21, 2022

The block endpoint does not include 'partupdrmv' for expired participation accounts algorand/indexer#1045

Closed

Added Participation Key Expiration Check #2924

Added Participation Key Expiration Check #2924

Uh oh!

Conversation

AlgoStephenAkiki commented Sep 20, 2021 • edited by tsachiherman Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

tsachiherman commented Sep 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tsachiherman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tsachiherman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tsachiherman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AlgoStephenAkiki commented Sep 20, 2021 •

edited by tsachiherman

Loading

codecov-commenter commented Sep 20, 2021 •

edited

Loading