Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(state): Copy Iterator Key() and Value() APIs return values. #3541

Merged
merged 9 commits into from
Jul 31, 2024
Merged

chore(state): Copy Iterator Key() and Value() APIs return values. #3541

merged 9 commits into from
Jul 31, 2024

Conversation

alesforz
Copy link
Contributor

@alesforz alesforz commented Jul 22, 2024 •
edited
Loading

Related to cometbft/cometbft-db#168.

Changes

We recently updated cometbft-db Iterator type Key() and Value() APIs to return their values directly instead of a copy.
Therefore, this PR ensures that code in cometbft that modifies or stores references to the return value of those APIs creates a copy before continuing.

PR checklist

- [ ] Tests written/updated

  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@alesforz alesforz added the chore label Jul 22, 2024
@alesforz alesforz self-assigned this Jul 22, 2024
@alesforz alesforz requested review from melekes and jmalicevic July 22, 2024 13:01
@sergio-mena
Copy link
Contributor

If/when this is ready for review, please hit the "ready for review" button

@alesforz alesforz marked this pull request as ready for review July 25, 2024 11:53
@alesforz alesforz requested review from a team as code owners July 25, 2024 11:53
melekes
melekes reviewed Jul 26, 2024
View reviewed changes
state/txindex/kv/kv.go Show resolved Hide resolved
abci/example/kvstore/kvstore.go Outdated Show resolved Hide resolved
@alesforz alesforz requested a review from melekes July 26, 2024 11:36
melekes
melekes approved these changes Jul 26, 2024
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alesforz ❤️

This PR should also bump cometbft-db version imho.

@alesforz
Copy link
Contributor Author

Thanks @alesforz ❤️

This PR should also bump cometbft-db version imho.

Shouldn't we first bump the Go version of cometBFT to 1.22.5, i.e., the same version that the latest cometbft-db uses?

@melekes
Copy link
Contributor

melekes commented Jul 26, 2024

Thanks @alesforz ❤️
This PR should also bump cometbft-db version imho.

Shouldn't we first bump the Go version of cometBFT to 1.22.5, i.e., the same version that the latest cometbft-db uses?

yes

alesforz added 9 commits July 31, 2024 14:46
@alesforz
updated abci/example/kvstore/kvstore.go to copy Iterator.Value() retu…
37dc7ad 
…rned object
@alesforz
updated state/txindex/kv/kv.go to copy Iterator.Value() returned object
c5d880f
@alesforz
updated state/indexer/block/kv/kv.go to copy Iterator.Value() returne…
4e82dad 
…d object
@alesforz
removed obsolete TODO comment. We implemented what the TODO asks for
b25ca97
@alesforz
updated state/txindex/kv/utils.go to copy Iterator.Key() returned object
f95de72
@alesforz
updated state/indexer/block/kv/kv.go to copy Iterator.Key() returned …
e4b9f0b 
…object
@alesforz
updated docs of LightStoreKeyLayout interface
a1a75e5
@alesforz
added changelog entry
646aada
@alesforz
Revert "updated abci/example/kvstore/kvstore.go to copy Iterator.Valu…
0bff6b2 
…e() returned object".

Copying the Iterator.Value() object is unnecessary here.

This reverts commit 9df0d8b.
@alesforz
Copy link
Contributor Author

alesforz commented Jul 31, 2024 •
edited
Loading

Thanks @alesforz ❤️
This PR should also bump cometbft-db version imho.

Shouldn't we first bump the Go version of cometBFT to 1.22.5, i.e., the same version that the latest cometbft-db uses?

yes

There is now a separate PR bumping cometbft-db version.
I would merge this PR first though, so that the change in the Iterator APIs will be in place before the update to cometbft-db.
I'm afraid that if we update cometbft-db version first, we will have a HEAD status that uses cometbft-db v0.13.0 but that does not copy Key() and Value(), thus causing problems.

@alesforz alesforz added the backport-to-v1.x Tell Mergify to backport the PR to v1.x label Jul 31, 2024
@alesforz alesforz added this pull request to the merge queue Jul 31, 2024
Merged via the queue into main with commit 3be35f0 Jul 31, 2024
39 checks passed
@alesforz alesforz deleted the db-iter-api-copy branch July 31, 2024 13:22
mergify bot pushed a commit that referenced this pull request Jul 31, 2024
@alesforz @mergify
chore(state): Copy Iterator Key() and Value() APIs return value…
9f6c671 
…s. (#3541)

Related to cometbft/cometbft-db#168.

#### Changes
We recently updated cometbft-db `Iterator` type `Key()` and `Value()`
APIs to return their values directly instead of a copy.
Therefore, this PR ensures that code in cometbft that modifies or stores
references to the return value of those APIs creates a copy before
continuing.

---

#### PR checklist

~- [ ] Tests written/updated~
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

(cherry picked from commit 3be35f0)
@mergify mergify bot mentioned this pull request Jul 31, 2024
Merged
3 tasks
alesforz added a commit that referenced this pull request Jul 31, 2024
@mergify @alesforz
chore(state): Copy Iterator Key() and Value() APIs return value…
da6365b 
…s. (backport #3541) (#3598)

Related to cometbft/cometbft-db#168.

#### Changes
We recently updated cometbft-db `Iterator` type `Key()` and `Value()`
APIs to return their values directly instead of a copy.
Therefore, this PR ensures that code in cometbft that modifies or stores
references to the return value of those APIs creates a copy before
continuing.

---

#### PR checklist

~- [ ] Tests written/updated~
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #3541 done by
[Mergify](https://mergify.com).

Co-authored-by: Alessandro Sforzin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melekes melekes melekes approved these changes

@jmalicevic jmalicevic Awaiting requested review from jmalicevic

Assignees

@alesforz alesforz

Labels
backport-to-v1.x Tell Mergify to backport the PR to v1.x chore
Projects
CometBFT
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alesforz @sergio-mena @melekes