Skip to content

Apply proposer boost to ancestors correctly#2760

Merged
hwwhww merged 5 commits intodevfrom
fix-proposer-boost-apply
Dec 7, 2021
Merged

Apply proposer boost to ancestors correctly#2760
hwwhww merged 5 commits intodevfrom
fix-proposer-boost-apply

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.

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 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