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.

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

docs: adr-13 - automatic gas price estimation #4048

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

cmwaters
Copy link
Contributor

First pass of the gas price estimation ADR. Comments are welcome!

@github-actions github-actions bot added the external Issues created by non node team members label Jan 17, 2025
@cmwaters cmwaters changed the title adr-13: automatic gas price estimation docs: adr-13 - automatic gas price estimation Jan 17, 2025
@vgonkivs vgonkivs added docs:adr ADR kind:docs For solely documentation PRs labels Jan 17, 2025
docs/adr/adr-013-automatic-gas-price-estimation.md Outdated Show resolved Hide resolved

### Defaults

By default, the same consensus node that the node is connected with, should be used as the `GasEstimator`. During construction, a user can optionally provide a grpc address that can be used instead.
Copy link
Member

Choose a reason for hiding this comment

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

Unclear what you mean by "a grpc address that can be used instead"

Node has one entrypoint for tx submission and it's through a core node's gRPC. So you're saying the GasEstimator will also be available through exactly the same gRPC conn as other state-related queries we perform on the core node?

Copy link
Member

Choose a reason for hiding this comment

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

My understanding is that we should provide new config field/flag for this service and try core GRPC if not specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes by default the consensus node will provide the GasEstimator service in the same gRPC connection. However users should be able to override that with a gRPC address of their own choosing which will be used just for gas/pricing estimation

Copy link
Member

Choose a reason for hiding this comment

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

So an entirely new config field in the state config that can specify addr of estimator service.

This should be included in ADR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I wasn't sure how exactly it would be implemented. Should I just add that line

Copy link
Member

Choose a reason for hiding this comment

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

Is there any chance that connection with this endpoint will be secured and we'll need a tls config or xToken to communicate with?


### Fallback

If there are any errors with interacting with the `GasEstimator` service, the node should log an error and fallback to using the minimum gas price.
Copy link
Member

Choose a reason for hiding this comment

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

Should you propagate this error back to the user somehow? Otherwise caller will have no idea why their txs might be taking relatively long.

Copy link
Member

Choose a reason for hiding this comment

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

Are there negative financial consequences for doing this fallback? Is it possible that by looping in fallback we spend much more gas then expected? If that's possible, I think we should error back to user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not an error to take a long time. There are other ways a user can view the progress of their transaction and with something like replace-by-fee in the future, they will even be able to boost the gas price of their transaction

Copy link
Member

Choose a reason for hiding this comment

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

But utilising the fallback is unexpected behaviour. I guess maybe if it's well documented enough how the estimator works then no error returned is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I don't think it's unexpected if it's specified but I was conscious that it may be better to fail loudly if the estimation service is unavailable.

An idea could be, in the manual submission path, allow for someone to specify the minimum that way users can easily encode that fallback.

Copy link
Member

Choose a reason for hiding this comment

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

What if the error occurs after submitting the transaction( e.g out of gas)? Should also fall back to a default solution with an additional warning to the user(in case he provides his own estimator service address)?

docs/adr/adr-013-automatic-gas-price-estimation.md Outdated Show resolved Hide resolved
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Is this ADR for node only or is there additional expected for the core? If not, I think we should mention here info about relation to core and how its gonna integrate with the Signer.


## Decision

Allow users of Celestia nodes to opt in to automatic gas price and gas usage estimation for every write method
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Allow users of Celestia nodes to opt in to automatic gas price and gas usage estimation for every write method
Allow users of Celestia nodes to opt in to automatic gas price and gas usage estimation for every write method.


### When to estimate the gas price

Users can opt in to automatic price estimation by using the existing `TxConfig.isGasPriceSet`. Previously, if this was false, the `DefaultGasPrice` was used, now if a user isn't manually setting the gas price, the node will query it from the `GasEstimator` service.
Copy link
Member

Choose a reason for hiding this comment

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

[quesion]
is this for full nodes? if so, how would light nodes get the estimation? would it be something like:

light node -> full node -> consensus full node gRPC? but aren't full nodes only connected to bridge nodes?

if so, should the requests pass by the bridge node?

Or, will all type of Celestia-node nodes take a consensus gRPC endpoint?

I guess that having a third party provider would make this way easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't full nodes also be connected to consensus node (or whatever the user defines as the grpc service)

Copy link
Member

@vgonkivs vgonkivs Jan 31, 2025

Choose a reason for hiding this comment

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

should we still use gasMultiplier? Or does It assume that results will be more accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Tbh, I think we should remove the gasMultiplier and assume that is taken care of on the server side. 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.

We can use it only in case of a fallback solution when requesting EstimateGas which simulates the transaction.

Copy link
Member

Choose a reason for hiding this comment

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

Tbh, I think we should remove the gasMultiplier and assume that is taken care of on the server side. What do you think?

Double checked and the test failed with out of gas. So I'll keep it.

@cmwaters
Copy link
Contributor Author

Is this ADR for node only or is there additional expected for the core? If not, I think we should mention here info about relation to core and how its gonna integrate with the Signer.

It's for node only. There is no plan to integrate with signer

@Wondertan
Copy link
Member

It's for node only. There is no plan to integrate with signer

Anything in particular about why not? The app provides blob submission as well and should support gas estimation as well, imo

@cmwaters
Copy link
Contributor Author

Anything in particular about why not? The app provides blob submission as well and should support gas estimation as well, imo

My mental model is that the Signer should just be about signing and be quite manual and anything automated is an abstraction above it, that way people can always import the signer with their own automation logic (if they don't like ours)

@Wondertan
Copy link
Member

I see you point on Signer and can agree with it. But my point is about support in app broadly within Signer or not.

Also, looking at the ADR again, its has no connection to node, besides minimal TxConfig field implementation details. I start to believe this document is misplaced.

@cmwaters
Copy link
Contributor Author

I see you point on Signer and can agree with it. But my point is about support in app broadly within Signer or not.

App won't use the API, at least not initially.

Also, looking at the ADR again, its has no connection to node, besides minimal TxConfig field implementation details. I start to believe this document is misplaced.

It's not just the config, it's also the usage of the API itself, it's whether we should estimate the gas locally or rely on the API, it's the guardrails in place and possibly a fallback. If not in node, where would you like this document to be placed

@renaynay
Copy link
Member

I guess it's fine to document the behaviour here as the expected behaviour of the default will change.


### Setting guardrails

A new field in the `TxConfig` will be introduced allowing for a user to use the gas price estimation but setting a maximum gas price as a cap that the user is willing to pay. If the price returned by the estimator is greater than the cap, likely due to a price spike, an error will be returned informing the user that the gas price required is currently greater than the user is willing to pay. If the user doesn't specify a cap, a hardcoded value that is 100x the default min gas price will be used (i.e. 0.2 utia).
Copy link
Member

@vgonkivs vgonkivs Jan 30, 2025

Choose a reason for hiding this comment

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

Can we assume that tx priority is medium in case the user wants us to use gas estimator? Otherwise, we need to add one more field specifying the priority.

Copy link
Member

Choose a reason for hiding this comment

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

or we can configure the node during the startup to use some priority by default.

Copy link
Member

@vgonkivs vgonkivs Jan 31, 2025

Choose a reason for hiding this comment

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

Q: why do we need to have any limits if the user hasn't specified them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is medium priority - users don't have to specify it if they don't want to. I guess it should also be present in the Node API as part of the config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Q: why do we need to have any limits if the user hasn't specified them?

Out of an abundance of caution (i.e. limit the damage this can do if the GasEstimation service is faulty)

Copy link
Member

Choose a reason for hiding this comment

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

I think it is worth adding that we should not use queried gas price that is less than minGasPrice

@Wondertan
Copy link
Member

It's not just the config, it's also the usage of the API itself, it's whether we should estimate the gas locally or rely on the API, it's the guardrails in place and possibly a fallback. If not in node, where would you like this document to be placed

Yes, TxConfig is part of the API and I am in favor of documenting the change. However, the ADR process, AFAIU works differently. There should have been an State/Core API ADR in the first place where this ( arguably tiny API change) would land on top with an entry in changelog section of the ADR. Others, more app specific changes would land together in the app repo.

Let's agree how we would this ideally and then see if we can go that path. If not, happy to hear short-term options to unblock this.

@vgonkivs
Copy link
Member

In my opinion, most of the logic from this ADR should be placed on the TxClient, since it is responsible for submitting the txs and it also has gas estimation logic. It turns we have the same logic(estimation) in both core_accessor and TxClient. I see core_accessor as a component that receives data Requests from user proxy it, converting types where it is necessary. All operations for re-submitting(blobs), gas estimation, should live inside TxClient and should be hidden(but it could be configurable by core_accessor - e.g. passing a custom estimator connection instead of the default one).

Core accessor over complicated with a lot of unneeded logic (example) and I propose to define and divide responsibilities between core_accessor and txClient.

@cmwaters
Copy link
Contributor Author

cmwaters commented Feb 3, 2025

There should have been an State/Core API ADR in the first place where this ( arguably tiny API change) would land on top with an entry in changelog section of the ADR

Sure I would agree with that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs:adr ADR external Issues created by non node team members kind:docs For solely documentation PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants