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.

Sign up for GitHub

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

Jump to bottom

[protocol change 52] Increasing max_gas_burnt to 300 #6221

Merged
merged 10 commits into from
Feb 15, 2022
Merged

[protocol change 52] Increasing max_gas_burnt to 300 #6221

merged 10 commits into from
Feb 15, 2022

Conversation

mm-near
Copy link
Contributor

@mm-near mm-near commented Feb 1, 2022

Increasing max_gas_burnt to 300

Testing and QA

Describe the testing plan for this protocol and why you are confident that it is ready to be stabilized.

Checklist

  • Link to nightly nayduck run (./scripts/nayduck.py, docs): https://nayduck.near.org/
  • Update CHANGELOG.md to include this protocol feature in the Unreleased section.

matklad
matklad reviewed Feb 1, 2022
View reviewed changes
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

Do we want to make this insta-stable? (ie, without protocol_features business?)

core/primitives/src/runtime/config_store.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
core/primitives/res/runtime_configs/52.json Outdated Show resolved Hide resolved
mm-near and others added 3 commits February 1, 2022 14:09
@mm-near @matklad
Update CHANGELOG.md
de30267 
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
@mm-near @matklad
Update core/primitives/src/runtime/config_store.rs
a2297e4 
Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>
@mm-near
2 spaces indent in json file
2f39356
matklad
matklad approved these changes Feb 1, 2022
View reviewed changes
Copy link
Contributor

@matklad matklad left a comment

Choose a reason for hiding this comment

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

LGTM!

Still not sure whether we should bother with a protocol_feature for this one, I don't think we decided one way or another.

My gut feeling is the process where we don't do protocol feature for purely config changes is OK.

bowenwang1996
bowenwang1996 requested changes Feb 3, 2022
View reviewed changes
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is a protocol change. If we do it the way it is done right now, if someone sends a transaction that consumes 201Tgas in a single call during the upgrade process, the network will split

core/primitives/src/runtime/config_store.rs
@@ -20,6 +20,8 @@ static CONFIGS: &[(ProtocolVersion, &[u8])] = &[
(48, include_config!("48.json")),
(49, include_config!("49.json")),
(50, include_config!("50.json")),
// max_gas_burnt increased to 300 TGas
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

core/primitives/src/version.rs
@@ -155,7 +155,7 @@ pub const PEER_MIN_ALLOWED_PROTOCOL_VERSION: ProtocolVersion = MAIN_NET_PROTOCOL
/// Current protocol version used on the main net.
/// Some features (e. g. FixStorageUsage) require that there is at least one epoch with exactly
/// the corresponding version
const MAIN_NET_PROTOCOL_VERSION: ProtocolVersion = 51;
const MAIN_NET_PROTOCOL_VERSION: ProtocolVersion = 52;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated to this PR but the naming confused me: why mainnet specifically?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was introduced in #5610. I think it's indeed better to rename it to STABLE_PROTOCOL_VERSION. We need this for PEER_MIN_ALLOWED_PROTOCOL_VERSION, to allow nightly peers to connect to mainnet I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree that STABLE_PROTOCOL_VERSION makes a lot more sense

bowenwang1996
bowenwang1996 approved these changes Feb 3, 2022
View reviewed changes
Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

Nvm I got confused

@mm-near
Copy link
Contributor Author

mm-near commented Feb 3, 2022

our tests are failing with:

[2022-02-01 13:21:31] INFO: Latest mainnet release is 1.23.1
  | Traceback (most recent call last):
  | File "tests/sanity/upgradable.py", line 179, in
  | main()
  | File "tests/sanity/upgradable.py", line 174, in main
  | test_protocol_versions()
  | File "tests/sanity/upgradable.py", line 54, in test_protocol_versions
  | f'If changed, protocol version of a new release can increase by at '
  | AssertionError: If changed, protocol version of a new release can increase by at most one.
  | Got protocol 50 version in release 1.23.1
  | but protocol 52 version in trunk

@matklad
Copy link
Contributor

matklad commented Feb 3, 2022

cc https://github.com/near/nearcore/pull/5913/files#r773203306

@bowenwang1996
Copy link
Collaborator

So it appears that we cannot merge this until 1.24.0 is published 🤔

@bowenwang1996 bowenwang1996 merged commit 9c6fb64 into master Feb 15, 2022
@bowenwang1996 bowenwang1996 deleted the 0201_max_gas_burnt branch February 15, 2022 03:19
@jakmeier jakmeier mentioned this pull request Mar 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowenwang1996 bowenwang1996 bowenwang1996 approved these changes

@matklad matklad matklad approved these changes

@mzhangmzz mzhangmzz Awaiting requested review from mzhangmzz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mm-near @matklad @bowenwang1996