ledger: fix `NextRewardsState()` #3403

tolikzinovyev · 2022-01-10T19:47:47Z

Summary

A modification of #3336. Added a new test where the rewards pool overspends and proposed a fix in NextRewardsState() requiring a consensus upgrade.

Test Plan

This is mostly tests.

tsachiherman · 2022-01-11T02:24:35Z

data/bookkeeping/block_test.go

+
+	params, ok := config.Consensus[protocol.ConsensusCurrentVersion]
+	require.True(t, ok)
+	params.RewardsCalculationFix = true


The test is "almost there". You'll need to confirm expected failure without the fix, and expected succeess with the fix.

It would also be a good idea to add expected failure after the fix, providing invalid input.

Done (1). Why test on invalid input?

testing it against invalid input ensures that the method does behave in the way we expect it to. I realize that there could be multitude of invalid inputs, and ideally, we need to analyze the codebase to ensure all the code-paths are taken and validated individually.

codecov-commenter · 2022-01-11T02:52:36Z

Codecov Report

Merging #3403 (a84a702) into master (2346615) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3403      +/-   ##
==========================================
- Coverage   47.65%   47.64%   -0.01%     
==========================================
  Files         370      370              
  Lines       59804    59810       +6     
==========================================
- Hits        28498    28495       -3     
- Misses      28001    28010       +9     
  Partials     3305     3305

Impacted Files	Coverage Δ
config/consensus.go	`84.64% <100.00%> (+0.05%)`	⬆️
data/bookkeeping/block.go	`54.74% <100.00%> (+4.55%)`	⬆️
ledger/internal/eval.go	`70.36% <100.00%> (ø)`
network/wsPeer.go	`65.83% <0.00%> (-2.78%)`	⬇️
crypto/merkletrie/trie.go	`66.42% <0.00%> (-2.19%)`	⬇️
crypto/merkletrie/node.go	`91.62% <0.00%> (-1.87%)`	⬇️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
catchup/service.go	`69.90% <0.00%> (-0.75%)`	⬇️
network/wsNetwork.go	`62.72% <0.00%> (-0.20%)`	⬇️
ledger/acctupdates.go	`65.77% <0.00%> (+0.95%)`	⬆️
... and 1 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 2346615...a84a702. Read the comment docs.

some styling suggestions.

tsachiherman · 2022-01-14T21:22:27Z

data/bookkeeping/block_test.go

 	require.Equal(t, uint64(2), state.RewardsLevel)
 	require.Equal(t, uint64(0), state.RewardsResidue)
+
+	assert.Zero(t, buf.Len())


in unit tests, you almost always want to use require and not assert. If the expression fails, there is no point in keep running the test. ( in fact, it could lead into some other test failures )

tsachiherman

For testing the expected empty log output, your change is fine.
However, you did not test the expected output in case of an error.

tolikzinovyev · 2022-01-14T21:36:55Z

For testing the expected empty log output, your change is fine. However, you did not test the expected output in case of an error.

The last test does plus some long running tests check that log output is non-empty. What other errors are you referring to?

tolikzinovyev · 2022-01-14T21:42:03Z

There are some other overflow checks in the code, but they really shouldn't happen...

tsachiherman · 2022-01-14T21:43:19Z

There are some other overflow checks in the code, but they really shouldn't happen...

Yes, I agree that it shouldn't happen under normal running conditions - but you're implementing a unit test; in this case, just explicitly check the output for the error string.

tolikzinovyev · 2022-01-15T00:41:04Z

There are some other overflow checks in the code, but they really shouldn't happen...

Yes, I agree that it shouldn't happen under normal running conditions - but you're implementing a unit test; in this case, just explicitly check the output for the error string.

Do you want me to cause those overflows to happen and check the logs? I'm not following.

tsachiherman · 2022-01-15T00:43:13Z

Yes, exactly. Pass artificial parameters that would reproduce the problem, and confirm the implementation.

…

On Fri, Jan 14, 2022, 19:41 Tolik Zinovyev ***@***.***> wrote: There are some other overflow checks in the code, but they really shouldn't happen... Yes, I agree that it shouldn't happen under normal running conditions - but you're implementing a unit test; in this case, just explicitly check the output for the error string. Do you want me to cause those overflows to happen and check the logs? I'm not following. — Reply to this email directly, view it on GitHub <#3403 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AF2OOHYZDG43MARDQ5HJJBTUWC7CXANCNFSM5LUP5VRQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because your review was requested.Message ID: ***@***.***>

* ledger: fix `NextRewardsState()` (#3403) ## Summary A modification of #3336. Added a new test where the rewards pool overspends and proposed a fix in `NextRewardsState()` requiring a consensus upgrade. ## Test Plan This is mostly tests. * Fix a potential problem of committing non-uniform consensus versions (#3453) If accountdb accumulates a large backlog, it is possible catchpoint tracker would attempt to commit a wider range than account updates tracker expects. * avoid generating log error on EnsureValidatedBlock / EnsureBlock (#3424) In EnsureBlock,, do not log as error message if the error is ledgercore.ErrNonSequentialBlockEval and the block round is in the past (i.e. already in the ledger). * Fix typo Fulll to Full (#3456) Fix typo * Fix worng message on restore crash db. (#3455) When crash state is found but could not be restored, noCrashState variable is used to report a warning. However, this variable was set to false in a case where there was no crash state, and the wrong warning was reported. * Adding new scenario for feature networks (#3451) Co-authored-by: Tolik Zinovyev <tolik@algorand.com> Co-authored-by: Pavel Zbitskiy <65323360+algorandskiy@users.noreply.github.com> Co-authored-by: Shant Karakashian <55754073+algonautshant@users.noreply.github.com>

* ledger: fix `NextRewardsState()` (#3403) ## Summary A modification of #3336. Added a new test where the rewards pool overspends and proposed a fix in `NextRewardsState()` requiring a consensus upgrade. ## Test Plan This is mostly tests. * Fix a potential problem of committing non-uniform consensus versions (#3453) If accountdb accumulates a large backlog, it is possible catchpoint tracker would attempt to commit a wider range than account updates tracker expects. * avoid generating log error on EnsureValidatedBlock / EnsureBlock (#3424) In EnsureBlock,, do not log as error message if the error is ledgercore.ErrNonSequentialBlockEval and the block round is in the past (i.e. already in the ledger). * Fix typo Fulll to Full (#3456) Fix typo * Fix worng message on restore crash db. (#3455) When crash state is found but could not be restored, noCrashState variable is used to report a warning. However, this variable was set to false in a case where there was no crash state, and the wrong warning was reported. * Adding new scenario for feature networks (#3451) * Fixing telemetry ports (#3497) Co-authored-by: Tolik Zinovyev <tolik@algorand.com> Co-authored-by: Pavel Zbitskiy <65323360+algorandskiy@users.noreply.github.com> Co-authored-by: Shant Karakashian <55754073+algonautshant@users.noreply.github.com> Co-authored-by: Jack <87339414+algojack@users.noreply.github.com>

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

tolikzinovyev added the Team Carbon-11 label Jan 10, 2022

tolikzinovyev requested a review from tsachiherman January 10, 2022 19:47

tolikzinovyev self-assigned this Jan 10, 2022

tolikzinovyev requested a review from cce January 10, 2022 21:47

tolikzinovyev changed the title ~~NextRewardsState() tests~~ Fix NextRewardsState() Jan 10, 2022

AlgoStephenAkiki and others added 6 commits January 10, 2022 21:14

Added unit tests

b3d5c99

Formatting

a546af2

Pr updates

bd724e2

changes

a25f882

Add a consensus upgrade.

a83cf07

fixes

805119b

tolikzinovyev force-pushed the rewards branch from 35e42c4 to 805119b Compare January 11, 2022 02:16

tsachiherman reviewed Jan 11, 2022

View reviewed changes

tsachiherman marked this pull request as ready for review January 11, 2022 02:28

Add a test without the fix.

a3b0740

tsachiherman changed the title ~~Fix NextRewardsState()~~ ledger: fix NextRewardsState() Jan 11, 2022

tsachiherman and others added 4 commits January 12, 2022 08:58

some styling suggestions.

d6d6cd7

Add log to the method arguments.

540f7b1

Merge pull request #1 from tsachiherman/tsachi/rewards-styles

74ffc6e

some styling suggestions.

Improve tests.

6b48bff

tolikzinovyev requested a review from tsachiherman January 14, 2022 20:50

tsachiherman reviewed Jan 14, 2022

View reviewed changes

tsachiherman suggested changes Jan 14, 2022

View reviewed changes

More overflow tests.

a84a702

Add missing partition test statements.

e409d7f

tolikzinovyev requested a review from tsachiherman January 19, 2022 20:57

tsachiherman approved these changes Jan 19, 2022

View reviewed changes

tsachiherman merged commit 2edd3de into algorand:master Jan 19, 2022

tsachiherman mentioned this pull request Feb 10, 2022

V31 consensus upgrade #3553

Merged

onetechnical mentioned this pull request Feb 11, 2022

go-algorand 3.4.0-beta #3613

Merged

egieseke mentioned this pull request Feb 16, 2022

Script to generate go-algorand release notes #3520

Closed

onetechnical mentioned this pull request Mar 1, 2022

go-algorand 3.4.2-stable #3700

Merged

tolikzinovyev deleted the rewards branch April 27, 2022 15:22

ledger: fix NextRewardsState() #3403

ledger: fix NextRewardsState() #3403

Uh oh!

Conversation

tolikzinovyev commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

tsachiherman Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

tsachiherman Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

tolikzinovyev Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

tsachiherman Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tsachiherman Jan 14, 2022

Choose a reason for hiding this comment

Uh oh!

tsachiherman left a comment

Choose a reason for hiding this comment

Uh oh!

tolikzinovyev commented Jan 14, 2022

Uh oh!

tolikzinovyev commented Jan 14, 2022

Uh oh!

tsachiherman commented Jan 14, 2022

Uh oh!

tolikzinovyev commented Jan 15, 2022

Uh oh!

tsachiherman commented Jan 15, 2022 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ledger: fix `NextRewardsState()` #3403

ledger: fix `NextRewardsState()` #3403

tolikzinovyev commented Jan 10, 2022 •

edited

Loading

codecov-commenter commented Jan 11, 2022 •

edited

Loading