Skip to content

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

Merged
alesforz merged 9 commits intomainfrom
db-iter-api-copy
Jul 31, 2024
Merged

chore(state): Copy Iterator Key() and Value() APIs return values.#3541
alesforz merged 9 commits intomainfrom
db-iter-api-copy

Conversation

@alesforz
Copy link
Collaborator

@alesforz alesforz commented Jul 22, 2024

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 jmalicevic and melekes July 22, 2024 13:01
@sergio-mena
Copy link
Collaborator

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 a review from a team as a code owner July 25, 2024 11:53
@alesforz alesforz requested a review from a team July 25, 2024 11:53
@alesforz alesforz requested a review from melekes July 26, 2024 11:36
Copy link
Collaborator

@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
Collaborator 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
Collaborator

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
Copy link
Collaborator Author

alesforz commented Jul 31, 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

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 this pull request to the merge queue Jul 31, 2024
Merged via the queue into main with commit 3be35f0 Jul 31, 2024
@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
…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)
alesforz pushed a commit that referenced this pull request Jul 31, 2024
…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 <alessandro@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants