build(deps): CometBFT no longer imports cometbft-db
#4601
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alesforz
added
dependencies
9 tasks
…e error it returns.
The test was not closing the databases it was creating in succession, thus causing a "lock held by current process" error when opening them a 2nd time.
The refactored code creats an on-disk instance of pebble, but the original code created an in-memory database. The benchmark now creates an in-memory instance of pebble.
2 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 10, 2024
…ronment` (#4639) ### Context Because `rpc.Environment` does not store a `GenesisDoc` in memory anymore, (see #1290), we don't need to create it as a singleton. The risk of storing multiple copies of the genesis in memory isn't there anymore, because we now load it from disk. ### This Change This PR removes the `sync.Once` construct that we put in place. ### Additional Note The change also ensures that [`TestProvider`](https://github.com/cometbft/cometbft/blob/b16c6fc2c8b2fc5a468fead32a8fe9057d6cce2f/light/provider/http/http_test.go#L36) in the `light/provider/http` package behaves as expected. In `main`, this test passes because it's set up using a [`MemDB`](https://github.com/cometbft/cometbft-db/blob/4cf60c715fe8daccb9dce3b24295575bd461d5d8/memdb.go#L52), which is a dummy in-memory store rather than a real database. Thus, database `Get` operations always succeed. However, in the context of the [work to remove cometbft-db](#4601), we now use a "real" database, i.e., one that a `Node` closes when it shuts down. Since the `Environment` object was treated as a singleton, each iteration of `TestProvider` created a new `Node` using the same underlying database. This database would be closed at the end of the first iteration when that iteration's `Node` shut down. Subsequent iterations then attempted to call `Get` on a closed database, causing a panic. This change fixes that issue. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - ~[ ] Updated relevant documentation (`docs/` or `spec/`) and code comments~
3 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 11, 2024
…4642) ### Context The [`TxIndexer`](https://github.com/cometbft/cometbft/blob/2b1db1c16bf2db16b81b49fef3581e79679fbed6/state/txindex/indexer.go#L17) interface defines how to index and search transactions. Its implementers need to interact with a database, which callers are typically expected to close when done. However, `TxIndexer` does not provide a `Close` method. This prevents closing the database used by the transaction indexer, causing goroutines associated with that database to leak. Two [tests](https://github.com/cometbft/cometbft/blob/2b1db1c16bf2db16b81b49fef3581e79679fbed6/internal/inspect/inspect_test.go#L29), `TestInspectConstructor` and `TestInspectRun`, show this problem indirectly. These tests check whether an [`Inspector`](https://github.com/cometbft/cometbft/blob/2b1db1c16bf2db16b81b49fef3581e79679fbed6/internal/inspect/inspect.go#L32) leaks goroutines. The `Inspector` sets up three databases: a block database, a state database, and a transaction indexer database. It closes only the block and state databases because it cannot close the transaction indexer’s database (because of the missing `Close` method). Therefore, the transaction indexer database's goroutines leak, and the two tests above detect that. In `main`, both tests pass because they use a [`MemDB`](https://github.com/cometbft/cometbft-db/blob/4cf60c715fe8daccb9dce3b24295575bd461d5d8/memdb.go#L52), an in-memory store that does not spawn goroutines. However, in the [work to remove cometbft-db](#4601), the tests switch to a “real” database that does spawn goroutines. Without a proper `Close` method in `TxIndexer`, these goroutines remain active, causing `TestInspectConstructor` and `TestInspectRun` to fail. ### This Change This PR adds a `Close` method to the `TxIndexer` interface and updates the existing implementation to properly close the underlying database. It also modifies the code to call `TxIndexer.Close` where needed, ensuring that database resources are released and preventing goroutine leaks. --- #### PR checklist - [x] 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
The test unfolds too quickly for the node's indexer to finish its operations. Therefore, the goroutine below that calls n.Stop() will close the indexer's database right when the indexer is trying to write to it at line state/txindex/indexer_service.go:109 thus causing a panic. This small sleep allows the indexer to finish its write operation before the node is shut down. Unfortunately, there is no graceful shutdown available for the database layer, i.e., make the database wait for all the active connections before closing. The test does not fail in `main` because the code in `main` does not close the indexer's database when shutting down a node (which causes its goroutines to leak. See #4642).
melekes
reviewed
Dec 12, 2024
1 task
2 tasks
8 tasks
### Context In reviewing #4601, we discussed about exporting the `DB`, `Batch`, and `Iterator` interfaces instead of leaving them in an `internal` package. After some thinking we decided to export them in a new non-internal package `cmtdb`. However, our implementation of those interfaces remain unexported. ### Changes This PR adds pkg `cmtdb` exporting the `DB`, `Batch`, and `Iterator` interfaces. Additionally, it changes the code to use this new package rather than the now obsolete `internal/storage` (which this PR removes).
Draft
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Partially addresses #4486.
Context
CometBFT will stop importing cometbft-db as a dependency to support multiple database backends. Instead, it will use pebble by default.
Changes
This PR removes cometbft-db as a dependency and changes all the code to use the new
internal/storagepkg instead.TODO
dbpackage in cometbft; does not import cometbft-db anymore. Done in this branch.DB,Batch, andIteratorinterfaces. Merged to this branch: feat(storage): ExportDBinterface #4690cmdfolder to create a new database instance usingconfig.DefaultDBProvider(). WIP: feat(storage): Top level commands create a new database usingconfig.DefaultDBProvider()#4697PR checklist
[ ] Changelog entry added in.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments