Skip to content

feat(storage): CometBFT no longer uses cometbft-db #4514

Closed
alesforz wants to merge 10 commits intomainfrom
feature/remove-cometbftdb
Closed

feat(storage): CometBFT no longer uses cometbft-db #4514
alesforz wants to merge 10 commits intomainfrom
feature/remove-cometbftdb

Conversation

@alesforz
Copy link
Collaborator

@alesforz alesforz commented Nov 19, 2024

This feature branch stores the work we are doing to close #1039.
We will merge related PRs to this branch rather than main.

Context

CometBFT will stop importing
cometbft-db as a dependency to support multiple database backends. Instead, it will use pebble by default.

Changes merged so far

  • New package storage in the internal packages (PR):
    • it exports the DB, Batch, and Iterator interfaces that were previously offered by cometbft-db.
    • it exports the pebbledb implementation of the above interfaces.
  • Unit tests for the implementation of the DB interface using pebble (PR).
  • Unit tests for the implementation of the Batch interface using pebble (PR).
  • Unit tests for the implementation of the Iterator interface using pebble (PR).
  • PrefixDB and its implementation of the DB, Batch, and Iterator interfaces (PR).
  • Removed cometbft-db from Dockerfiles, Makefiles, and CI (PR)

Note for Readers

All the code in this PR has already been reviewed and approved in the smaller PRs linked above. All commits are either merge commits of those PRs or merges from the main branch to keep this branch up to date.
Therefore, to review the changes, I suggest looking at the list above and going to the specific PR that implemented the changes you're interested in. This should be easier than trying to review all the changes at once in this PR.


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

Partially addresses #4486.

### Context
CometBFT will stop importing
[cometbft-db](https://github.com/cometbft/cometbft-db) as a dependency
to support multiple database backends. Instead, it will use
[pebble](https://github.com/cockroachdb/pebble) by default.

### Changes
This PR adds package `storage` to the `internal` packages: 
- it exports the `DB`, `Batch`, and `Iterator` interfaces that database
backends must implement to work with CometBFT. Previously, they were
offered by cometbft-db.
- it exports the pebbledb implementation of the above interfaces.

Users can provide their own implementation of these interfaces using a
different database under the hood. However, we will support and maintain
only the pebble implementation that CometBFT will use.

**Note**: this PR ports the code currently provided by cometbft-db. It
does not add new functionalities to either the interfaces or their
implementation using pebble. What it does:
- improve docs
- clearer error messages
- minor refactoring of nested `if` statements for readability

---

#### PR checklist

- ~[] Tests written/updated~
- ~[ ] 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
@alesforz alesforz self-assigned this Nov 19, 2024
@alesforz alesforz added dependencies Dependency updates config storage breaking A breaking change P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably labels Nov 19, 2024
@andynog
Copy link
Collaborator

andynog commented Nov 19, 2024

@alesforz the PR says We will merge related PRs to this branch rather than main. but this PR is to merge a feature into main. Just want to check if this is the desired behavior before reviewing this.

@alesforz alesforz linked an issue Nov 20, 2024 that may be closed by this pull request
9 tasks
…face. (#4515)

Partially addresses #4486.

### Context
CometBFT will stop importing
[cometbft-db](https://github.com/cometbft/cometbft-db) as a dependency
to support multiple database backends. Instead, it will use
[pebble](https://github.com/cockroachdb/pebble) by default.

### Changes
This PR adds unit tests to the pebble implementation of the `DB`
interface that CometBFT will use once we remove cometbft-db.
@alesforz
Copy link
Collaborator Author

@alesforz the PR says We will merge related PRs to this branch rather than main. but this PR is to merge a feature into main. Just want to check if this is the desired behavior before reviewing this.

What I mean is that I will work on smaller, separate PRs to implement the TODO list, so that they are easier to review and (hopefully) approved.
I will merge the branches of those PRs into this branch, i.e., feature/remove-cometbftdb.

Ideally, the code in this PR doesn't need review, because all of it has been already reviewed and approved in the smaller PRs.

Alessandro Sforzin added 2 commits November 22, 2024 11:40
…terface. (#4522)

Partially addresses #4486.

### Context
CometBFT will stop importing
[cometbft-db](https://github.com/cometbft/cometbft-db) as a dependency
to support multiple database backends. Instead, it will use
[pebble](https://github.com/cockroachdb/pebble) by default.

### Changes
This PR adds unit tests to the pebble implementation of the `Batch`
interface that CometBFT will use once we remove cometbft-db.
Alessandro Sforzin added 2 commits November 26, 2024 10:03
… interface. (#4530)

Partially addresses #4486.

### Context
CometBFT will stop importing
[cometbft-db](https://github.com/cometbft/cometbft-db) as a dependency
to support multiple database backends. Instead, it will use
[pebble](https://github.com/cockroachdb/pebble) by default.

### Changes
This PR adds unit tests to the pebble implementation of the `Iterator`
interface that CometBFT will use once we remove cometbft-db.
Alessandro Sforzin added 2 commits December 6, 2024 15:26
Partially addresses #4486.

### Context
CometBFT will stop importing
[cometbft-db](https://github.com/cometbft/cometbft-db) as a dependency
to support multiple database backends. Instead, it will use
[pebble](https://github.com/cockroachdb/pebble) by default.

### Changes
This PR adds type `PrefixDB` to `internal/storage` from cometbft-db. 
`PrefixDB` is a database wrapper that implements the`DB`, `Batch`, and
`Iterator` interfaces.

**Note**: this PR makes some changes to the implementation of
`PrefixDB`. Namely:
- `PrefixDB` no longer uses a mutex, because we think that it is
sufficient to rely on the wrapped database's concurrency control
mechanism. That is, concurrent calls to `PrefixDB`'s methods will
already "lock" on the calls to the wrapped DB methods.
- `NewPrefixDB` now returns an error if the given prefix is empty. We
think allowing the creation of a `PrefixDB` with no prefix was an
oversight in the design.
- `prefixDBBatch` methods now want a pointer receiver. 
- `PrefixDB.Compact` now prepends the prefix to `start` and `end` keys
defining the iterator's range. The previous implementation did not do
so, and this was likely an oversight. We believed this behavior
contradicted the definition of a `PrefixDB` as a logical database that
prepends the prefix to all keys before operating on the underlying
database.
@melekes
Copy link
Collaborator

melekes commented Dec 12, 2024

can this be closed in favor of #4601?

@alesforz
Copy link
Collaborator Author

alesforz commented Dec 12, 2024

can this be closed in favor of #4601?

No, this is the feature branch collecting all the work on "database consolidation" issue (see #4486, and more broadly #1039).
#4601 only removes cometbft-db as a dependency, and there is some more work to do once it is merged.

alesforz pushed a commit that referenced this pull request Dec 12, 2024
This reverts commit d91b7a3.

We will add a comprehensive changelog entry in the feature branch (#4514)
Partially addresses #4486.

This PR removes cometbft-db from cometbft build process:
- Dockerfiles
- Makefiles
- CI
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale For use by stalebot label Dec 23, 2024
@github-actions github-actions bot closed this Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking A breaking change config dependencies Dependency updates P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably stale For use by stalebot storage

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

CometBFT uses a single DB backend Refactor CometBFT to use a single underlying database

3 participants