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

Fix VC not properly handled getAggregatedAttestation's 404 error. #6254

Merged
merged 2 commits into from
May 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions AllTests-mainnet.md
Original file line number Diff line number Diff line change
Expand Up @@ -891,6 +891,7 @@ OK: 1/1 Fail: 0/1 Skip: 0/1
+ /eth/v1/validator/sync_committee_selections serialization/deserialization test OK
+ bestSuccess() API timeout test OK
+ firstSuccessParallel() API timeout test OK
+ getAggregatedAttestationDataScore() default test OK
+ getAggregatedAttestationDataScore() test vectors OK
+ getAttestationDataScore() test vectors OK
+ getLiveness() response deserialization test OK
Expand All @@ -899,7 +900,7 @@ OK: 1/1 Fail: 0/1 Skip: 0/1
+ getUniqueVotes() test vectors OK
+ normalizeUri() test vectors OK
```
OK: 11/11 Fail: 0/11 Skip: 0/11
OK: 12/12 Fail: 0/12 Skip: 0/12
## Validator change pool testing suite
```diff
+ addValidatorChangeMessage/getAttesterSlashingMessage OK
Expand Down Expand Up @@ -1019,4 +1020,4 @@ OK: 2/2 Fail: 0/2 Skip: 0/2
OK: 9/9 Fail: 0/9 Skip: 0/9

---TOTAL---
OK: 684/689 Fail: 0/689 Skip: 5/689
OK: 685/690 Fail: 0/690 Skip: 5/690
7 changes: 7 additions & 0 deletions beacon_chain/spec/eth2_apis/rest_types.nim
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ const

const
preferSSZ* = "application/octet-stream,application/json;q=0.9"
LowestScoreAggregatedAttestation* =
phase0.Attestation(
aggregation_bits: CommitteeValidatorsBits(BitSeq.init(1)))

static:
doAssert(ClientMaximumValidatorIds <= ServerMaximumValidatorIds)
Expand Down Expand Up @@ -615,6 +618,10 @@ type
fork_choice_nodes*: seq[RestNode]
extra_data*: RestExtraData

func isLowestScoreAggregatedAttestation*(a: phase0.Attestation): bool =
(a.data.slot == Slot(0)) and (a.data.index == 0'u64) and
(a.data.source.epoch == Epoch(0)) and (a.data.target.epoch == Epoch(0))

func `==`*(a, b: RestValidatorIndex): bool =
uint64(a) == uint64(b)

Expand Down
7 changes: 7 additions & 0 deletions beacon_chain/validator_client/api.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1708,6 +1708,13 @@ proc getAggregatedAttestation*(
handle400()
ApiResponse[GetAggregatedAttestationResponse].err(
ResponseInvalidError)
of 404:
# A 404 error must be returned if no attestation is available for the
# requested `attestation_data_root`. To address the issue #6184, we
# use empty GetAggregatedAttestationResponse.
ApiResponse[GetAggregatedAttestationResponse].ok(
GetAggregatedAttestationResponse(
data: LowestScoreAggregatedAttestation))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an issue here related to ethereum/beacon-APIs#445 but there's no API fix yet either, so this is fine within the current beacon API.

of 500:
handle500()
ApiResponse[GetAggregatedAttestationResponse].err(
Expand Down
6 changes: 6 additions & 0 deletions beacon_chain/validator_client/attestation_service.nim
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,12 @@ proc produceAndPublishAggregates(service: AttestationServiceRef,
err_name = exc.name, err_msg = exc.msg
return

if isLowestScoreAggregatedAttestation(aggAttestation):
warn "Aggregated attestation with the root was not seen by the " &
"beacon node",
attestation_root = shortLog(attestationRoot)
return

let pendingAggregates =
block:
var res: seq[Future[bool]]
Expand Down
11 changes: 10 additions & 1 deletion tests/test_validator_client.nim
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,15 @@ suite "Validator Client test suite":
score = shortScore(getAggregatedAttestationDataScore(adata))
check score == vector[1]

test "getAggregatedAttestationDataScore() default test":
let
adata = GetAggregatedAttestationResponse(
data: LowestScoreAggregatedAttestation)
score = shortScore(getAggregatedAttestationDataScore(adata))
check:
score == "0.0000"
isLowestScoreAggregatedAttestation(adata.data) == true

test "getSyncCommitteeContributionDataScore() test vectors":
for vector in ContributionDataVectors:
let
Expand Down Expand Up @@ -905,4 +914,4 @@ suite "Validator Client test suite":
res.isOk()
len(res.get().data) == 1
res.get().data[0].index == 100000
res.get().data[0].is_live == true
res.get().data[0].is_live == true
Loading