IF: Use finalizer diffs in instant_finality_extension #118

heifner · 2024-05-07T21:17:53Z

Instead of placing the entire finalizer set in the instant_finality_extension, only put a diff of the active finalizer policy in the extension.

…ad of a full list of finalizers

unittests/svnn_ibc_tests.cpp

ericpassmore · 2024-05-07T21:27:48Z

Note:start
group: IF
category: PERFORMANCE
summary: Instead of placing the entire finalizer set in the instant_finality_extension, only put a diff of the active finalizer policy in the extension.
Note:end

greg7mdp · 2024-05-08T00:23:25Z

I think ordered_diff should be templatized by size_type, because for policy diffs uint16_t is probably plenty enough to store in the remove_indexes and insert_indexes vectors.

libraries/libfc/include/fc/container/ordered_diff.hpp

libraries/chain/block_header_state.cpp

libraries/chain/include/eosio/chain/block_header_state.hpp

libraries/libfc/include/fc/container/ordered_diff.hpp

libraries/chain/include/eosio/chain/finality/finalizer_policy.hpp

linh2931

I started to wonder if it is worthy the trouble (time to diff and apply_diff, and more code) to pack diff only in the extension. Another side effect is one cannot just look at the extension to see the finalizers; previous extensions need to be checked.

…t. Added asserts to check for overflow.

libraries/libfc/include/fc/container/ordered_diff.hpp

libraries/chain/include/eosio/chain/finality/finalizer_policy.hpp

…o make usage more obvious.

libraries/chain/block_header_state.cpp

libraries/chain/include/eosio/chain/config.hpp

libraries/chain/block_header_state.cpp

greg7mdp · 2024-05-09T18:56:35Z

libraries/chain/include/eosio/chain/finality/finalizer_policy.hpp


 namespace eosio::chain {

+   static_assert(std::numeric_limits<uint16_t>::max() <= config::max_finalizers);


should be static_assert(config::max_finalizers <= std::numeric_limits<uint16_t>::max() + 1)

Yes, already had that in my changes for the next PR. Note you can't use max() + 1 because that overflows. So max_finalizers - 1.

greg7mdp · 2024-05-09T19:07:35Z

unittests/finalizer_update_tests.cpp

@@ -71,6 +71,18 @@ BOOST_AUTO_TEST_CASE(savanna_set_finalizer_multiple_test) { try {
   size_t num_keys    = 50u;
   size_t finset_size = 21u;

+   auto verify_block_finality_generation = [](const signed_block_ptr& block, uint32_t gen, const bls_public_key& key) {


Good check!

No big deal, but I think this is a more accurate name.

Suggested change

auto verify_block_finality_generation = [](const signed_block_ptr& block, uint32_t gen, const bls_public_key& key) {

auto verify_block_finality_policy_diff = [](const signed_block_ptr& block, uint32_t gen, const bls_public_key& key) {

heifner added 6 commits April 29, 2024 18:41

Gh-5 Add ordered_diff

6505e0c

Merge remote-tracking branch 'spring/main' into GH-5-diff-policies

ac63786

Merge remote-tracking branch 'spring/main' into GH-5-diff-policies

c528f0c

GH-5 Add some additional tests cases

7f4879e

GH-5 FC pack supports pair not tuple

1403867

GH-5 Provide a diff of finalizers in instant_finality_extension inste…

ac49b89

…ad of a full list of finalizers

heifner linked an issue May 7, 2024 that may be closed by this pull request

Use diffs for proposer finalizer policy and proposer policy in block_header extension #5

Closed

heifner added the OCI Work exclusive to OCI team label May 7, 2024

heifner commented May 7, 2024

View reviewed changes

unittests/svnn_ibc_tests.cpp Outdated Show resolved Hide resolved

GH-5 Fix include

8000be2

heifner requested review from greg7mdp and linh2931 May 7, 2024 21:26

heifner added 2 commits May 7, 2024 19:14

Merge remote-tracking branch 'spring/main' into GH-5-diff-policies

560eb78

GH-5 shared_ptr not needed

6397cbd

GH-5 Fix merge issue

16628b8

greg7mdp reviewed May 8, 2024

View reviewed changes

libraries/libfc/include/fc/container/ordered_diff.hpp Outdated Show resolved Hide resolved

linh2931 reviewed May 8, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

linh2931 reviewed May 8, 2024

View reviewed changes

heifner added 4 commits May 8, 2024 12:47

GH-5 Reduce max by one to equal uint16_t max

bc78d46

GH-5 Make the index type templated to save space in packed diff_resul…

ad7e9a7

…t. Added asserts to check for overflow.

GH-5 Use uint16_t for index type of diff since max_finalizers fit

7495f2a

GH-5 Add assert, update log message

8456ea5

greg7mdp requested changes May 8, 2024

View reviewed changes

libraries/libfc/include/fc/container/ordered_diff.hpp Show resolved Hide resolved

libraries/chain/include/eosio/chain/finality/finalizer_policy.hpp Outdated Show resolved Hide resolved

heifner added 3 commits May 8, 2024 16:25

GH-5 Take rvalue reference and return the container from apply_diff t…

ad4ced3

…o make usage more obvious.

Merge remote-tracking branch 'spring/main' into GH-5-diff-policies

1fd5fba

GH-5 Track full finalizer policy instead of diffs

6fe5ec2

heifner requested a review from greg7mdp May 8, 2024 23:41

heifner requested a review from linh2931 May 8, 2024 23:41

linh2931 approved these changes May 9, 2024

View reviewed changes

Merge remote-tracking branch 'spring/main' into GH-5-diff-policies

186299e

heifner added the consensus-protocol Change to the consensus protocol. Impacts light client validation. label May 9, 2024

greg7mdp requested changes May 9, 2024

View reviewed changes

libraries/chain/block_header_state.cpp Outdated Show resolved Hide resolved

greg7mdp reviewed May 9, 2024

View reviewed changes

libraries/chain/block_header_state.cpp Show resolved Hide resolved

libraries/chain/include/eosio/chain/config.hpp Outdated Show resolved Hide resolved

greg7mdp reviewed May 9, 2024

View reviewed changes

libraries/chain/block_header_state.cpp Outdated Show resolved Hide resolved

heifner added 4 commits May 9, 2024 12:20

GH-5 Verify instant_finality_extension finalizes diff

0117701

GH-5 Calculate policy instead of passing it

822ac38

GH-5 Verify instant_finality_extension diff

c4d4700

GH-5 Compare with correct in-flight finalizer policy

30c56b7

greg7mdp requested changes May 9, 2024

View reviewed changes

heifner added 2 commits May 9, 2024 14:43

GH-5 Fix static_assert

b9d2d47

GH-5 rename lambda

f69c2b7

heifner mentioned this pull request May 9, 2024

IF: Use proposer diffs in instant_finality_extension #125

Merged

greg7mdp approved these changes May 9, 2024

View reviewed changes

heifner merged commit 72c2958 into main May 9, 2024
36 checks passed

heifner deleted the GH-5-diff-policies branch May 9, 2024 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IF: Use finalizer diffs in instant_finality_extension #118

IF: Use finalizer diffs in instant_finality_extension #118

heifner commented May 7, 2024

ericpassmore commented May 7, 2024

greg7mdp commented May 8, 2024

This comment was marked as resolved.

linh2931 left a comment

greg7mdp May 9, 2024

heifner May 9, 2024

greg7mdp May 9, 2024


		namespace eosio::chain {

		static_assert(std::numeric_limits<uint16_t>::max() <= config::max_finalizers);

	auto verify_block_finality_generation = [](const signed_block_ptr& block, uint32_t gen, const bls_public_key& key) {
	auto verify_block_finality_policy_diff = [](const signed_block_ptr& block, uint32_t gen, const bls_public_key& key) {

IF: Use finalizer diffs in instant_finality_extension #118

IF: Use finalizer diffs in instant_finality_extension #118

Conversation

heifner commented May 7, 2024

ericpassmore commented May 7, 2024

greg7mdp commented May 8, 2024

This comment was marked as resolved.

linh2931 left a comment

Choose a reason for hiding this comment

greg7mdp May 9, 2024

Choose a reason for hiding this comment

heifner May 9, 2024

Choose a reason for hiding this comment

greg7mdp May 9, 2024

Choose a reason for hiding this comment