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.

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

Use Tendermint db-backend config value instead of compile-time DBBackend value #10948

Closed
4 tasks
dwedul-figure opened this issue Jan 13, 2022 · 13 comments · Fixed by #11188
Closed
4 tasks

Use Tendermint db-backend config value instead of compile-time DBBackend value #10948

dwedul-figure opened this issue Jan 13, 2022 · 13 comments · Fixed by #11188
Assignees
Labels
T: Dev UX UX for SDK developers (i.e. how to call our code)

Comments

@dwedul-figure
Copy link
Collaborator

dwedul-figure commented Jan 13, 2022

Summary

Use the Tendermint db-backend config value instead of the types.DBBackend value that must be set during compilation (e.g. with ldflags='-w -s -X github.com/cosmos/cosmos-sdk/types.DBBackend=rocksdb').

Problem Definition

As a blockchain engineer, I want to decide the db backend after compiling so that it's easier for nodes to switch from one backend to another.

For example, I'd like to enable support for both leveldb and rocksdb. But Cosmos-SDK prevents this because I must define github.com/cosmos/cosmos-sdk/types.DBBackend at compile time; I can only have one. Tendermint already allows this through compile-time tags that only enable support, e.g. -tags "gcc cleveldb rocksdb".

If I could support both, then node operators could stop their node, change the config, migrate their data, then start their node back up again. Since I can only support one, all of this must be done during an upgrade, increasing downtime.

Proposal

Refactor the types.utils func NewLevelDB and it's usage so that the tendermint config's db-backend value is used instead of backend (which is based on the DBBackend variable and set during an init func). Remove the DBBackend variable.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@tac0turtle
Copy link
Member

Im a fan of this, would you be open to submitting a pr?

@tac0turtle tac0turtle added the T: Dev UX UX for SDK developers (i.e. how to call our code) label Jan 14, 2022
@dwedul-figure
Copy link
Collaborator Author

Yeah. I can PR it. I'm not sure when I'll be able to get to it though. I'll assign it to myself when I start on it.

@ylsGit
Copy link

ylsGit commented Jan 20, 2022

#10828

@dwedul-figure dwedul-figure changed the title Use Tendermint db_backend config value instead of compile-time DBBackend value Use Tendermint db-backend config value instead of compile-time DBBackend value Feb 14, 2022
@dwedul-figure
Copy link
Collaborator Author

I've updated the ticket to reflect the Tendermint v0.35.x config key db-backend instead of the v0.34.x (and prior) db_backend key.

@dwedul-figure
Copy link
Collaborator Author

PR ready for review: #11188

@peterbourgon
Copy link

As a blockchain engineer, I want to decide the db backend after compiling so that it's easier for nodes to switch from one backend to another.

In what circumstances do you want to do this?

@faddat
Copy link
Contributor

faddat commented Feb 15, 2022

@peterbourgon If I had to guess, he is fiddling with the databases and testing, so it is useful to him to have multiple db's in the compiled binary-- I say this because it'd be useful to me, too.

Could always just recompile, but the current setup isn't great, it actually led to me making a mistake where I thought I'd enabled rocks but had only been using rocks on some data.

@tac0turtle
Copy link
Member

This is how things are done on the tendermint side, it is nice to have this feature imo. Setting a build flag has confused users.

@faddat
Copy link
Contributor

faddat commented Feb 15, 2022

speaking of this, does this look like a valid way to set those ldflags?

LDFLAGS='-w -s -X github.com/cosmos/cosmos-sdk/types.DBBackend=rocksdb' go install -tags rocksdb ./...

@dwedul-figure
Copy link
Collaborator Author

dwedul-figure commented Feb 15, 2022

As a blockchain engineer, I want to decide the db backend after compiling so that it's easier for nodes to switch from one backend to another.

In what circumstances do you want to do this?

One scenario is testing. It's easier to test different db backends if they're all running the same binary.

Another scenario is migrating to a new backend. It's significantly easier to manage with this change. By making it a config option, nodes can migrate to a new backend as they please rather than having to bring down the entire chain for the migration as an upgrade.

Additionally, it makes it easier to get real-word performance statistics based on actual use. You could have nodes with the exact same compiled binary running different backend-dbs. It removes one aspect of uncertainty in the stats.

If you're asking for a circumstance when I'd want to switch from one db backend to another, it's when there are performance issues that might be solved by using a different database backend.

The only situation I can think of where I'd WANT this to be a compile time decision is as a secret feature that's incubating.

@dwedul-figure
Copy link
Collaborator Author

speaking of this, does this look like a valid way to set those ldflags?

LDFLAGS='-w -s -X github.com/cosmos/cosmos-sdk/types.DBBackend=rocksdb' go install -tags rocksdb ./...

That looks right to me.

@robert-zaremba
Copy link
Collaborator

The use case makes lot of sense. Especially for upgrade proposals - validators should be able to decide what DB backend they want to use. Currently they would need to manually recompile the binary.

@dwedul-figure
Copy link
Collaborator Author

I added this note on the PR, but wanted to add it here too for visibility:

I realized last night that this change might be troublesome for some users of the SDK.

Currently, there is a possibility that a node has two different DB backends: one for the Tendermint DBs (that get their type from the config file's db-backend value), and one for the Cosmos-SDK DB (that get their type from the DBBackend variable). After this change, there would be just one type defined. If a node currently has a two different types, then after this change, it will only be able to read/write the Tendermint ones.

This doesn't appear to be an issue if one is goleveldb and the other cleveldb (after a superficial test). If any of the other db backend types are involved (and there's a discrepancy), it's a problem.

Nodes with two different db backends will either have to migrate their DBs, or rebuild them. I'm going to put an entry under API Breaking in the CHANGELOG.md, because that seems like the closest thing to the correct place to put it. With some preparation, the impact of this can be mitigated so that the migration/rebuilding doesn't need to be done during an upgrade.

To not need to migrate/rebuild during an upgrade, node operators would need to:

  1. Compile the chain with the same DBBackend as the node's db-backend (or db_backend if pre-Tendermint v0.35) value. I.e. with -ldflags '-w -s -X github.com/cosmos/cosmos-sdk/types.DBBackend=cleveldb'.
  2. Stop the node.
  3. Migrate the Cosmos-SDK-controlled DBs to the desired type. Unfortunately, I'm not aware of an easy way to do this.
  4. Restart the node using the executable made in step 1.

This way, during the upgrade, when DBBackend is no longer used, all the DBs are on the same db backend and this isn't a problem.

@dwedul-figure dwedul-figure moved this from Todo to In Progress in Provenance Core Protocol Team Mar 2, 2022
@dwedul-figure dwedul-figure self-assigned this Mar 2, 2022
Repository owner moved this from In Progress to Done in Provenance Core Protocol Team Mar 18, 2022
@iramiller iramiller moved this from Done to Archive in Provenance Core Protocol Team Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: Dev UX UX for SDK developers (i.e. how to call our code)
Projects
None yet
6 participants