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

Ignore subset aggregates #2847

Merged
merged 3 commits into from
May 17, 2022
Merged

Ignore subset aggregates #2847

merged 3 commits into from
May 17, 2022

Conversation

arnetheduck
Copy link
Contributor

When aggregates are propagated through the network, it is often the case
that a better aggregate has already been seen - in particular, this
happens when an aggregator has not been able to include itself in the
mesh and therefore publishes an aggregate with only its own
attestations.

This new ignore rule allows dropping all aggregates that are
(non-strict) subsets of aggregates that have already been seen on the
network. In particular, it does not mandate dropping aggregates where a
union of previous aggregates would cause it to become a subset).

The logic for allowing this is based on the premise that any aggregate
that has already been seen by a peer will also have been seen by its
neighbours - a subset aggregate (strict or not) brings no new value to
the aggregation algorithm, except in the extreme edge case where you
could combine several such sparse aggregates into a single, more dense
"combined" aggregate and thus use less block space.

Further, as a small benefit, computing the hash_tree_root of the full
aggregate is generally not done -however, hash_tree_root(data) is
already done for other purposes as this is used as index in the beacon
API.

When aggregates are propagated through the network, it is often the case
that a better aggregate has already been seen - in particular, this
happens when an aggregator has not been able to include itself in the
mesh and therefore publishes an aggregate with only its own
attestations.

This new ignore rule allows dropping all aggregates that are
(non-strict) subsets of aggregates that have already been seen on the
network. In particular, it does not mandate dropping aggregates where a
union of previous aggregates would cause it to become a subset).

The logic for allowing this is based on the premise that any aggregate
that has already been seen by a peer will also have been seen by its
neighbours - a subset aggregate (strict or not) brings no new value to
the aggregation algorithm, except in the extreme edge case where you
could combine several such sparse aggregates into a single, more dense
"combined" aggregate and thus use less block space.

Further, as a small benefit, computing the `hash_tree_root` of the full
aggregate is generally not done -however, `hash_tree_root(data)` is
already done for other purposes as this is used as index in the beacon
API.
@dapplion
Copy link
Collaborator

dapplion commented Mar 8, 2022

How would this affect gossip scoring? Should the current parameters be fine-tuned?

@dapplion
Copy link
Collaborator

dapplion commented Mar 8, 2022

Also, this should be tested in advance to not require a revert down the line like what happened with #2183

@ajsutton
Copy link
Contributor

ajsutton commented Mar 8, 2022

Should we consider the same change for SignedContributionAndProof messages as well?

@djrtwo
Copy link
Contributor

djrtwo commented Mar 9, 2022

How would this affect gossip scoring? Should the current parameters be fine-tuned?

This should be fine because it's an IGNORE which means the sender is not considered dishonest

Also, this should be tested in advance to not require a revert down the line like what happened with #2183

certainly. Should note though, that this actually piggy backs on the 2183 functionality of dropping dupes so would in theory reduce bandwidth through a known mechanism

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.

I'm fine with this change, but want to hear more client team/engineering input on complexity.

It piggy backs on a known mechanism for reducing bandwidth -- essentially dropping the message from gossip (which has amplificaiton factor) in favor of disseminating the unnecessary message through slower IWANT/IHAVE direct comms.

@arnetheduck
Copy link
Contributor Author

Should we consider the same change for SignedContributionAndProof messages as well?

Sure, why not :) d5f4c46

@arnetheduck
Copy link
Contributor Author

How would this affect gossip scoring? Should the current parameters be fine-tuned?

The spec says:
These are currently under investigation and will be spec'd and released to mainnet when they are ready. :)

Meaning this is in client territory for now - if your client has a score for an "expected message volume" not being met on the aggregate channel, seeing fewer messages will case gossipsub descoring.

Further, if your client penalizes peers for IGNORE messages, it will bias itself against those not implementing the new rule, since they will be sending messages that your client now ignores thus increasing the risk of a network partition as the bias sets in over time.

@ajsutton
Copy link
Contributor

ajsutton commented May 9, 2022

Implemented this in Teku. The implementation was quite straight forward and early indications are that there's a small reduction in CPU usage as a result.

@arnetheduck
Copy link
Contributor Author

small reduction in CPU usage as a result.

hopefully you should see a reduction in outgoing bandwidth as well!

over time, this should translate to an incoming bandwidth reduction as well.

@ajsutton
Copy link
Contributor

ajsutton commented May 9, 2022

Yes, also a slight decrease in outgoing bandwidth.

@djrtwo djrtwo merged commit bab5e40 into ethereum:dev May 17, 2022
@dapplion
Copy link
Collaborator

dapplion commented May 18, 2022

With

it does not mandate dropping aggregates where a union of previous aggregates would cause it to become a subset

The indication "not mandate" means "MAY NOT drop ..", or "MUST NOT drop .."? It's definitely cheaper to check against a single bitfield per attestation data than to look over all known aggregates. At least Lodestar pre-aggregates them in the operation pool, so we don't retain all aggregates from gossip as received.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants