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.

Sign up for GitHub

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

eth: Ensure maxFeePerGas in all transactions never exceed -masGasPrice defined by the user #2111

Merged
merged 5 commits into from
Dec 7, 2021
Merged

eth: Ensure maxFeePerGas in all transactions never exceed -masGasPrice defined by the user #2111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d7488f2
eth: Make maxFeePerGas in eth transactions never exceed maxGasPrice d…
leszko Nov 19, 2021
fa6d530
Update CHANGELOG_PENDING
leszko Nov 25, 2021
04bc0ff
eth: Remove logging error when signing the transaction
leszko Nov 30, 2021
ff84dec
doc: Update documentation about `maxGasPrice`
leszko Dec 1, 2021
c5ab81b
doc: Update documentation about `maxGasPrice`
leszko Dec 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG_PENDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#### General

- \#2114 Add option to repeat the benchmarking process (@jailuthra)
- \#2111 Ensure `maxFeePerGas` in all transactions never exceed `-masGasPrice` defined by the user (@leszko)

#### Broadcaster

Expand Down
6 changes: 5 additions & 1 deletion doc/ethereum.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,16 @@ After the EIP-1559 upgrade on Ethereum, the node treats the gas price as priorit

### Max gas price

The `maxGasPrice` parameter makes sure the transaction fee never exceeds the specified limit.
- If the current network gas price is higher than `maxGasPrice`, the transaction is not sent
- The transaction parameter `maxFeePerGas` is set to `maxGasPrice`

The following options can be used to get the max gas price:

- `curl localhost:7935/maxGasPrice`
- Run `livepeer_cli` and observe the max gas price in the node stats

The following options can be used to set the m gas price to `<MAX_GAS_PRICE>`, a Wei denominated value:
The following options can be used to set the max gas price to `<MAX_GAS_PRICE>`, a Wei denominated value:

- Start the node with `-maxGasPrice <MAX_GAS_PRICE>`
- `curl localhost:7935/setMaxGasPrice?maxGasPrice=<MAX_GAS_PRICE>`
Expand Down
2 changes: 1 addition & 1 deletion eth/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func TestSendTransaction_SendErr_DontUpdateNonce(t *testing.T) {
}, 1*time.Second, big.NewInt(0), nil)
gpm.gasPrice = big.NewInt(1)

tm := NewTransactionManager(client, gpm, &accountManager{}, 3*time.Second, 0)
tm := NewTransactionManager(client, gpm, &stubTransactionSigner{}, 3*time.Second, 0)

bi := NewBackend(client, signer, gpm, tm)

Expand Down
51 changes: 32 additions & 19 deletions eth/transactionManager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,15 @@ func NewTransactionManager(eth transactionSenderReader, gpm *GasPriceMonitor, si
}

func (tm *TransactionManager) SendTransaction(ctx context.Context, tx *types.Transaction) error {
sendErr := tm.eth.SendTransaction(ctx, tx)
adjustedRawTx := tm.newAdjustedTx(tx)
adjustedTx, err := tm.sig.SignTx(adjustedRawTx)
if err != nil {
return err
}

txLog, err := newTxLog(tx)
sendErr := tm.eth.SendTransaction(ctx, adjustedTx)

txLog, err := newTxLog(adjustedTx)
if err != nil {
txLog.method = "unknown"
}
Expand All @@ -112,11 +118,11 @@ func (tm *TransactionManager) SendTransaction(ctx context.Context, tx *types.Tra

// Add transaction to queue
tm.cond.L.Lock()
tm.queue.add(tx)
tm.queue.add(adjustedTx)
tm.cond.L.Unlock()
tm.cond.Signal()

glog.Infof("\n%vEth Transaction%v\n\nInvoking transaction: \"%v\". Inputs: \"%v\" Hash: \"%v\". \n\n%v\n", strings.Repeat("*", 30), strings.Repeat("*", 30), txLog.method, txLog.inputs, tx.Hash().String(), strings.Repeat("*", 75))
glog.Infof("\n%vEth Transaction%v\n\nInvoking transaction: \"%v\". Inputs: \"%v\" Hash: \"%v\". \n\n%v\n", strings.Repeat("*", 30), strings.Repeat("*", 30), txLog.method, txLog.inputs, adjustedTx.Hash().String(), strings.Repeat("*", 75))

return nil
}
Expand Down Expand Up @@ -159,7 +165,7 @@ func (tm *TransactionManager) replace(tx *types.Transaction) (*types.Transaction

// Bump gas price exceeds max gas price, return early
max := tm.gpm.MaxGasPrice()
newGasPrice := calcGasPrice(newRawTx)
newGasPrice := newRawTx.GasFeeCap()
if max != nil && newGasPrice.Cmp(max) > 0 {
return nil, fmt.Errorf("replacement gas price exceeds max gas price suggested=%v max=%v", newGasPrice, max)
}
Expand Down Expand Up @@ -234,30 +240,37 @@ func (tm *TransactionManager) checkTxLoop() {
}
}

func (tm *TransactionManager) newAdjustedTx(tx *types.Transaction) *types.Transaction {
baseTx := newDynamicFeeTx(tx)
if tm.gpm.MaxGasPrice() != nil {
baseTx.GasFeeCap = tm.gpm.MaxGasPrice()
Copy link
Member

Choose a reason for hiding this comment

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

I forget if we talked about this, but is there a reason for always setting this to the max gas price as opposed to only setting it to the max gas price if the max gas price exceeds the current value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both approaches will work. However, IMO the always setting max gas price is slightly better because:

  1. It's simpler for the user what happens.
  2. Since the replacement transaction needs to be at least 10% higher, we may end up never reaching exactly maxGasPrice.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable. But, this also means that a replacement tx would never be sent if the user sets a maxGasPrice because GasFeeCap is always set to maxGasPrice meaning that we are never able to increase GasFeeCap for a replacement tx. And the only scenario where a replacement tx could be sent is if the user does not set a maxGasPrice. I think this is okay, but just want to make sure we're on the same page about the expected behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. Actually, there is one more scenario for the replacement if the user manually increases maxGasPrice while the transaction is pending. Then the replacement can also happen.

}

return types.NewTx(baseTx)
}

func applyPriceBump(val *big.Int, priceBump uint64) *big.Int {
a := big.NewInt(100 + int64(priceBump))
b := new(big.Int).Mul(a, val)
return b.Div(b, big.NewInt(100))
}

// Calculate the gas price as gas tip cap + base fee
func calcGasPrice(tx *types.Transaction) *big.Int {
// Assume that the gas fee cap is calculated as gas tip cap + (baseFee * 2)
baseFee := new(big.Int).Div(new(big.Int).Sub(tx.GasFeeCap(), tx.GasTipCap()), big.NewInt(2))
return new(big.Int).Add(baseFee, tx.GasTipCap())
func newReplacementTx(tx *types.Transaction) *types.Transaction {
baseTx := newDynamicFeeTx(tx)
baseTx.GasFeeCap = applyPriceBump(tx.GasFeeCap(), priceBump)
baseTx.GasTipCap = applyPriceBump(tx.GasTipCap(), priceBump)

return types.NewTx(baseTx)
}

func newReplacementTx(tx *types.Transaction) *types.Transaction {
baseTx := &types.DynamicFeeTx{
Nonce: tx.Nonce(),
// geth requires the price bump to be applied to both the gas tip cap and gas fee cap
GasFeeCap: applyPriceBump(tx.GasFeeCap(), priceBump),
GasTipCap: applyPriceBump(tx.GasTipCap(), priceBump),
func newDynamicFeeTx(tx *types.Transaction) *types.DynamicFeeTx {
return &types.DynamicFeeTx{
To: tx.To(),
Nonce: tx.Nonce(),
GasFeeCap: tx.GasFeeCap(),
GasTipCap: tx.GasTipCap(),
Gas: tx.Gas(),
Value: tx.Value(),
Data: tx.Data(),
To: tx.To(),
}

return types.NewTx(baseTx)
}
67 changes: 46 additions & 21 deletions eth/transactionManager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func (sig *stubTransactionSigner) SignTx(tx *types.Transaction) (*types.Transact

func TestTxQueue(t *testing.T) {
assert := assert.New(t)
tx := types.NewTransaction(1, pm.RandAddress(), big.NewInt(100), 1, big.NewInt(100), pm.RandBytes(32))
tx := newStubDynamicTx()
q := transactionQueue{}
q.add(tx)
assert.Len(q, 1)
Expand All @@ -79,13 +79,15 @@ func TestTransactionManager_SendTransaction(t *testing.T) {
cond: sync.NewCond(&sync.Mutex{}),
eth: eth,
queue: q,
sig: &stubTransactionSigner{},
gpm: &GasPriceMonitor{},
}

// Test error
expErr := errors.New("SendTransaction error")
eth.err["SendTransaction"] = expErr

tx := types.NewTransaction(1, pm.RandAddress(), big.NewInt(100), 100000, big.NewInt(100), pm.RandBytes(68))
tx := newStubDynamicTx()

errLogsBefore := glog.Stats.Info.Lines()
assert.EqualError(
Expand Down Expand Up @@ -131,7 +133,7 @@ func TestTransactionManager_Wait(t *testing.T) {
expErr := errors.New("context deadline exceeded")
eth.err["TransactionByHash"] = expErr

tx := types.NewTransaction(1, pm.RandAddress(), big.NewInt(100), 100000, big.NewInt(100), pm.RandBytes(68))
tx := newStubDynamicTx()

receipt, err := tm.wait(tx)
assert.Nil(receipt)
Expand Down Expand Up @@ -193,9 +195,10 @@ func TestTransactionManager_Replace(t *testing.T) {
eth.err["TransactionByHash"] = ethereum.NotFound
tx, err = tm.replace(stubTx)
assert.Nil(tx)

assert.EqualError(
err,
fmt.Sprintf("replacement gas price exceeds max gas price suggested=%v max=%v", applyPriceBump(calcGasPrice(stubTx), priceBump), gpm.maxGasPrice),
fmt.Sprintf("replacement gas price exceeds max gas price suggested=%v max=%v", applyPriceBump(stubTx.GasFeeCap(), priceBump), gpm.maxGasPrice),
)
eth.err["TransactionByHash"] = nil

Expand All @@ -205,7 +208,7 @@ func TestTransactionManager_Replace(t *testing.T) {
assert.Nil(tx)
assert.EqualError(
err,
fmt.Sprintf("replacement gas price exceeds max gas price suggested=%v max=%v", applyPriceBump(calcGasPrice(stubTx), priceBump), gpm.maxGasPrice),
fmt.Sprintf("replacement gas price exceeds max gas price suggested=%v max=%v", applyPriceBump(stubTx.GasFeeCap(), priceBump), gpm.maxGasPrice),
)

// Error signing replacement tx
Expand Down Expand Up @@ -254,10 +257,8 @@ func TestTransactionManager_CheckTxLoop(t *testing.T) {
err: make(map[string]error),
}
q := transactionQueue{}
gasPrice := big.NewInt(10)
gpm := &GasPriceMonitor{
minGasPrice: big.NewInt(0),
maxGasPrice: big.NewInt(99999999),
gasPrice: big.NewInt(1),
}
sig := &stubTransactionSigner{
Expand All @@ -279,7 +280,7 @@ func TestTransactionManager_CheckTxLoop(t *testing.T) {
receipt := types.NewReceipt(pm.RandHash().Bytes(), false, 100000)
eth.receipt = receipt

stubTx := types.NewTransaction(1, pm.RandAddress(), big.NewInt(100), 100000, gasPrice, pm.RandBytes(68))
stubTx := newStubDynamicTx()

go tm.Start()
defer tm.Stop()
Expand Down Expand Up @@ -385,18 +386,6 @@ func TestApplyPriceBump(t *testing.T) {
assert.Equal(big.NewInt(55), res)
}

func TestCalcGasPrice(t *testing.T) {
assert := assert.New(t)

baseFee := big.NewInt(1000)
gasTipCap := big.NewInt(100)
gasFeeCap := new(big.Int).Add(gasTipCap, new(big.Int).Mul(baseFee, big.NewInt(2)))
tx := newStubDynamicFeeTx(gasFeeCap, gasTipCap)

gasPrice := calcGasPrice(tx)
assert.Equal(new(big.Int).Add(baseFee, gasTipCap), gasPrice)
}

func TestNewReplacementTx(t *testing.T) {
assert := assert.New(t)

Expand All @@ -405,15 +394,51 @@ func TestNewReplacementTx(t *testing.T) {

tx1 := newStubDynamicFeeTx(gasFeeCap, gasTipCap)
tx2 := newReplacementTx(tx1)
assert.NotEqual(tx1.Hash(), tx2.Hash())
assert.Equal(applyPriceBump(tx1.GasTipCap(), priceBump), tx2.GasTipCap())
assert.Equal(applyPriceBump(tx1.GasFeeCap(), priceBump), tx2.GasFeeCap())
assert.NotEqual(tx1.Hash(), tx2.Hash())
assertTxFieldsUnchanged(t, tx1, tx2)
}

func assertTxFieldsUnchanged(t *testing.T, tx1 *types.Transaction, tx2 *types.Transaction) {
assert := assert.New(t)

assert.Equal(tx1.Nonce(), tx2.Nonce())
assert.Equal(tx1.Gas(), tx2.Gas())
assert.Equal(tx1.Value(), tx1.Value())
assert.Equal(tx1.To(), tx2.To())
}

func TestNewAdjustedTx(t *testing.T) {
assert := assert.New(t)

gasTipCap := big.NewInt(100)
gasFeeCap := big.NewInt(1000)

tm := &TransactionManager{gpm: &GasPriceMonitor{}}
tx1 := newStubDynamicFeeTx(gasFeeCap, gasTipCap)

// Gas Price Monitor with no maxGasPrice
tx2 := tm.newAdjustedTx(tx1)
assert.Equal(tx1.GasFeeCap(), tx2.GasFeeCap())
assert.Equal(tx1.GasTipCap(), tx2.GasTipCap())
assert.Equal(tx1.Hash(), tx2.Hash())
assertTxFieldsUnchanged(t, tx1, tx2)

// maxGasPrice set in Gas Price Monitor
maxGasFee := big.NewInt(1100)
tm.gpm.maxGasPrice = maxGasFee
tx2 = tm.newAdjustedTx(tx1)
assert.Equal(maxGasFee, tx2.GasFeeCap())
assert.Equal(tx1.GasTipCap(), tx2.GasTipCap())
assert.NotEqual(tx1.Hash(), tx2.Hash())
assertTxFieldsUnchanged(t, tx1, tx2)
}

func newStubDynamicTx() *types.Transaction {
return newStubDynamicFeeTx(big.NewInt(100), big.NewInt(1000))
}

func newStubDynamicFeeTx(gasFeeCap, gasTipCap *big.Int) *types.Transaction {
addr := pm.RandAddress()
return types.NewTx(&types.DynamicFeeTx{
Expand Down