[SIP-45] Metrics and protocol config changes #20738

shio-coder · 2024-12-26T07:03:38Z

Description

This is the change that is proposed from SIP-45.

The change includes:

Adds a histogram metric for amplification_factor.
Adds K to protocol config. K is set to 5 starting from Protocol Version 71.

Test plan

CI

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

vercel · 2024-12-26T07:03:48Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 31, 2024 5:01am

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Dec 31, 2024 5:01am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Dec 31, 2024 5:01am
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Dec 31, 2024 5:01am

crates/sui-protocol-config/src/lib.rs

shio-coder · 2024-12-27T00:56:35Z

Hi @mwtian , I have updated the PR.
Please take another look. Thanks!

mwtian

Pushed some changes to fix the tests.

mwtian · 2024-12-28T18:54:08Z

crates/sui-protocol-config/src/lib.rs

@@ -3076,6 +3083,13 @@ impl ProtocolConfig {

                    cfg.validator_validate_metadata_cost_base = Some(20000);
                }
+                71 => {
+                    if chain != Chain::Mainnet {


Was there a discussion to roll out the max gas price to testnet and mainnet separately, instead of in the same release? cc @dariorussi @sblackshear

This is just being conservative. If rolling out to mainnet at the same time is acceptable then let's do it!

Ok. After looking at the related logic a bit, it seems there are some interactions between gas price and max_tx_gas limit, among other effects of using high gas price. I think being a bit conservative is warranted here. We should definitely try out using very high gas price on testnet first before changing the limit on mainnet. We may need to update max_tx_gas or switch to a different parameter limiting gas unit instead of total gas. They can be done separately.

Yes I have justed realized there is max_tx_gas as well.
Tried to change it locally and that has just caused so many tests to fail (these tests use max_tx_gas as gas budget if not explicitly set). Looks like we are going to need another PR for it.

Agree let's not change max_tx_gas in this PR. In fact we can postpone changing max_gas_price too if you think just changing it will not be useful. I have asked experts on the gas logic for the best way forward. cc @dariorussi @tzakian

please take a look at my main comment

dariorussi

I wondered about the max_tx_gas and I am not too sure.
I think we need some good testing here and let's find out who is going to do that.
Gas tests are here https://github.com/MystenLabs/sui/blob/main/crates/sui-core/src/unit_tests/gas_tests.rs and I think we should add something that plays with budget and price, and also some tests that executes the same transaction with different prices and see that the results are as expected.
Thoughts?

dariorussi · 2024-12-30T17:16:23Z

crates/sui-protocol-config/src/lib.rs

@@ -3076,6 +3083,13 @@ impl ProtocolConfig {

                    cfg.validator_validate_metadata_cost_base = Some(20000);
                }
+                71 => {
+                    if chain != Chain::Mainnet {


please take a look at my main comment

mwtian · 2024-12-30T19:16:41Z

I wondered about the max_tx_gas and I am not too sure. I think we need some good testing here and let's find out who is going to do that. Gas tests are here https://github.com/MystenLabs/sui/blob/main/crates/sui-core/src/unit_tests/gas_tests.rs and I think we should add something that plays with budget and price, and also some tests that executes the same transaction with different prices and see that the results are as expected. Thoughts?

Agree. I see some existing tests on gas budget and prices in the file. We can discuss later on what need to be added, and who should add them. Maybe the change to max gas price can be moved to another PR along with the additional tests and adjustments to other parameters.

shio-coder · 2024-12-31T00:24:07Z

Sounds good folks. I have removed max_gas_price change in this PR.

dariorussi · 2024-12-31T15:11:27Z

Agree. I see some existing tests on gas budget and prices in the file. We can discuss later on what need to be added, and who should add them. Maybe the change to max gas price can be moved to another PR along with the additional tests and adjustments to other parameters.

to be honest I am not sure how max gas budget and a super high gas price and the coins to provide may interact.
We can look at the code a ton but I feel writing a bunch of tests is the most appropriate way to go.
As far as getting this in without testing I do not understand the reason and it seems a mistake.
I personally would love to see tests (and a great coverage) for this before getting in.
But others may feel otherwise...

dariorussi · 2024-12-31T15:16:23Z

Agree. I see some existing tests on gas budget and prices in the file. We can discuss later on what need to be added, and who should add them. Maybe the change to max gas price can be moved to another PR along with the additional tests and adjustments to other parameters.

to be honest I am not sure how max gas budget and a super high gas price and the coins to provide may interact. We can look at the code a ton but I feel writing a bunch of tests is the most appropriate way to go. As far as getting this in without testing I do not understand the reason and it seems a mistake. I personally would love to see tests (and a great coverage) for this before getting it in

I actually see that the gas has been entirely removed from the PR and getting it in is entire up to you folks.
Sorry I did not realize

mystenmark

Looks good!

no more changes to gas price

shio-coder requested review from a team and mystenmark as code owners December 26, 2024 07:03

vercel bot deployed to Preview – sui-docs December 26, 2024 07:05 View deployment

shio-coder temporarily deployed to sui-typescript-aws-kms-test-env December 26, 2024 19:46 — with GitHub Actions Inactive

mwtian reviewed Dec 26, 2024

View reviewed changes

crates/sui-protocol-config/src/lib.rs Outdated Show resolved Hide resolved

crates/sui-protocol-config/src/lib.rs Outdated Show resolved Hide resolved

shio-coder force-pushed the sip_45 branch from 7c11e59 to 4d761b7 Compare December 27, 2024 00:31

vercel bot deployed to Preview – sui-docs December 27, 2024 00:33 View deployment

shio-coder temporarily deployed to sui-typescript-aws-kms-test-env December 27, 2024 05:46 — with GitHub Actions Inactive

mwtian mentioned this pull request Dec 28, 2024

SIP-45: Prioritized Transaction Submission sui-foundation/sips#45

Merged

mwtian reviewed Dec 29, 2024

View reviewed changes

mwtian temporarily deployed to sui-typescript-aws-kms-test-env December 29, 2024 00:25 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 29, 2024 00:25 View deployment

mwtian requested a review from dariorussi December 29, 2024 04:25

dariorussi previously requested changes Dec 30, 2024

View reviewed changes

shio-coder and others added 6 commits December 31, 2024 08:08

[SIP-45] Add amplification_factor histogram

630ca14

[SIP-45] Make K configurable and increase max gas price for testnet

ce16e81

[SIP-45] Use protocol version 71

e80b997

fix snapshot errors

36a44a2

fix txn fuzz tests

06ae314

Remove max gas price change

fa6065c

shio-coder force-pushed the sip_45 branch from 366100b to fa6065c Compare December 31, 2024 00:17

vercel bot deployed to Preview – sui-docs December 31, 2024 00:19 View deployment

shio-coder temporarily deployed to sui-typescript-aws-kms-test-env December 31, 2024 02:12 — with GitHub Actions Inactive

mwtian requested a review from dariorussi December 31, 2024 02:12

revert txn fuzzer test change

1e93d7a

mwtian temporarily deployed to sui-typescript-aws-kms-test-env December 31, 2024 04:59 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs December 31, 2024 05:01 View deployment

mwtian approved these changes Dec 31, 2024

View reviewed changes

mystenmark approved these changes Dec 31, 2024

View reviewed changes

mwtian merged commit 04a5869 into MystenLabs:main Dec 31, 2024
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIP-45] Metrics and protocol config changes #20738

[SIP-45] Metrics and protocol config changes #20738

shio-coder commented Dec 26, 2024 •

edited by mwtian

Loading

vercel bot commented Dec 26, 2024 •

edited

Loading

shio-coder commented Dec 27, 2024

mwtian left a comment

mwtian Dec 28, 2024

shio-coder Dec 29, 2024

mwtian Dec 29, 2024

shio-coder Dec 29, 2024

mwtian Dec 29, 2024

dariorussi Dec 30, 2024

dariorussi left a comment

dariorussi Dec 30, 2024

mwtian commented Dec 30, 2024

shio-coder commented Dec 31, 2024

dariorussi commented Dec 31, 2024

dariorussi commented Dec 31, 2024

mystenmark left a comment

[SIP-45] Metrics and protocol config changes #20738

[SIP-45] Metrics and protocol config changes #20738

Conversation

shio-coder commented Dec 26, 2024 • edited by mwtian Loading

Description

Test plan

Release notes

vercel bot commented Dec 26, 2024 • edited Loading

shio-coder commented Dec 27, 2024

mwtian left a comment

Choose a reason for hiding this comment

mwtian Dec 28, 2024

Choose a reason for hiding this comment

shio-coder Dec 29, 2024

Choose a reason for hiding this comment

mwtian Dec 29, 2024

Choose a reason for hiding this comment

shio-coder Dec 29, 2024

Choose a reason for hiding this comment

mwtian Dec 29, 2024

Choose a reason for hiding this comment

dariorussi Dec 30, 2024

Choose a reason for hiding this comment

dariorussi left a comment

Choose a reason for hiding this comment

dariorussi Dec 30, 2024

Choose a reason for hiding this comment

mwtian commented Dec 30, 2024

shio-coder commented Dec 31, 2024

dariorussi commented Dec 31, 2024

dariorussi commented Dec 31, 2024

mystenmark left a comment

Choose a reason for hiding this comment

shio-coder commented Dec 26, 2024 •

edited by mwtian

Loading

vercel bot commented Dec 26, 2024 •

edited

Loading