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

Remove PriceMin check from attempt builder #14370

Merged
merged 4 commits into from
Sep 12, 2024
Merged

Remove PriceMin check from attempt builder #14370

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dc7fa13
Remove PriceMin check from attempt builder
dimriou Sep 9, 2024
69885bd
Remove TipCapMin
dimriou Sep 9, 2024
3917bee
Merge branch 'develop' into remove_min_price_validation
dimriou Sep 9, 2024
73d7881
Update changeset
dimriou Sep 10, 2024
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
5 changes: 5 additions & 0 deletions .changeset/warm-experts-promise.md
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading