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.

Sign up for GitHub

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

Jump to bottom

reporter: use temp QueryContainsBytes API and fix stalling issue #38

Merged
merged 4 commits into from
Sep 14, 2022
Merged

reporter: use temp QueryContainsBytes API and fix stalling issue #38

merged 4 commits into from
Sep 14, 2022

Conversation

SebastianElvis
Copy link
Member

@SebastianElvis SebastianElvis commented Sep 14, 2022 •
edited

fixes #26, fixes #36

This PR

  • uses the temporary QueryContainsBytes for checking block inclusion. It avoids gogoproto customtype feature, which seems to have bugs.
  • fixes the stalling issue. The issue is rooted in the denom of fee: if setting a wrong denom, then the tx will be in mempool forever, making the invocation stalling forever.

Example execution:

image

Future works:

  • Use QueryContains
  • Make all denom bbn rather than stake

@SebastianElvis SebastianElvis mentioned this pull request Sep 14, 2022
Open
vitsalis
vitsalis approved these changes Sep 14, 2022
View reviewed changes
Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

LGTM! Although not entirely sure why we want to use stake (I know it resolves the bug) since this might be an issue with Babylon.

sample-vigilante.yml
@@ -20,7 +20,7 @@ babylon:
account-prefix: bbn
keyring-backend: test
gas-adjustment: 1.2
gas-prices: 0.01ubbn
gas-prices: 2stake
Copy link
Member

Choose a reason for hiding this comment

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

Why is this 2stake instead of ubbn? Maybe it is the fault of Babylon? From my understanding the stake denomination in Babylon would be the same as the coin denomination.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know. It seems that Babylon still demands stake to be the denom for gas prices, tx fees, etc.. I found this issue from the trace log of Babylon. Will investigate to see if we can replace the stake denom completely.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, do we want to enforce bbn denom before the demo?

Copy link
Member

Choose a reason for hiding this comment

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

Not a strict requirement, but let's create an issue for now.

@SebastianElvis SebastianElvis merged commit 98f8aa1 into main Sep 14, 2022
@SebastianElvis SebastianElvis deleted the bugfix-stalling branch September 14, 2022 01:58
@vitsalis
Copy link
Member

vitsalis commented Sep 14, 2022 •
edited

btw this doesn't fix #36. Still needs investigation imo. Can happen after other priorities come into effect.

gitferry pushed a commit that referenced this pull request Jan 25, 2024
@KonradStaniec
Config for tx zmq endpoint (#38)
6f798d0 
* Config for tx zmq endpoint

* Add seq endpoint config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitsalis vitsalis vitsalis approved these changes

@gusin13 gusin13 Awaiting requested review from gusin13

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@SebastianElvis @vitsalis