fix(db): Fix pebbleDB integration #23552
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to enhance the flexibility of database backend configuration in the simulation environment. Changes span multiple files, including Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
This comment has been minimized.
This comment has been minimized.
alpe
changed the title
| @@ -72,7 +72,7 @@ func (db *PebbleDB) Get(key []byte) ([]byte, error) { | |||
| return nil, fmt.Errorf("failed to perform PebbleDB read: %w", err) | |||
| } | |||
|
|
|||
| return bz, closer.Close() | |||
| return slices.Clone(bz), closer.Close() | |||
There was a problem hiding this comment.
This line is the fix. Cloning the data ensures it remains valid after the closer call
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
store/v2/CHANGELOG.md (1)
34-34: Consider adding more context to the changelog entry.While the entry follows the correct format, it could be more descriptive about the specific issue that was fixed. Consider adding details about the data invalidation issue and memory leak prevention.
-* [#23552](https://github.com/cosmos/cosmos-sdk/pull/23552) Fix pebbleDB integration +* [#23552](https://github.com/cosmos/cosmos-sdk/pull/23552) Fix pebbleDB integration to prevent data invalidation and potential memory leaks when accessing data after closing
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
store/v2/CHANGELOG.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
store/v2/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-store-v2
- GitHub Check: test-store
- GitHub Check: test-simapp-v2
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: test-system-v2
- GitHub Check: golangci-lint
- GitHub Check: build
- GitHub Check: Summary
🔇 Additional comments (1)
store/v2/CHANGELOG.md (1)
32-33: LGTM! Section follows the guidelines.The new "Bug Fixes" section is correctly formatted and placed appropriately under the "Unreleased" section.
|
Looks that this should be backported to v0.50 and v0.52 |
|
I mean, the fix is in store/V2, but me maybe there is a similar bug in "V1" |
* main: (61 commits) build(deps): Bump cosmossdk.io/x/tx from 1.0.1 to 1.1.0 (#23547) feat(client/v2): add map support (#23544) fix(db): Fix pebbleDB integration (#23552) build(deps): Bump buf.build/gen/go/cometbft/cometbft/protocolbuffers/go from 1.36.3-20241120201313-68e42a58b301.1 to 1.36.4-20241120201313-68e42a58b301.1 in /api (#23520) build(deps): Bump github.com/golang/glog from 1.2.3 to 1.2.4 in /tools/hubl (#23541) fix(x/tx): add an option to encode maps using amino json (ref #23513) (#23539) build(deps): Bump cosmossdk.io/collections from 1.0.0 to 1.1.0 (#23534) build(deps): Bump github.com/bytedance/sonic from 1.12.7 to 1.12.8 in /log (#23516) chore: fix changelog for `v0.52.0-rc.2` (#23502) build(deps): Bump google.golang.org/protobuf from 1.36.3 to 1.36.4 in /x/upgrade (#23512) chore(collections): bring in `protocodec` in collections directly (#23515) feat(x/tx): add an option to encode maps using amino json (#23513) fix(codec): provide legacy amino og type (#23504) chore: remove unecessary SDKGasMeter impl (#23503) refactor: replace `golang.org/x/exp` with stdlib (#23501) build(deps): Bump google.golang.org/grpc from 1.69.4 to 1.70.0 in /tools/cosmovisor (#23496) build(deps): Bump google.golang.org/grpc from 1.69.4 to 1.70.0 in /store (#23494) build(deps): Bump github.com/hashicorp/go-plugin from 1.6.2 to 1.6.3 in /store (#23493) ci: Fix Ci job target (#23500) feat(server/v2): add swagger server component (#23486) ...
Description
Closes: #23133
The loaded data became invalid when the closer was called. From the pebble Godocs:
Note to reviewer:
I was not able to create an integration test to replicate the behaviour within reasonable time. Instead, I used the sims to trigger a failure. Therefore this PR also contains some minor modifications to the sims tests to replicate this.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Improvements