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: Integrate new finality core algorithm #2245

Merged
merged 24 commits into from
Feb 22, 2024

Conversation

linh2931
Copy link
Member

@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

@arhag arhag linked an issue Feb 20, 2024 that may be closed by this pull request
@ericpassmore
Copy link
Contributor

Note:start
group: IF
category: INTERNALS
summary: Simplification of finality core algorithm
Note:end

libraries/chain/controller.cpp Outdated Show resolved Hide resolved
libraries/chain/controller.cpp Show resolved Hide resolved
@@ -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()};
Copy link
Member

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?

Copy link
Member Author

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.

libraries/chain/include/eosio/chain/block_header_state.hpp Outdated Show resolved Hide resolved
libraries/chain/block_header_state.cpp Outdated Show resolved Hide resolved
libraries/chain/block_header_state_legacy.cpp Outdated Show resolved Hide resolved
libraries/chain/controller.cpp Outdated Show resolved Hide resolved
@greg7mdp
Copy link
Contributor

Shouldn't the finality_core files be in the hotstuff directories?

@linh2931
Copy link
Member Author

Shouldn't the finality_core files be in the hotstuff directories?

I think hotstuff directory will eventually go away and finality_core is mostly for block_header_state.

@greg7mdp
Copy link
Contributor

Was file block_header_state_tests.cpp deleted and not replaced?

@greg7mdp
Copy link
Contributor

I think hotstuff directory will eventually go away.

Maybe so, but not having it in hotstuff is inconsistent for now I think.

// 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();
Copy link
Contributor

@greg7mdp greg7mdp Feb 22, 2024

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:

Suggested change
const size_t ref_index = block_num - last_final_block_num();
const size_t ref_index = block_num - refs.front().block_num();

Copy link
Member Author

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.

Copy link
Contributor

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.

libraries/chain/finality_core.cpp Outdated Show resolved Hide resolved
…estamp to building_block_input instead to avoid of a duplicately passing parent_id
@linh2931
Copy link
Member Author

Was file block_header_state_tests.cpp deleted and not replaced?

It is deleted as the tests there were only for testing previous block_header_state_core's next. New tests can be added later in a new finality_core_tests.cpp.

@greg7mdp
Copy link
Contributor

It is deleted as the tests there were only for testing previous block_header_state_core's next. New tests can be added later in a new finality_core_tests.cpp.

In that case we need a new issue to make sure we don't forget to add these tests.

@linh2931
Copy link
Member Author

In that case we need a new issue to make sure we don't forget to add these tests.

#2259 was created for that.

@linh2931
Copy link
Member Author

Maybe so, but not having it in hotstuff is inconsistent for now I think.

I think for new files we can start to stay away from that directory.

@linh2931 linh2931 merged commit 74da986 into hotstuff_integration Feb 22, 2024
26 checks passed
@linh2931 linh2931 deleted the new_finality_core branch February 22, 2024 22:03
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.

IF: Integrate new finality core algorithm
4 participants