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

New protocol version #14227

Merged
merged 14 commits into from Oct 2, 2023
Merged

New protocol version #14227

merged 14 commits into from Oct 2, 2023

Conversation

psteckler
Copy link
Member

@psteckler psteckler commented Sep 27, 2023 •
edited by deepthiskumar

Implementation of RFC 0049 (in #13993).

Some ancillary changes:

  • the current-protocol-version daemon CLI flag is removed (the current version is hard-coded)
  • the ability to set the current protocol version is likewise removed
  • the chain id, instead of using the old major version, uses both the transaction and network components of the protocol version (the implementation is not used)
  • the protocol_versions table in the archive db is updated

The protocol version type gets bumped to V2, but that did not require bumping any other versioned types, which are all types not in mainnet.

The implementation version is kept as a separate type, so that the proposed protocol version doesn't have to mention the implementation.

The config has separate items for the protocol version and implementation version. I put some numbers there: reviewer, please suggest better ones, if appropriate.

New daemon log:

2023-09-27 20:50:45 UTC [Info] Daemon running protocol version 3.0, with implementation version 2.0

Tested advanced chain-id-inputs with a local seed daemon:

Genesis state hash: 3NKiigEqgn8ZCsT6q3gVeReVdPu3XiBD6XmqnuvUUQzJ9MLWWKi3
Genesis_constants:
  Protocol:          `Assoc ([("k", `Int (290));
                               ("slots_per_epoch", `Int (7140));
                               ("slots_per_sub_window", `Int (7));
                               ("delta", `Int (0));
                               ("genesis_state_timestamp",
                                `String ("2020-09-16 10:15:00.000000Z"))
                               ])
  Txn pool max size: 3000
  Num accounts:      None

Snark keys:
  b8879f677f622a1d86648030701f43e1
  de7b52316bf9a5feaef2bf4295294ee9
  86b438a7a0876894739663a91c0d3c79
Protocol transaction version: 3
Protocol network version: 2

The version linter failures are expected.

Closes #14143.

CI nightly run https://buildkite.com/o-1-labs-2/mina-end-to-end-nightlies/builds/555

@psteckler psteckler requested review from a team as code owners September 27, 2023 21:20
@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

nholland94
nholland94 requested changes Oct 2, 2023
View reviewed changes
src/lib/protocol_version/protocol_version.ml Outdated
module V1 = struct
type t = A.V1.t = { major : int; minor : int; patch : int }
module V2 = struct
type t = A.V2.t = { transaction : int; network : int }
Copy link
Member

Choose a reason for hiding this comment

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

We should probably keep the patch in place, for information purposes only. It could be useful in the future for activating features within a soft fork that require a certain threshold of the network to support it before enabling (eg networking changes that today we can only ship as hard forks can sometimes be shipped as soft forks using this approach). We will only check or otherwise utilize the transaction and network versions for now though.

src/lib/protocol_version/protocol_version.ml Outdated

let compatible_with_daemon current_protocol_version =
Int.equal current_protocol_version.major (get_current ()).major
let current = of_string_exn current_string
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just inject the transaction and network versions separately as integers, instead of parsing it on bootup everytime, and crashing if it is misconfigured.

src/lib/protocol_version/protocol_impl.ml Outdated
[%%versioned
module Stable = struct
module V1 = struct
type t = A.V1.t = { api : int; patch : int; tag : string option }
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not to add this for now. @mrmr1993 discussed doing something like this on the original RFC, but we did not come to a final decision, and resolved to decide whether or not to do something like this later. We do need to figure this out, but let's have the discussion first for now.

src/lib/ledger_catchup/super_catchup.ml Outdated
@@ -273,7 +273,7 @@ let verify_transition ~context:(module Context : CONTEXT) ~trust_system
(Mina_block.header transition)
|> Protocol_version.to_string ) )
; ( "daemon_current_protocol_version"
, `String Protocol_version.(get_current () |> to_string) )
, `String Protocol_version.(current |> to_string) )
Copy link
Member

Choose a reason for hiding this comment

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

NIT: pipes from single args to single functions read poorly. Prefer Protocol_version.(to_string current) in this case.

@psteckler
Copy link
Member Author

!ci-build-me

@nholland94
Copy link
Member

!approved-for-mainnet

@psteckler
Copy link
Member Author

!ci-build-me

@psteckler
Copy link
Member Author

!ci-build-me

@deepthiskumar
Copy link
Member

!approved-for-mainnet

@deepthiskumar deepthiskumar merged commit 5d48d72 into berkeley Oct 2, 2023
33 of 35 checks passed
@deepthiskumar deepthiskumar deleted the feature/new-protocol-version branch October 2, 2023 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nholland94 nholland94 nholland94 approved these changes

@ghost-not-in-the-shell ghost-not-in-the-shell ghost-not-in-the-shell approved these changes

@MartinMinkov MartinMinkov MartinMinkov approved these changes

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@psteckler @nholland94 @deepthiskumar @ghost-not-in-the-shell @MartinMinkov