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

Change get_latest_attesting_balance() to get_weight() #3250

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

benjaminion
Copy link
Contributor

@benjaminion benjaminion commented Feb 15, 2023

Proposal to rename get_latest_attesting_balance() in the fork choice to get_weight().

I am trying to write about this function, and it's frustratingly difficult since the name "latest attesting balance" is quite ambiguous.

A few reasons:

  • "Weight" better reflects this quantity's role in the protocol. It chimes with the "heaviest" in GHOST.
  • Validators attest to a block directly, and only implicitly to its ancestors. This is the origin of the ambiguity - I want to distinguish between the direct latest attesting balance of a block (those validators who attested to that block), and the latest attesting balance in the sense of this function, which is the sum of all those direct latest attesting balances for all its descendants. It's all quite cumbersome. Calling the latter the block's weight would be much simpler.
  • Now that we have included proposer boost in the calculation, "latest attesting balance" is not really accurate any more.

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.

Right, I see the what-could-be-annoying ambiguity this name brings to explaining to outsiders/new spec readers. I'm not opposed to making this shift

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.

It makes sense to me. 👍

I git-blame it, this function was about "getting the latest attesting balance" at the beginning but then changed a lot after iterations.

get_weight sounds good to me. Another idea is get_score as we the return value is attestation_score + proposer_score. Either way is better than status-quo.

It would be great to get more 👍 from client teams before merging this phase0 change.

@hwwhww hwwhww added general:presentation Presentation (as opposed to content) scope:fork-choice phase0 labels Feb 16, 2023
@djrtwo
Copy link
Contributor

djrtwo commented Feb 17, 2023

CC: @adiasg who spends a lot of time on fc specs

@hwwhww hwwhww merged commit 6baa953 into ethereum:dev Feb 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:presentation Presentation (as opposed to content) phase0 scope:fork-choice
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants