fix: Prevent newExporter crash if tree.ndb is nil by chillyvee · Pull Request #622 · cosmos/iavl

chillyvee · 2022-11-20T16:42:58Z

Found a case where dig chain did not have all stores configured with a nodedb (for whatever reason)

It is better to return an error so that cosmos-sdk can determine whether to skip the misconfigured store or panic.

Would also like to backport to fix v0.19.4 (but need assistance to target the right branch)

robert-zaremba · 2022-11-22T01:05:06Z

+	}
+	// CV Prevent crash on incrVersionReaders if tree.ndb == nil
+	if tree.ndb == nil {
+		fmt.Printf("iavl/export newExporter failed to create because tree.ndb is nil\n")


this function should return error. Doing fmt.Printf doesn't make sense here.

agree, lets break the api.

Could just return (nil, err) with fmt.Errorf to start with. Then any more complicated error handling can be followed up in other commits/improvment?

Or is there some error standard you're trying to maintain?

yea that makes sense, we would then handle this in the application

Returning error now, updated api/test

…ments

chillyvee · 2022-11-22T12:46:16Z

Last commit returns explicit error as recommended.
Tests also updated

robert-zaremba

lgtm, added one suggestion

robert-zaremba · 2022-11-22T18:09:21Z

+	}
+	// CV Prevent crash on incrVersionReaders if tree.ndb == nil
+	if tree.ndb == nil {
+		return nil, ErrorTreeNodeDbNil


maybe we can have one error type: ErrNotIniitalizedTree. And then use wrapping to add more details, eg:

ErrNotIniitalizedTree.Wrap("ndb is nil")

chillyvee · 2022-11-22T18:34:04Z

change log updated, errors wrapped

chillyvee · 2022-11-22T18:34:20Z

note change in errors package

tac0turtle · 2022-11-23T23:39:48Z

-func newExporter(tree *ImmutableTree) *Exporter {
+func newExporter(tree *ImmutableTree) (*Exporter, error) {
+	if tree == nil {
+		return nil, errors.Wrap(ErrNotInitalizedTree, "tree is nil")


can we use fmt.Errorf to avoid introducing GitHub.com/pkg/errors as a dependency

My mistake. I thought the team was looking to call Wrap() specifically. Will change it again.

no worries, pkg/errors is deprecated with go 1.18 and beyond because of fmt.errorf and/or errors pkg.

Learn something new every day

chillyvee · 2022-11-24T11:14:34Z

Updated : change from requested Wrap to fmt.Errorf

tac0turtle

thank you for submitting this!!! we can look at back porting this to avoid the issues being seen.

tac0turtle · 2022-11-24T11:53:46Z

linting and tests are failing, could you fix and we will handle backport

chillyvee · 2022-11-24T12:32:13Z

linting and tests are failing, could you fix and we will handle backport

Will give it a shot and let you know if help is required :)

chillyvee · 2022-11-24T12:43:54Z

Issue was hiding in the benchmarks, fixed and running through tests.

chillyvee · 2022-11-24T13:01:26Z

looks like there's some fumpting to do (sorry not yet completely familiar with all the tests in this repo)

chillyvee · 2022-11-24T13:07:06Z

went fumpting

chillyvee · 2022-11-24T13:08:25Z

Also as a note, noticed:

dfde9a5 iavl: remove grpc server (#499) [GitHub]
0       129     cmd/iavlserver/README.md
0       207     cmd/iavlserver/main.go

But in Makefile it still tries to build iavlserver

ifeq ($(COLORS_ON),)
        go install ./cmd/iaviewer
        go install ./cmd/iavlserver <-- FAILS
else
        go install $(CMDFLAGS) ./cmd/iaviewer
        go install $(CMDFLAGS) ./cmd/iavlserver <-- FAILS
endif

Want it removed?

chillyvee · 2022-11-24T13:09:57Z

Removed failing build of cmd/iavlserver. Let me know if you want it back.

tac0turtle · 2022-11-25T14:01:40Z

thanks for the pr @chillyvee

tac0turtle · 2022-11-25T15:02:04Z

@Mergifyio backport release/v0.19.x

tac0turtle · 2022-11-25T15:04:20Z

@Mergifyio backport release/v0.19.x

mergify · 2022-11-25T15:05:02Z

backport release/v0.19.x

✅ Backports have been created

Details

#631 fix: Prevent newExporter crash if tree.ndb is nil (backport #622) has been created for branch release/v0.19.x

Co-authored-by: Marko <marbar3778@yahoo.com> (cherry picked from commit 2777659) # Conflicts: # export.go

Co-authored-by: Chill Validation <92176880+chillyvee@users.noreply.github.com> Co-authored-by: Marko Baricevic <marbar3778@yahoo.com>

) (#631)" This reverts commit 2af006a.

…osmos#622) (cosmos#631)" This reverts commit 2af006a.

… (cosmos#631) Co-authored-by: Chill Validation <92176880+chillyvee@users.noreply.github.com> Co-authored-by: Marko Baricevic <marbar3778@yahoo.com>

v0.19.5 + cosmos/iavl#622

Prevent newExporter crash if tree.ndb is nil

055432e

chillyvee requested a review from a team November 20, 2022 16:42

chillyvee mentioned this pull request Nov 20, 2022

CV add snapshot manager to dig app notional-labs/dig#425

Merged

robert-zaremba reviewed Nov 22, 2022

View reviewed changes

ImmutableTree.Export() returns error if newExporter is given nil argu…

8db8b72

…ments

robert-zaremba reviewed Nov 22, 2022

View reviewed changes

Wrap newExporter errors into ErrNotInitalizedTree

46067ec

tac0turtle reviewed Nov 23, 2022

View reviewed changes

change from requested Wrap to fmt.Errorf

e856bd2

tac0turtle approved these changes Nov 24, 2022

View reviewed changes

Merge branch 'master' into cv_rescue_tree_ndb_nil_master

ec2b499

tac0turtle changed the title ~~Prevent newExporter crash if tree.ndb is nil~~ fix: Prevent newExporter crash if tree.ndb is nil Nov 24, 2022

fix benchmark test

f62821d

go fumpting

6fb6de0

remove build for previously removed iavlserver

5b9853c

tac0turtle added the backport/v0.19.x label Nov 25, 2022

tac0turtle merged commit 2777659 into cosmos:master Nov 25, 2022

mergify Bot mentioned this pull request Nov 25, 2022

fix: Prevent newExporter crash if tree.ndb is nil (backport #622) #631

Merged

mergify Bot pushed a commit that referenced this pull request Nov 25, 2022

fix: Prevent newExporter crash if tree.ndb is nil (#622)

f05e7fb

Co-authored-by: Marko <marbar3778@yahoo.com> (cherry picked from commit 2777659) # Conflicts: # export.go

tac0turtle added a commit that referenced this pull request Dec 8, 2022

fix: Prevent newExporter crash if tree.ndb is nil (backport #622) (#631)

2af006a

Co-authored-by: Chill Validation <92176880+chillyvee@users.noreply.github.com> Co-authored-by: Marko Baricevic <marbar3778@yahoo.com>

tac0turtle added a commit that referenced this pull request Dec 8, 2022

Revert "fix: Prevent newExporter crash if tree.ndb is nil (backport #622

d842fb9

) (#631)" This reverts commit 2af006a.

yihuang pushed a commit to yihuang/iavl that referenced this pull request Jan 11, 2023

Revert "fix: Prevent newExporter crash if tree.ndb is nil (backport c…

ae78cc5

…osmos#622) (cosmos#631)" This reverts commit 2af006a.

yihuang pushed a commit to yihuang/iavl that referenced this pull request Jan 26, 2023

Revert "fix: Prevent newExporter crash if tree.ndb is nil (backport c…

1bafbad

…osmos#622) (cosmos#631)" This reverts commit 2af006a.

assafmo added a commit to scrtlabs/SecretNetwork that referenced this pull request May 25, 2023

Bump IAVL to v0.19.5+

e10f8be

v0.19.5 + cosmos/iavl#622

coderabbitai Bot mentioned this pull request May 13, 2024

chore: changelog onto master #946

Merged

Conversation

chillyvee commented Nov 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chillyvee Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chillyvee commented Nov 22, 2022

Uh oh!

robert-zaremba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chillyvee commented Nov 22, 2022

Uh oh!

chillyvee commented Nov 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chillyvee commented Nov 24, 2022

Uh oh!

tac0turtle left a comment

Choose a reason for hiding this comment

Uh oh!

tac0turtle commented Nov 24, 2022

Uh oh!

chillyvee commented Nov 24, 2022

Uh oh!

chillyvee commented Nov 24, 2022

Uh oh!

chillyvee commented Nov 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chillyvee commented Nov 24, 2022

Uh oh!

chillyvee commented Nov 24, 2022

Uh oh!

chillyvee commented Nov 24, 2022

Uh oh!

tac0turtle commented Nov 25, 2022

Uh oh!

tac0turtle commented Nov 25, 2022

Uh oh!

tac0turtle commented Nov 25, 2022

Uh oh!

mergify Bot commented Nov 25, 2022

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

chillyvee commented Nov 20, 2022 •

edited

Loading

chillyvee Nov 22, 2022 •

edited

Loading

chillyvee commented Nov 24, 2022 •

edited

Loading