Skip to content

IF: Update new proposer policy algorithm #74

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

Merged
heifner merged 12 commits into from
May 2, 2024
Merged

IF: Update new proposer policy algorithm #74

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c4bbe57
GH-6 Fix issue with setting proposed proposer schedule more than once…
heifner Apr 26, 2024
4c0ff25
GH-6 Add comment
heifner Apr 30, 2024
528490f
Merge remote-tracking branch 'spring/main' into GH-6-proposer-policy-2
heifner Apr 30, 2024
5adf5f0
GH-6 Additional fixes and test cases
heifner Apr 30, 2024
51d4488
GH-6 Update comment
heifner Apr 30, 2024
7e10458
GH-6 Update logic to increment version on every new proposal schedule…
heifner May 1, 2024
5c310b5
GH-6 Update tests for new version change scheme
heifner May 1, 2024
887b057
GH-6 Revert back to flat_map for simpler and not storing unneeded imp…
heifner May 1, 2024
af51f07
Merge remote-tracking branch 'spring/main' into GH-6-proposer-policy-2
heifner May 1, 2024
ce2cb2e
GH-6 Simplify by doing diff during assemble_block
heifner May 2, 2024
091ea81
GH-6 Use optional instead of tuple
heifner May 2, 2024
2778bf7
Merge branch 'main' into GH-6-proposer-policy-2
heifner May 2, 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
61 changes: 25 additions & 36 deletions libraries/chain/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,38 +491,27 @@ struct building_block {

uint32_t get_block_num() const { return block_num; }

// returns the next proposer schedule version and true if different
// returns the next proposer schedule version and true if producers should be proposed in block
// if producers is not different then returns the current schedule version (or next schedule version)
std::tuple<uint32_t, bool> get_next_proposer_schedule_version(const vector<producer_authority>& producers) const {
std::tuple<uint32_t, bool> get_next_proposer_schedule_version(const shared_vector<shared_producer_authority>& producers) const {
Copy link
Contributor

@greg7mdp greg7mdp May 2, 2024

Choose a reason for hiding this comment

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

This could return std::optional<uint32_t>, i.e. return a new version if one is needed.

assert(active_proposer_policy);

auto get_next_sched = [&]() -> const producer_authority_schedule& {
// if there are any policies already proposed but not active yet then they are what needs to be compared
if (!parent.proposer_policies.empty()) {
block_timestamp_type active_time = detail::get_next_next_round_block_time(timestamp);
if (auto itr = parent.proposer_policies.find(active_time); itr != parent.proposer_policies.cend()) {
// Same active time, a new proposer schedule will replace this entry, `next` therefore is the previous
if (itr != parent.proposer_policies.begin()) {
return (--itr)->second->proposer_schedule;
}
// no previous to what will be replaced, use active
return active_proposer_policy->proposer_schedule;
}
// will not replace any proposed policies, use next to become active
return parent.proposer_policies.begin()->second->proposer_schedule;
if (!parent.proposer_policies.empty()) { // proposed in-flight
// return the last proposed policy to use for comparison
return (--parent.proposer_policies.end())->second->proposer_schedule;
}

// none currently in-flight, use active
return active_proposer_policy->proposer_schedule;
};

const producer_authority_schedule& lhs = get_next_sched();

if (std::ranges::equal(lhs.producers, producers)) {
return {lhs.version, false};
auto v = lhs.version;
if (!std::equal(lhs.producers.begin(), lhs.producers.end(), producers.begin(), producers.end())) {
++v;
return {v, true};
}

return {lhs.version + 1, true};
return {v, false};
}

};
Expand Down Expand Up @@ -613,7 +602,7 @@ struct building_block {
v);
}

std::tuple<uint32_t, bool> get_next_proposer_schedule_version(const vector<producer_authority>& producers) const {
std::tuple<uint32_t, bool> get_next_proposer_schedule_version(const shared_vector<shared_producer_authority>& producers) const {
return std::visit(
overloaded{[](const building_block_legacy&) -> std::tuple<uint32_t, bool> { return {-1, false}; },
[&](const building_block_if& bb) -> std::tuple<uint32_t, bool> {
Expand Down Expand Up @@ -911,7 +900,7 @@ struct pending_state {
_block_stage);
}

std::tuple<uint32_t, bool> get_next_proposer_schedule_version(const vector<producer_authority>& producers) const {
std::tuple<uint32_t, bool> get_next_proposer_schedule_version(const shared_vector<shared_producer_authority>& producers) const {
return std::visit(overloaded{
[&](const building_block& stage) -> std::tuple<uint32_t, bool> {
return stage.get_next_proposer_schedule_version(producers);
Expand Down Expand Up @@ -3146,10 +3135,16 @@ struct controller_impl {
auto process_new_proposer_policy = [&](auto&) -> void {
const auto& gpo = db.get<global_property_object>();
if (gpo.proposed_schedule_block_num) {
new_proposer_policy = std::make_unique<proposer_policy>();
new_proposer_policy->active_time = detail::get_next_next_round_block_time(bb.timestamp());
new_proposer_policy->proposer_schedule = producer_authority_schedule::from_shared(gpo.proposed_schedule);
ilog("Scheduling proposer schedule change at ${t}: ${s}", ("t", new_proposer_policy->active_time)("s", new_proposer_policy->proposer_schedule));

auto [version, should_propose] = pending->get_next_proposer_schedule_version(gpo.proposed_schedule.producers);
if (should_propose) {
new_proposer_policy = std::make_unique<proposer_policy>();
new_proposer_policy->active_time = detail::get_next_next_round_block_time(bb.timestamp());
new_proposer_policy->proposer_schedule = producer_authority_schedule::from_shared(gpo.proposed_schedule);
new_proposer_policy->proposer_schedule.version = version;
ilog("Scheduling proposer schedule change at ${t}: ${s}",
("t", new_proposer_policy->active_time)("s", new_proposer_policy->proposer_schedule));
}
Copy link
Contributor

@greg7mdp greg7mdp May 2, 2024

Choose a reason for hiding this comment

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

No need to change if you don't want to, but I kinda like this more functional style where the lambda returns the new proposer_policy (if any) , and we use the return value directly without an intermediate variable.

         // Any proposer policy?
         auto process_new_proposer_policy = [&](auto&) -> std::unique_ptr<proposer_policy> {
            std::unique_ptr<proposer_policy> new_proposer_policy;
            const auto& gpo = db.get<global_property_object>();
            if (gpo.proposed_schedule_block_num) {

               auto [version, should_propose] = pending->get_next_proposer_schedule_version(gpo.proposed_schedule.producers);
               if (should_propose) {
                  new_proposer_policy                    = std::make_unique<proposer_policy>();
                  new_proposer_policy->active_time       = detail::get_next_next_round_block_time(bb.timestamp());
                  new_proposer_policy->proposer_schedule = producer_authority_schedule::from_shared(gpo.proposed_schedule);
                  new_proposer_policy->proposer_schedule.version = version;
                  ilog("Scheduling proposer schedule change at ${t}: ${s}",
                       ("t", new_proposer_policy->active_time)("s", new_proposer_policy->proposer_schedule));
               }

               db.modify( gpo, [&]( auto& gp ) {
                  gp.proposed_schedule_block_num = std::optional<block_num_type>();
                  gp.proposed_schedule.version = 0;
                  gp.proposed_schedule.producers.clear();
               });
            }
            return new_proposer_policy;
         };

         auto assembled_block =
            bb.assemble_block(thread_pool.get_executor(), protocol_features.get_protocol_feature_set(), fork_db,
                              apply_s<std::unique_ptr<proposer_policy>>(chain_head, process_new_proposer_policy),
                              validating, std::move(validating_qc_data), validating_bsp);


db.modify( gpo, [&]( auto& gp ) {
gp.proposed_schedule_block_num = std::optional<block_num_type>();
Expand Down Expand Up @@ -5232,25 +5227,19 @@ int64_t controller_impl::set_proposed_producers( vector<producer_authority> prod

assert(pending);

auto [version, diff] = pending->get_next_proposer_schedule_version(producers);
if (!diff)
return version;

producer_authority_schedule sch;
sch.version = version;
// sch.version is set in assemble_block
sch.producers = std::move(producers);

ilog( "proposed producer schedule with version ${v}", ("v", sch.version) );

// overwrite any existing proposed_schedule set earlier in this block
// store schedule in gpo so it will be rolledback if transaction fails
auto cur_block_num = chain_head.block_num() + 1;
auto& gpo = db.get<global_property_object>();
db.modify( gpo, [&]( auto& gp ) {
gp.proposed_schedule_block_num = cur_block_num;
gp.proposed_schedule = sch;
});

return sch.version;
return std::numeric_limits<uint32_t>::max();
}

int64_t controller_impl::set_proposed_producers_legacy( vector<producer_authority> producers ) {
Expand Down
5 changes: 5 additions & 0 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 @@ -69,6 +69,11 @@ struct block_header_state {
proposer_policy_ptr active_proposer_policy; // producer authority schedule, supports `digest()`

// block time when proposer_policy will become active
// current algorithm only two entries possible, for the next,next round and one for block round after that
// The active time is the next,next producer round. For example,
// round A [1,2,..12], next_round B [1,2,..12], next_next_round C [1,2,..12], D [1,2,..12]
// If proposed in A1, A2, .. A12 becomes active in C1
// 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
Expand Down
6 changes: 4 additions & 2 deletions libraries/chain/include/eosio/chain/webassembly/interface.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ namespace webassembly {
*
* @param packed_producer_schedule - vector of producer keys
*
* @return -1 if proposing a new producer schedule was unsuccessful, otherwise returns the version of the new proposed schedule.
* @return pre-savanna: -1 if proposing a new producer schedule was unsuccessful, otherwise returns the version of the new proposed schedule.
* post-savanna: -1 if proposing a new producer schedule was unsuccessful, otherwise returns max uint32_t
*/
int64_t set_proposed_producers(legacy_span<const char> packed_producer_schedule);

Expand All @@ -169,7 +170,8 @@ namespace webassembly {
* @param packed_producer_format - format of the producer data blob.
* @param packed_producer_schedule - packed data of representing the producer schedule in the format indicated.
*
* @return -1 if proposing a new producer schedule was unsuccessful, otherwise returns the version of the new proposed schedule.
* @return pre-savanna: -1 if proposing a new producer schedule was unsuccessful, otherwise returns the version of the new proposed schedule.
* post-savanna: -1 if proposing a new producer schedule was unsuccessful, otherwise returns max uint32_t
*/
int64_t set_proposed_producers_ex(uint64_t packed_producer_format, legacy_span<const char> packed_producer_schedule);

Expand Down
145 changes: 132 additions & 13 deletions unittests/producer_schedule_if_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,19 @@ bool compare_schedules( const vector<producer_authority>& a, const producer_auth
BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) try {
create_accounts( {"alice"_n,"bob"_n,"carol"_n} );

// set_producers in same block, do it the explicit way to use a diff expiration and avoid duplicate trx
auto set_producers_force = [&](const std::vector<account_name>& producers) {
static int unique = 0; // used to force uniqueness of transaction
auto schedule = get_producer_authorities( producers );
fc::variants schedule_variant;
schedule_variant.reserve(schedule.size());
for( const auto& e: schedule ) {
schedule_variant.emplace_back(e.get_abi_variant());
}
push_action( config::system_account_name, "setprods"_n, config::system_account_name,
fc::mutable_variant_object()("schedule", schedule_variant), DEFAULT_EXPIRATION_DELTA + (++unique));
};

while (control->head_block_num() < 3) {
produce_block();
}
Expand All @@ -132,7 +145,7 @@ BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) t

// set a new proposer policy sch1
set_producers( {"alice"_n} );
vector<producer_authority> sch1 = {
vector<producer_authority> alice_sch = {
producer_authority{"alice"_n, block_signing_authority_v0{1, {{get_public_key("alice"_n, "active"), 1}}}}
};

Expand All @@ -145,15 +158,15 @@ BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) t

// set another ploicy to have multiple pending different active time policies
set_producers( {"bob"_n,"carol"_n} );
vector<producer_authority> sch2 = {
vector<producer_authority> bob_carol_sch = {
producer_authority{"bob"_n, block_signing_authority_v0{ 1, {{get_public_key("bob"_n, "active"),1}}}},
producer_authority{"carol"_n, block_signing_authority_v0{ 1, {{get_public_key("carol"_n, "active"),1}}}}
};
produce_block();

// set another ploicy should replace sch2
set_producers( {"bob"_n,"alice"_n} );
vector<producer_authority> sch3 = {
vector<producer_authority> bob_alice_sch = {
producer_authority{"bob"_n, block_signing_authority_v0{ 1, {{get_public_key("bob"_n, "active"),1}}}},
producer_authority{"alice"_n, block_signing_authority_v0{ 1, {{get_public_key("alice"_n, "active"),1}}}}
};
Expand All @@ -163,13 +176,13 @@ BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) t

// sch1 must become active no later than 2 rounds but sch2 cannot become active yet
BOOST_CHECK_EQUAL( control->active_producers().version, 1u );
BOOST_CHECK_EQUAL( true, compare_schedules( sch1, control->active_producers() ) );
BOOST_CHECK_EQUAL( true, compare_schedules( alice_sch, control->active_producers() ) );

produce_blocks(config::producer_repetitions);

// sch3 becomes active
BOOST_CHECK_EQUAL( 2u, control->active_producers().version ); // should be 2 as sch2 was replaced by sch3
BOOST_CHECK_EQUAL( true, compare_schedules( sch3, control->active_producers() ) );
// sch3 becomes active, version should be 3 even though sch2 was replaced by sch3
BOOST_CHECK_EQUAL( 3u, control->active_producers().version );
BOOST_CHECK_EQUAL( true, compare_schedules( bob_alice_sch, control->active_producers() ) );

// get to next producer round
auto prod = produce_block()->producer;
Expand All @@ -179,19 +192,125 @@ BOOST_FIXTURE_TEST_CASE( proposer_policy_progression_test, validating_tester ) t
set_producers( {"bob"_n,"alice"_n} ); // same as before, so no change
produce_blocks(config::producer_repetitions);
produce_blocks(config::producer_repetitions);
BOOST_CHECK_EQUAL( 2u, control->active_producers().version ); // should be 2 as not different so no change
BOOST_CHECK_EQUAL( true, compare_schedules( sch3, control->active_producers() ) );
BOOST_CHECK_EQUAL( 3u, control->active_producers().version ); // should be 3 as not different so no change
BOOST_CHECK_EQUAL( true, compare_schedules( bob_alice_sch, control->active_producers() ) );

// test no change to proposed schedule, only the first one will take affect
for (size_t i = 0; i < config::producer_repetitions*2-1; ++i) {
BOOST_CHECK_EQUAL( 2u, control->active_producers().version ); // should be 2 as not taken affect yet
BOOST_CHECK_EQUAL( true, compare_schedules( sch3, control->active_producers() ) );
BOOST_CHECK_EQUAL( 3u, control->active_producers().version ); // should be 3 as not taken affect yet
BOOST_CHECK_EQUAL( true, compare_schedules( bob_alice_sch, control->active_producers() ) );
set_producers( {"bob"_n,"carol"_n} );
set_producers_force({"bob"_n,"carol"_n} );
set_producers_force({"bob"_n,"carol"_n} );
produce_block();
}
produce_block();
BOOST_CHECK_EQUAL( 3u, control->active_producers().version ); // should be 3 now as bob,carol now active
BOOST_CHECK_EQUAL( true, compare_schedules( sch2, control->active_producers() ) );
BOOST_CHECK_EQUAL( 4u, control->active_producers().version ); // should be 4 now as bob,carol now active
BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );

// get to next producer round
prod = produce_block()->producer;
for (auto b = produce_block(); b->producer == prod; b = produce_block());

// test change in same block where there is an existing proposed that is the same
set_producers( {"bob"_n,"alice"_n} );
produce_block();
set_producers( {"bob"_n,"carol"_n} );
set_producers_force({"bob"_n,"carol"_n} );
produce_blocks(config::producer_repetitions-1);
produce_blocks(config::producer_repetitions);
BOOST_CHECK_EQUAL( 6u, control->active_producers().version ); // should be 6 now as bob,carol now active
BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );

// test change in same block where there is no change
set_producers( {"bob"_n,"alice"_n} );
set_producers_force({"bob"_n,"carol"_n} ); // put back, no change expected
produce_blocks(config::producer_repetitions);
produce_blocks(config::producer_repetitions);
BOOST_CHECK_EQUAL( 6u, control->active_producers().version ); // should be 6 now as bob,carol is still active
BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );

// get to next producer round
prod = produce_block()->producer;
for (auto b = produce_block(); b->producer == prod; b = produce_block());

// test two in-flight
// round A [1,2,..12], next_round B [1,2,..12], next_next_round C [1,2,..12], D [1,2,..12]
// propose P1 in A2, active in C1
// propose P2 in B2, active in D1
// propose P3 in B3, active in D1, replaces P2
produce_block();
set_producers({"alice"_n}); // A2, P1
produce_blocks(config::producer_repetitions-1); // A12
produce_block();
set_producers({"bob"_n,"carol"_n}); // B2
produce_block();
set_producers({"bob"_n, "alice"_n} ); // P3
produce_blocks(config::producer_repetitions-2); // B12
produce_block(); // C1
BOOST_CHECK_EQUAL( 7u, control->active_producers().version );
BOOST_CHECK_EQUAL( true, compare_schedules( alice_sch, control->active_producers() ) );
produce_blocks(config::producer_repetitions); // D1
BOOST_CHECK_EQUAL( 9u, control->active_producers().version );
BOOST_CHECK_EQUAL( true, compare_schedules( bob_alice_sch, control->active_producers() ) );

// get to next producer round
prod = produce_block()->producer;
for (auto b = produce_block(); b->producer == prod; b = produce_block());

// test two in-flight, P1 == P3, so no change
// round A [1,2,..12], next_round B [1,2,..12], next_next_round C [1,2,..12], D [1,2,..12]
// propose P1 in A2, active in C1
// propose P2 in B2, active in D1
// propose P3 in B3, active in D1, replaces P2
produce_block();
set_producers({"bob"_n,"carol"_n}); // A2, P1
produce_blocks(config::producer_repetitions-1); // A12
produce_block();
set_producers({"alice"_n}); // B2
produce_block();
set_producers({"bob"_n,"carol"_n}); // P3 == P1
produce_blocks(config::producer_repetitions-2); // B12
produce_block(); // C1
BOOST_CHECK_EQUAL( 10u, control->active_producers().version );
BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );
produce_blocks(config::producer_repetitions); // D1
BOOST_CHECK_EQUAL( 12u, control->active_producers().version );
BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );

// get to next producer round
prod = produce_block()->producer;
for (auto b = produce_block(); b->producer == prod; b = produce_block());

// test two in-flight, ultimately no change
produce_block(); // 1
set_producers({"bob"_n,"carol"_n});
produce_block(); // 2
set_producers({"alice"_n});
produce_block(); // 3
set_producers({"carol"_n,"alice"_n});
produce_block(); // 4
set_producers({"carol"_n});
produce_block(); // 5
set_producers({"alice"_n});
produce_block(); // 6
set_producers({"bob"_n,"carol"_n});
produce_blocks(config::producer_repetitions-6);
set_producers({"bob"_n});
produce_block(); // 2
set_producers({"bob"_n,"carol"_n});
produce_block(); // 3
set_producers({"carol"_n,"bob"_n});
produce_block(); // 4
set_producers({"alice"_n} );
produce_block(); // 5
set_producers({"bob"_n,"carol"_n});
produce_blocks(config::producer_repetitions-5); // B12
BOOST_CHECK_EQUAL( 17u, control->active_producers().version );
BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );
produce_blocks(config::producer_repetitions);
BOOST_CHECK_EQUAL( 22u, control->active_producers().version );
BOOST_CHECK_EQUAL( true, compare_schedules( bob_carol_sch, control->active_producers() ) );

} FC_LOG_AND_RETHROW()

Expand Down