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 all 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
225 changes: 97 additions & 128 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,88 @@ const proposer_policy& block_header_state::get_last_proposed_proposer_policy() c
return *it->second;
}

// This function evaluates possible promotions from pending to 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 the pending slot is open, 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) {
// Common case: if no pending policy and no proposed_finalizer_policies to evaluate,
// just return
if (!prev.pending_finalizer_policy.has_value() && prev.proposed_finalizer_policies.empty() ) {
return;
}

auto lib = next_header_state.core.last_final_block_num();
const auto& prev_pending = prev.pending_finalizer_policy;
const auto& prev_proposed = prev.proposed_finalizer_policies;
auto& next_pending = next_header_state.pending_finalizer_policy;
auto& next_proposed = next_header_state.proposed_finalizer_policies;

// Evaluate pending first.
bool pending_slot_open = true;
if (prev_pending.has_value()) {
if (prev_pending->first <= lib) {
// The block associated with the pending has become final, promote pending to active
next_header_state.active_finalizer_policy = prev_pending->second;
} else {
// Pending not yet to become final, copy it to next_header_state
next_pending = prev_pending;
pending_slot_open = false; // no slot openned up
}
}

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

// Find the target proposed policy which is the proposed policy with the greatest
// associated block number that is less than or equal to the new LIB number
auto first_reversible = std::ranges::find_if(prev_proposed, [&](const auto& p) { return p.first > lib; });
auto target = first_reversible > prev_proposed.begin() ? first_reversible - 1 : prev_proposed.end();

// Promote target policy to pending if the pending slot is available, otherwise
// copy it to next_proposed
if (target != prev_proposed.end()) {
if (pending_slot_open) {
// promote the target to pending
// https://github.com/AntelopeIO/spring/issues/306 will update generation properly
auto block_num = next_header_state.block_num();
next_pending.emplace(block_num, target->second);
} else {
// leave the target alone in the proposed policies
next_proposed.emplace_back(*target);
}

if (first_reversible != prev_proposed.end()) {
// Copy proposed policies with an associated block number greater than that of the target
// proposed policy (implictly remove any proposed policies with an associated block
// number less than that of the target proposed policy)
next_proposed.insert( next_proposed.end(), first_reversible, prev_proposed.end());
}
} else {
// No target proposed policy exists, just copy the previous proposed policies to next
next_proposed = prev_proposed;
}
}

// -------------------------------------------------------------------------------------------------
// `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 +233,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 +242,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 @@ -376,37 +377,5 @@ block_header_state block_header_state::next(const signed_block_header& h, valida
return next_header_state;
}

// -------------------------------------------------------------------------------
// 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;
}

} // namespace eosio::chain

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
4 changes: 0 additions & 4 deletions libraries/chain/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5112,10 +5112,6 @@ block_state_legacy_ptr controller::head_block_state_legacy()const {
});
}

bool controller::head_sanity_check()const {
return apply<bool>(my->chain_head, [](const auto& head) { return head->sanity_check(); });
}

const signed_block_ptr& controller::head_block()const {
return my->chain_head.block();
}
Expand Down
30 changes: 12 additions & 18 deletions libraries/chain/include/eosio/chain/block_header_state.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,6 @@ struct finality_digest_data_v1 {
//
// When this current block itself becomes final, the policy becomes active.
// ------------------------------------------------------------------------------------------
struct finalizer_policy_tracker {
enum class state_t { proposed = 0, pending };
state_t state;
finalizer_policy_ptr policy;
};

struct building_block_input {
block_id_type parent_id;
block_timestamp_type parent_timestamp;
Expand Down Expand Up @@ -86,10 +80,15 @@ struct block_header_state {
// If proposed in B1, B2, .. B12 becomes active in D1
flat_map<block_timestamp_type, proposer_policy_ptr> proposer_policies;

// track in-flight finalizer policies. This is a `multimap` because the same block number
// can hold a `proposed` and a `pending` finalizer_policy. When that block becomes final, the
// `pending` becomes active, and the `proposed` becomes `pending` (for a different block number).
flat_multimap<block_num_type, finalizer_policy_tracker> finalizer_policies;
// Track in-flight proposed finalizer policies.
// When the block associated with a proposed finalizer policy becomes final,
// it becomes pending.
std::vector<std::pair<block_num_type, finalizer_policy_ptr>> proposed_finalizer_policies;
// Track in-flight pending finalizer policy. At most one pending
// finalizer policy at any moment.
// When the block associated with the pending finalizer policy becomes final,
// it becomes active.
std::optional<std::pair<block_num_type, finalizer_policy_ptr>> pending_finalizer_policy;

// generation increases by one each time a new finalizer_policy is proposed in a block
// It matches the finalizer policy generation most recently included in this block's `if_extension` or its ancestors
Expand Down Expand Up @@ -135,8 +134,6 @@ struct block_header_state {
return qc_claim > core.latest_qc_claim();
}

bool sanity_check() const; // does sanity check of block_header_state, returns true if successful

const vector<digest_type>& get_new_protocol_feature_activations() const;
const producer_authority& get_scheduled_producer(block_timestamp_type t) const;

Expand All @@ -149,13 +146,10 @@ using block_header_state_ptr = std::shared_ptr<block_header_state>;

}

FC_REFLECT_ENUM( eosio::chain::finalizer_policy_tracker::state_t, (proposed)(pending))

FC_REFLECT( eosio::chain::finalizer_policy_tracker, (state)(policy))

FC_REFLECT( eosio::chain::block_header_state, (block_id)(header)
(activated_protocol_features)(core)(active_finalizer_policy)
(active_proposer_policy)(proposer_policies)(finalizer_policies)
(finalizer_policy_generation)(last_pending_finalizer_policy_digest)(header_exts))
(active_proposer_policy)(proposer_policies)(proposed_finalizer_policies)
(pending_finalizer_policy)(finalizer_policy_generation)
(last_pending_finalizer_policy_digest))

FC_REFLECT( eosio::chain::finality_digest_data_v1, (major_version)(minor_version)(active_finalizer_policy_generation)(final_on_strong_qc_block_num)(finality_tree_digest)(last_pending_finalizer_policy_and_base_digest) )
2 changes: 0 additions & 2 deletions libraries/chain/include/eosio/chain/block_header_state_legacy.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,6 @@ struct block_header_state_legacy : public detail::block_header_state_legacy_comm
void sign( const signer_callback_type& signer );
void verify_signee()const;

bool sanity_check() const { return true; }

const vector<digest_type>& get_new_protocol_feature_activations()const;
};

Expand Down
Loading
Loading