Redistributing fees #4967

georgemitenkov · 2022-10-11T19:13:45Z

First PR to enable transaction fees distribution. In particular, it adds:

API for processing transaction fees, see transaction_fee.move
API for aggregatable coin used to collect the fees, see coin.move
API for distributing fees, see stake.move

Also, Move unit tests were added to simulate block execution.

Next PR: smoke and e2e tests, as this PR has quite a bit of changes already :)

georgemitenkov · 2022-10-12T00:03:53Z

@junkil-park

zekun000

overall looks great!

aptos-move/framework/aptos-framework/sources/coin.move

aptos-move/framework/aptos-framework/sources/fee_distribution.move

aptos-move/framework/aptos-framework/sources/reconfiguration.move

georgemitenkov · 2022-11-30T11:08:46Z

cc @zekun000 @movekevin @junkil-park - this draft has now all the APIs (I hope). I am adding e2e tests at the moment, and in particular tests that include reconfigurations that remove validators.

movekevin

First pass. This is great so far and most comments are minor and more about code structuring. There's one edge case that we have discussed and still need to resolve: what happens if there are remaining undistributed fees because the proposer leaves the validator set in the same block that they propose. This can happen for blocks that contain governance proposal execution.

aptos-move/framework/aptos-framework/sources/reconfiguration.move

aptos-move/framework/aptos-framework/sources/transaction_fee.move

aptos-move/framework/aptos-framework/sources/transaction_validation.move

aptos-move/framework/aptos-framework/sources/stake.move

movekevin · 2022-11-30T20:36:06Z

aptos-move/framework/aptos-framework/sources/stake.move

+
+    /// Stores the transaction fee collected to the specified validator address.
+    public(friend) fun add_transaction_fee(validator_addr: address, fee: Coin<AptosCoin>) acquires ValidatorFees {
+        let fees_table = &mut borrow_global_mut<ValidatorFees>(@aptos_framework).fees_table;


Should we add a safeguard here in case the ValidatorFees resource has not been initialized?

Then we would have a coin which we cannot drop (assuming we guard with if !exists<..>(..) return? Both CollectedFeesPerBlock and ValidatorFees are published at the same time, though.

Hmm that's true that we can't drop or burn the coins here 🤔 It's fine not to have the safeguard then if those 2 resources are guaranteed to be published together. cc @junkil-park

aptos-move/framework/aptos-framework/sources/stake.move

movekevin

I think this looks good now. There are 3 followups I think we should do:

Add e2e (move tests) or smoke tests for fee distribution.
Add an initialization script that will be used as the first step in the multistep proposal that would first initialize the required resources and then enable the feature flag
Some manual testing in local network + devnet

movekevin · 2022-12-07T19:14:53Z

aptos-move/framework/aptos-framework/sources/coin.move

+
+    /// Creates a new aggregatable coin with value overflowing on `limit`. Note that this function can
+    /// only be called by Aptos Framework (0x1) account for now becuase of `create_aggregator`.
+    public(friend) fun initialize_aggregatable_coin<CoinType>(aptos_framework: &signer): AggregatableCoin<CoinType> {


Do we need these functions here or can this be done directly in transaction_fee (with added friendship between transaction_fee and aggregatable coin)?

initialize_aggregatable_coin and is_aggregatable_coin_zero can be moved to transaction_fee, but drain_aggregatable_coin, merge_aggregatable_coin and collect_from_into_aggregatable_coin have to stay in coin module to allow calling Coin constructor/destructor.

aptos-move/framework/aptos-framework/sources/reconfiguration.move

aptos-move/framework/aptos-framework/sources/transaction_fee.move

1. Why we set proposer to option::none() 2. How cases when a validator is removed are handled.

aptos-move/framework/aptos-framework/sources/block.move

aptos-move/framework/aptos-framework/sources/coin.move

github-actions · 2022-12-13T00:29:00Z

✅ Forge suite `land_blocking` success on `70acfcdeb3e105c96bd556e690c52e1c86157954`

performance benchmark with full nodes : 5834 TPS, 6733 ms latency, 13700 ms p99 latency,(!) expired 3320 out of 2494580 txns
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

github-actions · 2022-12-13T00:33:59Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `70acfcdeb3e105c96bd556e690c52e1c86157954`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 70acfcdeb3e105c96bd556e690c52e1c86157954 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7302 TPS, 5362 ms latency, 7600 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 70acfcdeb3e105c96bd556e690c52e1c86157954
compatibility::simple-validator-upgrade::single-validator-upgrade : 4577 TPS, 9006 ms latency, 12000 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 70acfcdeb3e105c96bd556e690c52e1c86157954
compatibility::simple-validator-upgrade::half-validator-upgrade : 4581 TPS, 8947 ms latency, 11200 ms p99 latency,no expired txns
4. upgrading second batch to new version: 70acfcdeb3e105c96bd556e690c52e1c86157954
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6216 TPS, 6251 ms latency, 10200 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 70acfcdeb3e105c96bd556e690c52e1c86157954 passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

Enables transaction fees distribution. In particular, this adds: 1. API for processing transaction fees, see `transaction_fee.move` 2. API for aggregatable coin used to collect the fees, see `coin.move` 3. API for distributing fees, see `stake.move` Also, Move unit tests are added to simulate the block execution.

georgemitenkov requested review from gelash and zekun000 October 18, 2022 16:34

zekun000 reviewed Oct 21, 2022

View reviewed changes

aptos-move/framework/aptos-framework/sources/coin.move Outdated Show resolved Hide resolved

aptos-move/framework/aptos-framework/sources/coin.move Outdated Show resolved Hide resolved

aptos-move/framework/aptos-framework/sources/fee_distribution.move Outdated Show resolved Hide resolved

georgemitenkov added 9 commits November 21, 2022 11:01

[framework] Added resource to hold proposer/balance info

f4bafca

[framework] Added distribution functions

8e89b18

[framework] Added functionality to set fees receiver

8150979

[framework] Added e2e Move test for distribution

20c4eea

[framework] Comment fix

9e8421f

[framework] Fixed failing code publishing test

d7fd4b3

[framework] Improved new coin name

d626aba

[framework] Added docs

27fb8ee

[docs] fixed gas fees distrivution docs

03c6175

georgemitenkov force-pushed the george/gas-redistribution branch from 1f72fd0 to 03c6175 Compare November 21, 2022 10:17

georgemitenkov added 8 commits November 22, 2022 21:39

[framework] Refactored gas fee collection

5380cb9

[framework] Full support for transaction fee collection

c17ef96

[framework] Added fees table in staking module

dc1f29a

[framework] Nits for gas fee collection

b7fadee

Merge branch 'main' into george/gas-redistribution

db4269f

added missing markdowns and added coin test

33a12b9

added stake fee distribution test and removed vm signer

fb5d89d

fixed aggregator draining on reconfiguration

7a35ea6

georgemitenkov commented Nov 30, 2022

View reviewed changes

aptos-move/framework/aptos-framework/sources/reconfiguration.move Outdated Show resolved Hide resolved

Merge branch 'main' into george/gas-redistribution

f4921b2

movekevin requested changes Nov 30, 2022

View reviewed changes

georgemitenkov added 4 commits December 1, 2022 01:30

addressed most of the comments

8fcd4bc

Merge branch 'main' into george/gas-redistribution

8bc498b

[framework] Added function for burn percentage upgrade

54fafa2

[framework] Moved txn fee distribution into update_stake_pool

8161782

movekevin approved these changes Dec 7, 2022

View reviewed changes

Merge branch 'main' into george/gas-redistribution

30b7e66

georgemitenkov marked this pull request as ready for review December 7, 2022 21:21

georgemitenkov requested review from davidiw and wrwg as code owners December 7, 2022 21:21

georgemitenkov changed the title ~~[WIP] Redistributing fees~~ Redistributing fees Dec 7, 2022

georgemitenkov added 3 commits December 9, 2022 11:39

[framework] Added comments

fcb12aa

1. Why we set proposer to option::none() 2. How cases when a validator is removed are handled.

Merge branch 'main' into george/gas-redistribution

e1b15ad

[framework] Fixed tnx fee distribution staking test

a9ec53f

zekun000 approved these changes Dec 9, 2022

View reviewed changes

aptos-move/framework/aptos-framework/sources/block.move Show resolved Hide resolved

aptos-move/framework/aptos-framework/sources/coin.move Outdated Show resolved Hide resolved

georgemitenkov added 4 commits December 10, 2022 00:08

[framework] Addressed comments

24983f0

Merge branch 'main' into george/gas-redistribution

b293c26

Merge branch 'main' into george/gas-redistribution

443c3d2

Made lint happy

70acfcd

georgemitenkov added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Dec 12, 2022

This comment has been minimized.

Sign in to view

georgemitenkov merged commit 5738125 into main Dec 13, 2022

georgemitenkov deleted the george/gas-redistribution branch December 13, 2022 09:30

georgemitenkov mentioned this pull request Dec 20, 2022

[AIP-7][Discussion] Transaction fee distribution aptos-foundation/AIPs#23

Closed

Markuze mentioned this pull request Dec 26, 2022

markuze/aptos perf #5998

Closed

Markuze mentioned this pull request Jan 3, 2023

[Network] Adding AptosPerf Client #6054

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redistributing fees #4967

Redistributing fees #4967

georgemitenkov commented Oct 11, 2022 •

edited

Loading

georgemitenkov commented Oct 12, 2022

zekun000 left a comment

georgemitenkov commented Nov 30, 2022

movekevin left a comment

movekevin Nov 30, 2022

georgemitenkov Nov 30, 2022

movekevin Nov 30, 2022

movekevin left a comment

movekevin Dec 7, 2022

georgemitenkov Dec 9, 2022 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

Redistributing fees #4967

Redistributing fees #4967

Conversation

georgemitenkov commented Oct 11, 2022 • edited Loading

georgemitenkov commented Oct 12, 2022

zekun000 left a comment

Choose a reason for hiding this comment

georgemitenkov commented Nov 30, 2022

movekevin left a comment

Choose a reason for hiding this comment

movekevin Nov 30, 2022

Choose a reason for hiding this comment

georgemitenkov Nov 30, 2022

Choose a reason for hiding this comment

movekevin Nov 30, 2022

Choose a reason for hiding this comment

movekevin left a comment

Choose a reason for hiding this comment

movekevin Dec 7, 2022

Choose a reason for hiding this comment

georgemitenkov Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 13, 2022

✅ Forge suite land_blocking success on 70acfcdeb3e105c96bd556e690c52e1c86157954

github-actions bot commented Dec 13, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 70acfcdeb3e105c96bd556e690c52e1c86157954

georgemitenkov commented Oct 11, 2022 •

edited

Loading

georgemitenkov Dec 9, 2022 •

edited

Loading

✅ Forge suite `land_blocking` success on `70acfcdeb3e105c96bd556e690c52e1c86157954`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `70acfcdeb3e105c96bd556e690c52e1c86157954`