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

Apply proposer boost to ancestors correctly #2760

Merged
merged 5 commits into from
Dec 7, 2021
Merged

Conversation

adiasg
Copy link
Contributor

@adiasg adiasg commented Dec 2, 2021

Fixes #2757 by correctly applying proposer score boost to ancestors of the boosted block.
This removes the simplification changes from #2753.

specs/phase0/fork-choice.md Outdated Show resolved Hide resolved
@@ -182,12 +182,13 @@ def get_latest_attesting_balance(store: Store, root: Root) -> Gwei:
and get_ancestor(store, store.latest_messages[i].root, store.blocks[root].slot) == root)
))
proposer_score = Gwei(0)
if store.proposer_boost_root != Root() and root == store.proposer_boost_root:
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 conditional implies that get_latest_attesting_balance applies the proposer score boost only to the proposer's block, but not its ancestors. This is not the correct method to apply the proposer score boost.

The boost should be applied like usual LMD votes (i.e., store.latest_messages) - all ancestors of store.proposer_boost_root should get the additional weight.

adiasg and others added 2 commits December 1, 2021 20:11
Co-authored-by: Hsiao-Wei Wang <hsiaowei.eth@gmail.com>
@adiasg adiasg removed the request for review from djrtwo December 2, 2021 14:52
Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Thank @realbigsean for the find!

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

logic looks correct. some suggestions for alternative formating

Comment on lines 186 to 187
store.proposer_boost_root != Root()
and get_ancestor(store, store.proposer_boost_root, store.blocks[root].slot) == root
Copy link
Contributor

Choose a reason for hiding this comment

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

generally should avoid multi-line ifs. This is also a bit secretly too pythonic (it assumes that if the first bool fails, the second is not executed, avoiding the undefined behavior if Root() being passed into get_ancestor

suggest somthing like the following

    if store.proposer_boost_root != Root():
        if get_ancestor():
            ...

An alternative, I personally prefer this:

    if store.proposer_boost_root == Root():
        return attestation_score
    proposer_score = Gwei(0)
    if get_ancestor():
        ...
    return attestation_score + proposer_score

I think that very nicely reads as "If proposer_boost is not enabled, just return the attestation_score, otherwise calculate the proposer score and return the sum"

@hwwhww hwwhww merged commit 876c5b0 into dev Dec 7, 2021
@hwwhww hwwhww deleted the fix-proposer-boost-apply branch December 7, 2021 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Applying proposer boost to ancestors
3 participants