-
Notifications
You must be signed in to change notification settings - Fork 72
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: Integrate new finality core algorithm #2245
Conversation
Note:start |
…mestamp assert to its correct form
…d block_header_state_core which has been replaced by new finality_core
…operator for qc_claim comparison
… to source file; add comments to functions which did not have comments
libraries/chain/controller.cpp
Outdated
@@ -691,10 +698,14 @@ struct building_block { | |||
.new_protocol_feature_activations = new_protocol_feature_activations() | |||
}; | |||
|
|||
// get current block reference | |||
block_ref current_block {parent_id(), timestamp()}; |
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 doesn't look correct. How can current block have parent id and current timestamp?
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.
Thank you Kevin for catching this! I have fixed it and also renamed it to parent_block to reduce confusion.
Shouldn't the |
I think hotstuff directory will eventually go away and |
Was file |
Maybe so, but not having it in |
// If refs.empty() == true, then by invariant 3, current_block_num() == last_final_block_num(), | ||
// and therefore it is impossible to satisfy the precondition. So going forward, it is safe to assume refs.empty() == false. | ||
|
||
const size_t ref_index = block_num - last_final_block_num(); |
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.
While it works, it is a little odd to use links here. Since we know the refs are monotonically increasing by one block numbers, I think the following is more natural:
const size_t ref_index = block_num - last_final_block_num(); | |
const size_t ref_index = block_num - refs.front().block_num(); |
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.
oh we cannot do that as refs can be empty while last_final_block_num()
always returns a valid value.
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.
line above says:
So going forward, it is safe to assume refs.empty() == false.
…estamp to building_block_input instead to avoid of a duplicately passing parent_id
It is deleted as the tests there were only for testing previous |
In that case we need a new issue to make sure we don't forget to add these tests. |
#2259 was created for that. |
I think for new files we can start to stay away from that directory. |
@arhag proposed a new finality core algorithm, which is simpler and clearer, and makes other issues simpler to implement, for example, #2125 and #2080.
This PR replaced existing
block_header_state_core
with the new algorithm and updated relevant code to use it.Resolved #2244