Skip to content

build: use only goleveldb#9742

Closed
faddat wants to merge 188 commits intotendermint:mainfrom
faddat:goleveldb-only
Closed

build: use only goleveldb#9742
faddat wants to merge 188 commits intotendermint:mainfrom
faddat:goleveldb-only

Conversation

@faddat
Copy link
Contributor

@faddat faddat commented Nov 22, 2022

PHILOSOPHY.md specifies that there should be only one single obvious way to do things.

While the cosmos-sdk suffers from some database-related and iavl-related performance issues, tendermint does not.

Instead of refactoring to sqlite, which would be a great deal of code and a fairly uncertain path, this PR takes only goleveldb and memdb from tm-db and allows us to:

  • reduce build complexity
  • reduce test complexity
  • support more system architectures
  • reduce configuration complexity
  • deprecate tm-db
  • improve code readability
  • highlight the tm-db -> cosmos-db migration path

There's never been an issue with goleveldb in Tendermint itself, so instead of reinventing the wheel, this PR eliminates the database choices that make a number of tasks related to this repository far more complex than they need to be.


PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

tac0turtle and others added 30 commits July 15, 2019 18:59
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* makefile adds

* add files and commands

* remove make tools and lint

* changelog add
* tm-cmn to tm-db

* minor change

* root level files
* unexport rand

* gofmt
* add rocksdb

* move codes

* dir update and misc fixes

* fix tests

* Update db.go

Co-Authored-By: Anton Kaliaev <anton.kalyaev@gmail.com>
Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* changelog entry

* add link to PR
Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.3.0 to 1.4.0.
- [Release notes](https://github.com/stretchr/testify/releases)
- [Commits](stretchr/testify@v1.3.0...v1.4.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.22.0 to 1.24.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.22.0...v1.24.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
* Switch from fork to orginal

- gorocksdb merged what we needed: tecbot/gorocksdb#174

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

* change comment
- update golang to 1.13 for ci and remove unneeded env sets

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>
* Testing out ci

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

* some linting

* add changelog entry
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.24.0 to 1.25.1.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.24.0...v1.25.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
* RC for 0.2.1

- new release:x

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

* bump to 0.3
* memdb: test Iterator

* fix test
* Return Errors instead of Panicing

- closes tendermint#28
- closes tendermint#4

Return errors and have the application decide how they want to handle panicing :smile

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

* more error changes to prefix and mem dbs

* added errors to other dbs and tests pass

* linting and testing fixes

* add go mod verify to linting

* minor cleanup and changes added to cleveldb

* return error from Has()`

* address some PR comments

* some minor touch up on returning errors

* remvoe .vscode from .gitignore

* error: wraped some errors and comment

- add comment about why the error is ignored
- wraped a couple of errors with error.Wrap

* bring back expect in test case

* add error to iterator and reverse iterator

* add changelog entry
* Add tests for proto files

- imported gogo/protobuf for remotedb to generate tests and update the .pb.go file

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

* add changelog entry
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.25.1 to 1.26.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.25.1...v1.26.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
* Follow up to Panice to Err PR

- there were some post merge comments, this is meant to address them

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

* fix returns with errors

* address pr comments, still need to add func error

* address more comments

* fixes to rocks and other dbs

* fixl linting issues

* Apply suggestions from code review
* release: release v0.4

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

* update changelog
Bumps [github.com/pkg/errors](https://github.com/pkg/errors) from 0.8.1 to 0.9.1.
- [Release notes](https://github.com/pkg/errors/releases)
- [Commits](pkg/errors@v0.8.1...v0.9.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
* removal: remove fsdb

- remove fsdb as some parts are not implemented and we do ont use it anywhere

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

* add changelog entry
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.26.0 to 1.27.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.26.0...v1.27.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
* testing: tests were not being run on all dbs

- added tsting for all dbs
- this should of been caught earlier
- will add this to ci as well

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

* remove todo

* add custom image

* remove rocks from tags (test)

* test go get

* locatation

* one more location

* update rocks

* docs: expand readme

Signed-off-by: Marko Baricevic <marbar3778@yahoo.com>

* test remove go get

* fix `

* Update README.md

Co-Authored-By: Anton Kaliaev <anton.kalyaev@gmail.com>

* fix boltdb readme

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.27.0 to 1.27.1.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.27.0...v1.27.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.4.0 to 1.5.0.
- [Release notes](https://github.com/stretchr/testify/releases)
- [Commits](stretchr/testify@v1.4.0...v1.5.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Bumps [github.com/stretchr/testify](https://github.com/stretchr/testify) from 1.5.0 to 1.5.1.
- [Release notes](https://github.com/stretchr/testify/releases)
- [Commits](stretchr/testify@v1.5.0...v1.5.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
@faddat faddat mentioned this pull request Nov 22, 2022
3 tasks
@faddat faddat marked this pull request as ready for review November 22, 2022 11:11
@faddat faddat requested a review from ebuchman as a code owner November 22, 2022 11:11
@faddat faddat requested a review from a team November 22, 2022 11:11
@faddat faddat changed the title build: PHILOSOPHY.md application to databases build: use only goleveldb Nov 22, 2022
@faddat faddat mentioned this pull request Nov 22, 2022
10 tasks
@faddat
Copy link
Contributor Author

faddat commented Nov 23, 2022

Hi-- this PR should be changed to include tm-db's commit history.

I talked to @liamsi about the "delete everything" commit in tendermint's commit history, and learned that it was bringing together consensus and abci. I'd like to make this one, more like that one.

@faddat faddat marked this pull request as draft November 23, 2022 13:04
@ethanfrey
Copy link
Contributor

I cannot comment on the code, but agree with the point.

Tendermint should strive to the simplest secure way to do what it does (which is complex enough).
We don't need to complicate it more than needed with extra abstractions and options.

If goleveldb doesn't work, we find a new one... maybe support both for some time as a transition, then transition to only the other one. But if it does work, why worry about something else.

@ethanfrey
Copy link
Contributor

ethanfrey commented Nov 23, 2022

On another note, what could use a different db is events.
But loosely coupled, not a postgresql driver here. And then still custom query language that tries to hit the db.

Like just dump out events to a simple message queue (enabled/disabled in config) and there can be external solutions to consume those events, index them, expose APIs. These external APIs may be better or worse or even buggy. But very importantly they will not impact the security or stability of Tendermint

I'm all for removing code and allowing non-consensus stuff to be handled elsewhere

@faddat faddat marked this pull request as ready for review November 23, 2022 15:37
@thanethomson
Copy link
Contributor

While I agree in terms of the philosophy, we unfortunately cannot accommodate this specific change, as we believe there's still quite a bit of work to be done before we can settle on LevelDB, especially in light of some of our planned changes toward improving storage efficiencies for operators. See #9749.

We also cannot accommodate such a substantial breaking change in our current development cycle as we work towards Tendermint v0.38 with full ABCI 2.0 (a.k.a. ABCI++). Our approach to technical risk management from v0.37 onwards involves making smaller breaking changes across releases, while attempting to release more frequently. The soonest we may consider such a change will probably be in the v0.39 release cycle (Q1/Q2 of 2023), and will have to be according to #9749.

@faddat, if you would like to help us make substantial changes, please come discuss with us synchronously in our community call first so we can share our current priorities and plans?

Like just dump out events to a simple message queue (enabled/disabled in config) and there can be external solutions to consume those events, index them, expose APIs. These external APIs may be better or worse or even buggy. But very importantly they will not impact the security or stability of Tendermint

@ethanfrey have you had a chance to evaluate #9437? I still have some tweaks planned for that ADR to address some of the comments, but the overarching philosophy is to provide a configurable way to offload the requisite data (e.g. events) such that an external process can index and expose it however it wants.

@ethanfrey
Copy link
Contributor

@ethanfrey have you had a chance to evaluate #9437? I still have some tweaks planned for that ADR to address some of the comments, but the overarching philosophy is to provide a configurable way to offload the requisite data (e.g. events) such that an external process can index and expose it however it wants.

No, I haven't seen the ADR. This is something I brought up in late 2017 as the current event system was being built. I am a bit tired of being the grumpy old man who gives advice that is ignored and ends up saying "I told you so". So I just ignore most discussions these days.

I took a quick look at #9437 and agree with the general direction. But I would probably go more radical in the redesign / simplification. Not sure why only one "external companion service", and that could use some examples. I have quite some ideas here and have been sketching this out trying to get a usable indexing service for CosmWasm contracts.

If people will seriously listen to and discuss my ideas, I can make some input. But I've spent years of being ignored by all core teams on architecture, so very little desire to spend hours on this without serious engagement of all decision makers on the architecture.

mergify bot pushed a commit that referenced this pull request Nov 24, 2022
## NOTE: this pr exclusively runs commands from the makefile found here


This PR ONLY runs `make format` ... then `make mockery`



Its purpose is to ensure that the review scope of other PR's, which changed .go files and thus triggered the linter that only runs conditionally, have smaller review 
scopes, and should be merged before:

#9738
#9739
#9742






---

#### PR checklist

- [x] Tests written/updated, or no tests needed
- [x] `CHANGELOG_PENDING.md` updated, or no changelog entry needed
- [x] Updated relevant documentation (`docs/`) and code comments, or no
      documentation updates needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.