Committee-aware attestation packing

[rkapka]

Max-Cover is executed on all attestations for a slot bundled together, without considering committees. This is very bad because an attestation for committee C0 with bits 11111110 and an attestation for committee C1 with the same bits will be treated as equal, meaning that Max-Cover will treat the attestation for C1 as not improving coverage, even though it's an entirely distinct set of votes. As a result it will most likely place this attestation after an attestation with bits 00000001 (for whatever committee).

If the number of attestations in the block is less then MAX_ATTESTATIONS, then this a non-issue because all attestations will get into the block anyway. But for blocks that hit the maximum number of attestations, which is 128, the packing can be made more optimal.

The new algorithm:

// sort attestations as follows:
//
// - all attestations selected by max-cover are ordered before leftover attestations
// - within selected/leftover, attestations are ordered by slot, with higher slot coming first
// - within a slot, all top attestations (one per committee) are ordered before any second-best attestations, second-best before third-best etc.
// - within top/second-best/etc. attestations (one per committee), attestations are ordered by bit count, with higher bit count coming first

The new algorithm is hidden behind a feature flag called EnableCommitteeAwarePacking (although it does more than dividing attestations into committees)

[potuz]

Why not move this logic withing the sort() method?

[potuz]

Is there a test where we check for leftovers? I think that with the parameters passed to MaxCover the leftover map will always be empty unless there are contained aggregation bits, in which case anyway we would drop them. I suspect that with these parameters you can simply ignore the leftovers from MaxCover and just consider the selected ones.

[rkapka]

atts := getAtts([]testData{
			{4, bitfield.Bitlist{0b00000001, 0b1}},
			{4, bitfield.Bitlist{0b11100001, 0b1}},
			{1, bitfield.Bitlist{0b11000000, 0b1}},
			{2, bitfield.Bitlist{0b11100000, 0b1}},
			{4, bitfield.Bitlist{0b10000011, 0b1}},
			{4, bitfield.Bitlist{0b11111000, 0b1}},
			{1, bitfield.Bitlist{0b11100000, 0b1}},
			{3, bitfield.Bitlist{0b11000000, 0b1}},
		})
		want := getAtts([]testData{
			{4, bitfield.Bitlist{0b11111000, 0b1}},
			{4, bitfield.Bitlist{0b10000011, 0b1}},
			{3, bitfield.Bitlist{0b11000000, 0b1}},
			{2, bitfield.Bitlist{0b11100000, 0b1}},
			{1, bitfield.Bitlist{0b11100000, 0b1}},
			{4, bitfield.Bitlist{0b11100001, 0b1}},
			{4, bitfield.Bitlist{0b00000001, 0b1}},
			{1, bitfield.Bitlist{0b11000000, 0b1}},
		})

This is an example of a test where we check that left over aggregates are sorted last. I agree that in all such tests each leftover atts has bits that are already covered.

[potuz]

LGTM