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

Set -maxGasPrice flag equal to max fee per gas, current implementation makes spending unpredictable #2084

Closed
0xVires opened this issue Nov 2, 2021 · 8 comments · Fixed by #2111 or #2171
Assignees

Comments

@0xVires
Copy link

0xVires commented Nov 2, 2021

I really would suggest to implement the -maxGasPrice as setting the max fee per gas. The current implementation makes it really unpredictable how much gas you'll end up spending. As an example: I just had a ticket redeemed at 155 gwei despite having set the maxGasPrice to 110: https://etherscan.io/tx/0x3b07dbf395c69fb028a63d69596c50d1b4c268aa5f4f417850089b9f69c5baea
Here's what happened:

  • In block 13536342, the base fee was at 109, which triggered the transaction.
  • This block and the next 2 were full, which caused the base fee to increase 12.5% per block.
  • Now since the tx was submitted with a priority fee of only 1 gwei, it is unlikely to be included during high demand/when blocks are full. I guess I was lucky since block 13536345 was almost empty, so my 1 gwei priority tx was included.
  • However, by then the base fee was at 155 gwei which means I ended up paying almost 50% more than expected.

With the current implementation, you could easily end up paying twice as much as your -maxGasPrice setting, which seems very counterintuitive to me.

@bjp99
Copy link

bjp99 commented Nov 4, 2021

I called reward and paid gas fee of 136 gwei with -maxGasPrice set to 120 gwei. When the transaction was pending it said max gas price of 202 gwei. When i call manually on etherscan i can set the max gas price and it will not confirm over the gas price set.

Using livepeer_cli
https://etherscan.io/tx/0x9c9c02ee2ffa0bd7d357bca9a8894f712ea001cf3f1eab89c4d1e9ab28847209

Using etherscan
https://etherscan.io/tx/0x2ece8d43a6bb33f39ec3cb92095796211a318dd73b5ed52edcb94252cf009215

@yondonfu
Copy link
Member

yondonfu commented Nov 5, 2021

Yeah the suggestion makes sense. Working on it.

@yondonfu yondonfu self-assigned this Nov 5, 2021
@0xVires
Copy link
Author

0xVires commented Nov 5, 2021

Thank you!
I'll also add my suggestion from discord here - but I'd totally understand if the implementation is too complicated and not worth the time since we're moving to L2 anyways in a few months:

So for me the ideal implementation would look like this:

  • Automatically submit the reward claim tx right at the beginning of the round with the set -maxGasPrice - no matter what the current gas price is
  • If the tx was not included after 2880 blocks into the current round (half of one round, so after ~11h), start increasing the maxGasPrice of the tx
  • Increase the gas price by 10% every 250 blocks or so (~1h) until it gets included

Another thing: It might be worth considering to set the minimum priority fee to 2 gwei. Based on experience, this allows the tx to confirm considerably faster than when set to 1 or 0.5 gwei - while making almost no difference in the total tx cost.

@0xVires
Copy link
Author

0xVires commented Nov 23, 2021

I'm not sure if this should be a separate issue, but I feel like this is kind of on topic since it only concerns those who set the -maxFeePerGas flag:
Would it be possible to also add a retry mechanism like the one for the ticket redemption? Currently, if the new round starts and the gas price is higher than the maxFeePerGas, the O does not call the reward tx. It's then up to the user to manually call it or it won't be called at all.
It would be great if the node would keep checking the gas price and automatically submits the tx as soon as the gas price is lower than the set maxFeePerGas (like for the ticket redemption).

@leszko
Copy link
Contributor

leszko commented Nov 23, 2021

Let me summarize what we currently have and we can think how to approach it.

Current implementation

when -maxGasPrice is not set

  1. Send transaction with parameters:
    • max priority fee per gas => value suggested by the network
    • max fee per gas => 2 * base fee + max priority fee per gas
  2. Wait 5 min for the transaction to get processed, if not then
  3. Send the replacement transaction with parameters:
    • max priority fee per gas => 1.11 * previous max priority fee per gas
    • max fee per gas => 1.11 * previous max fee per gas
  4. Repeat step 2

To me it looks reasonable, we try to gradually increase max fee per gas until the transaction is processed.

when -maxGasPrice is set

  1. Check if the current gas price is higher than max gas price set by user
    1.1. if it's higher, then do not send transaction at all
    1.2. if it's lower, then send the transaction with parameters:
    • max priority fee per gas => value suggested by the network
    • max fee per gas => 2 * base fee + max priority fee per gas
  2. Wait 5 min for the transaction to get processed, if not then
  3. Check if 1.11 * previous max priority fee per gas is higher than max gas price set by user
    3.1. if it's higher, then do not send the replacement transaction (just still wait for the initial transaction to get processed)
    3.2. if it's lower, then send the replacement transaction with parameters
    • max priority fee per gas => 1.11 * previous max priority fee per gas
    • max fee per gas => 1.11 * previous max fee per gas
  4. Repeat step 2

Issues

There are a few issue with the current approach, (already thanks to @0xVires):

  • Issue 1: The transaction may not be sent at all (point 1.1, if the value max gas price is currently too low)
  • Issue 2: User may pay more than max gas price (transaction passes point 1, but base fee increases in the meantime, initial problem in this GH issue)
  • Issue 3: Replacement transaction may not be sent (point 3.1), so our initial transaction may be pending (and therefore block all next transactions)

Proposed solution

I think that we should do the following:

  1. Always explicitly set max fee per gas to max gas price set by user => fixes issue 2
  2. Always send the transaction, even if the current price is higher than max gas price set by user => fixes issue 1

Concerning the replacement logic, I think we should leave it as it is. That will mean that it won't activate until a user explicitly changes max gas price to a value at least 10% higher (or disables max gas price at all). Otherwise, we would need to automatically increase the price to a value higher than max fee per gas which could make the user pay more than max fee per gas. That would mean that the issue 3 is not automatically addressed, but requires an interaction from the user.

@yondonfu @0xVires what are you thoughts?

@0xVires
Copy link
Author

0xVires commented Nov 25, 2021

Always explicitly set max fee per gas to max gas price set by user => fixes issue 2
Always send the transaction, even if the current price is higher than max gas price set by user => fixes issue 1

So as soon as the new round starts, the reward call would be submitted with the set -maxGasPrice? That would be nice and in that case we don't need the retry mechanism that I've suggested.
Only issue I see is that sometimes the user might need to manually increase the gas price (if the -maxGasPrice is set too low) or they're ending up with stuck txs.
Since this quite a change from the current function of the -maxGasPrice flag (from: "don't do anything unless the gas price is lower than the set value" to: "always send the tx"), it's important to make this clear in the new release. Users of this flag will need to know how to replace pending txs with higher priced txs.

Also how is the ticket redemption handled? Will it also be changed to "always send the tx with the set -maxGasPrice"?

@leszko
Copy link
Contributor

leszko commented Nov 25, 2021

Thanks for the comment @0xVires . I also had a similar discussion with @yondonfu.

As you noted, if we change the logic to always send the transaction, it's a big shift for the user, because then users would need to do manual interactions. So, we decided that it may not be such a good idea and maybe it's better to not send transactions that are highly probably to end up pending until some user interaction.

In that case, we could follow your idea and keep track of unsent transactions and try to resend them later (when the base fee changes or if users manually change maxGasPrice). To make the discussion and the implementation more manageable, I split it into two parts:

@leszko
Copy link
Contributor

leszko commented Jan 7, 2022

Reopening, because we had to revert the related PR #2111 in the revert PR #2163. This was caused by the bug #2152.

The problem was that the initial implementation was based on modifying the transaction in transactionManager:SendTranscation(), this in result caused a deadlock, because client.CheckTx() checked the hash of origin tx instead of the modified tx.

Another approach that we plan to implement instead is to use TransactOpts to set GasFeeCap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment