feat!: Introduce feemarket module #3028

stana-miric · 2024-03-27T11:25:39Z

Description

Note: This is a new PR description written by @dusan-maksimovic after the latest changes were made by @stana-miric and myself.

This PR includes:

Integration of the feemarket module
Removal of auth module's DeductFeeDecorator from the app (replaced by the feemarket)
Removal of globalfee module from the app (also replaced by the feemarket)

The main changes are in "ante" and "app" directories, and other changes are removal of globalfee module and fixing of failing tests.

Currently, we are targeting the latest feemarket commit on SDK 47. Once Skip creates a new release for SDK 47, we should target that tag instead.

Feemarket module is configured in following way:

BaseFee (minimal allowed, as well as the initial value) = 0.005uatom, which is the current min gas price on the mainnet that is enforced by the globalfee module
DistributeFees = true, which means that tx fees will not be burned (which is the default EIP-1559 implementation), but they would instead be sent to the fee collector and later distributed to validators and delegators, as it was the case before the feemarket was introduced. Also, feemarket will send some amount of tx fees to the block proposer, if the tx sender decided to pay fees above the minimal required base fee amount
MaxBlockUtilization = 100000000, which is the current max block gas, as per this proposal
TargetBlockUtilization = 50000000, 50% of MaxBlockUtilization which means the base fee will start increasing once the block gas consumptions exceeds 50% of the block gas limit

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...

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

tests/ics/feemarket_test.go

app/upgrades/v16/upgrades.go

tests/integration/test_utils.go

Co-authored-by: Dusan Maksimovic <dusan.maksimovic@ethernal.tech>

Co-authored-by: Stana Miric <stana.miric@ethernal.tech>

Co-authored-by: Dusan Maksimovic <dusan.maksimovic@ethernal.tech>

Co-authored-by: Stana Miric <stana.miric@ethernal.tech>

dusan-maksimovic · 2024-05-31T12:44:35Z

go.mod

@@ -230,9 +230,15 @@ replace (
 	// Use special SDK v0.47.x release with support for both ICS and LSM
 	github.com/cosmos/cosmos-sdk => github.com/cosmos/cosmos-sdk v0.47.13-ics-lsm

+	github.com/cosmos/gogoproto => github.com/cosmos/gogoproto v1.4.10


These three new replacement directives are due to the feemarket module bumping this dependencies, after which Gaia stopped compiling, so we had to revert to previous versions.

dusan-maksimovic · 2024-05-31T12:49:31Z

ante/ante.go

 )

+// UseFeeMarketDecorator to make the integration testing easier: we can switch off its ante and post decorators with this flag


This is a workaround to make the integration tests based on IBC testing framework work without the need to override the SendMsgs() function, since this would also require changes to the ICS repository to modify the CCVTestSuite. The only thing we would need to override is to specify the tx fees, which is not essential for these tests, so we decided not to do it. Also, our non-determinism tests were failing because the tx fees are not provided, and we are unable to provide the tx fees to the framework used, so we opted for this work-around. For the override of the SendMsgs() see the FeeMarketTestSuite.

tests/e2e/e2e_bank_test.go

tests/e2e/e2e_distribution_test.go

app/app.go

p-offtermatt · 2024-06-07T11:05:20Z

As commented on Slack, I think we should have a fallback for when the feemarket is disabled, e.g. via governance

…if feemarket is disabled

dusan-maksimovic · 2024-06-13T09:04:05Z

tests/e2e/scripts/hermes_bootstrap.sh

-gas_price = { price = 0.00001, denom = 'uatom' }
-gas_multiplier = 1.2
+gas_price = { price = 0.005, denom = 'uatom' }
+gas_multiplier = 2


TODO: revert this value to 1.2 (in all 4 places) once the gas estimation is fixed in the feemarket module and we upgrade to a new version

We need to add the dynamic fees for hermes. I think that will fix the high gas_multiplier.

https://hermes.informal.systems/documentation/configuration/dynamic-gas-fees.html

dusan-maksimovic · 2024-06-13T09:13:56Z

@p-offtermatt Skip team already implemented a fallback to alternative (arbitrary) decorator, so I just provided the auth modules' DeductFeesDecorator and introduced minTxFeesChecker instead of noOpTxChecker to enforce the minimum fees if the feemarket is disabled.

p-offtermatt

LGTM. I looked mostly at the params.
From experimenting a bit, it seems an arbitrary 1000 block range of usual activity on the hub would need to have ~30 times bigger usage to actually make the price consistently higher than the base.

I didn't look as closely at the wiring, but just in terms of configuration of the feemarket, I think it's good.

MSalopek

E2E were updated, globalfee was removed and feemarket was integrated.

Any patches to the feemarket (if needed) will be reflected in a different PRs.

mpoke

Great work. See my comments below.

.changelog/unreleased/features/3028-add-feemarket

.changelog/unreleased/state-breaking/3028-add-feemarket

mpoke · 2024-06-18T10:46:19Z

.changelog/unreleased/features/3028-add-feemarket

Add one more changelog entry under api-breaking. Removing the globalfee module breaks the API, right?

mpoke · 2024-06-18T10:47:20Z

Makefile

@@ -21,7 +21,7 @@ BUILDDIR ?= $(CURDIR)/build
 TEST_DOCKER_REPO=cosmos/contrib-gaiatest

 GO_SYSTEM_VERSION = $(shell go version | cut -c 14- | cut -d' ' -f1 | cut -d'.' -f1-2)
-REQUIRE_GO_VERSION = 1.21
+REQUIRE_GO_VERSION = 1.22


Do we need to bump the go version? If so, please add a changelog entry.

Yes.

Added changelog.

mpoke · 2024-06-18T10:57:24Z

docs/docs/architecture/adr/README.md

Why is this file duplicated?

It has existed before this PR. Hence, not relevant here. It can be removed in a different PR.

mpoke · 2024-06-18T11:26:32Z

app/upgrades/v18/upgrades.go

+	params.DistributeFees = true
+	params.MinBaseGasPrice = sdk.MustNewDecFromStr("0.005")
+	params.MaxBlockUtilization = 100000000


Are these params as discussed with Philip?

Yes.

Also, these are the current network params.

Co-authored-by: Marius Poke <marius.poke@posteo.de>

stana-miric assigned dusan-maksimovic and stana-miric Mar 27, 2024

MSalopek reviewed Mar 27, 2024

View reviewed changes

tests/ics/feemarket_test.go Outdated Show resolved Hide resolved

stana-miric commented Apr 5, 2024

View reviewed changes

app/upgrades/v16/upgrades.go Outdated Show resolved Hide resolved

dusan-maksimovic force-pushed the blocksdk-fee-market branch from 3524455 to 9052464 Compare May 27, 2024 11:50

github-advanced-security bot found potential problems May 27, 2024

View reviewed changes

tests/integration/test_utils.go Dismissed Show resolved Hide resolved

tests/integration/test_utils.go Dismissed Show resolved Hide resolved

dusan-maksimovic force-pushed the blocksdk-fee-market branch 2 times, most recently from b0a7667 to 89d57b3 Compare May 27, 2024 13:36

stana-miric and others added 15 commits May 31, 2024 12:21

Added feemarket module

67ed1ef

Co-authored-by: Dusan Maksimovic <dusan.maksimovic@ethernal.tech>

Added blocksdk support

5a68fa1

Co-authored-by: Dusan Maksimovic <dusan.maksimovic@ethernal.tech>

Removed globalfee from app

cc65aae

Co-authored-by: Dusan Maksimovic <dusan.maksimovic@ethernal.tech>

Cleanup and lint fix

0c4d33a

Co-authored-by: Dusan Maksimovic <dusan.maksimovic@ethernal.tech>

added post-handler and test for feemarket

ffbe220

Co-authored-by: Stana Miric <stana.miric@ethernal.tech>

tests fix part 1

247596b

Co-authored-by: Dusan Maksimovic <dusan.maksimovic@ethernal.tech>

test fix part 2

9b9ac32

Co-authored-by: Dusan Maksimovic <dusan.maksimovic@ethernal.tech>

added changelog

22fdf42

Co-authored-by: Dusan Maksimovic <dusan.maksimovic@ethernal.tech>

update x/feemarket dependency

0be4697

fix broken dep update for feemarket

943a116

Fixed merge errors & unit tests

dff7000

Co-authored-by: Stana Miric <stana.miric@ethernal.tech>

Removed Skip Block SDK

080ffa1

Co-authored-by: Stana Miric <stana.miric@ethernal.tech>

set upgrade params and test fixes

d745944

fix e2e tests & cleanup

c517b48

merge fix

8cdce49

dusan-maksimovic force-pushed the blocksdk-fee-market branch from 46a6d14 to 8cdce49 Compare May 31, 2024 10:33

dusan-maksimovic changed the title ~~feat!: Introduce skip block sdk and fee market module~~ feat!: Introduce feemarket module May 31, 2024

dusan-maksimovic reviewed May 31, 2024

View reviewed changes

dusan-maksimovic marked this pull request as ready for review May 31, 2024 12:52

dusan-maksimovic requested review from zmanian and crodriguezvega as code owners May 31, 2024 12:52

merge main branch into blocksdk-fee-market

0b51473

mpoke mentioned this pull request Jun 7, 2024

Add Fee-Abstraction to Hub #3141

Draft

bermuell reviewed Jun 7, 2024

View reviewed changes

tests/e2e/e2e_bank_test.go Outdated Show resolved Hide resolved

tests/e2e/e2e_distribution_test.go Outdated Show resolved Hide resolved

p-offtermatt reviewed Jun 7, 2024

View reviewed changes

app/app.go Outdated Show resolved Hide resolved

simplify the check for expected user balances in e2e tests

b69c7b5

dusan-maksimovic force-pushed the blocksdk-fee-market branch from 686b3f0 to b69c7b5 Compare June 7, 2024 12:07

This was referenced Jun 12, 2024

[Bug]: Inaccurate gas estimations when feemarket is enabled skip-mev/feemarket#115

Closed

[Bug]: Submitting transaction with the "--fee-granter" flag fails during simulate because of panic in feemarket PostHandle() skip-mev/feemarket#114

Closed

upgrade to a new version of the feemarket & minimum fees enforcement …

0fce83e

…if feemarket is disabled

dusan-maksimovic reviewed Jun 13, 2024

View reviewed changes

MSalopek added 3 commits June 13, 2024 22:18

deps: bump feemarket v1.0.2-sdk47; bump hermes v1.9.0

2e52f4f

Merge branch 'main' into blocksdk-fee-market

686744f

scripts: update hermes configs

37541f9

p-offtermatt reviewed Jun 14, 2024

View reviewed changes

MSalopek added 5 commits June 14, 2024 14:07

update changelogs (depr x/globalfee)

2ae514c

e2e: change sequence on validator genesis acc

bd887ff

e2e: mv hermes calls in ibc_tests

f57b72e

e2e: adjust gas in staking ops

eedc862

e2e: mv extension tests; update feegrant tests

9f3afd2

MSalopek approved these changes Jun 14, 2024

View reviewed changes

MSalopek added 2 commits June 17, 2024 17:39

deps: bump feemarket v1.0.3-sdk47

18fc6e4

Merge branch 'main' into blocksdk-fee-market

9c52fe7

mpoke approved these changes Jun 18, 2024

View reviewed changes

MSalopek and others added 3 commits June 18, 2024 13:28

Update .changelog/unreleased/features/3028-add-feemarket

d387a7f

Co-authored-by: Marius Poke <marius.poke@posteo.de>

Update .changelog/unreleased/state-breaking/3028-add-feemarket

4d4662c

Co-authored-by: Marius Poke <marius.poke@posteo.de>

update changelogs

417c987

MSalopek merged commit 2758025 into main Jun 18, 2024
17 checks passed

MSalopek deleted the blocksdk-fee-market branch June 18, 2024 12:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Introduce feemarket module #3028

feat!: Introduce feemarket module #3028

stana-miric commented Mar 27, 2024 •

edited by dusan-maksimovic

Loading

dusan-maksimovic May 31, 2024

dusan-maksimovic May 31, 2024 •

edited

Loading

p-offtermatt commented Jun 7, 2024

dusan-maksimovic Jun 13, 2024

MSalopek Jun 13, 2024

dusan-maksimovic commented Jun 13, 2024

p-offtermatt left a comment

MSalopek left a comment

mpoke left a comment

mpoke Jun 18, 2024

MSalopek Jun 18, 2024

mpoke Jun 18, 2024

MSalopek Jun 18, 2024 •

edited

Loading

mpoke Jun 18, 2024

MSalopek Jun 18, 2024

mpoke Jun 18, 2024 •

edited

Loading

MSalopek Jun 18, 2024

		)

		// UseFeeMarketDecorator to make the integration testing easier: we can switch off its ante and post decorators with this flag

feat!: Introduce feemarket module #3028

feat!: Introduce feemarket module #3028

Conversation

stana-miric commented Mar 27, 2024 • edited by dusan-maksimovic Loading

Description

Author Checklist

Reviewers Checklist

Choose a reason for hiding this comment

dusan-maksimovic May 31, 2024 • edited Loading

Choose a reason for hiding this comment

p-offtermatt commented Jun 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dusan-maksimovic commented Jun 13, 2024

p-offtermatt left a comment

Choose a reason for hiding this comment

MSalopek left a comment

Choose a reason for hiding this comment

mpoke 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

MSalopek Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpoke Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stana-miric commented Mar 27, 2024 •

edited by dusan-maksimovic

Loading

dusan-maksimovic May 31, 2024 •

edited

Loading

MSalopek Jun 18, 2024 •

edited

Loading

mpoke Jun 18, 2024 •

edited

Loading