-
Notifications
You must be signed in to change notification settings - Fork 38
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
node: init blockchain state on startup #3069
Conversation
Fixes #3066 on cold starts. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3069 +/- ##
==========================================
- Coverage 22.36% 22.36% -0.01%
==========================================
Files 793 793
Lines 58698 58713 +15
==========================================
- Hits 13130 13129 -1
- Misses 44667 44683 +16
Partials 901 901 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
although proposed change is rly needed, it does not cover division by zero totally. Zero can still come from the service pov. I suggest:
- preserve zero return in implementations
- throw more clear panic in the service
we can also consider defaulting
@carpawell #3066 (comment) what did u mean to fix? |
Epoch duration can't be 0. But this can be an operator error. In which case panic is not the best way to handle the problem. I'd suggest relying on proper value everywhere, but check it on update (including node start) and using the default if it's wrong. Also, looks like |
|
@cthulhu-rider, i meant do not encode meta info if it is not requested. It does not relate to panic directly so i would like to put this fix to another PR (like #3063 with some title change). |
@roman-khimov, agree, will fix but IMO not in a fast panic fix PR. |
@cthulhu-rider, what do you mean? It shares the same netmap state that is updated right after creation after this PR. |
just a remark, i faced this when hotfixed original panic with default to get working tests. Anyway, these are different places which can potentially panic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node/nemtap:
and i still recommend to throw particular panic in places where the original one could occur
cmd/neofs-node/config.go
Outdated
@@ -743,6 +743,11 @@ func initBasics(c *cfg, key *keys.PrivateKey, stateStorage *state.PersistentStor | |||
|
|||
eDuration, err := nmWrap.EpochDuration() | |||
fatalOnErr(err) | |||
if eDuration == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's all in neofs-node
, likely you can have some nState.UpdateEpochDuration()
that will handle this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, degraded logs a little then
It's like adding dead code, not worth the trouble. This panic is rather obvious one even when the only thing you see is line number and "division by zero". |
a30919b
to
7028440
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont forget about linter
Fallback to 240 blocks default epoch duration if unexpected zero value received. Zero value is not acceptable, and it is hard to predict how the system reacts to it (panic was observed at least once). Refs #3066. Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
Same opinion, zero division is a funny cult mistake, in go IMO not needed any context, devs will always understand what it is about, admins can do nothing about it |
7028440
to
b0b1270
Compare
Fixes #3066 on cold starts.