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.

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

IF: Rename existing block_header_state and block_state to block_header_state_legacy and block_state_legacy #1947

Closed
wants to merge 6 commits into from

Conversation

linh2931
Copy link
Member

@linh2931 linh2931 commented Dec 1, 2023

  • Renamed structs block_header_state to block_header_state_legacy, block_state to block_state_legacy;
  • Changed block_header_state.{hpp,cpp} to block_header_state_legacy.{hpp,cpp} and change block_state.{hpp,cpp} to block_state_legacy.{hpp,cpp};
  • Updated snapshot handling;
  • Made the _legacy versions match what is in 5.0

Resolved #1940

@greg7mdp
Copy link
Contributor

greg7mdp commented Dec 1, 2023

@linh2931 , it looks loke you removed the current hotstuff_integration changes, so for example block_state.hpp was removed, and block_header_state.hpp is almost emptied. I think you should have left the existing code living in the old classes, even if it is unused for now. We will need both versions, the one with the old code, and the new hotstuff-based ones.

Looking at the changes, I think even more that this change should be done in the main branch, and merged into hotstuff_integration. @arhag what do you think?

@linh2931
Copy link
Member Author

linh2931 commented Dec 2, 2023

@linh2931 , it looks loke you removed the current hotstuff_integration changes, so for example block_state.hpp was removed, and block_header_state.hpp is almost emptied. I think you should have left the existing code living in the old classes, even if it is unused for now. We will need both versions, the one with the old code, and the new hotstuff-based ones.

Looking at the changes, I think even more that this change should be done in the main branch, and merged into hotstuff_integration. @arhag what do you think?

I did not remove anything. All existing code except new IF code is in _legacy. I think we want to have a clean block_state/block_header_state {.hpp/.cpp} which only contain the code we are writing for hotstuff. That's why block_header_state{hpp/cpp} only have the new block_header_state_core class. @arhag.

I don't think we are ready to put this into main at the moment. The snapshot fix is one of the reasons.

@greg7mdp
Copy link
Contributor

greg7mdp commented Dec 4, 2023

Upon reflection, I don't think this renaming is the way to go (if it is done in hotstuff_integration and not main).

I think we should leave the old names alone, and create the new classes as block_header_state_v2 and block_state_v2, which is what I have started to do in my branch, and move the hotstuff code to the new classes.

After merging hotstuff_integration to main, we can always do a renaming old classes -> legacy and removing the _v2 suffix from the new classes if we like.

@linh2931
Copy link
Member Author

linh2931 commented Dec 4, 2023

Upon reflection, I don't think this renaming is the way to go (if it is done in hotstuff_integration and not main).

I think we should leave the old names alone, and create the new classes as block_header_state_v2 and block_state_v2, which is what I have started to do in my branch, and move the hotstuff code to the new classes.

After merging hotstuff_integration to main, we can always do a renaming old classes -> legacy and removing the _v2 suffix from the new classes if we like.

We should discuss this at this afternoon's meeting. If we were to do the renaming on main eventually, why not doing it now as we are developing the new classes? The merging seems not that bad as shown when merging from hotsuff_integration to this renaming branch.

@linh2931
Copy link
Member Author

linh2931 commented Dec 6, 2023

Replaced by #1951

@linh2931 linh2931 closed this Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename existing block_header_state and block_state to block_header_state_legacy and block_state_legacy
2 participants