Consensus rules cannot be broken in finishOperation #563
Conversation
Codecov Report
@@ Coverage Diff @@
## master #563 +/- ##
==========================================
+ Coverage 61.01% 64.85% +3.84%
==========================================
Files 159 114 -45
Lines 11239 6911 -4328
==========================================
- Hits 6857 4482 -2375
+ Misses 3575 1899 -1676
+ Partials 807 530 -277
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
At this time, when error occurred in finish~ process the panic will be reasonable. @Charleslee522 If possible, when error or problem found in finish~ process, how about go to Syncing?
lib/node/runner/checker.go
Outdated
| if err = baTarget.Save(st); err != nil { | ||
| return | ||
| if err := baTarget.Save(st); err != nil { | ||
| return err |
There was a problem hiding this comment.
Like finishPayment, this also just can make panic, is there any reason to return error?
There was a problem hiding this comment.
Since it writes to LevelDB, there are much more potential for failure, that's why it's treated differently.
There was a problem hiding this comment.
I agreed.
finishXXX should not return an error.
How about using MustXXX pattern?
|
@spikeekips : Those are different kind of errors. At the moment, this I'm usually fine with recovering from programming errors, but with our development process and the amount of work being done on Sebak, it's not realistic to do it at the moment. |
|
@Geod24 Hmm. Most error cases can be from SEBAK itself, but there are plenty cases, error occurr like,
Db fail -> process end, it seems unnatural. In most if failed cases, panic can be good, but disk full sebak should ensure, I think. Logging critic message will be enough. |
|
I never ever use panic unless it's really unrecoverable error that the layer above should not be handling. It's an "abort the entire program" kind of error which you don't encounter that often. And A panic cannot be recovered by a different goroutine. My general rule: avoid as much as possible. |
Yeah, that's why I only panic on read, not on write. |
The consensus rules are checked in checker_ballot_transaction's `ValidateOp`. So if this error happens, it means there's an important bug in sebak, and that the consensus rules were bypassed. As a result, we remove the checks which are not necessary, and panic on operations that shouldn't return an error.
The consensus rules are checked in checker_ballot_transaction's
ValidateOp.So if this error happens, it means there's an important bug in sebak,
and that the consensus rules were bypassed.
As a result, we remove the checks which are not necessary,
and panic on operations that shouldn't return an error.