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

renaming fork-previous-global-slot to fork-genesis-slot #14014

Merged
merged 24 commits into from
Sep 26, 2023
Merged

renaming fork-previous-global-slot to fork-genesis-slot #14014

merged 24 commits into from
Sep 26, 2023

Conversation

ghost-not-in-the-shell
Copy link
Contributor

@ghost-not-in-the-shell ghost-not-in-the-shell commented Aug 30, 2023 •
edited
Loading

Explain your changes:
Renaming fork_previous_global_slot to fork_genesis_global_slot. We are doing this change because when generating the new genesis block for a hard-forked chain, we are taking the value of fork_previous_global_slot as the new genesis slot number. So if we give it the slot number of previous block, then we could have 2 blocks that have same slot number on the chain. This renaming is intended to inform the user that this slot number is for the new genesis block, so it should be given a value such as previous_global_slot + 1.

Explain how you tested your changes:
*

Checklist:

  • Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues? List them

@ghost-not-in-the-shell ghost-not-in-the-shell requested a review from a team as a code owner August 30, 2023 18:26
@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

psteckler
psteckler reviewed Aug 31, 2023
View reviewed changes
src/lib/genesis_constants/genesis_constants.ml Outdated
@@ -174,8 +174,7 @@ module Constraint_constants = struct
fork_previous_state_hash
; previous_length = Mina_numbers.Length.of_int fork_previous_length
; previous_global_slot =
Copy link
Member

Choose a reason for hiding this comment

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

does this record field need to change, also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

psteckler
psteckler reviewed Aug 31, 2023
View reviewed changes
Copy link
Member

@psteckler psteckler left a comment

Choose a reason for hiding this comment

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

I think you'll need to change the field name in type Fork_constants.t and also Runtime_config.Fork_config.t.

That affects the hard fork integration test, which isn't run in PR CI.

@ghost-not-in-the-shell ghost-not-in-the-shell requested a review from a team as a code owner September 5, 2023 16:04
@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

This was referenced Sep 5, 2023
Closed
Closed
@psteckler
Copy link
Member

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

Tang Jiawei added 2 commits September 9, 2023 03:27
@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

deepthiskumar
deepthiskumar requested changes Sep 19, 2023
View reviewed changes
Copy link
Member

@deepthiskumar deepthiskumar left a comment

Choose a reason for hiding this comment

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

Please include the reason for this change in the PR description and also, how this is intended to be used.

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@ghost-not-in-the-shell
Copy link
Contributor Author

!ci-build-me

@deepthiskumar
Copy link
Member

!approved-for-mainnet

@ghost-not-in-the-shell ghost-not-in-the-shell merged commit 2e7ba61 into berkeley Sep 26, 2023
2 checks passed
@ghost-not-in-the-shell ghost-not-in-the-shell deleted the renaming-previous-slot-to-genesis-slot branch September 26, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mitschabaude mitschabaude mitschabaude approved these changes

@deepthiskumar deepthiskumar deepthiskumar approved these changes

@psteckler psteckler psteckler 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.
4 participants
@ghost-not-in-the-shell @psteckler @deepthiskumar @mitschabaude