feat(state/core_accessor): add fee estimator #4087

vgonkivs · 2025-02-04T14:59:32Z

Implemenation of the ADR-13
This PR enables third-party estimation for the submit transactions in the node(besides PFB). By default, fee estimation relies on the consensus node to which the node is connected. But the user can now provide a non-default endpoint, so the node will query gas price and gas from it. It can be provided via cli(using "core.estimator.address"). The address will be added to the node's config as FeeEstimatorAddress.
Additionally, the user can now provide a maxGasPrice for every submit tx(via max.gas.price flag)- the max price that the user is willing to pay for the transaction. The transaction will not be submitted in case when estimated gas price exceeds the maxGasPrice(default: minGasPrice*100)

codecov-commenter · 2025-02-13T16:04:13Z

Codecov Report

Attention: Patch coverage is 68.23529% with 54 lines in your changes missing coverage. Please review.

Project coverage is 44.97%. Comparing base (2469e7a) to head (539f554).
Report is 451 commits behind head on main.

Files with missing lines	Patch %	Lines
state/estimator.go	66.66%	19 Missing and 6 partials ⚠️
state/tx_config.go	54.83%	14 Missing ⚠️
state/core_access.go	55.00%	6 Missing and 3 partials ⚠️
nodebuilder/core/flags.go	66.66%	3 Missing and 1 partial ⚠️
nodebuilder/state/cmd/state.go	90.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4087      +/-   ##
==========================================
+ Coverage   44.83%   44.97%   +0.14%     
==========================================
  Files         265      310      +45     
  Lines       14620    22834    +8214     
==========================================
+ Hits         6555    10270    +3715     
- Misses       7313    11472    +4159     
- Partials      752     1092     +340

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

go.mod

renaynay

The biggest comment rn is just that core_access is super messy 😭 Quite hard to read. Is there anything that can be done to make the code more readable?

renaynay · 2025-03-03T14:08:54Z

libs/utils/address.go

-func SanitizeAddr(addr string) (string, error) {
-	original := addr
+// NormalizeAddress extracts the host and port, removing unsupported schemes.
+func NormalizeAddress(addr string) string {


why does SanitizeAddr not work for this?

it additionally cuts the port.
addr = strings.Split(addr, ":")[0]

renaynay · 2025-03-03T14:15:41Z

nodebuilder/state/core.go

@@ -23,13 +24,14 @@ func coreAccessor(
 	fraudServ libfraud.Service[*header.ExtendedHeader],
 	network p2p.Network,
 	client *grpc.ClientConn,
+	address core.EstimatorAddress,


can't you just grab the core cfg there and take from cfg directly instead of supplying individual value to fx?

We are using core.EstimatorAddress only from the core config:

type Config struct { IP string Port string TLSEnabled bool XTokenPath string FeeEstimatorAddress EstimatorAddress }

renaynay · 2025-03-03T14:17:28Z

state/core_access.go

-
-	log = logging.Logger("state")
+	ErrInvalidAmount        = errors.New("state: amount must be greater than zero")
+	errGasPriceExceedsLimit = errors.New("state: estimated gasPrice exceeds max gasPrice")


why unexported?

Made exported

renaynay · 2025-03-03T14:25:07Z

state/estimator.go

+	if e.estimatorConn != nil && e.estimatorConn.GetState() != connectivity.Shutdown {
+		gasPrice, gas, err := signer.QueryGasUsedAndPrice(ctx, e.estimatorConn, priority.toApp(), rawTx)
+		if err == nil {
+			return gasPrice, gas, nil
+		}
+		log.Warn("failed to query gas used and price from the estimator endpoint.", "err", err)
+	}
+
+	if e.defaultClientConn == nil || e.defaultClientConn.GetState() == connectivity.Shutdown {
+		return 0, 0, errors.New("connection with the consensus node is dropped")
+	}
+
+	gasPrice, gas, err := signer.QueryGasUsedAndPrice(ctx, e.defaultClientConn, priority.toApp(), rawTx)
+	if err != nil {
+		log.Warn("state: failed to query gas used and price from the default endpoint", "err", err)
+	}


you can unify these to clean it up and just determine which conn to used based on if estimator conn exists.

What err will be returned if you pass a conn where connectivity shutdown to the signer.QueryGasUsedAndPrice? If it's readable (conn is closed) then we cna just return that unwrapped

renaynay · 2025-03-05T12:41:24Z

state/core_access.go

@@ -136,6 +139,7 @@ func (ca *CoreAccessor) Start(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("querying minimum gas price: %w", err)
 	}
+	ca.estimator.connect()


Start function could use some cleanup (better organisation w.r.t spacing + order)

Wondertan · 2025-03-06T17:38:59Z

Did we tested this manually? Like run a node with an estimator submitting a real blob?

vgonkivs · 2025-03-06T20:02:06Z

Submit blobs is using a linear formula to calculate gas. I tested it with Transfer

Wondertan · 2025-03-06T20:31:23Z

Oh, i see, didnt know that, but you made it with Transfer - to confirm?

vgonkivs · 2025-03-07T13:01:18Z

It worked fine last month when I was testing it iirc. I'm getting an error from the estimation endpoint now. Trying to clarify with the app team.

Wondertan · 2025-03-07T13:12:24Z

Ok, let's retest it.

BTW, are we sure that estimation is only for Transfers and not blobs? Seems weird that we make this issue a priority if its only for transfers

vgonkivs · 2025-03-07T13:22:13Z

BTW, are we sure that estimation is only for Transfers and not blobs?

Yes, as per ADR blobs should be estimated using formula. All other types of the Txs have to use estimation service

vgonkivs · 2025-03-07T14:12:52Z

Tested on Trasfer transaction with numia-mocha public endpoint.

Wondertan

Skipping review of semantical nits.
If it works - it works

cristaloleg · 2025-03-10T14:49:29Z

state/core_access.go

+	err = ca.estimator.Start(ctx)
+	if err != nil {
+		log.Warn("state: failed to connect to estimator endpoint", "err", err)
+	}


should we return err here? if no - comment why it's ok to skip it.

we should actually return an error here bc the only time err is returned from estimator.Start is if grpc conn fails to be created which is an actual err.

state/estimator_test.go

state/tx_config.go

renaynay · 2025-03-12T13:29:04Z

state/core_access.go

+	err = ca.estimator.Start(ctx)
+	if err != nil {
+		log.Warn("state: failed to connect to estimator endpoint", "err", err)
+	}


we should actually return an error here bc the only time err is returned from estimator.Start is if grpc conn fails to be created which is an actual err.

renaynay · 2025-03-12T13:35:30Z

state/estimator.go

+	msg sdktypes.Msg,
+) (float64, uint64, error) {
+	if cfg.GasPrice() != DefaultGasPrice && cfg.GasLimit() != 0 {
+		return cfg.GasPrice(), cfg.GasLimit(), nil


so if both GasPrice and GasLimit are set in the config, GasPrice sanity check (checking if it exceeds the MaxGasPrice) can be bypassed? Is the idea here that if the user sets gas themselves that they wouldn't set a max gas price, and if they set a max gas price, that means they probably didn't specify gas and wants the node to calculate for them (within some upper bound)?

We should document this behaviour here

comments throughout this function would be nice.

Done. Not sure about "comments throughout this function", I don't see a good reason for that, added Go-doc only.

renaynay · 2025-03-12T13:50:43Z

state/estimator.go

+	priority TxPriority,
+	rawTx []byte,
+) (gasPrice float64, gas uint64, err error) {
+	conns := []*grpc.ClientConn{e.estimatorConn, e.defaultClientConn}


So the idea here is that there will /can be an estimator service that will perform this function, but if not, the default validator you're using to submit txs will also be able to estimate.

From what I understand your comment is just informational, nothing to change, right?

cristaloleg · 2025-03-12T14:18:41Z

Closing in favour of #4168

Thank you @vgonkivs for your time and effort 🫡

vgonkivs added area:state Related to fetching state and state execution kind:feat Attached to feature PRs labels Feb 4, 2025

vgonkivs self-assigned this Feb 4, 2025

vgonkivs force-pushed the fee_estimator branch 2 times, most recently from 90f851c to 7c01a6f Compare February 4, 2025 15:07

vgonkivs force-pushed the fee_estimator branch from 7c01a6f to 56afde4 Compare February 13, 2025 14:56

vgonkivs marked this pull request as ready for review February 13, 2025 16:06

vgonkivs requested review from renaynay, Wondertan, walldiss and cristaloleg as code owners February 13, 2025 16:06

cristaloleg reviewed Feb 13, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

renaynay reviewed Mar 5, 2025

View reviewed changes

vgonkivs added 4 commits March 6, 2025 15:53

feat: add fee estimator

744b7f9

fix test

0e63366

fix go mod

a5ecdaa

refactoring

539f554

vgonkivs force-pushed the fee_estimator branch from 6fc589d to 539f554 Compare March 6, 2025 16:38

vgonkivs requested review from cristaloleg and renaynay March 6, 2025 17:12

vgonkivs requested a review from cmwaters March 7, 2025 13:22

Wondertan approved these changes Mar 10, 2025

View reviewed changes

cristaloleg reviewed Mar 10, 2025

View reviewed changes

state/estimator_test.go Show resolved Hide resolved

cristaloleg reviewed Mar 10, 2025

View reviewed changes

state/tx_config.go Show resolved Hide resolved

renaynay reviewed Mar 12, 2025

View reviewed changes

cristaloleg mentioned this pull request Mar 12, 2025

feat(state/core_accessor): add fee estimator #4168

Merged

cristaloleg closed this Mar 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(state/core_accessor): add fee estimator #4087

feat(state/core_accessor): add fee estimator #4087

vgonkivs commented Feb 4, 2025 •

edited

Loading

codecov-commenter commented Feb 13, 2025 •

edited

Loading

renaynay left a comment

renaynay Mar 3, 2025

vgonkivs Mar 5, 2025

renaynay Mar 3, 2025

vgonkivs Mar 6, 2025

renaynay Mar 3, 2025

vgonkivs Mar 7, 2025

renaynay Mar 3, 2025

vgonkivs Mar 7, 2025

renaynay Mar 5, 2025

Wondertan commented Mar 6, 2025

vgonkivs commented Mar 6, 2025

Wondertan commented Mar 6, 2025

vgonkivs commented Mar 7, 2025

Wondertan commented Mar 7, 2025

vgonkivs commented Mar 7, 2025 •

edited

Loading

vgonkivs commented Mar 7, 2025 •

edited

Loading

Wondertan left a comment

cristaloleg Mar 10, 2025

renaynay Mar 12, 2025

cristaloleg Mar 12, 2025

renaynay Mar 12, 2025

renaynay Mar 12, 2025

renaynay Mar 12, 2025

cristaloleg Mar 12, 2025

renaynay Mar 12, 2025

cristaloleg Mar 12, 2025

cristaloleg commented Mar 12, 2025

feat(state/core_accessor): add fee estimator #4087

feat(state/core_accessor): add fee estimator #4087

Conversation

vgonkivs commented Feb 4, 2025 • edited Loading

codecov-commenter commented Feb 13, 2025 • edited Loading

Codecov Report

renaynay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wondertan commented Mar 6, 2025

vgonkivs commented Mar 6, 2025

Wondertan commented Mar 6, 2025

vgonkivs commented Mar 7, 2025

Wondertan commented Mar 7, 2025

vgonkivs commented Mar 7, 2025 • edited Loading

vgonkivs commented Mar 7, 2025 • edited Loading

Wondertan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristaloleg commented Mar 12, 2025

vgonkivs commented Feb 4, 2025 •

edited

Loading

codecov-commenter commented Feb 13, 2025 •

edited

Loading

vgonkivs commented Mar 7, 2025 •

edited

Loading

vgonkivs commented Mar 7, 2025 •

edited

Loading