Aggregate and pack sync committee messages#15608
Conversation
rkapka
force-pushed
the
agg-sc-messages
branch
from
d03e6ac to
13f77a1
Compare
rkapka
force-pushed
the
agg-sc-messages
branch
from
13f77a1 to
0d90d56
Compare
| assert.Equal(t, 0, len(atts), "Did not delete unaggregated attestation") | ||
| } | ||
|
|
||
| func TestProposer_GetSyncAggregate_OK(t *testing.T) { |
There was a problem hiding this comment.
I moved this test to proposer_altair_test.go so that all sync committee tests are in the same file
| if err != nil { | ||
| return nil, errors.Wrap(err, "could not get aggregated sync committee messages") | ||
| } | ||
| proposerContributions = append(proposerContributions, aggregatedContributions...) |
There was a problem hiding this comment.
Isn't this adding aggregated contributions to the current list of already aggregated contributions proposerContributions? shouldn't we aggregate them together otherwise we are likely to create non-trivial intersections among the signing indices rendering the aggregates unaggregatable?
There was a problem hiding this comment.
I forgot that if we aggregate messages that are already part of some aggregate with messages that are not, it can be hard to get a better total aggregate. One mitigation I see is to discard messages that are already in proposerContributions by looking at appropriate bits. Did you have another solution in mind?
rkapka
force-pushed
the
agg-sc-messages
branch
from
27ad1bb to
8d03f2e
Compare
* Aggregate and pack sync committee messages * test * simplify error check * changelog <3 * fix assert import * remove parallelization * use sync committee cache * ignore bits already set in pool messages * fuzz fix * cleanup * test panic fix * clear cache in tests
* Aggregate and pack sync committee messages * test * simplify error check * changelog <3 * fix assert import * remove parallelization * use sync committee cache * ignore bits already set in pool messages * fuzz fix * cleanup * test panic fix * clear cache in tests
3 participants
What type of PR is this?
Feature
What does this PR do? Why is it needed?
When Prysm packs a sync aggregate into a block, it only considers sync committee contributions, which are messages sent by sync committee aggregators. If there are unaggregated sync committee messages in the pool, they are ignored, even if they could be aggregated together with sync committee contributions to make a better final sync aggregate.
This can be easily observed by running a tiny devnet with 64 or so validators. In such a devnet for most slots there is at least one sync committee subnet without an aggregator, and this results in a block with no sync committee votes for that subnet. On a real testnet or Mainnet the effect of this PR will be much smaller, but sync committee efficiency can get better when there are unaggregated messages in the pool for validator indices not present in any sync committee contribution.
Other notes for review
The aggregation code invokes a goroutine for each subnet, which speeds up the aggregation part by 3-4x (tested for 64 and 8196 validators). One peculiar thing that I noticed on the devnet with 64 validators is that aggregation time was more or less constant for each slot, even though some slots had aggregators for all subnets and some just for half of subnets. I believe the reason for this is in how the sync committee message cache is designed. Items are keyed by slot and popped only when the maximum size is exceeded, which means that all seen messages for the current slot are always in the cache. A potential redesign is out of scope for this PR, but maybe we should delete matching messages when the node receives an aggregate, and that way only aggregate the minimum amount of messages. Since most messages are contained in aggregates, there should be no more need for parallelization.Acknowledgements