Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Improve block-selection strategy #10727

Closed
wants to merge 7 commits into from
Closed

Improve block-selection strategy #10727

wants to merge 7 commits into from

Conversation

Wizdave97
Copy link
Contributor

@Wizdave97 Wizdave97 commented Jan 24, 2022 •
edited
Loading

Fixes paritytech/grandpa-bridge-gadget#16

Round selection formula updated to match

round_number =
      (1 - M) * session_start  
+    M * (best_beefy + NEXT_POWER_OF_TWO((best_grandpa - best_beefy + 1) / 2))

as outlined in https://github.com/paritytech/grandpa-bridge-gadget/blob/td-docs/docs/beefy.md#round-selection

Session boundary API that is used here is already implemented in #10669

acatangiu
acatangiu reviewed Jan 25, 2022
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Changes look good, only have some small comments. We also need #10669 merged before this PR.

client/beefy/src/worker.rs Outdated
@@ -146,18 +141,28 @@ where
B: Block,
BE: Backend<B>,
C: Client<B, BE>,
C::Api: BeefyApi<B>,
C::Api: BeefyApi<B, NumberFor<B>>,
{
/// Return `true`, if we should vote on block `number`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Return `true`, if we should vote on block `number`
/// Return `true`, if we should vote on block with `header`

client/beefy/src/worker.rs Outdated
let session_start = if let Some(session_boundary) = session_boundary {
session_boundary
} else {
debug!(target: "beefy", "🥩 Could not retrieve session boundary- won't vote for: {:?}", number);
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 at least warning here:

Suggested change
debug!(target: "beefy", "🥩 Could not retrieve session boundary- won't vote for: {:?}", number);
warn!(target: "beefy", "🥩 Could not retrieve session boundary - won't vote for: {:?}", number);

client/beefy/src/worker.rs
fn should_vote_on(&self, header: &B::Header) -> bool {
let number = *header.number();
let at = BlockId::hash(header.hash());
let session_boundary = self.client.runtime_api().get_session_boundary(&at).ok();
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to open an enhancement issue to cache session boundary in the worker and not have to call runtime api for every block here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll keep that in mind.

client/beefy/src/worker.rs Outdated
Comment on lines 473 to 474
// if the session_start as a mandatory block does not have a beefy justification yet, we vote on
// it
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
// if the session_start as a mandatory block does not have a beefy justification yet, we vote on
// it
// if the session_start as a mandatory block does not have a beefy justification yet,
// we vote on it

client/beefy/src/lib.rs Outdated
Comment on lines 129 to 130
/// Minimal delta between blocks, BEEFY should vote for
pub min_block_delta: u32,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not remove this. You can still use it on top of your changes as described in beefy.md #L224.

@acatangiu acatangiu added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jan 25, 2022
@stale
Copy link

stale bot commented Feb 25, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 25, 2022
@seunlanlege
Copy link
Contributor

Bump

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 25, 2022
@stale
Copy link

stale bot commented Mar 27, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 27, 2022
@acatangiu
Copy link
Contributor

Alternative PR #10882 merged

@acatangiu acatangiu closed this Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@acatangiu acatangiu Awaiting requested review from acatangiu

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve block-selection strategy
3 participants
@Wizdave97 @seunlanlege @acatangiu