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.

Sign up for GitHub

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

New implementation of proposed and pending finalizer policies evaluation to fix edge cases in finalizer policy changes #307

Merged
merged 15 commits into from
Jun 25, 2024
Merged

New implementation of proposed and pending finalizer policies evaluation to fix edge cases in finalizer policy changes #307

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1584b9e
Make at most one pending finalizer policy and update tests
linh2931 Jun 17, 2024
c8f81d1
Replace finalizer_policies with proposed_finalizer_policies and pendi…
linh2931 Jun 18, 2024
96d9b4a
Update deep-mind.log
linh2931 Jun 18, 2024
bc96527
Create unit tests for evaluate_finalizer_policies_for_promotion function
linh2931 Jun 18, 2024
db35679
Merge branch 'main' into at_most_one_fin_pol
linh2931 Jun 18, 2024
6151763
update deep-mind.log
linh2931 Jun 18, 2024
6410f20
Copy pending_finalizer_policy directly into active_finalizer_policy i…
linh2931 Jun 18, 2024
1345220
Remove unneeded header_exts from block_header_state's reflect
linh2931 Jun 18, 2024
c8fd384
Merge branch 'main' into at_most_one_fin_pol
linh2931 Jun 18, 2024
fde0ac2
Add tests to simulate finalizer policies change edge cases
linh2931 Jun 20, 2024
b307f4c
Restructure tests and add more comments so that they are easier to un…
linh2931 Jun 21, 2024
4165e87
Remove unnecessary proposed_finalizer_policies_valid and sanity_check…
linh2931 Jun 23, 2024
51e092e
Make evaluate_finalizer_policies_for_promotion() more concise
linh2931 Jun 24, 2024
7d284a1
Remove an outdated comment
linh2931 Jun 24, 2024
9a523b3
Update no_proposed_promotion_no_pending_promotion_1_test
linh2931 Jun 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
253 changes: 131 additions & 122 deletions libraries/chain/block_header_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,8 @@ digest_type block_header_state::compute_base_digest() const {
fc::raw::pack( enc, header );
fc::raw::pack( enc, core );

for (const auto& fp_pair : finalizer_policies) {
fc::raw::pack( enc, fp_pair.first );
const finalizer_policy_tracker& tracker = fp_pair.second;
fc::raw::pack( enc, tracker.state );
assert(tracker.policy);
fc::raw::pack( enc, *tracker.policy );
}
fc::raw::pack( enc, proposed_finalizer_policies );
fc::raw::pack( enc, pending_finalizer_policy );

assert(active_proposer_policy);
fc::raw::pack( enc, *active_proposer_policy );
Expand Down Expand Up @@ -68,37 +63,19 @@ const vector<digest_type>& block_header_state::get_new_protocol_feature_activati

// The last proposed finalizer policy if none proposed or pending then the active finalizer policy
const finalizer_policy& block_header_state::get_last_proposed_finalizer_policy() const {
if (!finalizer_policies.empty()) {
for (auto ritr = finalizer_policies.rbegin(); ritr != finalizer_policies.rend(); ++ritr) {
if (ritr->second.state == finalizer_policy_tracker::state_t::proposed)
return *ritr->second.policy;
}
return *finalizer_policies.rbegin()->second.policy;
if (!proposed_finalizer_policies.empty()) {
return *proposed_finalizer_policies.back().second;
} else if (pending_finalizer_policy.has_value()) {
return *pending_finalizer_policy->second;
}
return *active_finalizer_policy;
}

// The last pending finalizer policy if none pending then the active finalizer policy
// Used to populate last_pending_finalizer_policy_digest which is expected to be the highest generation pending
// Used to populate last_pending_finalizer_policy_digest
const finalizer_policy& block_header_state::get_last_pending_finalizer_policy() const {
if (!finalizer_policies.empty()) {
// lambda only used when asserts enabled
[[maybe_unused]] auto highest_pending_generation = [this]() {
finalizer_policy_ptr highest;
for (auto ritr = finalizer_policies.rbegin(); ritr != finalizer_policies.rend(); ++ritr) {
if (ritr->second.state == finalizer_policy_tracker::state_t::pending) {
if (!highest || highest->generation < ritr->second.policy->generation)
highest = ritr->second.policy;
}
}
return highest;
};
for (auto ritr = finalizer_policies.rbegin(); ritr != finalizer_policies.rend(); ++ritr) {
if (ritr->second.state == finalizer_policy_tracker::state_t::pending) {
assert(highest_pending_generation() == ritr->second.policy);
return *ritr->second.policy;
}
}
if (pending_finalizer_policy.has_value()) {
return *pending_finalizer_policy->second;
}
return *active_finalizer_policy;
}
Expand All @@ -114,6 +91,121 @@ const proposer_policy& block_header_state::get_last_proposed_proposer_policy() c
return *it->second;
}

bool proposed_finalizer_policies_valid(const std::vector<std::pair<block_num_type, finalizer_policy_ptr>>& proposed_fin_policies) {
// check that proposed_finalizer_policies is sorted in ascending order without
// repeations; this also implies at most *one* proposed finalizer policy exists
// for one block number.
block_num_type prev_block_num(0);

for (auto it = proposed_fin_policies.begin(); it != proposed_fin_policies.end(); ++it) {
if (it->first < prev_block_num) // must be in strictly ascending order
return false;
else
prev_block_num = it->first;
}
return true;
}

// This function evaluates possible promotions from pending to active
// (removing any pending policies that are known at that time to never become active)
// and from proposed to pending (removing any proposed policies that are known at that
// time to never become pending)
//
// In particular,
// 1. If there is a pending policy, determine whether it should be promoted to active.
// If the associated block number is less than or equal to the new LIB number,
// the pending policy should be promoted to active. This also means the pending slot
// is now open for a possible promotion of a proposed policy to pending.
// This guarantees that there will be at most one pending policy at any given time.
// 2. If there is any proposed policy with an associated block number that is less than
// or equal to the new LIB number:
// i. Find the proposed policy with the greatest associated block number that is still
// less than or equal to the new LIB number (call this the target proposed policy).
// ii. Remove any proposed policies with an associated block number less than that of
// the target proposed policy.
// iii. If there is no pending policy, promote that target proposed policy to pending.
// Otherwise, leave the target proposed policy (and any other proposed policies with
// greater associated block numbers) alone in the proposed policy queue.
//
void evaluate_finalizer_policies_for_promotion(const block_header_state& prev,
Copy link
Contributor

@greg7mdp greg7mdp Jun 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was attempting to see if I could come up with a more concise version of this function and here is what I came up with (it passes the tests if you comment out the generation update which is new and not supported by the tests):

void evaluate_finalizer_policies_for_promotion(const block_header_state& prev,
                                               block_header_state& next_header_state) {
   if (prev.pending_finalizer_policy || !prev.proposed_finalizer_policies.empty()) {
      auto lib = next_header_state.core.last_final_block_num();
      next_header_state.proposed_finalizer_policies = prev.proposed_finalizer_policies;
      next_header_state.pending_finalizer_policy    = prev.pending_finalizer_policy;

      auto& pending  = next_header_state.pending_finalizer_policy;
      auto& proposed = next_header_state.proposed_finalizer_policies;
      auto first_reversible = std::ranges::find_if(proposed, [&](const auto& p) { return p.first > lib; });

      if (pending) {
         if (pending->first <= lib) {
            // promote it to active
            next_header_state.active_finalizer_policy = std::move(pending->second);
            //next_header_state.active_finalizer_policy->generation = prev.active_finalizer_policy->generation + 1;
            pending.reset();
         } else {
            if (first_reversible > proposed.begin()) {
               // if we have more than one irreversible policies in `proposed`, we need to keep only the latest one.
               proposed.erase(proposed.begin(), first_reversible - 1);
            }
         }
      }

      if (!pending) {
         // check if a proposed policy is eligible to become pending (i.e. associated with an
         // irreversible block).
         // This will be the policy associated to the largest block number <= lib.
         if (first_reversible > proposed.begin()) {
            pending = *(first_reversible - 1);
            pending->first = next_header_state.block_num();
            proposed.erase(proposed.begin(), first_reversible);
         }
      }
   }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I adapted and tried to conform to the algorithm specified in the issue description #292 as it is easier to follow.

51e092e

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I myself find this version significantly harder to follow.

block_header_state& next_header_state) {
assert(proposed_finalizer_policies_valid(prev.proposed_finalizer_policies));

auto lib = next_header_state.core.last_final_block_num();

// Promote pending to active if it is time to do so, otherwise keep
// pending as is by copying it to next_header_state
bool pending_slot_open = true;
if (prev.pending_finalizer_policy.has_value()) {
if (prev.pending_finalizer_policy->first <= lib) {
// The block associated with the policy has become final,
// promote it to active
next_header_state.active_finalizer_policy = prev.pending_finalizer_policy->second;
} else {
// Copy to next_header_state
next_header_state.pending_finalizer_policy = prev.pending_finalizer_policy;
pending_slot_open = false; // no slot openned up
}
}

// Nothing more to do if existing proposed_finalizer_policies is empty
if (prev.proposed_finalizer_policies.empty()) {
return;
}

// * Find the target proposed policy (a policy whose associated block number is
// the largest block number is less than or eqaul to LIB.
// * Find the first policy whose associated block number is greater than LIB;
// this is used to garbage collecting proposed policies that will never become
// pending.
auto itr = prev.proposed_finalizer_policies.begin();
auto target_policy_itr = prev.proposed_finalizer_policies.end();
auto first_policy_after_lib_itr = prev.proposed_finalizer_policies.end();
while (itr != prev.proposed_finalizer_policies.end()) {
if (itr->first <= lib) {
// proposed_finalizer_policies is sorted in the order of block_num
assert(target_policy_itr == prev.proposed_finalizer_policies.end() ||
target_policy_itr->first < itr->first);

target_policy_itr = itr;
} else {
// first_policy_after_lib_itr can be set only once
assert(first_policy_after_lib_itr == prev.proposed_finalizer_policies.end());

first_policy_after_lib_itr = itr;
break;
}

++itr;
}

// Promote target policy to pending if the pending slot is available, otherwise
// copy it to next_header_state.proposed_finalizer_policies
if (target_policy_itr != prev.proposed_finalizer_policies.end()) {
if (pending_slot_open) {
// When the block with `block_num` becomes final, the pending polilcy is promoted
// to active
auto block_num = next_header_state.block_num();
next_header_state.pending_finalizer_policy.emplace(block_num, target_policy_itr->second);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should update the generation to 1 + active_finalizer_policy->generation as soon as it becomes pending.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be handled by #306. A comment was added to the code indicating it will be done there.

Copy link
Contributor

@greg7mdp greg7mdp Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add this one line in this PR so that the generation of pending policies is correct for @systemzax 's testing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@systemzax is waiting for #306 to test:
Now when we promote a proposed finalizer policy to pending, we set its generation number to 1 plus the generation number of the prior active finalizer_policy (which may have just become active within the same next function).

The change is to be done there right after this PR.

} else {
next_header_state.proposed_finalizer_policies.emplace_back(*target_policy_itr);
}
}

// copy remainder of proposed policies
if (first_policy_after_lib_itr != prev.proposed_finalizer_policies.end()) {
assert(next_header_state.proposed_finalizer_policies.empty() ||
next_header_state.proposed_finalizer_policies.back().first < first_policy_after_lib_itr->first);

next_header_state.proposed_finalizer_policies.insert(
next_header_state.proposed_finalizer_policies.end(),
first_policy_after_lib_itr,
prev.proposed_finalizer_policies.end());
}

assert(proposed_finalizer_policies_valid(next_header_state.proposed_finalizer_policies));
}

// -------------------------------------------------------------------------------------------------
// `finish_next` updates the next `block_header_state` according to the contents of the
// header extensions (either new protocol_features or instant_finality_extension) applicable to this
Expand Down Expand Up @@ -174,64 +266,7 @@ void finish_next(const block_header_state& prev,
// ----------------
next_header_state.active_finalizer_policy = prev.active_finalizer_policy;

if (!prev.finalizer_policies.empty()) {
auto lib = next_header_state.core.last_final_block_num();
auto it = prev.finalizer_policies.begin();
if (it->first > lib) {
// we have at least one `finalizer_policy` in our map, but none of these is
// due to become active of this block because lib has not advanced enough, so
// we just copy the multimap and keep using the same `active_finalizer_policy`
// ---------------------------------------------------------------------------
next_header_state.finalizer_policies = prev.finalizer_policies;
} else {
auto next_block_num = next_header_state.block_num();

while (it != prev.finalizer_policies.end() && it->first <= lib) {
const finalizer_policy_tracker& tracker = it->second;
if (tracker.state == finalizer_policy_tracker::state_t::pending) {
// new finalizer_policy becomes active
// -----------------------------------
next_header_state.active_finalizer_policy.reset(new finalizer_policy(*tracker.policy));
} else {
assert(tracker.state == finalizer_policy_tracker::state_t::proposed);

// The block where finalizer_policy was proposed has became final. The finalizer
// policy will become active when `next_block_num` becomes final.
//
// So `tracker.policy` should become `pending` at `next_block_num`.
//
// Either insert a new `finalizer_policy_tracker` value, or update the `pending`
// policy if there is already one at `next_block_num` (which can happen when
// finality advances multiple block at a time, and more than one policy move from
// proposed to pending.
//
// Since we iterate finalizer_policies which is a multimap sorted by block number,
// the last one we add will be for the highest block number, which is what we want.
// ---------------------------------------------------------------------------------
auto range = next_header_state.finalizer_policies.equal_range(next_block_num);
auto itr = range.first;
for (; itr != range.second; ++itr) {
if (itr->second.state == finalizer_policy_tracker::state_t::pending) {
itr->second.policy = tracker.policy;
break;
}
}
if (itr == range.second) {
// there wasn't already a pending one for `next_block_num`, add a new tracker
finalizer_policy_tracker t { finalizer_policy_tracker::state_t::pending, tracker.policy };
next_header_state.finalizer_policies.emplace(next_block_num, std::move(t));
}
}
++it;
}
if (it != prev.finalizer_policies.end()) {
// copy remainder of pending finalizer_policy changes
// --------------------------------------------------
next_header_state.finalizer_policies.insert(boost::container::ordered_unique_range_t(),
it, prev.finalizer_policies.end());
}
}
}
evaluate_finalizer_policies_for_promotion(prev, next_header_state);

next_header_state.last_pending_finalizer_policy_digest = fc::sha256::hash(next_header_state.get_last_pending_finalizer_policy());

Expand All @@ -240,17 +275,16 @@ void finish_next(const block_header_state& prev,

// a new `finalizer_policy` was proposed in the previous block, and is present in the previous
// block's header extensions.
// Add this new proposal to the `finalizer_policies` multimap which tracks the in-flight proposals,
// increment the generation number, and log that proposal (debug level).
// Add this new proposal to the `proposed_finalizer_policies` which tracks the in-flight proposals,
// and log that proposal (debug level).
// ------------------------------------------------------------------------------------------------
if (log)
dlog("New finalizer policy proposed in block ${id}..: ${pol}",
("id", prev.block_id.str().substr(8,16))("pol", new_finalizer_policy));
next_header_state.finalizer_policy_generation = new_finalizer_policy.generation;
next_header_state.finalizer_policies.emplace(
next_header_state.block_num(),
finalizer_policy_tracker{finalizer_policy_tracker::state_t::proposed,
std::make_shared<finalizer_policy>(std::move(new_finalizer_policy))});
next_header_state.proposed_finalizer_policies.emplace_back(
std::make_pair(next_header_state.block_num(),
std::make_shared<finalizer_policy>(std::move(new_finalizer_policy))));
} else {
next_header_state.finalizer_policy_generation = prev.finalizer_policy_generation;
}
Expand Down Expand Up @@ -380,32 +414,7 @@ block_header_state block_header_state::next(const signed_block_header& h, valida
// do some sanity checks on block_header_state
// -------------------------------------------------------------------------------
bool block_header_state::sanity_check() const {
// check that we have at most *one* proposed and *one* pending `finalizer_policy`
// for any block number
// -----------------------------------------------------------------------------
block_num_type block_num(0);
bool pending{false}, proposed{false};

for (auto it = finalizer_policies.begin(); it != finalizer_policies.end(); ++it) {
if (block_num != it->first) {
pending = proposed = false;
block_num = it->first;
}
const auto& tracker = it->second;
if (tracker.state == finalizer_policy_tracker::state_t::proposed) {
if (proposed)
return false;
else
proposed = true;
}
if (tracker.state == finalizer_policy_tracker::state_t::pending) {
if (pending)
return false;
else
pending = true;
}
}
return true;
return proposed_finalizer_policies_valid(proposed_finalizer_policies);
}

} // namespace eosio::chain
Expand Down
13 changes: 7 additions & 6 deletions libraries/chain/block_state.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ block_state_ptr block_state::create_if_genesis_block(const block_state_legacy& b
result.active_proposer_policy->active_time = bsp.timestamp();
result.active_proposer_policy->proposer_schedule = bsp.active_schedule;
result.proposer_policies = {}; // none pending at IF genesis block
result.finalizer_policies = {}; // none pending at IF genesis block
result.proposed_finalizer_policies = {}; // none proposed at IF genesis block
result.pending_finalizer_policy = std::nullopt; // none pending at IF genesis block
result.finalizer_policy_generation = 1;
result.header_exts = bsp.header_exts;

Expand Down Expand Up @@ -144,7 +145,8 @@ block_state::block_state(snapshot_detail::snapshot_block_state_v7&& sbs)
.active_finalizer_policy = std::move(sbs.active_finalizer_policy),
.active_proposer_policy = std::move(sbs.active_proposer_policy),
.proposer_policies = std::move(sbs.proposer_policies),
.finalizer_policies = std::move(sbs.finalizer_policies),
.proposed_finalizer_policies = std::move(sbs.proposed_finalizer_policies),
.pending_finalizer_policy = std::move(sbs.pending_finalizer_policy),
.finalizer_policy_generation = sbs.finalizer_policy_generation,
.last_pending_finalizer_policy_digest = sbs.last_pending_finalizer_policy_digest
}
Expand Down Expand Up @@ -345,10 +347,9 @@ finality_data_t block_state::get_finality_data() {
// For Genesis Block, use the active finalizer policy which was proposed in the block.
proposed_finalizer_policy = *active_finalizer_policy;
} else {
auto range = finalizer_policies.equal_range(block_num());
for (auto itr = range.first; itr != range.second; ++itr) {
if (itr->second.state == finalizer_policy_tracker::state_t::proposed) {
proposed_finalizer_policy = *itr->second.policy;
for (const auto& p: proposed_finalizer_policies) {
if (p.first == block_num()) {
proposed_finalizer_policy = *p.second;
break;
}
}
Expand Down
Loading
Loading