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

rusk: fix high gas limit TXs taking longer #615

Merged
merged 1 commit into from Mar 7, 2022
Merged

Conversation

ureeves
Copy link
Member

@ureeves ureeves commented Mar 1, 2022

  • Add fork method to RuskState allowing for copying the underlying
    NetworkState
  • Change execute_state_transition to keep transactions based on
    gas_spent instead of gas_limit

Resolves: #605

@ureeves ureeves added type:enhancement Issues concerning code or feature improvement (performance, refactoring, etc) module:rusk Issues related to rusk module labels Mar 1, 2022
@ureeves ureeves requested review from ZER0 and herr-seppia March 1, 2022 10:53
@ureeves ureeves self-assigned this Mar 1, 2022
@ureeves ureeves force-pushed the high-gas-limit-605 branch 2 times, most recently from 149c8e6 to f1d36e9 Compare March 1, 2022 10:56
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

Some optimisation in the comments that can be addressed in some followup PRs.

Btw I'll approve it after you add at least a test to cover this implementation

rusk/src/lib/services/state.rs Outdated Show resolved Hide resolved
rusk/src/lib/services/state.rs Show resolved Hide resolved
@autholykos autholykos marked this pull request as draft March 2, 2022 09:03
@ureeves ureeves force-pushed the high-gas-limit-605 branch 2 times, most recently from 363f117 to 6072e34 Compare March 6, 2022 21:33
@ureeves
Copy link
Member Author

ureeves commented Mar 6, 2022

I added a test proving the behavior is as expected. I also played around with the suggested performance improvements and even though there were gains (marginal ones), I've decided against including them due to a substantial increase in code footprint. We can always discuss them in a future PR/issue.

@ureeves ureeves force-pushed the high-gas-limit-605 branch 2 times, most recently from 6264a75 to 20ef4e4 Compare March 6, 2022 22:06
@ureeves ureeves marked this pull request as ready for review March 6, 2022 22:13
@ureeves ureeves requested a review from herr-seppia March 6, 2022 22:13
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

LGTM

&refunds[i as usize],
&receiver,
amount,
1_000_000_000,
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that is now safe to send txs even with GasLimit higher than the BlockGasLimit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does. As long as the gas spent is ultimately smaller or equal than the gas left in the block it will pass.

- Add `fork` method to `RuskState` allowing for copying the underlying
  `NetworkState`
- Change `execute_state_transition` to keep transactions based on
  `gas_spent` instead of `gas_limit`

Resolves: #605
@ureeves ureeves merged commit 9d6fae7 into master Mar 7, 2022
@ureeves ureeves deleted the high-gas-limit-605 branch March 7, 2022 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:rusk Issues related to rusk module type:enhancement Issues concerning code or feature improvement (performance, refactoring, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

High gas limit transactions take longer to process
2 participants