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
Support custom minimum gas price #1930
Conversation
} | ||
|
||
if !isFlagSet["minGasPrice"] { | ||
*minGasPrice = netw.minGasPrice |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this line out of the *ethController
conditional block because we'd want to set the min gas price using the network config regardless of whether -ethController
was specified or not.
@@ -515,7 +519,7 @@ func main() { | |||
CleanupInterval: cleanupInterval, | |||
TTL: smTTL, | |||
RedeemGas: redeemGas, | |||
SuggestGasPrice: backend.SuggestGasPrice, | |||
SuggestGasPrice: client.Backend().SuggestGasPrice, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ensures that tx cost estimation in LocalSenderMonitor uses the gas price returned by GasPriceMonitor.
return c.backend, nil | ||
} | ||
func (c *client) Backend() Backend { | ||
return c.backend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped the error in the return type for this method because there should always be a backend configured when a client is instantiated and doing so makes it easier to call backend methods when the caller only has access to the client i.e. we can do client.Backend().SomeMethod()
without additional error handling.
@@ -59,9 +59,25 @@ func (gpm *GasPriceMonitor) GasPrice() *big.Int { | |||
gpm.gasPriceMu.RLock() | |||
defer gpm.gasPriceMu.RUnlock() | |||
|
|||
if gpm.gasPrice.Cmp(gpm.minGasPrice) < 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were already dropping polled gas prices that were below the min gas price, but this ensures that the lowest value returned by GasPrice() is min gas price.
b, err := s.LivepeerNode.Eth.Backend() | ||
if err != nil { | ||
respondWith400(w, err.Error()) | ||
if s.LivepeerNode.Eth == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this check so livepeer_cli
doesn't immediately crash the node when running in off-chain mode. Though other things can probably crash the node when running in off-chain mode. I haven't take a thorough look - this just happened to be the one crash scenario that stood out to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me 👍 the comments you added in the review were really helpful. I just wanted to point out that https://github.com/livepeer/go-livepeer/pull/1923/files does some restructuring in the eth
package (see comment) so it might be worth to base off of that PR instead (which in itself using nv/tx-fixes
as a base branch).
70df136
to
e09c66f
Compare
f3d581d
to
ba31cae
Compare
Rebased off of master and switched the target branch to master as well now that #1915 has been merged. |
Looks like there is a failing test, I assume because minGasPrice isn't set for the test. |
Fixed in 5eb1ee1. |
LGTM let's squash the fixup 👍 |
5eb1ee1
to
2c0812c
Compare
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
What does this pull request do? Explain your changes. (required)
Adds support for configuring a custom minimum gas price using the
-minGasPrice
flag, requests to the/setMinGasPrice
webserver endpoint andlivepeer_cli
.This PR is based off of #1915 which injects GasPriceMonitor into the backend type defined in
eth
so in this PR we define setters/getters for minGasPrice for GasPriceMonitor which are used by the backend.Specific updates (required)
See commit history.
How did you test each of these updates (required)
Added unit tests and tested manually using devtool to confirm that if the last polled gas price < minGasPrice, txs will be submitted with minGasPrice.
Does this pull request close any open issues?
Fixes #1914
Checklist:
make
runs successfully./test.sh
pass