fix(store/commitment/iavl): honor tree.Remove error firstly by odeke-em · Pull Request #18651 · cosmos/cosmos-sdk

odeke-em · 2023-12-07T07:13:58Z

Previously the check was if !res and then it would return the error below but that has a consequence of ignoring/swallowing up the error returned by (*iavl.MutableTree).Remove. Even more is the fact that when !res, an error occurred and it could be anything else, thus checking against the error firstly ensures that we only return a synthesized error when we are certain the key wasn't in the tree.

Fixes #18650

Summary by CodeRabbit

Bug Fixes
- Improved error handling for tree removal operations, ensuring that accurate error messages are propagated to the user.
- Snapshot failures are now correctly reported to the user, rather than creating an empty snapshot without notification.

coderabbitai · 2023-12-07T07:14:04Z

Walkthrough

The changes involve improving error handling in the IavlTree.Remove method. The method now prioritizes checking for errors before considering the result of the removal operation. This ensures that errors are not missed and are properly propagated to the caller, aligning with the objectives to handle specific error scenarios more effectively.

Changes

File Path	Change Summary
`store/commitment/iavl/tree.go`	Updated `IavlTree.Remove` to check and return errors before evaluating the result of the removal operation.
`store/CHANGELOG.md`	Documented the changes in error handling for `iavl.MutableTree.Remove` and the Snapshotter's error propagation.

Assessment against linked issues

Objective	Addressed	Explanation
Update `IavlTree.Remove` to check errors before result (res) (#18650)	✅
Handle errors related to leaf nodes, nil keys, and missing values (#18650)	❓	The summary does not provide enough detail to confirm if all specific error scenarios are addressed.
Align error handling with identified scenarios without being overshadowed by result-based checks (#18650)	✅
Notify the relevant contributor for awareness and potential input (#18650)	❌	This task is non-coding and thus not verifiable through the code changes.

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

coderabbitai

Review Status

Actionable comments generated: 5

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between f6df368 and 4991053.

Files selected for processing (2)

store/CHANGELOG.md (1 hunks)
store/commitment/iavl/tree.go (1 hunks)

Additional comments: 1

store/commitment/iavl/tree.go (1)

29-39: The changes to the Remove method correctly prioritize error handling before checking the result, which aligns with the PR's objective to ensure that errors are not overlooked. This is a critical improvement for the reliability and correctness of the method.

store/CHANGELOG.md

julienrbrt · 2023-12-08T12:26:46Z

store/CHANGELOG.md

Can you move this under unreleased?

Done and thank you @julienrbrt!

Previously the check was if !res and then it would return the error below but that has a consequence of ignoring/swallowing up the error returned by (*iavl.MutableTree).Remove. Even more is the fact that when !res, an error occurred and it could be anything else, thus checking against the error firstly ensures that we only return a synthesized error when we are certain the key wasn't in the tree. Fixes #18650

coderabbitai

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 112f6cb and 9409254.

Files selected for processing (2)

store/CHANGELOG.md (2 hunks)
store/commitment/iavl/tree.go (1 hunks)

Additional comments: 3

store/CHANGELOG.md (2)

61-61: The summary incorrectly suggests that the Snapshotter's failure propagation change is part of a different pull request. The changelog entry for PR #16588 correctly documents this change as part of the current codebase.

38-38: The changelog entry for PR #18651 accurately describes the changes made to the iavl.MutableTree.Remove method, ensuring that errors are propagated first to the caller.

store/commitment/iavl/tree.go (1)

29-39: The changes to the Remove method correctly prioritize error handling before checking the result of the removal operation. This ensures that errors are not ignored and are properly communicated to the caller, aligning with the PR objectives to fix the error handling bug.

* feat: secp256k1 public key constant time (cosmos#18026) Signed-off-by: bizk <santiago.yanzon1999@gmail.com> * chore: Fixed changelog duplicated items (cosmos#18628) * adr: Un-Ordered Transaction Inclusion (cosmos#18553) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * docs: lint ADR-070 (cosmos#18634) * fix(baseapp)!: postHandler should run regardless of result (cosmos#18627) * docs: fix typos in adr-007-specialization-groups.md (cosmos#18635) * chore: alphabetize labels (cosmos#18640) * docs(x/circuit): add note on ante handler (cosmos#18637) Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> * fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * refactor(store/v2): updates from integration (cosmos#18633) * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(store/v2): snapshot manager (cosmos#18458) * chore(client/v2): fix typos in the README.md (cosmos#18657) * fix(baseapp): protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654) Co-authored-by: unknown unknown <unknown@unknown> * chore: fix several minor typos (cosmos#18660) * chore(tools/confix/cmd): fix typo in view.go (cosmos#18659) * refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655) * feat(accounts): use gogoproto API instead of protov2. (cosmos#18653) Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651) * build(deps): Bump actions/stale from 8 to 9 (cosmos#18656) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(docs): fix typos & wording in docs (cosmos#18667) * chore: fix several typos. (cosmos#18666) * feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: marbar3778 <marbar3778@yahoo.com> Co-authored-by: Marko <marko@baricevic.me> * feat(store/v2): add SetInitialVersion in SC (cosmos#18665) * feat(client/keys): support display discreetly for `keys add` (cosmos#18663) Co-authored-by: Julien Robert <julien@rbrt.fr> * ci: add misspell action (cosmos#18671) * chore: typos fix by misspell-fixer (cosmos#18683) Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> * chore: add v0.50.2 changelog to main (cosmos#18682) * build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor(bank): remove .String() calls (cosmos#18175) Co-authored-by: Facundo <facundomedica@gmail.com> * ci: use codespell instead of misspell-fixer (cosmos#18686) Co-authored-by: Marko <marbar3778@yahoo.com> * feat(gov): add proposal types and spam votes (cosmos#18532) * feat(accounts): use account number as state prefix for account state (cosmos#18664) Co-authored-by: unknown unknown <unknown@unknown> * chore: typos fixes by cosmos-sdk bot (cosmos#18689) Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: marbar3778 <marbar3778@yahoo.com> * feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688) * refactor: remove panic usage in keeper methods (cosmos#18636) * ci: rename pr name in misspell job (cosmos#18693) Co-authored-by: Marko <marko@baricevic.me> * build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(client/keys): support display discreetly for keys export (cosmos#18684) * feat(x/gov): better gov genesis validation (cosmos#18707) --------- Signed-off-by: bizk <santiago.yanzon1999@gmail.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Carlos Santiago Yanzon <27785807+bizk@users.noreply.github.com> Co-authored-by: yihuang <huang@crypto.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: Akaonetwo <107335783+Akare123@users.noreply.github.com> Co-authored-by: Marko <marbar3778@yahoo.com> Co-authored-by: Julien Robert <julien@rbrt.fr> Co-authored-by: dreamweaverxyz <153101746+dreamweaverxyz@users.noreply.github.com> Co-authored-by: Pioua <136521243+dzizazda@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cool-developer <51834436+cool-develope@users.noreply.github.com> Co-authored-by: leonarddt05 <139609434+leonarddt05@users.noreply.github.com> Co-authored-by: testinginprod <98415576+testinginprod@users.noreply.github.com> Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: Sukey <35202440+sukey2008@users.noreply.github.com> Co-authored-by: axie <152680487+azukiboy@users.noreply.github.com> Co-authored-by: Luke Ma <867273263@qq.com> Co-authored-by: Emmanuel T Odeke <emmanuel@orijtech.com> Co-authored-by: 0xn4de <109149873+0xn4de@users.noreply.github.com> Co-authored-by: hattizai <150505746+hattizai@users.noreply.github.com> Co-authored-by: Devon Bear <itsdevbear@berachain.com> Co-authored-by: Marko <marko@baricevic.me> Co-authored-by: Halimao <1065621723@qq.com> Co-authored-by: Cosmos SDK <113218068+github-prbot@users.noreply.github.com> Co-authored-by: github-merge-queue <github-merge-queue@users.noreply.github.com> Co-authored-by: Facundo <facundomedica@gmail.com> Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com>

odeke-em requested review from a team, alexanderbez, cool-develope and julienrbrt December 7, 2023 07:13

odeke-em force-pushed the store-respect-Remove.error-firstly branch 2 times, most recently from 4aedf30 to 4991053 Compare December 7, 2023 21:56

coderabbitai bot reviewed Dec 7, 2023

View reviewed changes

store/CHANGELOG.md Outdated Show resolved Hide resolved

store/CHANGELOG.md Outdated Show resolved Hide resolved

store/CHANGELOG.md Outdated Show resolved Hide resolved

odeke-em force-pushed the store-respect-Remove.error-firstly branch from 4991053 to 2ee2063 Compare December 8, 2023 02:50

likhita-809 approved these changes Dec 8, 2023

View reviewed changes

odeke-em force-pushed the store-respect-Remove.error-firstly branch from 2ee2063 to da54529 Compare December 8, 2023 11:43

odeke-em enabled auto-merge December 8, 2023 11:44

julienrbrt reviewed Dec 8, 2023

View reviewed changes

odeke-em force-pushed the store-respect-Remove.error-firstly branch from da54529 to 48908fc Compare December 8, 2023 12:28

odeke-em requested a review from julienrbrt December 8, 2023 13:40

odeke-em force-pushed the store-respect-Remove.error-firstly branch from 48908fc to 9409254 Compare December 8, 2023 13:42

coderabbitai bot reviewed Dec 8, 2023

View reviewed changes

julienrbrt approved these changes Dec 8, 2023

View reviewed changes

odeke-em added this pull request to the merge queue Dec 8, 2023

Merged via the queue into main with commit 1e216af Dec 8, 2023

odeke-em deleted the store-respect-Remove.error-firstly branch December 8, 2023 14:00

marcello33 mentioned this pull request Jan 22, 2024

Upstream fetch (#2) #19163

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(store/commitment/iavl): honor tree.Remove error firstly#18651

fix(store/commitment/iavl): honor tree.Remove error firstly#18651
odeke-em merged 1 commit intomainfrom
store-respect-Remove.error-firstly

odeke-em commented Dec 7, 2023 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 7, 2023 •

edited

Loading

Walkthrough

Changes

Assessment against linked issues

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julienrbrt Dec 8, 2023

Uh oh!

odeke-em Dec 8, 2023

Uh oh!

coderabbitai bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

odeke-em commented Dec 7, 2023 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Assessment against linked issues

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julienrbrt Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

odeke-em Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

odeke-em commented Dec 7, 2023 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 7, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)