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

chore: send one msg per tx if batch of msgs fails #98

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

RafilxTenfen
Copy link
Contributor

@RafilxTenfen RafilxTenfen commented Jan 17, 2025

Currently, the covenant emulator sends multiple msgs in the same transaction, but if any of these msgs fails, the entire transaction is reverted, so the idea is to try to batch it all first, if it fails it sends multiple transactions each one with a single message inside of it

A good portion of the code added was a copy/adapt from https://github.com/babylonlabs-io/babylon/blob/cd0bbcd98be5e4dda081f7330140cf9dbee4c94d/client/client/tx.go#L76

and it could be solved by adding an option in the Babylon client to send multiple transactions in the same block each one with a single TX inside of it

Modified e2e run in CI, to initiate one test in each machine, since each StartManagerWithFinalityProvider initiates a few processes and one test might affect the other

As the fix is a secondary option it becomes hard to e2e test, so I disabled the batch msgs in a single tx to test. CI run sending one msg per tx https://github.com/babylonlabs-io/covenant-emulator/actions/runs/12890756219/job/35941075950 and https://github.com/babylonlabs-io/covenant-emulator/actions/runs/12892910683/job/35948185923

Closes: #87

.github/workflows/ci.yml Fixed Show fixed Hide fixed
.github/workflows/ci.yml Fixed Show fixed Hide fixed
.github/workflows/ci.yml Fixed Show fixed Hide fixed
.github/workflows/ci.yml Fixed Show fixed Hide fixed
.github/workflows/ci.yml Fixed Show fixed Hide fixed
.github/workflows/ci.yml Fixed Show fixed Hide fixed
.github/workflows/ci.yml Fixed Show fixed Hide fixed
.github/workflows/ci.yml Fixed Show fixed Hide fixed
.github/workflows/ci.yml Fixed Show fixed Hide fixed
@RafilxTenfen RafilxTenfen self-assigned this Jan 21, 2025
@RafilxTenfen RafilxTenfen requested a review from gitferry January 21, 2025 18:15
@RafilxTenfen RafilxTenfen changed the title chore: init of copying relayer msgs chore: send one msg per tx if batch of msgs fails Jan 21, 2025
@RafilxTenfen RafilxTenfen marked this pull request as ready for review January 21, 2025 18:16
clientcontroller/babylon_msg_log.go Show resolved Hide resolved
clientcontroller/babylon_msg_log.go Show resolved Hide resolved
@@ -0,0 +1,221 @@
package clientcontroller
Copy link
Member

Choose a reason for hiding this comment

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

actually this whole file should be deleted, I just removed it from relayer code I copied over (those where the annoying logs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you recomending to remove these log calls?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, mb must have missed it, just got triggered as that's the code from the relayer repo. We can leave the logs in, but I think for us more important are the metrics in general

@RafilxTenfen RafilxTenfen requested a review from Lazar955 January 21, 2025 22:36
@@ -0,0 +1,836 @@
package clientcontroller
Copy link
Member

Choose a reason for hiding this comment

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

Ah somehow I missed this file, could we modify our client code in Babylon repo to add this. As it's pretty much a duplicate now of 429. It might be better than adding a bunch of same code here. Would make sense to do it after 429 is merged.
Unless this is urgent and we need to get it in now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KonradStaniec not sure if this is urgent to be in the next release

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.

One failed message in transactions makes whole batch fail
2 participants