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

Remove parallel gas #3615

Merged
merged 4 commits into from
Aug 14, 2024
Merged

Remove parallel gas #3615

merged 4 commits into from
Aug 14, 2024

Conversation

grarco
Copy link
Collaborator

@grarco grarco commented Aug 12, 2024

Describe your changes

Closes #3587.

Depends-On: #3573

This PR removes the parallel gas dividers and the VpsGas type which isn't needed anymore. It also removes the gas used field from VpsResult.

Checklist before merging

  • If this PR has some consensus breaking changes, I added the corresponding breaking:: labels
    • This will require 2 reviewers to approve the changes

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 97.43590% with 3 lines in your changes missing coverage. Please review.

Project coverage is 61.22%. Comparing base (9fb1037) to head (0c07a1b).
Report is 15 commits behind head on main.

Files Patch % Lines
crates/node/src/protocol.rs 97.11% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3615      +/-   ##
==========================================
- Coverage   61.22%   61.22%   -0.01%     
==========================================
  Files         312      312              
  Lines      101394   101374      -20     
==========================================
- Hits        62083    62067      -16     
+ Misses      39311    39307       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

grarco added a commit that referenced this pull request Aug 12, 2024
@grarco grarco marked this pull request as ready for review August 12, 2024 14:31
@@ -481,8 +481,6 @@ pub struct VpsResult {
pub accepted_vps: BTreeSet<Address>,
/// The addresses whose VPs rejected the transaction
pub rejected_vps: BTreeSet<Address>,
/// The total gas used by all the VPs
pub gas_used: VpsGas,
Copy link
Collaborator Author

@grarco grarco Aug 12, 2024

Choose a reason for hiding this comment

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

I've removed this because I believe it carries little information to the final user (since VpsResult ends up being published in the tx result event) given that it only mentions the amount of gas used by vps without specifying the distribution across the different vps and inside each vp (compilation, execution, host Fns,...)

grarco added a commit that referenced this pull request Aug 12, 2024
@grarco grarco force-pushed the grarco/no-parallel-gas branch from 47402c4 to 83b1e33 Compare August 12, 2024 15:16
@grarco
Copy link
Collaborator Author

grarco commented Aug 12, 2024

Actually my changes to the VpsResult are causing the failures to the IBC e2e tests: I suppose it's because this changes the format of TxResult so we might need to change something in our Hermes implementation for this to work

@grarco grarco requested review from tzemanovic and Fraccaman August 12, 2024 16:04
tzemanovic pushed a commit that referenced this pull request Aug 13, 2024
@tzemanovic tzemanovic force-pushed the grarco/no-parallel-gas branch from 83b1e33 to 13097bc Compare August 13, 2024 10:23
@tzemanovic
Copy link
Member

I rebased onto #3573 to update hermes

tzemanovic added a commit to heliaxdev/hermes that referenced this pull request Aug 13, 2024
@tzemanovic tzemanovic added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Aug 13, 2024
Copy link
Contributor

mergify bot commented Aug 13, 2024

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

⛓️ Depends-On Requirements

Requirement based on the presence of Depends-On in the body of the pull request

@tzemanovic
Copy link
Member

ah, looks like there's still some issue with e2e test ibc_upgrade_client

@Fraccaman Fraccaman removed the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Aug 13, 2024
@grarco
Copy link
Collaborator Author

grarco commented Aug 14, 2024

ah, looks like there's still some issue with e2e test ibc_upgrade_client

@yito88 pushed a commit that fixes it, we should be good now

@tzemanovic
Copy link
Member

tzemanovic commented Aug 14, 2024

@yito88 pushed a commit that fixes it, we should be good now

ah, I think we need it in the base PR - will add it there

@tzemanovic tzemanovic added the merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass label Aug 14, 2024
Copy link
Contributor

mergify bot commented Aug 14, 2024

Hey @grarco, your pull request has been dequeued due to the following reason: CONFLICT_WITH_BASE_BRANCH.
Sorry about that, but you can requeue the PR by using @mergifyio requeue if you think this was a mistake.

@tzemanovic tzemanovic force-pushed the grarco/no-parallel-gas branch from bc2ddf8 to 0c07a1b Compare August 14, 2024 15:07
@grarco
Copy link
Collaborator Author

grarco commented Aug 14, 2024

Seems like another test is failing, I'll have a look

mergify bot added a commit that referenced this pull request Aug 14, 2024
@grarco
Copy link
Collaborator Author

grarco commented Aug 14, 2024

Seems like another test is failing, I'll have a look

Ok it just seems that this is test is a bit flaky and sometimes 3 retries are not enough

@mergify mergify bot merged commit de6f4cc into main Aug 14, 2024
19 checks passed
@mergify mergify bot deleted the grarco/no-parallel-gas branch August 14, 2024 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking: protocol gas merge Ready to merge - mergifyio bot will add the PR to merge queue when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove gas dividers
3 participants