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

[RelayMiner] Use gas for claim and proof txs #1018

Merged
merged 25 commits into from
Jan 7, 2025
Merged

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Dec 20, 2024

Summary

This pull request includes several changes to the pkg/client package, focusing on adding gas limit and gas price parameters to transaction signing and broadcasting functions, as well as making related updates in tests and configurations.

It also updates the BroadcastTx method in pkg/client/tx/context.go to use BroadcastTxSync instead of BroadcastTxAsync for better error handling during the check-tx ABCI operation.

Minor Fix: Changed the URL in makefiles/relay.mk to include a trailing slash.

Issue

image

Type of change

Select one or more from the following:

Testing

  • Documentation: make docusaurus_start; only needed if you make doc changes
  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@red-0ne red-0ne added bug Something isn't working relayminer Changes related to the Relayminer labels Dec 20, 2024
@red-0ne red-0ne added this to the Shannon Beta TestNet Support milestone Dec 20, 2024
@red-0ne red-0ne self-assigned this Dec 20, 2024
@okdas okdas added the push-image CI related - pushes images to ghcr.io label Dec 20, 2024
Copy link

The image is going to be pushed after the next commit.

You can use make trigger_ci to push an empty commit.

If you also want to run E2E tests, please add devnet-test-e2e label.

Copy link
Member

@Olshansk Olshansk 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 handling this so quickly!

pkg/client/interface.go Outdated Show resolved Hide resolved
pkg/client/supplier/client.go Outdated Show resolved Hide resolved
pkg/client/tx/client.go Outdated Show resolved Hide resolved
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

You may need to run make trigger_ci to submit an empty commit that'll trigger the tests.

GCP workloads (requires changing the namespace to 1018)
Grafana network dashboard for devnet-issue-1018

@okdas
Copy link
Member

okdas commented Dec 20, 2024

{"level":"info","message":"Starting relay miner..."}
{"level":"info","message":"starting relayer sessions manager"}
{"level":"info","endpoint":"localhost:6060","message":"starting a pprof endpoint"}
{"level":"info","endpoint":":9090","message":"serving metrics"}
{"level":"info","message":"starting relayer proxy"}
{"level":"info","server host":"0.0.0.0:8545","message":"starting relay proxy server"}
{"level":"info","message":"starting ring cache"}
{"level":"info","session_end_height":34020,"claim_window_open_height":34022,"message":"waiting & blocking until the earliest claim commit height offset seed block height"}
{"level":"info","session_end_height":34020,"claim_window_open_height":34022,"claim_window_open_block_hash":"cd1ef990487790b33d764349f0ef61e30dfd835d4383228209d5e140584fe6f5","message":"observed earliest claim commit height offset seed block height"}
{"level":"info","session_end_height":34020,"claim_window_open_height":34022,"claim_window_open_block_hash":"cd1ef990487790b33d764349f0ef61e30dfd835d4383228209d5e140584fe6f5","earliest_claim_commit_height":34022,"message":"waiting & blocking until the earliest claim commit height for this supplier"}
{"level":"info","session_end_height":34020,"claim_window_open_height":34022,"claim_window_open_block_hash":"cd1ef990487790b33d764349f0ef61e30dfd835d4383228209d5e140584fe6f5","earliest_claim_commit_height":34022,"message":"observed earliest claim commit height"}
{"level":"error","error":"out of gas in location: ReadFlat; gasWanted: 100000, gasUsed: 100905: out of gas: error during ABCI check tx","message":"failed to create claims"}
{"level":"error","error":"out of gas in location: ReadFlat; gasWanted: 100000, gasUsed: 100905: out of gas: error during ABCI check tx"}

Seems like we need to bump the fees if we're not going to make it configurable at the moment.

@red-0ne red-0ne requested review from Olshansk and okdas January 2, 2025 17:17
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

Very cool - thanks @red-0ne! 🚀

pkg/client/tx/context.go Show resolved Hide resolved
pkg/client/tx/context.go Outdated Show resolved Hide resolved
pkg/relayer/cmd/cmd.go Outdated Show resolved Hide resolved
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Looks great! Requesting changes just because I know this is still an active WIP.

pkg/client/tx/context.go Show resolved Hide resolved
pkg/client/tx/context.go Show resolved Hide resolved
pkg/client/tx/client.go Outdated Show resolved Hide resolved
pkg/client/tx/client.go Outdated Show resolved Hide resolved
pkg/client/tx/client.go Outdated Show resolved Hide resolved
pkg/client/tx/client.go Show resolved Hide resolved
pkg/client/tx/client.go Show resolved Hide resolved
pkg/relayer/cmd/cmd.go Outdated Show resolved Hide resolved
pkg/client/tx/client.go Outdated Show resolved Hide resolved
pkg/deps/config/suppliers.go Show resolved Hide resolved
@red-0ne red-0ne requested a review from Olshansk January 4, 2025 12:02
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Awesome work, ty @red-0ne!

@red-0ne red-0ne merged commit 92fbd03 into main Jan 7, 2025
10 checks passed
red-0ne added a commit that referenced this pull request Jan 9, 2025
This pull request includes several changes to the `pkg/client` package,
focusing on adding gas limit and gas price parameters to transaction
signing and broadcasting functions, as well as making related updates in
tests and configurations.

It also updates the `BroadcastTx` method in `pkg/client/tx/context.go`
to use `BroadcastTxSync` instead of `BroadcastTxAsync` for better error
handling during the check-tx ABCI operation.

Minor Fix: Changed the URL in `makefiles/relay.mk` to include a trailing
slash.

![image](https://github.com/user-attachments/assets/535e99b3-621b-46bd-a335-49e167c50cba)

Select one or more from the following:

- [ ] New feature, functionality or library
- [ ] Consensus breaking; add the `consensus-breaking` label if so. See
- [x] Bug fix
- [ ] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [x] **LocalNet E2E Tests**: `make test_e2e`
- [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

- [x] I have tested my changes using the available tooling
- [x] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [x] I have left TODOs throughout the codebase, if applicable

---------

Co-authored-by: Dima K. <[email protected]>
Co-authored-by: Dmitry K <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working devnet devnet-test-e2e push-image CI related - pushes images to ghcr.io relayminer Changes related to the Relayminer
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants