SubmitAggregateAndProof now prefers its own validator attestations #6566
Conversation
|
Please hold off on review, I misunderstood a test helper and this test is not performing as I want it to do. |
|
OK now it's ready. |
Codecov Report
@@ Coverage Diff @@
## master #6566 +/- ##
==========================================
+ Coverage 60.07% 61.77% +1.70%
==========================================
Files 323 366 +43
Lines 27422 28625 +1203
==========================================
+ Hits 16473 17683 +1210
+ Misses 8733 8534 -199
- Partials 2216 2408 +192 |
| // The aggregator should prefer an attestation that they have signed. We check this by | ||
| // looking at the attestation's committee index against the validator's committee index | ||
| // and check the aggregate bits to ensure the validator's index is set. | ||
| if aggregatedAtt.Data.CommitteeIndex == req.CommitteeIndex && |
There was a problem hiding this comment.
What happens here? Assume attestations are in the following order:
indexInCommittee=0
AggregationBits has a length 4
local aggregation bits is 0b0001
local head root is 0xA
First attestation: {HeadRoot: 0xA, AggregationBits: 0x0001}
Second attestation: {HeadRoot: 0xB, AggregationBits: 0x1100} // Best
Wouldn't second attestation overrides the first attestation because it has more bits? We want the first attestation to be test
There was a problem hiding this comment.
The best is only replaced by an attestation where where 0b0001 is set, so the first attestation is considered the best.
There was a problem hiding this comment.
When the second attestation comes, wouldn't you get:
aggregatedAtt.Data.CommitteeIndex == req.CommitteeIndex true
aggregatedAtt.AggregationBits.BitAt(indexInCommittee) false
!best.AggregationBits.BitAt(indexInCommittee) false
aggregatedAtt.AggregationBits.Count() > best.AggregationBits.Count()) true
true && false && false || true equals to true?
There was a problem hiding this comment.
It’s true && false && (false || true)
There was a problem hiding this comment.
Ah I missed the (), note to self dont review code sunday late.
What about this case?
local aggregation bits is 0b0001
local head root is 0xA
First attestation: {HeadRoot: 0xB, AggregationBits: 0x0010}
Second attestation: {HeadRoot: 0xC, AggregationBits: 0x1100}
Second attestation should be the best one but I don't think it'd be replaced given this condition?
true && false && (false || true)
There was a problem hiding this comment.
Yeah that is correct. I figured that the aggregator would almost always have their attestation present, but let me see about this.
There was a problem hiding this comment.
OK this has been added. Please review 8827cb1
…sm into prefer-own-attestation
What type of PR is this?
Feature
What does this PR do? Why is it needed?
When selected as an aggregator, the beacon node should be "greedy" in the validators favor. That is to say that the aggregator should prefer its own attestation for broadcasting above all other attestations to consider.
This implementation takes the first attestation as the "best" and then tries to find a better attestation by checking the following conditions:
Replace "best" IF
In the case that the "best" attestation already has the validator aggregator bit set, we also do a bit count comparsion to choose the most aggregated attestation.
Which issues(s) does this PR fix?
Fixes #6565
Other notes for review