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

refactor: remove blocksdb #1257

Closed
wants to merge 2 commits into from
Closed

refactor: remove blocksdb #1257

wants to merge 2 commits into from

Conversation

Reecepbcups
Copy link
Member

@Reecepbcups Reecepbcups commented Sep 14, 2024

Summary

This is a test PR to see if we can / should remove this.

Source graph shows we only used it internally. Only 2 issues have ever been posted about it, of which only 1 was external (Bearachain) from 2022. They are not even Cosmos anymore so their usecase is out. Any PRs are from 2022 during the initial creation

Further, we purposely do not use this feature as it causes CI failures due to some sqlite locks / non deterministic test behavior (was a big change I focused on in the v7->v8 migration).

Benfits

TODO:

  • For the FindTxs() commands removed for thorchain / cosmos, we should just query consensus at a given height, then query the tx events from a given txs. if this is all we need it for we can reduce a TON of complexity here.

Copy link

vercel bot commented Sep 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
interchaintest-docs ⬜️ Ignored (Inspect) Sep 14, 2024 1:43am

@@ -427,82 +423,6 @@ func (tn *ChainNode) Height(ctx context.Context) (int64, error) {
return height, nil
}

// FindTxs implements blockdb.BlockSaver.
func (tn *ChainNode) FindTxs(ctx context.Context, height int64) ([]blockdb.Tx, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

refactor this to query cometbft for a given block, get the Txs, base64 decode, and query each tx hash from consensus directly to get the EventAttributes. Then query the block to get the FinalizeBlockEvents.

While this is less efficient, it removes a TON of code for a function that does not seem to be called hardly ever

@@ -404,82 +400,6 @@ func (tn *ChainNode) Height(ctx context.Context) (int64, error) {
return height, nil
}

// FindTxs implements blockdb.BlockSaver.
func (tn *ChainNode) FindTxs(ctx context.Context, height int64) ([]blockdb.Tx, error) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@agouin
Copy link
Member

agouin commented Sep 14, 2024

The blocks db was instrumental in development of relayer v2, packet forward middleware async acks, and more. It is a lightweight indexer and easily allows you to inspect transaction lifecycle on involved chains. There is also the terminal UI built on top of it for browsing the indexed data.

The usage is much more for developers to see sequence of events of blocks, transactions, and events, rather than any programmatic usage.

@Reecepbcups Reecepbcups deleted the reece/remove-blockdb branch September 16, 2024 15:46
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.

2 participants