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

Fix proposal policy transition rules #483

Merged
merged 28 commits into from
Aug 13, 2024
Merged

Fix proposal policy transition rules #483

merged 28 commits into from
Aug 13, 2024

Conversation

linh2931
Copy link
Member

@linh2931 linh2931 commented Aug 6, 2024

Consider the following case:

  1. In round 1, a block proposes a proposer policy.
  2. In round 2, the block in the 12th time slot of the round is not present.
  3. In round 3, there exists at least one block.

In the current implementation, the first block in round 3 will not use the proposer policy that was proposed in round 1 despite that being our intention. That would however be the case if there was a block present in the 12th time slot of round 2.

Fixing the bug means that at the time of building a new block that is the first block of a round, it will always choose as the active proposal policy the last proposed proposer policy in the blockchain that was not proposed from a block in the current round or the one prior.

Instead of a map to track all the proposed proposal policies in the block_header_state, only two optional fields: latest_pending_proposer_policy and latest_proposed_proposer_policy are needed. Additionally, instead of tracking an active_time with each proposed policy, we just track the timestamp in which it was proposed: proposal_time.

When creating a block_header_state for the first block of a round, those fields are computed by:

  1. If latest_proposed_proposer_policy is not nullopt and its proposal_time indicates that it was proposed in a round before the prior round, then promote latest_proposed_proposer_policy to active_proposer_policy. latest_pending_proposer_policy and latest_proposed_proposer_policy should be set to nullopt at this point in time (latest_proposed_proposer_policy may still be changed in later steps).
  2. Otherwise, if latest_pending_proposer_policy is not nullopt, promote latest_pending_proposer_policy to active_proposer_policy. latest_pending_proposer_policy should be set to nullopt at this point in time (may still be changed in later steps).
  3. If latest_proposed_proposer_policy is not nullopt and latest_pending_proposer_policy is now nullopt, latest_proposed_proposer_policy is moved into latest_pending_proposer_policy. If it is moved, then latest_proposed_proposer_policy is set to nullopt at this point in time (may still be changed in later steps).

The final related step should occur for any block (whether it is the first block of the round or not) and can only be computed after the block has been assembled:

  1. If this block proposes a new proposer policy, that policy should be set in latest_proposed_proposer_policy (replacing whatever happened to be there).

Resolves #454

@linh2931 linh2931 marked this pull request as draft August 6, 2024 22:23
@linh2931
Copy link
Member Author

linh2931 commented Aug 8, 2024

@heifner The commit 59e716e passed most tests, but failed a few. Can you see anything obviously wrong? Thanks.

unittests/unit_test -t producer_schedule_savanna_tests
Random number generator seeded to 1723148650
Running 5 test cases...
unittests/producer_schedule_savanna_tests.cpp(35): error: in "producer_schedule_savanna_tests/verify_producer_schedule_after_savanna_activation": check head().block_num() == expected_block_num has failed [25 != 24]
unittests/producer_schedule_savanna_tests.cpp(35): error: in "producer_schedule_savanna_tests/verify_producer_schedule_after_savanna_activation": check head().block_num() == expected_block_num has failed [49 != 48]
unittests/producer_schedule_savanna_tests.cpp(364): error: in "producer_schedule_savanna_tests/proposer_policy_progression_test": check 17u == control->active_producers().version has failed [17 != 12]
unittests/producer_schedule_savanna_tests.cpp(367): error: in "producer_schedule_savanna_tests/proposer_policy_progression_test": check 22u == control->active_producers().version has failed [22 != 17]
nittests/producer_schedule_savanna_tests.cpp(468): error: in "producer_schedule_savanna_tests/policy_switching_corner_case_test": check b->producer == "alice"_n has failed [eosio != alice]

@linh2931
Copy link
Member Author

@arhag The current PR has not implemented the enhancement described by #454:

not promote the policy if the proposal_time of the policy is greater than the last_final_block_timestamp (computed after considering the claim of the current block).

This is due to the fact the producer_plugin needs to know the producer for the next block

const producer_authority scheduled_producer = chain.active_producers().get_scheduled_producer(block_time);

But at that time we don't know the QC claim of the next block and cannot calculate the last_final_block_timestamp.

But we know the last_final_block_timestamp of the head block. We could check against it. This will miss one corner case that if the next block advances the LIB.

@linh2931 linh2931 marked this pull request as ready for review August 12, 2024 12:23
libraries/chain/include/eosio/chain/controller.hpp Outdated Show resolved Hide resolved
libraries/chain/controller.cpp Outdated Show resolved Hide resolved
@ericpassmore
Copy link
Contributor

Note:start
group: STABILITY
category: INTERNALS
summary: Improve finalizer policy transition by always promoting a proposer policy to active on the first block of a round. That proposer policy should never be the one that was proposed in the prior round. It should have been proposed at least two rounds back.
Note:end

libraries/chain/block_header_state.cpp Outdated Show resolved Hide resolved
return detail::get_scheduled_producer(get_active_proposer_policy_for_block_at(t)->proposer_schedule.producers, t);
}

const producer_authority_schedule* block_header_state::get_next_producer_schedule() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? We need a timestamp to know if these will be active.

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 is changed to pending_producers.

Copy link
Contributor

Choose a reason for hiding this comment

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

This still seems like an odd API to me. What do we need it for?

Copy link
Member Author

Choose a reason for hiding this comment

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

By net_plugin and chain_plugin. Please see the discussion in telegram channel.

libraries/chain/block_header_state.cpp Outdated Show resolved Hide resolved
libraries/testing/tester.cpp Outdated Show resolved Hide resolved
libraries/testing/tester.cpp Outdated Show resolved Hide resolved
@linh2931 linh2931 merged commit 2c484a2 into main Aug 13, 2024
36 checks passed
@linh2931 linh2931 deleted the proposal_change branch August 13, 2024 20:27
@linh2931 linh2931 restored the proposal_change branch August 14, 2024 11:14
@heifner heifner added the consensus-protocol Change to the consensus protocol. Impacts light client validation. label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-protocol Change to the consensus protocol. Impacts light client validation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix proposer policy transition rules
4 participants