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

Simplify startEvaluator() #2812

Merged
merged 2 commits into from
Aug 30, 2021

Conversation

tolikzinovyev
Copy link
Contributor

Summary

The current code creates an impression that the evaluator can be used for round 0 which is not true. This PR simplifies the code, so this is more evident.

Test Plan

Relying on existing tests.

@tolikzinovyev tolikzinovyev self-assigned this Aug 26, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2021

Codecov Report

Merging #2812 (5fd5db7) into master (2b03a60) will increase coverage by 0.00%.
The diff coverage is 86.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2812   +/-   ##
=======================================
  Coverage   47.11%   47.11%           
=======================================
  Files         349      349           
  Lines       56424    56424           
=======================================
+ Hits        26582    26583    +1     
- Misses      26865    26868    +3     
+ Partials     2977     2973    -4     
Impacted Files Coverage Δ
ledger/eval.go 76.12% <86.66%> (ø)
network/requestTracker.go 70.25% <0.00%> (-0.87%) ⬇️
catchup/service.go 68.57% <0.00%> (-0.78%) ⬇️
network/wsPeer.go 74.37% <0.00%> (-0.28%) ⬇️
data/transactions/verify/txn.go 45.08% <0.00%> (ø)
ledger/acctupdates.go 62.55% <0.00%> (+0.08%) ⬆️
network/wsNetwork.go 61.09% <0.00%> (+0.18%) ⬆️
catchup/peerSelector.go 100.00% <0.00%> (+1.04%) ⬆️
ledger/blockqueue.go 85.05% <0.00%> (+1.14%) ⬆️

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 2b03a60...5fd5db7. Read the comment docs.

@tolikzinovyev tolikzinovyev marked this pull request as ready for review August 27, 2021 15:05
ledger/eval.go Outdated
@@ -383,20 +383,32 @@ func (l *Ledger) StartEvaluator(hdr bookkeeping.BlockHeader, paysetHint int) (*B
}

func startEvaluator(l ledgerForEvaluator, hdr bookkeeping.BlockHeader, proto config.ConsensusParams, paysetHint int, validate bool, generate bool) (*BlockEvaluator, error) {
if hdr.Round == 0 {
return nil, fmt.Errorf("cannot start evaluator for round 0")
Copy link
Contributor

Choose a reason for hiding this comment

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

create global constant error using errors.New(), and use it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

ledger/eval.go Outdated
Comment on lines 429 to 432
prevProto, ok := config.Consensus[prevHeader.CurrentProtocol]
if !ok {
return nil, protocol.Error(prevHeader.CurrentProtocol)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

move this section right after

prevHeader, err := l.BlockHdr(hdr.Round - 1)
 	if err != nil {
 		return nil, fmt.Errorf(
 			"can't evaluate block %v without previous header: %v", hdr.Round, err)
 	}

for further clarity. also, fix %v -> %d for the round number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

few small requests, but looks good otherwise.

@algorandskiy
Copy link
Contributor

I personally do not see any value in this change.

Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Are we sure this function is never called on round 0? It appears that previously it would be a NoOp (or a crash?), now it's an error.

@tolikzinovyev
Copy link
Contributor Author

I tried using it for Indexer for round 0, I got an error. It's probably in

	poolAddr := eval.prevHeader.RewardsPool
	// get the reward pool account data without any rewards
	incentivePoolData, _, err := l.LookupWithoutRewards(eval.prevHeader.Round, poolAddr)
	if err != nil {
		return nil, err
	}

eval.prevHeader is unset, so we are trying to get a balance for zero address.

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.

look good. thanks for the changes.

@tsachiherman tsachiherman merged commit 390edd1 into algorand:master Aug 30, 2021
@tolikzinovyev tolikzinovyev deleted the simplify-start-evaluator branch January 6, 2022 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants