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

Bugfix: would_be_leader_shortly_fn period set to 1 slot instead of 20 #32468

Merged

Conversation

apfitzge
Copy link
Contributor

@apfitzge apfitzge commented Jul 12, 2023

Problem

#30618 did a bad copy pasta of constants.
Found this while investigating #32179 further, added some notes there on investigation.

#30618 was originally merged into v1.16. I reccommend we backport this.
My intent is to keep this fix PR simple so that it can be backported, and add tests to master in a follow-up PR.

Summary of Changes

revert constant for HOLD-only period from 20 slots to 1 slot.

Fixes #

@@ -53,8 +53,10 @@ impl DecisionMaker {
})
},
|| {
poh_recorder
.would_be_leader(HOLD_TRANSACTIONS_SLOT_OFFSET * DEFAULT_TICKS_PER_SLOT)
poh_recorder.would_be_leader(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is would_be_leader_shortly_fn.

@apfitzge apfitzge changed the title would_be_leader_shortly_fn period set to 1 slot instead of 20 Bugfix: would_be_leader_shortly_fn period set to 1 slot instead of 20 Jul 12, 2023
@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #32468 (c2bf174) into master (a3ada9c) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #32468     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         778      778             
  Lines      210200   210200             
=========================================
- Hits       172640   172592     -48     
- Misses      37560    37608     +48     

Copy link
Contributor

@carllin carllin left a comment

Choose a reason for hiding this comment

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

The side effect of this bug is leaders stop forwarding if they receive transactions within 20 slots instead of 2?

@apfitzge
Copy link
Contributor Author

apfitzge commented Jul 12, 2023

The side effect of this bug is leaders stop forwarding if they receive transactions within 20 slots instead of 2?

Exactly. The pattern we use to have (and want) is:

>20 slots: Forward
<=20 slots: ForwardAndHold
<2 slots: Hold  

the bug makes it so Hold instead of ForwardAndHold:

>20 slots: Forward
<=20 slots: Hold
<2 slots: Hold  

@apfitzge apfitzge merged commit 0b5421f into solana-labs:master Jul 12, 2023
@apfitzge apfitzge added the v1.16 PRs that should be backported to v1.16 label Jul 12, 2023
@apfitzge apfitzge deleted the bugfix/decision_maker_holding_period branch July 12, 2023 23:55
mergify bot pushed a commit that referenced this pull request Jul 12, 2023
apfitzge added a commit that referenced this pull request Jul 13, 2023
…d of 20 (backport of #32468) (#32469)

Co-authored-by: Andrew Fitzgerald <apfitzge@gmail.com>
fix: would_be_leader_shortly_fn period set to 1 slot instead of 20 (#32468)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.16 PRs that should be backported to v1.16
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants