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

Support for /block_search RPC endpoint #810

Closed
wants to merge 10 commits into from
Closed

Support for /block_search RPC endpoint #810

wants to merge 10 commits into from

Conversation

RiccardoM
Copy link
Contributor

This PR adds the support to the /block_search RPC endpoint that was introduced with PR #6626, made available with v0.34.9 and is already available inside the Cosmos SDK with v0.42.4.

This endpoint is needed to allow to search for block events that are not emitted by any transaction and are found only inside the begin_block_events or end_block_events arrays. Without this endpoint it is not possible to search for such events.

Copy link
Contributor

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

I agree this is an important addition and the code looks good to my eyes. (Simon is the final reviewer there however).

My big question is documentation and displaying errors well, so a client could decide to handle the case of an older chain.

*
* @see https://docs.tendermint.com/master/rpc/#/Info/block_search
*/
public async blockSearch(params: requests.BlockSearchParams): Promise<responses.BlockSearchResponse> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good to me (in terms of functionality).

My one question is that this is only available in tendermint v0.34.9+, correct? Minimally that should be documented in the comments along with a link to the PR. However, what happens when someone tries to run this on tendermint v0.34.8 node? There should be a clearly defined error case that distinguishes "network error" from "tendermint version too old" or it will be very hard for people to debug this.

Maybe @webmaster128 has some idea how to do this properly - it seems this error would be thrown in this.doCall

Copy link
Member

Choose a reason for hiding this comment

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

I think a note regarding compatibility here would be nice.

I would not get into the version checking business again, which led to a lot of trouble sttarting with Tendermint 0.34. If the backend does not support an RPC method, it will return some kind of error that explains the problem somehow.

Copy link
Member

@webmaster128 webmaster128 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 four your contribution.

Could you write a test for this? I assume it is tricky to get values via plain Tendermint but at least it should be possible to send a request and parse and empty result. This would most likely require an update in scripts/tendermint/all_start.sh and scripts/tendermint/all_stop.sh from 0.34.0 to a sufficiently new version.

*
* @see https://docs.tendermint.com/master/rpc/#/Info/block_search
*/
public async blockSearch(params: requests.BlockSearchParams): Promise<responses.BlockSearchResponse> {
Copy link
Member

Choose a reason for hiding this comment

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

I think a note regarding compatibility here would be nice.

I would not get into the version checking business again, which led to a lot of trouble sttarting with Tendermint 0.34. If the backend does not support an RPC method, it will return some kind of error that explains the problem somehow.

@webmaster128 webmaster128 added this to the 0.26.0 milestone May 25, 2021
@ethanfrey
Copy link
Contributor

Note, we discussed this in the ts-relayer repo.

I agreed that no reasonable stargate blockchain is using something below cosmos sdk v0.42.4, and anyone could upgrade to tendermint v0.34.9 without breaking changes. So the ts-relayer will just support those versions as minimum versions.

@RiccardoM
Copy link
Contributor Author

I've tried updating the Tendermint version to v0.34.9 (the latest one) as @webmaster128 suggested. However, I found out that all the Tendermint versions starting from v0.34.4 do not return a valid version, which results in the adaptorForVersion method to throw an exception (which makes fail all the tests).

I've opened a Tendermint issue here: tendermint/tendermint#6488

RiccardoM and others added 2 commits May 26, 2021 11:45
- Added block search tests
- Incremented v0.34 Tendermint version to 0.34.9
@webmaster128
Copy link
Member

Thank you for exploring this, @RiccardoM. Much appreciated. In #813 I pulled out the upgrade of the local Tendermint version for testing. When we have this merged, you can rebase your PR and have 0.34.10 available.

Upgrade Tendermint version in tests to 0.34.10 and adapt code
@webmaster128
Copy link
Member

#813 is merged. Could you rebase this PR on top of current main?

RiccardoM added 4 commits May 27, 2021 07:04
- Added block search tests
- Incremented v0.34 Tendermint version to 0.34.9
…riccardo/add-block-search

# Conflicts:
#	packages/tendermint-rpc/src/tendermint34/tendermint34client.spec.ts
#	scripts/tendermint/all_start.sh
#	scripts/tendermint/all_stop.sh
@RiccardoM
Copy link
Contributor Author

@webmaster128 I've rebased the PR into the main branch. In order for all the tests to pass I had to slightly edit the response parsers as they were assuming things that are not always true. Particularly, if you query block with height = 1 (the genesis block), the following happens:

  • block.header.last_block_id.hash is empty
  • block.header.last_block_id.parts.hash is empty
  • block.header.app_hash is empty

So I had to remove the associated assertNotEmpty from the code

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Particularly, if you query block with height = 1 (the genesis block), the following happens:

makes sense, thank you.

@RiccardoM RiccardoM requested a review from webmaster128 May 27, 2021 07:18
@RiccardoM
Copy link
Contributor Author

@webmaster128 Applied all the fixes

@webmaster128
Copy link
Member

Some of the new tests are failing now. Could you fix those?

Also could you give me edit permission of the branch? Then I would create a CHANGELOG entry and fix up the commit history in order to get the changes from my other PR out of this diff.

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.

3 participants