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

fix(server): use CometBFT fallback for db_backend #17181

Closed
wants to merge 3 commits into from

Conversation

k-yang
Copy link
Contributor

@k-yang k-yang commented Jul 28, 2023

Description

Our team uses rocksdb as the underlying database, but the application still wrote LevelDB files under data/application.db. Our config.toml file had db_backend = "rocksdb" and the documentation in app.toml says the following:

# AppDBBackend defines the database backend type to use for the application and snapshots DBs.
# An empty string indicates that a fallback will be used.
# First fallback is the deprecated compile-time types.DBBackend value.
# Second fallback (if the types.DBBackend also isn't set), is the db-backend value set in Tendermint's config.toml.
app-db-backend = ""

There are two things wrong with this comment:

  1. We set the legacy -X github.com/cosmos/cosmos-sdk/types.DBBackend=rocksdb linker flag at build time, but it had no effect. Just searching the cosmos-sdk/types directory shows no DBBackend variable anymore, so it was removed. The TOML comment is thus outdated, and this PR removes that misleading comment.

  2. The CosmosSDK application doesn't actually use the db_backend value from config.toml, due to a typo in the lookup key. When running the startup code through a debugger, we saw that the Viper config contains a db_backend field, but the code is looking for a db-backend field (note the underscore and hyphen mismatch). Please see the screenshot below. This PR fixes the lookup and fixes the typo in the TOML comment.

Screenshot 2023-07-28 at 4 22 08 PM

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • run make lint and make test
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@k-yang k-yang marked this pull request as ready for review July 28, 2023 21:02
@k-yang k-yang requested a review from a team as a code owner July 28, 2023 21:02
@k-yang k-yang changed the title fix: use correct config key for db_backend fix: use CometBFT fallback for db_backend Jul 28, 2023
@julienrbrt julienrbrt added backport/0.46.x PR scheduled for inclusion in the v0.46's next stable release backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release labels Jul 28, 2023
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find!

@@ -79,8 +79,7 @@ iavl-disable-fastnode = {{ .BaseConfig.IAVLDisableFastNode }}

# AppDBBackend defines the database backend type to use for the application and snapshots DBs.
# An empty string indicates that a fallback will be used.
# First fallback is the deprecated compile-time types.DBBackend value.
# Second fallback (if the types.DBBackend also isn't set), is the db-backend value set in CometBFT's config.toml.
# The fallback is the db_backend value set in CometBFT's config.toml.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update this comment in tools/confix/data/v0.46-app.toml, v0.47-app.toml and v0.50-app.toml?

@julienrbrt julienrbrt changed the title fix: use CometBFT fallback for db_backend fix(server): use CometBFT fallback for db_backend Jul 28, 2023
@@ -53,6 +53,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (baseapp) [#17159](https://github.com/cosmos/cosmos-sdk/pull/17159) Validators can propose blocks that exceed the gas limit.
* (x/group) [#17146](https://github.com/cosmos/cosmos-sdk/pull/17146) Rename x/group legacy ORM package's error codespace from "orm" to "legacy_orm", preventing collisions with ORM's error codespace "orm".
* (x/bank) [#17170](https://github.com/cosmos/cosmos-sdk/pull/17170) Avoid empty spendable error message on send coins.
* (db_backend) [#17181](https://github.com/cosmos/cosmos-sdk/pull/17181) Fix `db_backend` lookup fallback from `config.toml`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* (db_backend) [#17181](https://github.com/cosmos/cosmos-sdk/pull/17181) Fix `db_backend` lookup fallback from `config.toml`
* (server) [#17181](https://github.com/cosmos/cosmos-sdk/pull/17181) Fix `db_backend` lookup fallback from `config.toml`.

Copy link
Collaborator

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this fix @k-yang. Just chiming in after review to kindly request that we add a simple test that checks that the different backends are correctly returned otherwise this change can regress quietly and corrupt your data silently as y'all found before.

@julienrbrt
Copy link
Member

@k-yang We would gladly include this fix in our following patch releases, would you be up to implement the comments to make this PR good to go?

holyxiaoxin added a commit to Switcheo/cosmos-sdk that referenced this pull request Aug 16, 2023
holyxiaoxin added a commit to Switcheo/cosmos-sdk that referenced this pull request Aug 16, 2023
@julienrbrt
Copy link
Member

Thank you for your contribution. I am going to take over this and make the required changes.
We will include this fix in our next patch releases that are coming very soon.

@julienrbrt julienrbrt closed this Aug 16, 2023
holyxiaoxin added a commit to Switcheo/cosmos-sdk that referenced this pull request Aug 22, 2023
* feat: add before and after ModuleEndBlock

* feat: add pruning-start-height flag to prune cmd

* fix: db_backend lookup in config.toml

see: cosmos#17181

* add refund-handler to base-app

* fix bug with refundHandler msCache

* append refund events to anteEvents

---------

Co-authored-by: yan-soon <soonxiangyan@gmail.com>
yan-soon pushed a commit to Switcheo/cosmos-sdk that referenced this pull request Sep 19, 2023
yan-soon added a commit to Switcheo/cosmos-sdk that referenced this pull request Nov 2, 2023
* Add liquidity provider fee pool

* Add deliver tx and before commit hooks

* Implements Valuer and Scanner interface for Int and Dec

* Add after committer hook

* Add liquidity provider fee pool to module account invariant

* Add liquidity provider fee pool to distribution genesis check

* Revert "Emit anthandler events even if txn fails or panics"

This reverts commit c5adf7a.

* Fix deliverTxer not including ante events on failed txns

* Change authz module name from const to var

* feat: increase DefaultGasLimit for cli tx

* Add msg handler middleware

* add: Hooks for send keeper

* fix: Typos

* fix: Type error

* add error handling to hooks

* add IsModuleAccount function in AccountKeeper to check if a supplied address is a module account.

* add transfer hooks  BeforeMultiSend and  AfterMultiSend for multisend functionality

* fix make install issue

* remove unneeded hooks

* Delete version specifally

* Delete version

* Panic on error

* Add logs

* Hardcode version delete

* Force overwrite

* Update iavl

* Force delete version

* Fix ver deletion

* Rollback properly

* Fix merge regressions

* Fix rollback cmd

* add evm mapping logic in accountKeeper and bankKeeper

* edit getAccount to stay in line with initial cosmos sdk contract, ie return nil when address is nil instead of panic

* revert HasAccount to its original definition in case future libraries/code uses it.
set function which checks for account with mapping condition included name to be AccountExists instead

* reverts commit b45c030 "revert HasAccount to its original definition in case future libraries/code uses it."

* create GetMappedAccountAddressIfExists helper method in accountKeeper

* refactor GetMappedAccountAddressIfExists to GetMergedAccountAddressIfExists and remove GetMappedAccountAddressIfExists in BaseSendKeeper

* remove getting merged account logic in getAllBalances as it is already present in IterateAccountBalances

* code clean up

* move extraction of address from key to be in app logic rather than in accountKeeper

* Revert "move extraction of address from key to be in app logic rather than in accountKeeper"

This reverts commit e7af659.

* add guard for iteration of EthAddressToCosmosAddress and CosmosAddressToEthAddressKey to prevent panic

* update iavl

* Code Refactoring: Refactor telemetry to begin and end block (#316)

* Remove hardcoded telemetry from modules

* Add telemetry to begin/endblock

* Remove test labels

* add GetMappedAddress helper method

* ensure that event emitted from transfer of funds is from mapped address

* Add telemetry to GRPC handler (#320)

* feat/api node for off oracle service [WIP] (#333)

* Add new address field to `app.toml`

* Refactor typo

* Refactor naming

* Update oracle address port

* add refund-handler to base-app

* fix bug with refundHandler msCache

* append refund events to anteEvents

* feat: add before and after ModuleEndBlock

* fix: db_backend lookup in config.toml

see: cosmos#17181

* feat: telemetry monitor-store-size-interval config

* fix conflicts from cherry-picking

* add back legacy router to handle legacy sdk.msg routing

* update proto related files

* Revert "update proto related files"

This reverts commit 8145796.

* update distribution proto

* add back value and scanner interface for LegacyDec

* update cosmossdk.io/math pseudo ver

* feat: add pruning-start-height flag to prune cmd

* update distribution proto and rerun make proto-gen

* update distribution proto and rerun make proto-gen again

* change begin and end blockers for gov, staking and upgrade modules to accept ptr keeper instead

* remove duplicate pruning cmd flag

* use cosmos iavl instead of our fork

* remove previous forked rollback code

* update cometbft to v0.38.0 to fully utilise ABCI++

* Revert "update cometbft to v0.38.0 to fully utilise ABCI++"

This reverts commit 57a9676.

---------

Co-authored-by: Ivan Poon <ravenxce@gmail.com>
Co-authored-by: Lee Yik Jiun <yikjiun.lee@switcheo.network>
Co-authored-by: holyxiaoxin <jr.loves.maths@gmail.com>
Co-authored-by: Ivan Poon <ivan.poon@switcheo.network>
Co-authored-by: PC <spclight@gmail.com>
Co-authored-by: Randy <randy75828@gmail.com>
Co-authored-by: lance.lan <lmengsplist96@gmail.com>
Co-authored-by: Nicholas Chung <84615041+chungnicholas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release backport/v0.50.x PR scheduled for inclusion in the v0.50's next stable release backport/0.46.x PR scheduled for inclusion in the v0.46's next stable release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants