Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-048 a multi-tier in-protocol gas price system #10653
docs: ADR-048 a multi-tier in-protocol gas price system #10653
Changes from 6 commits
e2024d3
ff773ae
e263882
712011f
c85d663
3598e72
3048168
f74c0f4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's clarify how the tires will be set. I propose to make it configurable (both number and size of a tire), ideally through gov param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, the constant gas price is a special case of dynamic gas price, and a single dynamic price tier should be similar to an eip-1559 design.
The main difference between this proposal and a eip-1559 design is the tx prioritization:
Do you think we can generalize this too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's already general enough. I propose to clarify how the amount of tires will be set and if we can configure it once the chain is running (I propose yest - through the gov param).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, added a parameter schema, the
tier
field should be simply the index to the tier list.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we still need to order transactions in a tire. Probably we still need to add gas price component here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the point in this design is to not order tx by price directly. Of course, we still have the issue if people all flooded to the higher tier, the lower tier won't have a chance to get included, the potential solution reserves block area for each tier, but that'll need to change tendermint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know that ordering only by price is not the perfect solution, but it's the most general one which is good for simapp example, until we have something better. We want transactions prioritized by type , price and and transaction distribution (by type).
That being said, an ADR proposal must be more complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the tires can definitely be full, so we should decide about the order. It can be FIFO or or anything... I think ordering by gas price makes most of the sense in this design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the order would be
FIFO
under the same tier/priority.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so let's add it to the doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ADR needs the TM core to change the tx behavior in mempool. Maybe needs TM core to support the plug-in mempool(separate the mempool component from TM core) for supporting the ADR requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tendermint 0.35 can already prioritize transactions, and we also have a primitive example in the SDK how to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tx_tier
returns number, so we should remove.priority
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, the original one is good I think,
tx_tier(tx)
returns thegas_tier
, which is used to index the tier parameter listParams.tiers
to get the tier parameters, then use the configured.priority
field of it as the tx priority.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tier
is an optional field for transaction, the application could custom priorities for different transaction types without consideringtier
by implementing this interface for customized transaction type in application level.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see some sort of fallback gas policy that applies to all transaction types. This could be a possible instantiation of the fall back policy.
Takes for instance Terra Oracle Msgs or or GravityEthereum messages.
For example, the Terra ante handler checks to see if all the msgs in the multimsg are oracle messages and applies one fee policy and otherwise applies the fallback fee policy. https://github.com/terra-money/core/blob/main/custom/auth/ante/spamming_prevention.go#L50-L104
We really want to enable use cases like Terra to operate without having to override the entire interface and instead provide middleware that composes with this system. If all the middleware before the multi tier fee module returns nil, then the multi tier fee model applies. In the terra example, if a multi msg combines both oracle msg and bank send messages than the multi tier fee model would apply to both. If the multi msg is all Oracle messages than the messages would have max priority or post ABCI++ use reserved oracle message block space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Muggle-Du totally - we don't want to have a single choice.
@zmanian good point - this should definitely compose with other functionalities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a negative consequence: the update will require wallets & tools update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it might need the TM core protocol upgrades support.