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

Don't check GPM before mining tx (instead, handle error from state transition if it happens) #1500

Merged
merged 3 commits into from
Apr 13, 2021

Conversation

oneeman
Copy link
Contributor

@oneeman oneeman commented Apr 12, 2021

Description

This is an optimization for block generation. Previously, for each transaction we want to add, we check that its gas price meets the gas price minimum before applying the transaction. However, this is unnecessary, because applying the transaction includes a GPM check as well (inside the state transition). This PR removes the redundant check and instead handles the error that is returned by the state transition if the transaction's gas price is below the GPM. If the error appears, it breaks out of the loop (which is the same as what we were previously doing if the gas price was below the GPM).

Other Changes

  • In the state transition, if the transaction's gas price doesn't meet the GPM, it will now log that at the debug level rather than the error level.

Tested

  • Benchmarking with a single validator and with 100 validators confirms this results in a noticeable improvement in block generation time
  • Have not tested core.ErrGasPriceDoesNotExceedMinimum actually being returned. This will not normally happened, because the transaction pool does not admit transactions below the GPM, and if the GPM increases it removes transactions that don't meet the GPM. As such, this code path (and also the original code path breaking out of the loop due to GPM) are unlikely to ever occur (they would only happen if the gas price minimum increases beyond some pending transaction's gas price and the miner gets the list of transactions before the transaction pool has had time to remove the transaction).
  • However, the code path in the state transition which rejects transactions due to being below the GPM has previously been confirmed to work.

Backwards compatibility

No incompatibilities. This would result in the same transactions being included in the proposed block as before.

@oneeman oneeman requested a review from a team as a code owner April 12, 2021 17:44
Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

lgtm

@oneeman oneeman force-pushed the oneeman/second-mining-optimization branch from c671a89 to 1da2a99 Compare April 12, 2021 22:05
miner/worker.go Show resolved Hide resolved
Co-authored-by: Victor "Nate" Graf <victor@clabs.co>
@oneeman
Copy link
Contributor Author

oneeman commented Apr 12, 2021

Linter caught an error I had seen previously but missed when doing this PR: that the break is breaking out of the case and not the loop. Will fix in an additional commit.

@oneeman oneeman merged commit a2e1568 into master Apr 13, 2021
@oneeman oneeman deleted the oneeman/second-mining-optimization branch April 13, 2021 14:25
oneeman pushed a commit that referenced this pull request Apr 13, 2021
oneeman pushed a commit that referenced this pull request Apr 13, 2021
Cherry-picked PRs:

* Update ledger contracts data (#1497)
* Fix homebrew error in CI (due to outdated version). (#1503)
* Remove error log when gas < intrinsic gas. (#1502)
* Don't check GPM before mining tx (instead, handle error from state transition if it happens) (#1500)
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.

None yet

3 participants