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

avoid generating log error on EnsureValidatedBlock / EnsureBlock #3424

Merged

Conversation

algonautshant
Copy link
Contributor

@algonautshant algonautshant commented Jan 16, 2022

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

Added a test to verify the behavior.

Resolves https://github.com/algorand/go-algorand-internal/issues/1831

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.

Thinking about this over the weekend, I think it would be better to "convert" the ledgercore.ErrNonSequentialBlockEval to ledgercore.BlockInLedgerError in ledger.AddBlock when err != nil and EvaluatorRound <= errNSBE.LatestRound.

The handling for both cases is the same and it would simplify the client code.

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.

in addition to the above, please also make sure to use distinguishable log messages, so that we could trace these back if/when needed.

@codecov-commenter
Copy link

codecov-commenter commented Jan 19, 2022

Codecov Report

Merging #3424 (a87d991) into master (2346615) will increase coverage by 0.02%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3424      +/-   ##
==========================================
+ Coverage   47.65%   47.67%   +0.02%     
==========================================
  Files         370      370              
  Lines       59804    59808       +4     
==========================================
+ Hits        28498    28516      +18     
+ Misses      28001    27986      -15     
- Partials     3305     3306       +1     
Impacted Files Coverage Δ
ledger/ledger.go 63.63% <20.00%> (-0.91%) ⬇️
data/ledger.go 37.95% <71.42%> (+13.86%) ⬆️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
data/abi/abi_type.go 87.67% <0.00%> (-0.95%) ⬇️
catchup/service.go 69.90% <0.00%> (-0.75%) ⬇️
network/wsPeer.go 68.05% <0.00%> (-0.56%) ⬇️
network/requestTracker.go 71.12% <0.00%> (+0.86%) ⬆️
ledger/acctupdates.go 65.96% <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 2346615...a87d991. Read the comment docs.

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.

@tsachiherman tsachiherman merged commit 9d49473 into algorand:master Jan 20, 2022
jannotti pushed a commit that referenced this pull request Jan 24, 2022
* 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>
jannotti pushed a commit that referenced this pull request Jan 25, 2022
* 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>
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.

None yet

4 participants