Return errors instead of panic #3782

MarinX · 2019-03-01T21:45:16Z

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

This is related to issue #3741 where fetching data from invalid store, package panic.
This PR modifies subspace.go to return errors instead of panic.
It also updates other packages that import subspace and handles error.

Modify tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

alexanderbez

I think these changes are fine, but we should keep consistency. What are your thoughts on updating the remaining begin/end blockers?

x/mint/abci_app.go

codecov · 2019-03-01T21:55:23Z

Codecov Report

Merging #3782 into develop will decrease coverage by 0.22%.
The diff coverage is 25%.

@@             Coverage Diff             @@
##           develop    #3782      +/-   ##
===========================================
- Coverage    60.02%   59.79%   -0.23%     
===========================================
  Files          212      212              
  Lines        15107    15158      +51     
===========================================
- Hits          9068     9064       -4     
- Misses        5418     5450      +32     
- Partials       621      644      +23

MarinX · 2019-03-02T12:50:25Z

@alexanderbez should be all good now.
Since I did not want to go deeper into packages and change all panics to errors, I suggest maybe to panic only on commands and main (if we want to recover from it).
Packages should not panic.

I see there is a thread #870 so I will put my input there to discuss if this something to consider.

- Allow gaiacli to access host's raw-usb slot. - Promote the package to stable. Closes: #3714

cwgoes · 2019-03-15T14:39:15Z

Needs a rebase for #3828.

jackzampolin · 2019-03-16T16:44:24Z

A failing lint here as well as some conflicts in PENDING.md. Looks great otherwise tho @MarinX !

MarinX · 2019-03-16T20:56:23Z

I fixed the merge problem.
The problem with lint is that if we return(handle) the error, we need to fix interfaces which again follows handling the errors in core packages and single value returns.

Its not a problem for me to do it, but since it will be a significant change, I need your input before tackling this.

alexanderbez · 2019-03-16T23:18:14Z

Which interfaces need adjustment @MarinX ?

MarinX · 2019-03-16T23:58:34Z

For example, fixing the lint issue for the func:

func (keeper Keeper) GetTallyParams(ctx sdk.Context) TallyParams

will become

func (keeper Keeper) GetTallyParams(ctx sdk.Context) (TallyParams, error)

which then needs adjustment to interfaces which will yield an error in build for other packages by using single value context.
We have around 20 functions that needs to return error, in which I suspect more packages will trigger an error.
So its a big rewrite of modifying interfaces, catching the errors and returning it to main caller.

EDIT:
So the question is, do we fix those lints in this PR ( which will require rewrites) or do we create a new PR where we continue handling the error?

fedekunze · 2019-03-19T15:36:47Z

cmd/gaia/app/app.go

+	validatorUpdates, endBlockerTags, err := staking.EndBlocker(ctx, app.stakingKeeper)
+	if err != nil {
+		return abci.ResponseEndBlock{}, err
+	}
 	tags = append(tags, endBlockerTags...)

 	app.assertRuntimeInvariants()

 	return abci.ResponseEndBlock{


is there a NewResponseEndBlock function on the abci pkg we could use to replace this everywhere?

fedekunze · 2019-03-19T15:38:29Z

So the question is, do we fix those lints in this PR ( which will require rewrites) or do we create a new PR where we continue handling the error?

this PR is big enough now, let's continue on a separate PR

MarinX · 2019-03-20T14:47:01Z

Exactly,
This PR is just a fix for returning the error instead of panic.
Handling the error should be another PR which will be a huge rewrite (hopefully lints would be also fixed)

alexanderbez · 2019-03-20T15:52:08Z

So what is the consensus on this @MarinX? Seems like lint is failing. What is the bare minimum we need to change to fix the root issue?

MarinX · 2019-03-27T15:53:33Z

rebased

MarinX · 2019-03-27T15:56:22Z

@alexanderbez solution I found is to check the error and add TODO when we are going to rewrite the handling of errors. This new commit suppresses the lint errors.

rigelrozanski

This is bit crazy with 8K line additions... I'm assuming key commits haven't been merged into this branch, obviously need to merge recent develop before I'm able to review here

.circleci/config.yml

alexanderbez

Needs a pending log entry, but otherwise LGTM. Thanks @MarinX.

alessio

I have to block this due to the lack of a changelog entry file; otherwise this looks good to me. May I suggest to run sdkch add breaking sdk @MarinX?

…evelop

MarinX · 2019-04-03T09:09:32Z

I have to block this due to the lack of a changelog entry file; otherwise this looks good to me. May I suggest to run sdkch add breaking sdk @MarinX?

Added.
Also, merged with latest changes

fedekunze

LGTM

alessio

Approving. Hence merging (@rigelrozanski dismissing your review as the author did ultimately merge latest develop)

The author merged develop latest changes

jackzampolin · 2019-04-04T17:39:17Z

Thank you very much for the contribution @MarinX!!!

This reverts commit 985aae5.

rigelrozanski

So this PR was just reverted and I wanted to take some time to briefly reflect on design decisions which this PR breaks. Again, I do want to apologize for any wasted energy, I am grateful for new excited developers to be contributing to this code base. If after reading this comment, you still believe there are places where the panic should be converted to an error, I suggest a new similar PR (however, substantially smaller) be opened which I will review.

Design notes:
Overall, within the SDK we value safety over liveness.
When an a value is expected in state and cannot be properly retrieved, the state of the blockchain is broken and should panic immediately at that location. Notably the k.paramSpace.Get(...) functions should in fact not be returning an error but panicking in place. If within the codebase it is expected behaviour that the parameter may not exist in the substore, the k.paramSpace.GetIfExists(...) function should be used (this function will not panic if the value does not exist).

Within this PR there are also a number of highly unsafe TODO's introduced whereby old panics are left as unchecked errors - these changes effectively allow for unsafe state to be introduced into the blockchain without halting it! This means that by forgetting to add an important parameter into the genesis state would not be caught when the blockchain started and would continue to operate potentially with a vital parameter simply set to zero.

Within unmarshal functionality (which I there were a few changes), if an improper unmarshal is made it means there is improper state stored within the blockchain which of course means that the blockchain state is in error and should halt. There are of course certain situations when retrieving input from the user that possible faulty input is plausible and should not panic, but this should be all limited to the client/rest receivers (and not unmarshalling state already stored in the blockchain)

Lastly I wanted to mention an error return arguments were introduced into endblocker functionality of the modules needlessly the state still panicked instead of returning an error - which is fine however because it never should have been returning an error - but I'm assuming this was not the intention of the PR.

MarinX requested review from cwgoes, ebuchman and rigelrozanski as code owners March 1, 2019 21:45

alexanderbez requested review from jackzampolin and mossid March 1, 2019 21:51

alexanderbez reviewed Mar 1, 2019

View reviewed changes

x/mint/abci_app.go Show resolved Hide resolved

MarinX changed the title ~~Return errors instead of panic~~ [WIP] Return errors instead of panic Mar 1, 2019

alexanderbez added the WIP label Mar 3, 2019

MarinX changed the title ~~[WIP] Return errors instead of panic~~ Return errors instead of panic Mar 3, 2019

cwgoes pushed a commit that referenced this pull request Mar 4, 2019

Merge PR #3782: Finalise snapcraft.yaml

5d05b7f

- Allow gaiacli to access host's raw-usb slot. - Promote the package to stable. Closes: #3714

fedekunze reviewed Mar 19, 2019

View reviewed changes

MarinX requested a review from zramsay as a code owner March 27, 2019 15:21

fedekunze added ready-for-review and removed WIP labels Mar 27, 2019

rigelrozanski previously requested changes Mar 27, 2019

View reviewed changes

alessio reviewed Mar 27, 2019

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

MarinX added 10 commits March 29, 2019 22:03

Fix conflict

d4d2a03

Updated PENDING.md

15721b8

Fix conflict

21c7c39

Updated PENDING.md

37f50ea

Fix conflict

fe85e02

Updated PENDING.md

61d28c4

Fix conflict

9a6c410

Return EmptyTags instead of nil

4e34bad

Use named return params

5641573

Update comment with PR number

c02021f

alexanderbez approved these changes Apr 1, 2019

View reviewed changes

alessio suggested changes Apr 1, 2019

View reviewed changes

alessio requested a review from rigelrozanski April 1, 2019 23:01

MarinX added 2 commits April 3, 2019 11:01

Merge branch 'develop' of https://github.com/cosmos/cosmos-sdk into d…

2a55045

…evelop

Add changelog for breaking sdk change

0a2ac3a

MarinX added 2 commits April 3, 2019 11:18

Fix assignment mismatch with tests

00d4bf8

Fix lint errors

33ae73c

fedekunze approved these changes Apr 3, 2019

View reviewed changes

alessio approved these changes Apr 4, 2019

View reviewed changes

alessio merged commit 985aae5 into cosmos:develop Apr 4, 2019

rigelrozanski mentioned this pull request Apr 5, 2019

crisis/invariant ConstantFee parameter check, and other missing parameters #4052

Closed

rigelrozanski added a commit that referenced this pull request Apr 5, 2019

Revert "Return errors instead of panic (#3782)"

7450b75

This reverts commit 985aae5.

rigelrozanski mentioned this pull request Apr 5, 2019

Revert "Return errors instead of panic" #4054

Merged

alexanderbez added a commit that referenced this pull request Apr 5, 2019

Revert "Return errors instead of panic (#3782)"

84e53ac

This reverts commit 985aae5.

alexanderbez mentioned this pull request Apr 5, 2019

Revert "Return errors instead of panic (#3782)" #4061

Closed

5 tasks

rigelrozanski added a commit that referenced this pull request Apr 5, 2019

Revert "Return errors instead of panic (#3782)" (#4054)

cec3065

This reverts commit 985aae5.

rigelrozanski reviewed Apr 5, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return errors instead of panic #3782

Return errors instead of panic #3782

MarinX commented Mar 1, 2019 •

edited

Loading

alexanderbez left a comment

codecov bot commented Mar 1, 2019 •

edited

Loading

MarinX commented Mar 2, 2019

cwgoes commented Mar 15, 2019

jackzampolin commented Mar 16, 2019

MarinX commented Mar 16, 2019

alexanderbez commented Mar 16, 2019

MarinX commented Mar 16, 2019 •

edited

Loading

fedekunze Mar 19, 2019

fedekunze commented Mar 19, 2019

MarinX commented Mar 20, 2019

alexanderbez commented Mar 20, 2019

MarinX commented Mar 27, 2019

MarinX commented Mar 27, 2019

rigelrozanski left a comment

alexanderbez left a comment

alessio left a comment

MarinX commented Apr 3, 2019

fedekunze left a comment

alessio left a comment

jackzampolin commented Apr 4, 2019

rigelrozanski left a comment

Return errors instead of panic #3782

Return errors instead of panic #3782

Conversation

MarinX commented Mar 1, 2019 • edited Loading

alexanderbez left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 1, 2019 • edited Loading

Codecov Report

MarinX commented Mar 2, 2019

cwgoes commented Mar 15, 2019

jackzampolin commented Mar 16, 2019

MarinX commented Mar 16, 2019

alexanderbez commented Mar 16, 2019

MarinX commented Mar 16, 2019 • edited Loading

fedekunze Mar 19, 2019

Choose a reason for hiding this comment

fedekunze commented Mar 19, 2019

MarinX commented Mar 20, 2019

alexanderbez commented Mar 20, 2019

MarinX commented Mar 27, 2019

MarinX commented Mar 27, 2019

rigelrozanski left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

MarinX commented Apr 3, 2019

fedekunze left a comment

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

jackzampolin commented Apr 4, 2019

rigelrozanski left a comment

Choose a reason for hiding this comment

MarinX commented Mar 1, 2019 •

edited

Loading

codecov bot commented Mar 1, 2019 •

edited

Loading

MarinX commented Mar 16, 2019 •

edited

Loading