Skip to content

Commit

Permalink
Remove PriceMin check from attempt builder (#14370)
Browse files Browse the repository at this point in the history
* Remove PriceMin check from attempt builder

* Remove TipCapMin

* Update changeset
  • Loading branch information
dimriou authored Sep 12, 2024
1 parent f6443a1 commit 882cdce
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 36 deletions.
5 changes: 5 additions & 0 deletions .changeset/warm-experts-promise.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"chainlink": minor
---

Remove PriceMin and TipCapMin check from attempt builder #internal
19 changes: 4 additions & 15 deletions core/chains/evm/txmgr/attempts.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ type evmTxAttemptBuilder struct {

type evmTxAttemptBuilderFeeConfig interface {
EIP1559DynamicFees() bool
TipCapMin() *assets.Wei
PriceMin() *assets.Wei
PriceMaxKey(common.Address) *assets.Wei
LimitDefault() uint64
}
Expand Down Expand Up @@ -172,7 +170,7 @@ func (c *evmTxAttemptBuilder) NewEmptyTxAttempt(ctx context.Context, nonce evmty
}

func (c *evmTxAttemptBuilder) newDynamicFeeAttempt(ctx context.Context, etx Tx, fee gas.DynamicFee, gasLimit uint64) (attempt TxAttempt, err error) {
if err = validateDynamicFeeGas(c.feeConfig, c.feeConfig.TipCapMin(), fee, etx); err != nil {
if err = validateDynamicFeeGas(c.feeConfig, fee, etx); err != nil {
return attempt, pkgerrors.Wrap(err, "error validating gas")
}

Expand Down Expand Up @@ -208,7 +206,7 @@ type keySpecificEstimator interface {

// validateDynamicFeeGas is a sanity check - we have other checks elsewhere, but this
// makes sure we _never_ create an invalid attempt
func validateDynamicFeeGas(kse keySpecificEstimator, tipCapMinimum *assets.Wei, fee gas.DynamicFee, etx Tx) error {
func validateDynamicFeeGas(kse keySpecificEstimator, fee gas.DynamicFee, etx Tx) error {
gasTipCap, gasFeeCap := fee.TipCap, fee.FeeCap

if gasTipCap == nil {
Expand All @@ -235,11 +233,6 @@ func validateDynamicFeeGas(kse keySpecificEstimator, tipCapMinimum *assets.Wei,
if gasFeeCap.Cmp(max) > 0 {
return pkgerrors.Errorf("cannot create tx attempt: specified gas fee cap of %s would exceed max configured gas price of %s for key %s", gasFeeCap.String(), max.String(), etx.FromAddress.String())
}
// Tip must be above minimum
minTip := tipCapMinimum
if gasTipCap.Cmp(minTip) < 0 {
return pkgerrors.Errorf("cannot create tx attempt: specified gas tip cap of %s is below min configured gas tip of %s for key %s", gasTipCap.String(), minTip.String(), etx.FromAddress.String())
}
return nil
}

Expand All @@ -257,7 +250,7 @@ func newDynamicFeeTransaction(nonce uint64, to common.Address, value *big.Int, g
}

func (c *evmTxAttemptBuilder) newLegacyAttempt(ctx context.Context, etx Tx, gasPrice *assets.Wei, gasLimit uint64) (attempt TxAttempt, err error) {
if err = validateLegacyGas(c.feeConfig, c.feeConfig.PriceMin(), gasPrice, etx); err != nil {
if err = validateLegacyGas(c.feeConfig, gasPrice, etx); err != nil {
return attempt, pkgerrors.Wrap(err, "error validating gas")
}

Expand Down Expand Up @@ -290,18 +283,14 @@ func (c *evmTxAttemptBuilder) newLegacyAttempt(ctx context.Context, etx Tx, gasP

// validateLegacyGas is a sanity check - we have other checks elsewhere, but this
// makes sure we _never_ create an invalid attempt
func validateLegacyGas(kse keySpecificEstimator, minGasPriceWei, gasPrice *assets.Wei, etx Tx) error {
func validateLegacyGas(kse keySpecificEstimator, gasPrice *assets.Wei, etx Tx) error {
if gasPrice == nil {
panic("gas price missing")
}
max := kse.PriceMaxKey(etx.FromAddress)
if gasPrice.Cmp(max) > 0 {
return pkgerrors.Errorf("cannot create tx attempt: specified gas price of %s would exceed max configured gas price of %s for key %s", gasPrice.String(), max.String(), etx.FromAddress.String())
}
min := minGasPriceWei
if gasPrice.Cmp(min) < 0 {
return pkgerrors.Errorf("cannot create tx attempt: specified gas price of %s is below min configured gas price of %s for key %s", gasPrice.String(), min.String(), etx.FromAddress.String())
}
return nil
}

Expand Down
3 changes: 0 additions & 3 deletions core/chains/evm/txmgr/attempts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,9 +171,6 @@ func TestTxm_NewDynamicFeeTx(t *testing.T) {
{"ignores global min gas price", assets.GWei(5), assets.GWei(5), func(c *toml.EVMConfig) {
c.GasEstimator.PriceMin = assets.GWei(6)
}, ""},
{"tip cap below min allowed", assets.GWei(5), assets.GWei(5), func(c *toml.EVMConfig) {
c.GasEstimator.TipCapMin = assets.GWei(6)
}, "specified gas tip cap of 5 gwei is below min configured gas tip of 6 gwei"},
}

for _, tt := range cases {
Expand Down
22 changes: 4 additions & 18 deletions core/chains/evm/txmgr/broadcaster_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1620,31 +1620,17 @@ func TestEthBroadcaster_ProcessUnstartedEthTxs_Errors(t *testing.T) {
// configured for the transaction pool.
// This is a configuration error by the node operator, since it means they set the base gas level too low.
underpricedError := "transaction underpriced"
localNextNonce := getLocalNextNonce(t, nonceTracker, fromAddress)
mustCreateUnstartedTx(t, txStore, fromAddress, toAddress, encodedPayload, gasLimit, value, testutils.FixtureChainID)

// Check gas tip cap verification
evmcfg2 := evmtest.NewChainScopedConfig(t, configtest.NewGeneralConfig(t, func(c *chainlink.Config, s *chainlink.Secrets) {
c.EVM[0].GasEstimator.EIP1559DynamicFees = ptr(true)
c.EVM[0].GasEstimator.TipCapDefault = assets.NewWeiI(0)
}))
ethClient.On("PendingNonceAt", mock.Anything, fromAddress).Return(localNextNonce, nil).Once()
eb2 := NewTestEthBroadcaster(t, txStore, ethClient, ethKeyStore, cfg, evmcfg2, &testCheckerFactory{}, false, nonceTracker)

retryable, err := eb2.ProcessUnstartedTxs(ctx, fromAddress)
require.Error(t, err)
require.Contains(t, err.Error(), "specified gas tip cap of 0 is below min configured gas tip of 1 wei for key")
assert.True(t, retryable)

gasTipCapDefault := assets.NewWeiI(42)

evmcfg2 = evmtest.NewChainScopedConfig(t, configtest.NewGeneralConfig(t, func(c *chainlink.Config, s *chainlink.Secrets) {
evmcfg2 := evmtest.NewChainScopedConfig(t, configtest.NewGeneralConfig(t, func(c *chainlink.Config, s *chainlink.Secrets) {
c.EVM[0].GasEstimator.EIP1559DynamicFees = ptr(true)
c.EVM[0].GasEstimator.TipCapDefault = gasTipCapDefault
}))
localNextNonce = getLocalNextNonce(t, nonceTracker, fromAddress)
localNextNonce := getLocalNextNonce(t, nonceTracker, fromAddress)
ethClient.On("PendingNonceAt", mock.Anything, fromAddress).Return(localNextNonce, nil).Once()
eb2 = NewTestEthBroadcaster(t, txStore, ethClient, ethKeyStore, cfg, evmcfg2, &testCheckerFactory{}, false, nonceTracker)
eb2 := NewTestEthBroadcaster(t, txStore, ethClient, ethKeyStore, cfg, evmcfg2, &testCheckerFactory{}, false, nonceTracker)

// Second was underpriced but above minimum
ethClient.On("SendTransactionReturnCode", mock.Anything, mock.MatchedBy(func(tx *gethTypes.Transaction) bool {
Expand All @@ -1659,7 +1645,7 @@ func TestEthBroadcaster_ProcessUnstartedEthTxs_Errors(t *testing.T) {
return tx.Nonce() == localNextNonce && tx.GasTipCap().Cmp(big.NewInt(0).Add(gasTipCapDefault.ToInt(), big.NewInt(0).Mul(evmcfg2.EVM().GasEstimator().BumpMin().ToInt(), big.NewInt(2)))) == 0
}), fromAddress).Return(commonclient.Successful, nil).Once()

retryable, err = eb2.ProcessUnstartedTxs(ctx, fromAddress)
retryable, err := eb2.ProcessUnstartedTxs(ctx, fromAddress)
require.NoError(t, err)
assert.False(t, retryable)

Expand Down

0 comments on commit 882cdce

Please sign in to comment.