Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Promote dangerous cond. from just warning to panic#8439

Merged
ryoqun merged 1 commit intosolana-labs:masterfrom
ryoqun:double-remove-account-panic
Feb 25, 2020
Merged

Promote dangerous cond. from just warning to panic#8439
ryoqun merged 1 commit intosolana-labs:masterfrom
ryoqun:double-remove-account-panic

Conversation

@ryoqun
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun commented Feb 25, 2020

Problem

Critically bad condition is just a warning. I've been worried this for long time... Now, I'm pretty confident with the code base and the erroneous case should not happen and is really dangerous if happened ever. Dunno why this was just a warning...

Summary of Changes

  • Promote a warning to a panic (assertion) to make that abnormal condition more visible to us.

Background

Split off from #8337

@ryoqun ryoqun requested a review from sakridge February 25, 2020 15:13
if count > 0 {
*count_and_status = (count - 1, status);
} else {
warn!("count value 0 for slot {}", self.slot_id);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This actually happened locally in the way-before past, locally.

If this ever happens, it's very likely for this to cause the disastrous event of ledger inconsistency by continuing silently like this without being panic!-ed.

@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Feb 25, 2020

@sakridge What do you think about this PR? :)

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 25, 2020

Codecov Report

Merging #8439 into master will decrease coverage by <.1%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #8439     +/-   ##
========================================
- Coverage    80.3%   80.3%   -0.1%     
========================================
  Files         254     254             
  Lines       56221   56235     +14     
========================================
+ Hits        45158   45169     +11     
- Misses      11063   11066      +3

Copy link
Copy Markdown
Contributor

@sakridge sakridge left a comment

Choose a reason for hiding this comment

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

Yea, sounds like a good idea to me.

@ryoqun ryoqun merged commit 0b48c8e into solana-labs:master Feb 25, 2020
mergify Bot pushed a commit that referenced this pull request Feb 25, 2020
mergify Bot pushed a commit that referenced this pull request Feb 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants