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

Proposer head v5 #12075

Merged
merged 14 commits into from
Mar 4, 2023
Merged

Proposer head v5 #12075

merged 14 commits into from
Mar 4, 2023

Conversation

potuz
Copy link
Contributor

@potuz potuz commented Mar 3, 2023

This PR adds reorg logic when proposing a block via GetBeaconBlock. It fixes a bug that is introduced in #12034 in which this PR is based.

@potuz potuz requested a review from a team as a code owner March 3, 2023 16:07
@potuz potuz added Blocked Blocked by research or external factors Ready For Review A pull request ready for code review labels Mar 3, 2023
"github.com/prysmaticlabs/prysm/v3/encoding/bytesutil"
ethpb "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1"
"github.com/prysmaticlabs/prysm/v3/time/slots"
"github.com/sirupsen/logrus"
"go.opencensus.io/trace"
)

// reorgLateBlockCountAttestations is the time until the end of the slot in which we count
// attestations to see if we will reorg the incoming block
const reorgLateBlockCountAttestations = 2 * time.Second
Copy link
Member

Choose a reason for hiding this comment

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

The naming is a little weird, I also prefer 2 more than 10 than having 12-2...

log.WithError(err).Error("Could not process attestations and update head")
}
// process attestations and update head in forkchoice
vs.ForkFetcher.ForkChoicer().Lock()
Copy link
Member

Choose a reason for hiding this comment

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

Wow. we missed the lock before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I always had it but the locking mechanism moved from within UpdateHead to here

@@ -182,6 +183,11 @@ func ConfigureBeaconChain(ctx *cli.Context) error {
logDisabled(disablePeerScorer)
cfg.EnablePeerScorer = false
}
cfg.DisableReorgLateBlocks = true
Copy link
Member

Choose a reason for hiding this comment

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

Optional, I wish we have EnableReorgLateBlocks instead of DisableReorgLateBlocks. The flag is enable-reorg-late-blocks and it makes everything a bit easier to read

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 want to make this default later, so the permanent config will be Disable, that's why I internally made this so that the changes are minimal when we flip the flag

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Besides the naming consideration, I dont have any further feedback. This looks good to me. The feature flag rename will be in the subsequent PR

@potuz potuz removed the Blocked Blocked by research or external factors label Mar 4, 2023
@potuz potuz merged commit 93514de into develop Mar 4, 2023
@delete-merged-branch delete-merged-branch bot deleted the proposer-head-v5 branch March 4, 2023 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants