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

feat: sketch the spec of feemarket module #10463

Closed
wants to merge 5 commits into from

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Oct 29, 2021

Description

WIP #9963

Merge the idea of feemarket module in ethermint
into cosmos-sdk.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

WIP cosmos#9963

Merge the idea of [feemarket module in ethermint](https://github.com/tharsis/ethermint/tree/main/x/feemarket)
into cosmos-sdk.
@tomtau
Copy link
Contributor

tomtau commented Oct 29, 2021

FYI @fedekunze

@yihuang yihuang changed the title sketch the spec of feemarket module feat: sketch the spec of feemarket module Oct 29, 2021
@tomtau
Copy link
Contributor

tomtau commented Oct 29, 2021

FYI @hxrts #8224 (comment)

I think the spec is quite straightforward. I'd suggest opening a PR with ADR first though, because the overall impact is unclear from the spec itself. A few points off the top of my head I'd like to see:

  1. interaction with the Tendermint 0.35 mempool prioritization: this spec sketch doesn't seem to include the EIP-1559 "tip mechanism"
  2. upgrade transitions: should one init the parameters in the same way as for genesis, or one could do better in upgrades where there's the previous block info?
  3. the "disable empty blocks" option in Tendermint: could it still work when the gas price info is being calculated in every block?

@hxrts hxrts requested a review from fedekunze October 29, 2021 15:28

This document specifies the feemarket module of the Cosmos SDK.

The feemarket module implements a base gas prices in consensus level.
Copy link
Contributor

@liamsi liamsi Oct 30, 2021

Choose a reason for hiding this comment

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

Suggested change
The feemarket module implements a base gas prices in consensus level.
The feemarket module implements a base gas price at the consensus level.

or: base gas prices

@hxrts
Copy link
Contributor

hxrts commented Oct 31, 2021

Excited for this! So you're aware, there is some work from the tx working group (see WG links here) that you may want to be aware of. Would also be good to get someone on the arch/planning calls to coordinate (@tomtau just added you).

@yihuang
Copy link
Collaborator Author

yihuang commented Oct 31, 2021

FYI @hxrts #8224 (comment)

I think the spec is quite straightforward. I'd suggest opening a PR with ADR first though, because the overall impact is unclear from the spec itself. A few points off the top of my head I'd like to see:

  1. interaction with the Tendermint 0.35 mempool prioritization: this spec sketch doesn't seem to include the EIP-1559 "tip mechanism"

Since we validate the tx.Fee() >= expectedFee, user can always pay fee higher than minimal one, it's just we don't order the mempool based on the tip fee, the good thing is we don't need to change tx format (change the fee fields) in this proposal, I guess we can do that together with the mempool prioritization change.

  1. upgrade transitions: should one init the parameters in the same way as for genesis, or one could do better in upgrades where there's the previous block info?

it seems not much difference for genesis or in the middle of the chain.

  1. the "disable empty blocks" option in Tendermint: could it still work when the gas price info is being calculated in every block?

The base gas price overall will be higher with "disable empty blocks", since the empty blocks will have the side-effect of lowering the base gas prices.

@hxrts
Copy link
Contributor

hxrts commented Nov 1, 2021

  1. agree, we would ideally do this together with mempool prioritisation, which is essential for creating a functioning fee market. @alexanderbez likely has the most context + @cmwaters wrote this in response "To me it seems the way fees are calculated and the use of them as a measurement of priority in the mempool are largely orthogonal and I don't foresee any issue with this persons proposal and the prioritized mempool"
  2. I like the idea of persisting the fee across upgrades (one less thing to think about), in either high or low-fee scenarios it seems like the worst case would be a price correction over the course of a few blocks. I think we may still want to ensure the fee is non-zero.
  3. This is an interesting case. Seems like we may not know the real gas fee if we don't know the number of empty blocks. This should only happen if there are no transactions in the mempool with fee >= minfee, which means the minfee should either be falling, or its at the absolute min and there are no transactions at all. In either case I think we can maybe assume validator altruism and have them absorb the gas cost. Open to suggestions here though.

Comment on lines +11 to +13
The total gas used by current block is stored in chain state at the `EndBlock` event.

It's initialized to `-1` in `InitGenesis`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to add another .md file for how the state (kv store) looks like? In particular:

  • under which key is this total gas stored in the substore? (e.g. 0x0)
  • which encoding to use for the integer?

Also see SPEC_SPEC for more info.

@fedekunze
Copy link
Collaborator

I don't understand why is this being added to the SDK, why rely on another dependency for Ethermint (DynamicFeeTxs) and the JSON-RPC? the timelines and release cycles for both projects are very different

@tomtau
Copy link
Contributor

tomtau commented Nov 29, 2021

I don't understand why is this being added to the SDK, why rely on another dependency for Ethermint (DynamicFeeTxs) and the JSON-RPC? the timelines and release cycles for both projects are very different

the context is "Consensus Fees" WG for Cosmos SDK: #9058
#8224
#9962 #9963

So it's a separate effort and the only relation is that it may get inspiration for the implementation.

@yihuang yihuang marked this pull request as draft December 1, 2021 16:10
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jan 16, 2022
@yihuang
Copy link
Collaborator Author

yihuang commented Jan 16, 2022

need to redo this, closing now.

@yihuang yihuang closed this Jan 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants