Skip to content

Commit

Permalink
review
Browse files Browse the repository at this point in the history
  • Loading branch information
@rkapka
rkapka committed Aug 1, 2024
1 parent f5a38d2 commit 2768038
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 60 deletions.
65 changes: 21 additions & 44 deletions beacon-chain/rpc/prysm/v1alpha1/validator/proposer_attestations.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,27 +113,12 @@ func (vs *Server) packAttestations(ctx context.Context, latestState state.Beacon
if err != nil {
return nil, err
}
var sorted proposerAtts
if features.Get().EnableCommitteeAwarePacking {
sorted, err = deduped.sort()
if err != nil {
return nil, err
}
} else {
sorted, err = deduped.sortByProfitability()
if err != nil {
return nil, err
}
}

atts = sorted.limitToMaxAttestations()

atts, err = vs.filterAttestationBySignature(ctx, atts, latestState)
sorted, err := deduped.sort()
if err != nil {
return nil, err
}

return atts, nil
atts = sorted.limitToMaxAttestations()
return vs.filterAttestationBySignature(ctx, atts, latestState)
}

// filter separates attestation list into two groups: valid and invalid attestations.
Expand All @@ -153,14 +138,6 @@ func (a proposerAtts) filter(ctx context.Context, st state.BeaconState) (propose
return validAtts, invalidAtts
}

// sortByProfitability orders attestations by highest slot and by highest aggregation bit count.
func (a proposerAtts) sortByProfitability() (proposerAtts, error) {
if len(a) < 2 {
return a, nil
}
return a.sortByProfitabilityUsingMaxCover()
}

// sortByProfitabilityUsingMaxCover orders attestations by highest slot and by highest aggregation bit count.
// Duplicate bits are counted only once, using max-cover algorithm.
func (a proposerAtts) sortByProfitabilityUsingMaxCover() (proposerAtts, error) {
Expand Down Expand Up @@ -239,16 +216,23 @@ func (a proposerAtts) sort() (proposerAtts, error) {
return a, nil
}

if features.Get().EnableCommitteeAwarePacking {
return a.sortBySlotAndCommittee()
}
return a.sortByProfitabilityUsingMaxCover()
}

// Separate attestations by slot, as slot number takes higher precedence when sorting.
// Also separate by committee index because maxcover will prefer attestations for the same
// committee with disjoint bits over attestations for different committees with overlapping
// bits, even though same bits for different committees are separate votes.
func (a proposerAtts) sortBySlotAndCommittee() (proposerAtts, error) {
type slotAtts struct {
candidates map[primitives.CommitteeIndex]proposerAtts
selected map[primitives.CommitteeIndex]proposerAtts
leftover map[primitives.CommitteeIndex]proposerAtts
}

// Separate attestations by slot, as slot number takes higher precedence when sorting.
// Also separate by committee index because maxcover will prefer attestations for the same
// committee with disjoint bits over attestations for different committees with overlapping
// bits, even though same bits for different committees are separate votes.
var slots []primitives.Slot
attsBySlot := map[primitives.Slot]*slotAtts{}
for _, att := range a {
Expand All @@ -267,7 +251,7 @@ func (a proposerAtts) sort() (proposerAtts, error) {
sa.selected = make(map[primitives.CommitteeIndex]proposerAtts)
sa.leftover = make(map[primitives.CommitteeIndex]proposerAtts)
for ci, committeeAtts := range sa.candidates {
sa.selected[ci], sa.leftover[ci], err = committeeAtts.sortByProfitabilityUsingMaxCover_committeeAwarePacking()
sa.selected[ci], err = committeeAtts.sortByProfitabilityUsingMaxCover_committeeAwarePacking()
if err != nil {
return nil, err
}
Expand All @@ -290,42 +274,35 @@ func (a proposerAtts) sort() (proposerAtts, error) {

// sortByProfitabilityUsingMaxCover orders attestations by highest aggregation bit count.
// Duplicate bits are counted only once, using max-cover algorithm.
func (a proposerAtts) sortByProfitabilityUsingMaxCover_committeeAwarePacking() (selected proposerAtts, leftover proposerAtts, err error) {
func (a proposerAtts) sortByProfitabilityUsingMaxCover_committeeAwarePacking() (proposerAtts, error) {
if len(a) < 2 {
return a, nil, nil
return a, nil
}
candidates := make([]*bitfield.Bitlist64, len(a))
for i := 0; i < len(a); i++ {
var err error
candidates[i], err = a[i].GetAggregationBits().ToBitlist64()
if err != nil {
return nil, nil, err
return nil, err
}
}
// Add selected candidates on top, those that are not selected - append at bottom.
selectedKeys, _, err := aggregation.MaxCover(candidates, len(candidates), true /* allowOverlaps */)
if err != nil {
log.WithError(err).Debug("MaxCover aggregation failed")
return a, nil, nil
return a, nil
}

// Pick selected attestations first, leftover attestations will be appended at the end.
// Both lists will be sorted by number of bits set.
selected = make(proposerAtts, selectedKeys.Count())
leftover = make(proposerAtts, selectedKeys.Not().Count())
selected := make(proposerAtts, selectedKeys.Count())
for i, key := range selectedKeys.BitIndices() {
selected[i] = a[key]
}
for i, key := range selectedKeys.Not().BitIndices() {
leftover[i] = a[key]
}
sort.Slice(selected, func(i, j int) bool {
return selected[i].GetAggregationBits().Count() > selected[j].GetAggregationBits().Count()
})
sort.Slice(leftover, func(i, j int) bool {
return leftover[i].GetAggregationBits().Count() > leftover[j].GetAggregationBits().Count()
})
return selected, leftover, nil
return selected, nil
}

// sortSlotAttestations assumes each proposerAtts value in the map is ordered by profitability.
Expand Down
61 changes: 45 additions & 16 deletions beacon-chain/rpc/prysm/v1alpha1/validator/proposer_attestations_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/prysmaticlabs/go-bitfield"
chainMock "github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain/testing"
"github.com/prysmaticlabs/prysm/v5/beacon-chain/operations/attestations"
"github.com/prysmaticlabs/prysm/v5/config/features"
"github.com/prysmaticlabs/prysm/v5/config/params"
"github.com/prysmaticlabs/prysm/v5/consensus-types/primitives"
"github.com/prysmaticlabs/prysm/v5/crypto/bls/blst"
Expand All @@ -19,7 +20,7 @@ import (
"github.com/prysmaticlabs/prysm/v5/time/slots"
)

func TestProposer_ProposerAtts_sortByProfitability(t *testing.T) {
func TestProposer_ProposerAtts_sort(t *testing.T) {
type testData struct {
slot primitives.Slot
bits bitfield.Bitlist
Expand All @@ -36,7 +37,7 @@ func TestProposer_ProposerAtts_sortByProfitability(t *testing.T) {
t.Run("no atts", func(t *testing.T) {
atts := getAtts([]testData{})
want := getAtts([]testData{})
atts, err := atts.sortByProfitability()
atts, err := atts.sort()
if err != nil {
t.Error(err)
}
Expand All @@ -50,7 +51,7 @@ func TestProposer_ProposerAtts_sortByProfitability(t *testing.T) {
want := getAtts([]testData{
{4, bitfield.Bitlist{0b11100000, 0b1}},
})
atts, err := atts.sortByProfitability()
atts, err := atts.sort()
if err != nil {
t.Error(err)
}
Expand All @@ -66,7 +67,7 @@ func TestProposer_ProposerAtts_sortByProfitability(t *testing.T) {
{4, bitfield.Bitlist{0b11100000, 0b1}},
{1, bitfield.Bitlist{0b11000000, 0b1}},
})
atts, err := atts.sortByProfitability()
atts, err := atts.sort()
if err != nil {
t.Error(err)
}
Expand All @@ -84,7 +85,7 @@ func TestProposer_ProposerAtts_sortByProfitability(t *testing.T) {
{4, bitfield.Bitlist{0b11100000, 0b1}},
{1, bitfield.Bitlist{0b11000000, 0b1}},
})
atts, err := atts.sortByProfitability()
atts, err := atts.sort()
if err != nil {
t.Error(err)
}
Expand All @@ -105,7 +106,7 @@ func TestProposer_ProposerAtts_sortByProfitability(t *testing.T) {
{1, bitfield.Bitlist{0b00001100, 0b1}},
{1, bitfield.Bitlist{0b11001000, 0b1}},
})
atts, err := atts.sortByProfitability()
atts, err := atts.sort()
if err != nil {
t.Error(err)
}
Expand All @@ -130,7 +131,7 @@ func TestProposer_ProposerAtts_sortByProfitability(t *testing.T) {
{1, bitfield.Bitlist{0b11100000, 0b1}},
{1, bitfield.Bitlist{0b11000000, 0b1}},
})
atts, err := atts.sortByProfitability()
atts, err := atts.sort()
if err != nil {
t.Error(err)
}
Expand Down Expand Up @@ -162,15 +163,15 @@ func TestProposer_ProposerAtts_sortByProfitability(t *testing.T) {
{1, bitfield.Bitlist{0b11100000, 0b1}},
{1, bitfield.Bitlist{0b11000000, 0b1}},
})
atts, err := atts.sortByProfitability()
atts, err := atts.sort()
if err != nil {
t.Error(err)
}
require.DeepEqual(t, want, atts)
})
}

func TestProposer_ProposerAtts_sort(t *testing.T) {
func TestProposer_ProposerAtts_committeeAwareSort(t *testing.T) {
type testData struct {
slot primitives.Slot
bits bitfield.Bitlist
Expand All @@ -185,6 +186,11 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
}

t.Run("no atts", func(t *testing.T) {
flgs := features.Get()
flgs.EnableCommitteeAwarePacking = true
reset := features.InitWithReset(flgs)
defer reset()

atts := getAtts([]testData{})
want := getAtts([]testData{})
atts, err := atts.sort()
Expand All @@ -195,6 +201,11 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
})

t.Run("single att", func(t *testing.T) {
flgs := features.Get()
flgs.EnableCommitteeAwarePacking = true
reset := features.InitWithReset(flgs)
defer reset()

atts := getAtts([]testData{
{4, bitfield.Bitlist{0b11100000, 0b1}},
})
Expand All @@ -209,6 +220,11 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
})

t.Run("single att per slot", func(t *testing.T) {
flgs := features.Get()
flgs.EnableCommitteeAwarePacking = true
reset := features.InitWithReset(flgs)
defer reset()

atts := getAtts([]testData{
{1, bitfield.Bitlist{0b11000000, 0b1}},
{4, bitfield.Bitlist{0b11100000, 0b1}},
Expand All @@ -225,6 +241,11 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
})

t.Run("two atts on one of the slots", func(t *testing.T) {
flgs := features.Get()
flgs.EnableCommitteeAwarePacking = true
reset := features.InitWithReset(flgs)
defer reset()

atts := getAtts([]testData{
{1, bitfield.Bitlist{0b11000000, 0b1}},
{4, bitfield.Bitlist{0b11100000, 0b1}},
Expand All @@ -233,7 +254,6 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
want := getAtts([]testData{
{4, bitfield.Bitlist{0b11110000, 0b1}},
{1, bitfield.Bitlist{0b11000000, 0b1}},
{4, bitfield.Bitlist{0b11100000, 0b1}},
})
atts, err := atts.sort()
if err != nil {
Expand All @@ -243,6 +263,11 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
})

t.Run("compare to native sort", func(t *testing.T) {
flgs := features.Get()
flgs.EnableCommitteeAwarePacking = true
reset := features.InitWithReset(flgs)
defer reset()

// The max-cover based approach will select 0b00001100 instead, despite lower bit count
// (since it has two new/unknown bits).
t.Run("max-cover", func(t *testing.T) {
Expand All @@ -254,7 +279,6 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
want := getAtts([]testData{
{1, bitfield.Bitlist{0b11000011, 0b1}},
{1, bitfield.Bitlist{0b00001100, 0b1}},
{1, bitfield.Bitlist{0b11001000, 0b1}},
})
atts, err := atts.sort()
if err != nil {
Expand All @@ -265,6 +289,11 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
})

t.Run("multiple slots", func(t *testing.T) {
flgs := features.Get()
flgs.EnableCommitteeAwarePacking = true
reset := features.InitWithReset(flgs)
defer reset()

atts := getAtts([]testData{
{2, bitfield.Bitlist{0b11100000, 0b1}},
{4, bitfield.Bitlist{0b11100000, 0b1}},
Expand All @@ -278,8 +307,6 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
{3, bitfield.Bitlist{0b11000000, 0b1}},
{2, bitfield.Bitlist{0b11100000, 0b1}},
{1, bitfield.Bitlist{0b11100000, 0b1}},
{4, bitfield.Bitlist{0b11100000, 0b1}},
{1, bitfield.Bitlist{0b11000000, 0b1}},
})
atts, err := atts.sort()
if err != nil {
Expand All @@ -289,6 +316,11 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
})

t.Run("follows max-cover", func(t *testing.T) {
flgs := features.Get()
flgs.EnableCommitteeAwarePacking = true
reset := features.InitWithReset(flgs)
defer reset()

// Items at slot 4 must be first split into two lists by max-cover, with
// 0b10000011 being selected and 0b11100001 being leftover (despite naive bit count suggesting otherwise).
atts := getAtts([]testData{
Expand All @@ -307,9 +339,6 @@ func TestProposer_ProposerAtts_sort(t *testing.T) {
{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}},
})
atts, err := atts.sort()
if err != nil {
Expand Down

0 comments on commit 2768038

Please sign in to comment.