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.

Sign up for GitHub

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 atterstation effectivnes #493

Merged
merged 8 commits into from
Feb 10, 2021
Merged

Fix atterstation effectivnes #493

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6d00203
fix storing attestation effectiveness
BeroBurny Feb 8, 2021
2b8a0e5
improve tests
BeroBurny Feb 8, 2021
945ea3f
clear attestation effectiveness data on deleting validator
BeroBurny Feb 8, 2021
1263711
fix tests
BeroBurny Feb 9, 2021
8526709
fix attestation effectiveness saga
BeroBurny Feb 9, 2021
43d43c5
fix comments
BeroBurny Feb 10, 2021
e60cba1
fix lint
BeroBurny Feb 10, 2021
faff647
fix comments
BeroBurny Feb 10, 2021
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
7 changes: 4 additions & 3 deletions src/renderer/ducks/validator/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,15 @@ export type SignedNewAttestation = (
block: string,
index: number,
slot: number,
bits: string,
) => {
payload: {block: string; index: number; slot: number};
payload: {block: string; index: number; slot: number; bits: string};
meta: string;
};
export const signedNewAttestation = createAction<SignedNewAttestation>(
"validator/signedNewAttestation",
(publicKey: string, block: string, index: number, slot: number) => ({
payload: {block, index, slot},
(publicKey: string, block: string, index: number, slot: number, bits: string) => ({
payload: {block, index, slot, bits},
BeroBurny marked this conversation as resolved.
Show resolved Hide resolved
meta: publicKey,
}),
);
Expand Down
29 changes: 16 additions & 13 deletions src/renderer/ducks/validator/sagas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ function* removeValidatorSaga(
yield all([
database.validator.balance.delete(action.payload),
database.validator.beaconNodes.delete(action.payload),
database.validator.attestationEffectiveness.delete(action.payload),
]);
yield put(removeValidator(action.payload));
}
Expand Down Expand Up @@ -445,17 +446,19 @@ export function* getAttestationEffectiveness({
skippedQue++;
} else {
const sanitizedAttestations = result.filter(
({data}) => payload.block === toHex(data.beaconBlockRoot) && data.index === payload.index,
({data, aggregationBits}) =>
payload.block === toHex(data.beaconBlockRoot) &&
data.index === payload.index &&
data.slot === payload.slot &&
payload.bits === JSON.stringify(aggregationBits),
Copy link
Member

Choose a reason for hiding this comment

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

You need to check if bit on validator position in committee is set to true. I think you should also check source and target root.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

source and target contains epoch and root of slot as i check source slot in parent object of source and think checking for source and target can be bit a overkill and add unnecessary complexity

Copy link
Member

Choose a reason for hiding this comment

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

it contains epoch and epoch boundary root (it's basically validator vote) that are unreleated to slot. That said since, you are checking committee index, it's highly unlikely that someone created attestation with different source and target outside of chainguardian (in that case more pressing issue is slashing anyways).

);
if (!sanitizedAttestations.length) empty++;
else empty = 0;
sanitizedAttestations.forEach(({data}) => {
if (data.slot > inclusion && data.slot > payload.slot) {
inclusion = data.slot;
skipped += skippedQue;
skippedQue = 0;
}
});
else {
inclusion = range[index];
skipped += skippedQue;
skippedQue = 0;
empty = 0;
}
previousSlot = range[index];
}
});
Expand All @@ -464,11 +467,11 @@ export function* getAttestationEffectiveness({
* can conclude that code fund block where is attestation included and there is no reason to continue this loop
* (in some rare case can cause premature breaking a loop, if is a problem increase "maxEmptyBlocks")
* */
const maxEmptyBlocks = 3;
if (previousSlot !== 0 && empty >= maxEmptyBlocks + skippedQue) break;
const maxEmptyBlocks = 5;
if (inclusion !== 0 && empty >= maxEmptyBlocks + skippedQue) break;

lastSlot = newSlot.payload;
if (lastSlot > timeoutSlot) break;
if (lastSlot > timeoutSlot + skipped) break;
}
/**
* this handle case when is assertion included in block but is not visible in next block
Expand All @@ -486,7 +489,7 @@ export function* getAttestationEffectiveness({
epoch: computeEpochAtSlot(config, payload.slot),
inclusion,
slot: payload.slot,
efficiency: efficiency > 1 ? 1 : Math.round(efficiency * 1000) / 1000,
efficiency: efficiency > 1 ? 100 : efficiency > 0 ? Math.floor(efficiency * 100) : 0,
time: Date.now(),
});
}
Expand Down
1 change: 1 addition & 0 deletions src/renderer/services/eth2/client/eth2ApiClient/cgEth2BeaconPoolApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export class CgEth2BeaconPoolApi implements IBeaconPoolApi {
toHex(attestation.data.beaconBlockRoot),
attestation.data.index,
attestation.data.slot,
JSON.stringify(attestation.aggregationBits),
),
);
}
Expand Down
5 changes: 2 additions & 3 deletions src/renderer/services/utils/charts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,8 @@ export const getAttestationEfficiencyChartData = (
return new Array(7).fill(null).map((_, index) => {
const time = addDays(new Date(baseTime), index);
const value =
Math.round(
attestationEffectiveness.getAverageAttestationEfficiency(startOfDay(time), endOfDay(time)) * 100,
) || null;
Math.round(attestationEffectiveness.getAverageAttestationEfficiency(startOfDay(time), endOfDay(time))) ||
null;
const label = format(time, "EEEE");
return {value, label};
});
Expand Down
70 changes: 34 additions & 36 deletions test/e2e/redux/validator/getAttestationEffectiveness/saga.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ describe("getAttestationEffectiveness", () => {
it(
"attestation is visible in next block",
testAttestationEffectivenessSaga([{slotOffset: 1, skipped: false, empty: false}], {
efficiency: 1,
efficiency: 100,
inclusionOffset: 1,
}),
);
it(
"attestation is not visible in next block, but its attested is included",
testAttestationEffectivenessSaga([], {
efficiency: 1,
efficiency: 100,
inclusionOffset: 1,
}),
);
Expand All @@ -22,10 +22,9 @@ describe("getAttestationEffectiveness", () => {
[
{slotOffset: 1, skipped: false, empty: false},
{slotOffset: 2, skipped: false, empty: false},
{slotOffset: 3, skipped: false, empty: false},
],
{
efficiency: 0.5,
efficiency: 50,
inclusionOffset: 2,
},
),
Expand All @@ -37,10 +36,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 1, skipped: false, empty: false},
{slotOffset: 2, skipped: false, empty: false},
{slotOffset: 3, skipped: false, empty: false},
{slotOffset: 4, skipped: false, empty: false},
],
{
efficiency: 0.333,
efficiency: 33,
inclusionOffset: 3,
},
),
Expand All @@ -52,10 +50,9 @@ describe("getAttestationEffectiveness", () => {
[
{slotOffset: 1, skipped: true, empty: false},
{slotOffset: 2, skipped: false, empty: false},
{slotOffset: 3, skipped: false, empty: false},
],
{
efficiency: 1,
efficiency: 100,
inclusionOffset: 2,
},
),
Expand All @@ -67,10 +64,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 1, skipped: true, empty: false},
{slotOffset: 2, skipped: true, empty: false},
{slotOffset: 3, skipped: false, empty: false},
{slotOffset: 4, skipped: false, empty: false},
],
{
efficiency: 1,
efficiency: 100,
inclusionOffset: 3,
},
),
Expand All @@ -86,8 +82,8 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 5, skipped: false, empty: false},
],
{
efficiency: 1,
inclusionOffset: 4,
efficiency: 100,
inclusionOffset: 5,
},
),
);
Expand All @@ -100,10 +96,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 1, skipped: true, empty: false},
{slotOffset: 2, skipped: false, empty: false},
{slotOffset: 3, skipped: false, empty: false},
{slotOffset: 4, skipped: false, empty: false},
],
{
efficiency: 0.667,
efficiency: 66,
inclusionOffset: 3,
},
),
Expand All @@ -115,10 +110,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 1, skipped: false, empty: false},
{slotOffset: 2, skipped: true, empty: false},
{slotOffset: 3, skipped: false, empty: false},
{slotOffset: 4, skipped: false, empty: false},
],
{
efficiency: 0.667,
efficiency: 66,
inclusionOffset: 3,
},
),
Expand All @@ -132,10 +126,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 2, skipped: true, empty: false},
{slotOffset: 3, skipped: false, empty: false},
{slotOffset: 4, skipped: false, empty: false},
{slotOffset: 5, skipped: false, empty: false},
],
{
efficiency: 0.75,
efficiency: 75,
inclusionOffset: 4,
},
),
Expand All @@ -147,10 +140,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 1, skipped: false, empty: false},
{slotOffset: 3, skipped: true, empty: false},
{slotOffset: 4, skipped: false, empty: false},
{slotOffset: 5, skipped: false, empty: false},
],
{
efficiency: 0.75,
efficiency: 75,
inclusionOffset: 4,
},
),
Expand All @@ -163,10 +155,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 2, skipped: false, empty: false},
{slotOffset: 3, skipped: true, empty: false},
{slotOffset: 4, skipped: false, empty: false},
{slotOffset: 5, skipped: false, empty: false},
],
{
efficiency: 0.75,
efficiency: 75,
inclusionOffset: 4,
},
),
Expand All @@ -177,9 +168,22 @@ describe("getAttestationEffectiveness", () => {
it(
"attestation including slot is +16",
testAttestationEffectivenessSaga(
new Array(17).fill(null).map((_, index) => ({slotOffset: index + 1, skipped: false, empty: false})),
new Array(16).fill(null).map((_, index) => ({slotOffset: index + 1, skipped: false, empty: false})),
{
efficiency: 0.063,
efficiency: 6,
inclusionOffset: 16,
},
),
);
it(
"attestation including slot is +16 but all between are skipped",
testAttestationEffectivenessSaga(
[
...new Array(15).fill(null).map((_, index) => ({slotOffset: index + 1, skipped: false, empty: true})),
{slotOffset: 16, skipped: false, empty: false},
],
{
efficiency: 6,
inclusionOffset: 16,
},
),
Expand All @@ -191,10 +195,9 @@ describe("getAttestationEffectiveness", () => {
[
{slotOffset: 4, skipped: true, empty: false},
{slotOffset: 10, skipped: false, empty: false},
{slotOffset: 11, skipped: false, empty: false},
],
{
efficiency: 0.5,
efficiency: 50,
inclusionOffset: 10,
},
),
Expand All @@ -206,10 +209,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 5, skipped: false, empty: false},
{slotOffset: 9, skipped: true, empty: false},
{slotOffset: 10, skipped: false, empty: false},
{slotOffset: 11, skipped: false, empty: false},
],
{
efficiency: 0.5,
efficiency: 50,
inclusionOffset: 10,
},
),
Expand All @@ -228,10 +230,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 8, skipped: true, empty: false},
{slotOffset: 9, skipped: false, empty: false},
{slotOffset: 10, skipped: false, empty: false},
{slotOffset: 11, skipped: false, empty: false},
],
{
efficiency: 0.5,
efficiency: 50,
inclusionOffset: 10,
},
),
Expand All @@ -246,10 +247,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 8, skipped: true, empty: false},
{slotOffset: 9, skipped: false, empty: false},
{slotOffset: 10, skipped: false, empty: false},
{slotOffset: 11, skipped: false, empty: false},
],
{
efficiency: 0.5,
efficiency: 50,
inclusionOffset: 10,
},
),
Expand All @@ -265,10 +265,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 2, skipped: true, empty: false},
{slotOffset: 4, skipped: false, empty: true},
{slotOffset: 5, skipped: false, empty: false},
{slotOffset: 6, skipped: false, empty: false},
],
{
efficiency: 0.4,
efficiency: 40,
inclusionOffset: 5,
},
),
Expand All @@ -281,10 +280,9 @@ describe("getAttestationEffectiveness", () => {
{slotOffset: 2, skipped: false, empty: false},
{slotOffset: 4, skipped: false, empty: true},
{slotOffset: 5, skipped: false, empty: false},
{slotOffset: 6, skipped: false, empty: false},
],
{
efficiency: 0.2,
efficiency: 20,
inclusionOffset: 5,
},
),
Expand Down
18 changes: 11 additions & 7 deletions test/e2e/redux/validator/getAttestationEffectiveness/testAttestationEffectivenessSaga.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ import {BitList} from "@chainsafe/ssz";

const publicKey = "0x9331f1ec6672748ca7b080faff7038da35838f57d223db4f2cb5020246e6c31695c3fb3db0d78db13d266476e34e4e65";
const block = "0xc3687c87021f5b7855465caf6501b3f742f20f26b65cc7a107ff7a78f0b28b79";
const bitsString =
// eslint-disable-next-line max-len
"[false,false,true,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false,false]";
const bitsArray = JSON.parse(bitsString);
const committee = 11;
const slot = 466969;

Expand Down Expand Up @@ -43,7 +47,7 @@ export const testAttestationEffectivenessSaga = (
let lastSlot = slot;
const result = await expectSaga(
getAttestationEffectiveness,
signedNewAttestation(publicKey, block, committee, slot),
signedNewAttestation(publicKey, block, committee, slot, bitsString),
)
.provide({
select: () => selectedValidator,
Expand All @@ -62,13 +66,13 @@ export const testAttestationEffectivenessSaga = (
if (cases[index]) {
return mockBeaconBlockAttestations(
block,
effect.args[0],
slot,
committee,
cases[index].skipped,
cases[index].empty,
);
} else {
return mockBeaconBlockAttestations(block, effect.args[0], committee, false, true);
return mockBeaconBlockAttestations(block, slot, committee, false, true);
}
}
},
Expand Down Expand Up @@ -97,17 +101,17 @@ const mockBeaconBlockAttestations = (
for (let i = 0; i <= randomLength; i++) {
const randomIndex = Math.floor(Math.random() * 30);
if (randomIndex === index) continue;
blocks.push(createAttestations(slot - 1, randomIndex, block));
blocks.push(createAttestations(slot, randomIndex, block));
}
if (!empty) {
blocks.push(createAttestations(slot - 1, index, block));
blocks.push(createAttestations(slot, index, block, bitsArray));
}

return blocks;
};

const createAttestations = (slot: number, index: number, beaconBlockRoot: string): Attestation => ({
aggregationBits: createRandomAggregationBits(),
const createAttestations = (slot: number, index: number, beaconBlockRoot: string, bits?: boolean[]): Attestation => ({
aggregationBits: bits || createRandomAggregationBits(),
data: {
slot,
index,
Expand Down