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

generalized fee estimation interface #8561

Merged
merged 14 commits into from
Mar 8, 2023
Merged

generalized fee estimation interface #8561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5edd3ba
combine EVM legacy + dynamic fee estimation behind a single interface
aalu1418 Feb 24, 2023
dd078be
fix components that use txm estimator + regenerate mocks
aalu1418 Feb 24, 2023
68f7dad
fix tests
aalu1418 Feb 27, 2023
ce243aa
old Estimator => EvmEstimator
aalu1418 Feb 28, 2023
e543b43
condense gas estimation logic in broadcaster and confirmer
aalu1418 Feb 28, 2023
e32bfec
test coverage for WrappedEvmEstimator
aalu1418 Feb 28, 2023
69ce9a2
generic FeeEstimator interface
aalu1418 Mar 1, 2023
afc7859
generic PriorAttempt interface
aalu1418 Mar 1, 2023
94c99d8
move generic fee estimator types to common/txmgr/types + small feedba…
aalu1418 Mar 7, 2023
839ee60
Merge branch 'develop' into generalized-fee-estimation-interface
aalu1418 Mar 7, 2023
21043ff
remove duplicate geth/common import
aalu1418 Mar 7, 2023
24e105d
Merge branch 'develop' into generalized-fee-estimation-interface
aalu1418 Mar 7, 2023
2f69803
Merge branch 'develop' into generalized-fee-estimation-interface
aalu1418 Mar 8, 2023
ced4a24
Merge branch 'develop' into generalized-fee-estimation-interface
aalu1418 Mar 8, 2023
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
32 changes: 32 additions & 0 deletions common/txmgr/types/fee_estimator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package types

import "context"

// Opt is an option for a gas estimator
type Opt int

const (
// OptForceRefetch forces the estimator to bust a cache if necessary
OptForceRefetch Opt = iota
)

// PriorAttempt provides a generic interface for reading tx data to be used in the fee esimators
type PriorAttempt[FEE any, HASH any] interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, I don't think there is any reason for the common TxMgr components to use PriorAttempt interface.
It already doesn't use it, except to pass it to Estimator.BumpGas().
I believe we could always just pass in previous Attempts in the form of a generic TxAttempt. And the chain specific code should know what to extract from it, to bump gas correctly.

Not for this PR, but for later, let us aim to completely do away with this interface, and just reuse TxAttempt.

If you want, create a separate subtask or ticket for this, for later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏼 I will take a look at this part when I'm working with the TxBuilder

Fee() FEE
GetChainSpecificGasLimit() uint32
GetBroadcastBeforeBlockNum() *int64
GetHash() HASH
GetTxType() int
}

// FeeEstimator provides a generic interface for fee estimation
//
//go:generate mockery --quiet --name FeeEstimator --output ./mocks/ --case=underscore
type FeeEstimator[HEAD any, FEE any, MAXPRICE any, HASH any] interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

The MAXPRICE actually is also never needed to be passed in to estimator by the generic txmgr.
The Txmgr just reads a static config, and passes it in. Ideally it should be fetched directly by the estimator.

I would say, create a sub-task or ticket, to remove it from generic interface here later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How would it be fetched directly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is used on a job-specific basis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would agree with Sam, I think it's still needed as it's a different config variable then the global chain configs and can be different for each tx

Copy link
Contributor

Choose a reason for hiding this comment

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

This value is a keySpecfic config currently. GasEstimator can directly fetch that from config, if it has the Key available..

Here is a place where TxMgr currently fetches it:

chainlink/core/chains/evm/txmgr/eth_confirmer.go

Line 769 in 6b4bcc5

keySpecificMaxGasPriceWei := ec.config.KeySpecificMaxGasPriceWei(etx.FromAddress)

Doing this just makes it clear that the interface between coreTxm <--> GasEstimator doesn't require this value to be passed in.
This value is a static config defined for each Key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but then each gas estimator would need knowledge of the FromAddress for each key. it seems simpler to just pass gas parameters instead of having the estimators have knowledge of additional configs + addresses

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be passed in. It is used in keepers I think, specifically (or used to be)

OnNewLongestChain(context.Context, HEAD)
Start(context.Context) error
Close() error

GetFee(ctx context.Context, calldata []byte, feeLimit uint32, maxFeePrice MAXPRICE, opts ...Opt) (fee FEE, chainSpecificFeeLimit uint32, err error)
BumpFee(ctx context.Context, originalFee FEE, feeLimit uint32, maxFeePrice MAXPRICE, attempts []PriorAttempt[FEE, HASH]) (bumpedFee FEE, chainSpecificFeeLimit uint32, err error)
}
132 changes: 132 additions & 0 deletions common/txmgr/types/mocks/fee_estimator.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions core/chainlink.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ RUN go mod download
# Env vars needed for chainlink build
ARG COMMIT_SHA

COPY common common
COPY core core
COPY operator_ui operator_ui

Expand Down
17 changes: 9 additions & 8 deletions core/chains/evm/gas/arbitrum_estimator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/pkg/errors"
"golang.org/x/exp/slices"

txmgrtypes "github.com/smartcontractkit/chainlink/common/txmgr/types"
"github.com/smartcontractkit/chainlink/core/assets"
evmclient "github.com/smartcontractkit/chainlink/core/chains/evm/client"
"github.com/smartcontractkit/chainlink/core/logger"
Expand All @@ -34,7 +35,7 @@ type arbitrumEstimator struct {

cfg ArbConfig

Estimator // *l2SuggestedPriceEstimator
EvmEstimator // *l2SuggestedPriceEstimator

client ethClient
pollPeriod time.Duration
Expand All @@ -50,11 +51,11 @@ type arbitrumEstimator struct {
chDone chan struct{}
}

func NewArbitrumEstimator(lggr logger.Logger, cfg ArbConfig, rpcClient rpcClient, ethClient ethClient) Estimator {
func NewArbitrumEstimator(lggr logger.Logger, cfg ArbConfig, rpcClient rpcClient, ethClient ethClient) EvmEstimator {
lggr = lggr.Named("ArbitrumEstimator")
return &arbitrumEstimator{
cfg: cfg,
Estimator: NewL2SuggestedPriceEstimator(lggr, rpcClient),
EvmEstimator: NewL2SuggestedPriceEstimator(lggr, rpcClient),
client: ethClient,
pollPeriod: 10 * time.Second,
logger: lggr,
Expand All @@ -67,7 +68,7 @@ func NewArbitrumEstimator(lggr logger.Logger, cfg ArbConfig, rpcClient rpcClient

func (a *arbitrumEstimator) Start(ctx context.Context) error {
return a.StartOnce("ArbitrumEstimator", func() error {
if err := a.Estimator.Start(ctx); err != nil {
if err := a.EvmEstimator.Start(ctx); err != nil {
return errors.Wrap(err, "failed to start gas price estimator")
}
go a.run()
Expand All @@ -78,7 +79,7 @@ func (a *arbitrumEstimator) Start(ctx context.Context) error {
func (a *arbitrumEstimator) Close() error {
return a.StopOnce("ArbitrumEstimator", func() (err error) {
close(a.chStop)
err = errors.Wrap(a.Estimator.Close(), "failed to stop gas price estimator")
err = errors.Wrap(a.EvmEstimator.Close(), "failed to stop gas price estimator")
<-a.chDone
return
})
Expand All @@ -89,14 +90,14 @@ func (a *arbitrumEstimator) Close() error {
// - Limit is computed from the dynamic values perL2Tx and perL1CalldataUnit, provided by the getPricesInArbGas() method
// of the precompilie contract at ArbGasInfoAddress. perL2Tx is a constant amount of gas, and perL1CalldataUnit is
// multiplied by the length of the tx calldata. The sum of these two values plus the original l2GasLimit is returned.
func (a *arbitrumEstimator) GetLegacyGas(ctx context.Context, calldata []byte, l2GasLimit uint32, maxGasPriceWei *assets.Wei, opts ...Opt) (gasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) {
gasPrice, _, err = a.Estimator.GetLegacyGas(ctx, calldata, l2GasLimit, maxGasPriceWei, opts...)
func (a *arbitrumEstimator) GetLegacyGas(ctx context.Context, calldata []byte, l2GasLimit uint32, maxGasPriceWei *assets.Wei, opts ...txmgrtypes.Opt) (gasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) {
gasPrice, _, err = a.EvmEstimator.GetLegacyGas(ctx, calldata, l2GasLimit, maxGasPriceWei, opts...)
if err != nil {
return
}
gasPrice = a.gasPriceWithBuffer(gasPrice, maxGasPriceWei)
ok := a.IfStarted(func() {
if slices.Contains(opts, OptForceRefetch) {
if slices.Contains(opts, txmgrtypes.OptForceRefetch) {
ch := make(chan struct{})
select {
case a.chForceRefetch <- ch:
Expand Down
13 changes: 7 additions & 6 deletions core/chains/evm/gas/block_history_estimator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"

txmgrtypes "github.com/smartcontractkit/chainlink/common/txmgr/types"
"github.com/smartcontractkit/chainlink/core/assets"
evmclient "github.com/smartcontractkit/chainlink/core/chains/evm/client"
evmtypes "github.com/smartcontractkit/chainlink/core/chains/evm/types"
Expand Down Expand Up @@ -74,7 +75,7 @@ const BumpingHaltedLabel = "Tx gas bumping halted since price exceeds current bl

var ErrConnectivity = errors.New("transaction propagation issue: transactions are not being mined")

var _ Estimator = &BlockHistoryEstimator{}
var _ EvmEstimator = &BlockHistoryEstimator{}

//go:generate mockery --quiet --name Config --output ./mocks/ --case=underscore
type (
Expand Down Expand Up @@ -107,7 +108,7 @@ type (
// NewBlockHistoryEstimator returns a new BlockHistoryEstimator that listens
// for new heads and updates the base gas price dynamically based on the
// configured percentile of gas prices in that block
func NewBlockHistoryEstimator(lggr logger.Logger, ethClient evmclient.Client, cfg Config, chainID big.Int) Estimator {
func NewBlockHistoryEstimator(lggr logger.Logger, ethClient evmclient.Client, cfg Config, chainID big.Int) EvmEstimator {
ctx, cancel := context.WithCancel(context.Background())
b := &BlockHistoryEstimator{
ethClient: ethClient,
Expand Down Expand Up @@ -225,7 +226,7 @@ func (b *BlockHistoryEstimator) HealthReport() map[string]error {
return map[string]error{b.Name(): b.Healthy()}
}

func (b *BlockHistoryEstimator) GetLegacyGas(_ context.Context, _ []byte, gasLimit uint32, maxGasPriceWei *assets.Wei, _ ...Opt) (gasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) {
func (b *BlockHistoryEstimator) GetLegacyGas(_ context.Context, _ []byte, gasLimit uint32, maxGasPriceWei *assets.Wei, _ ...txmgrtypes.Opt) (gasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) {
ok := b.IfStarted(func() {
chainSpecificGasLimit = applyMultiplier(gasLimit, b.config.EvmGasLimitMultiplier())
gasPrice = b.getGasPrice()
Expand Down Expand Up @@ -264,7 +265,7 @@ func (b *BlockHistoryEstimator) getTipCap() *assets.Wei {
return b.tipCap
}

func (b *BlockHistoryEstimator) BumpLegacyGas(_ context.Context, originalGasPrice *assets.Wei, gasLimit uint32, maxGasPriceWei *assets.Wei, attempts []PriorAttempt) (bumpedGasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) {
func (b *BlockHistoryEstimator) BumpLegacyGas(_ context.Context, originalGasPrice *assets.Wei, gasLimit uint32, maxGasPriceWei *assets.Wei, attempts []EvmPriorAttempt) (bumpedGasPrice *assets.Wei, chainSpecificGasLimit uint32, err error) {
if b.config.BlockHistoryEstimatorCheckInclusionBlocks() > 0 {
if err = b.checkConnectivity(attempts); err != nil {
if errors.Is(err, ErrConnectivity) {
Expand All @@ -280,7 +281,7 @@ func (b *BlockHistoryEstimator) BumpLegacyGas(_ context.Context, originalGasPric
// checkConnectivity detects if the transaction is not being included due to
// some kind of mempool propagation or connectivity issue rather than
// insufficiently high pricing and returns error if so
func (b *BlockHistoryEstimator) checkConnectivity(attempts []PriorAttempt) error {
func (b *BlockHistoryEstimator) checkConnectivity(attempts []EvmPriorAttempt) error {
percentile := int(b.config.BlockHistoryEstimatorCheckInclusionPercentile())
// how many blocks since broadcast?
latestBlockNum := b.getCurrentBlockNum()
Expand Down Expand Up @@ -438,7 +439,7 @@ func calcFeeCap(latestAvailableBaseFeePerGas *assets.Wei, cfg Config, tipCap *as
return feeCap
}

func (b *BlockHistoryEstimator) BumpDynamicFee(_ context.Context, originalFee DynamicFee, originalGasLimit uint32, maxGasPriceWei *assets.Wei, attempts []PriorAttempt) (bumped DynamicFee, chainSpecificGasLimit uint32, err error) {
func (b *BlockHistoryEstimator) BumpDynamicFee(_ context.Context, originalFee DynamicFee, originalGasLimit uint32, maxGasPriceWei *assets.Wei, attempts []EvmPriorAttempt) (bumped DynamicFee, chainSpecificGasLimit uint32, err error) {
if b.config.BlockHistoryEstimatorCheckInclusionBlocks() > 0 {
if err = b.checkConnectivity(attempts); err != nil {
if errors.Is(err, ErrConnectivity) {
Expand Down
Loading