-
Notifications
You must be signed in to change notification settings - Fork 73
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
Rename block_header_state and block_state to block_header_state_legacy and block_state_legacy in preparation for Instant Fininality #1951
Conversation
…, cpp} and rename block_state.{hpp,cpp} to block_state_legacy.{hpp,cpp}
…ock_header_state_legacy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, Lin, sorry you had to redo the work in a new branch. I really feel it is for the best and will make our life easier in the future. Thanks!
libraries/chain/include/eosio/chain/block_header_state_legacy.hpp
Outdated
Show resolved
Hide resolved
No problem. Happy it is the best for our work! |
signal<void(const block_state_ptr&)> irreversible_block; | ||
signal<void(const block_state_legacy_ptr&)> accepted_block_header; | ||
signal<void(const block_state_legacy_ptr&)> accepted_block; | ||
signal<void(const block_state_legacy_ptr&)> irreversible_block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unfortunate. In hotstuff_integration
branch this will need to be a std::variant
of the two types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think we should change these signals to signal signed_block_ptr, block_id
instead of block_state_legacy_ptr
. I think that should be done either in main
and merged to hotstuff_integration
or done in hotstuff_integration
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do this in hotstuff_integration
after merging main
into it, as you have approved the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not be part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although, third-party plugins would need to be updated after this is merged. No third-party plugins will be impacted until 6.0 release. Before then, this will have to change one way or another anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I will go ahead merging this into main in 2 hours.
In preparation of Instant Finality work,
block_header_state
toblock_header_state_legacy
,block_header_state_ptr
toblock_header_state_legacy_ptr
,block_state
toblock_state_legacy
, andblock_state_ptr
toblock_state_legacy_ptr
;block_header_state.{hpp,cpp}
toblock_header_state_legacy.{hpp,cpp}
, andblock_state.{hpp,cpp}
toblock_state_legacy.{hpp,cpp}
;Resolved #1940