Improve AggregateAndProof gossip validation #2329

twoeths · 2021-04-06T04:04:45Z

Motivation

AggregateAndProof as TreeBacked to avoid multiple struct_hashTreeRoot
Increase JobQueue for AggregateAndProof
Filter out known aggregated attestation

Description

part of #2306

codeclimate · 2021-04-06T04:05:23Z

Code Climate has analyzed commit f45ffee and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

View more on Code Climate.

packages/lodestar/src/chain/validation/aggregateAndProof.ts

twoeths · 2021-04-07T04:18:55Z

some statistic (found cached AggregateAndProof / total)

slot 104922: 32/596
slot 104956: 99/555
slot 104954: 38/437
slot 104948: 50/525
slot 105057: 0/14
slot 105056: 2/49

twoeths · 2021-04-07T04:43:07Z

based on prater block explorer, I think there are at most 64 attestation data based on 64 different committee index of the same slot. There are ~600 different AggregateAndProof messages of the same slot because each aggregator see different aggregationBits. So we can improve the cache strategy by:

cache by AttestationData root + number of attesters (based on aggregationBits)
if same AttestationData root and not greater number of attesters, we can ignore that AggregationAndProof

what do you think @dapplion ?

dapplion · 2021-04-07T07:36:53Z

based on prater block explorer, I think there are at most 64 attestation data based on 64 different committee index of the same slot. There are ~600 different AggregateAndProof messages of the same slot because each aggregator see different aggregationBits. So we can improve the cache strategy by:

cache by AttestationData root + number of attesters (based on aggregationBits)

if same AttestationData root and not greater number of attesters, we can ignore that AggregationAndProof

what do you think @dapplion ?

@wemeetagain Can confirm but we should take any aggregate that includes new bits so the fork choice is aware of as many attestations as possible

twoeths · 2021-04-07T08:38:29Z

in that case we should cache AttestationData root + XOR of (aggregationBits):

{rootA, [0, 1, 1, 0]} comes => cache {rootA => [0, 1, 1, 0]}
{rootA, [1, 0, 1, 0]} comes => cache {rootA => [1, 1, 1, 0]}
{rootA, [1, 1, 0, 0]} comes => IGNORE since its aggregationBits are subset of the cached one
{rootA, [1, 1, 1, 0]} comes => IGNORE since its aggregationBits are same to the cached one
{rootA, [0, 0, 0, 1} comes => cache {rootA => [1, 1, 1, 1]}
from now on whenever rootA comes => IGNORE

twoeths · 2021-04-08T09:15:36Z

with the cache by AttestationData root hex + XOR (aggregationBits) we have (found seen gossip AggregationAndProof/total):

slot 113716: 397/546
slot 113720: 384/553
slot 113736: 394/596

so we don't have to validate every SignedAggregateAndProof like before, just around 30% of that

wemeetagain · 2021-04-09T15:45:01Z

packages/lodestar/src/db/seenAttestationCache.ts


 /**
 * USed to verify gossip attestation. When there are multiple
 * attestation from same validator
 */
 export class SeenAttestationCache {
  private cache: Map<string, boolean>;
-
+  // key as AttestationDelta root hex, value as aggregationBits


Suggested change

// key as AttestationDelta root hex, value as aggregationBits

// key as AttestationData root hex, value as aggregationBits

wemeetagain · 2021-04-09T15:45:56Z

packages/lodestar/src/db/seenAttestationCache.ts

+   * We're only interested in the AttestationData + aggregationBits inside AggregateAndProof.
+   */
+  private aggregateAndProofKey(value: phase0.AggregateAndProof): string {
+    return toHexString(this.config.types.phase0.AttestationData.hashTreeRoot(value.aggregate.data));


Suggested change

return toHexString(this.config.types.phase0.AttestationData.hashTreeRoot(value.aggregate.data));

return toHexString(this.config.getTypes(value.aggregate.slot).AttestationData.hashTreeRoot(value.aggregate.data));

wemeetagain · 2021-04-09T15:51:45Z

packages/lodestar/src/db/seenAttestationCache.ts

+      this.attDataCache.set(
+        key,
+        aggBit.map((_, i) => aggBit[i] || cachedAggBit[i])
+      );


Might consider something like:

for (const [i, bit] of aggBit.entries()) { if (bit) { cachedAggBit[i] = true; } }

tbh i'm not sure which is better.

wemeetagain · 2021-04-09T15:54:23Z

packages/lodestar/src/db/beacon.ts

@@ -48,7 +48,7 @@ export class BeaconDb extends DatabaseService implements IBeaconDb {
    this.badBlock = new BadBlockRepository(this.config, this.db);
    this.block = new BlockRepository(this.config, this.db);
    this.pendingBlock = new PendingBlockRepository(this.config, this.db);
-    this.seenAttestationCache = new SeenAttestationCache(5000);
+    this.seenAttestationCache = new SeenAttestationCache(this.config, 2048);


curious why this is decreased?

currently it's used for 2 caches: (committee) Attestation and AttestationAndProof but this PR changes to cache them separately so mathematically it should be 5000 / 2 = 2500, but I changed it to 2048

…x root and aggregationBits

3xtr4t3rr3str14l · 2021-04-12T18:52:18Z

packages/lodestar/src/network/gossip/validator.ts

@@ -20,7 +20,8 @@ import {
 // Numbers from https://github.com/sigp/lighthouse/blob/b34a79dc0b02e04441ba01fd0f304d1e203d877d/beacon_node/network/src/beacon_processor/mod.rs#L69
 const gossipQueueOpts: {[K in GossipType]: {maxLength: number; type: QueueType}} = {
  [GossipType.beacon_block]: {maxLength: 1024, type: QueueType.FIFO},
-  [GossipType.beacon_aggregate_and_proof]: {maxLength: 1024, type: QueueType.LIFO},
+  // this is different from lighthouse's, there are more gossip aggregate_and_proof than gossip block
+  [GossipType.beacon_aggregate_and_proof]: {maxLength: 4096, type: QueueType.LIFO},


what's the rationale for changing maxLength here?

the issue itself is the queue length is not enough to handle SignedAggregateAndProof gossip topic, it's ~600 gossip messages per slot

github-actions bot added the scope-networking All issues related to networking, gossip, and libp2p. label Apr 6, 2021

twoeths marked this pull request as ready for review April 6, 2021 04:24

twoeths requested review from dapplion, mpetrunic and wemeetagain as code owners April 6, 2021 04:24

dapplion reviewed Apr 6, 2021

View reviewed changes

packages/lodestar/src/chain/validation/aggregateAndProof.ts Outdated Show resolved Hide resolved

twoeths force-pushed the tuyen/gossip-aggregate-and-proof-validation branch from d65e58f to bdb80e5 Compare April 7, 2021 02:08

twoeths marked this pull request as draft April 7, 2021 11:29

twoeths force-pushed the tuyen/gossip-aggregate-and-proof-validation branch 2 times, most recently from 0426a73 to 6ca8401 Compare April 8, 2021 08:56

twoeths marked this pull request as ready for review April 8, 2021 09:15

wemeetagain requested changes Apr 9, 2021

View reviewed changes

twoeths added 2 commits April 10, 2021 10:28

Cache gossip AggregateAndProof

31f5b80

SeenAttestationCache to cache AggregateAndProof by AttestationData he…

2f9d8a6

…x root and aggregationBits

twoeths force-pushed the tuyen/gossip-aggregate-and-proof-validation branch from 6ca8401 to 995860f Compare April 10, 2021 03:43

Address PR comments

f45ffee

twoeths force-pushed the tuyen/gossip-aggregate-and-proof-validation branch from 995860f to f45ffee Compare April 10, 2021 03:45

twoeths requested review from dapplion and wemeetagain April 11, 2021 02:52

3xtr4t3rr3str14l reviewed Apr 12, 2021

View reviewed changes

dapplion approved these changes Apr 12, 2021

View reviewed changes

wemeetagain approved these changes Apr 12, 2021

View reviewed changes

wemeetagain merged commit e478ce6 into master Apr 12, 2021

wemeetagain deleted the tuyen/gossip-aggregate-and-proof-validation branch April 12, 2021 20:18

twoeths mentioned this pull request May 20, 2021

Altair 1.1.0-alpha.4 #2530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve AggregateAndProof gossip validation #2329

Improve AggregateAndProof gossip validation #2329

twoeths commented Apr 6, 2021

codeclimate bot commented Apr 6, 2021 •

edited

twoeths commented Apr 7, 2021

twoeths commented Apr 7, 2021

dapplion commented Apr 7, 2021

twoeths commented Apr 7, 2021

twoeths commented Apr 8, 2021

wemeetagain Apr 9, 2021

wemeetagain Apr 9, 2021

wemeetagain Apr 9, 2021

wemeetagain Apr 9, 2021

twoeths Apr 10, 2021

3xtr4t3rr3str14l Apr 12, 2021

twoeths Apr 12, 2021

	// key as AttestationDelta root hex, value as aggregationBits
	// key as AttestationData root hex, value as aggregationBits

	return toHexString(this.config.types.phase0.AttestationData.hashTreeRoot(value.aggregate.data));
	return toHexString(this.config.getTypes(value.aggregate.slot).AttestationData.hashTreeRoot(value.aggregate.data));

Improve AggregateAndProof gossip validation #2329

Improve AggregateAndProof gossip validation #2329

Conversation

twoeths commented Apr 6, 2021

codeclimate bot commented Apr 6, 2021 • edited

twoeths commented Apr 7, 2021

twoeths commented Apr 7, 2021

dapplion commented Apr 7, 2021

twoeths commented Apr 7, 2021

twoeths commented Apr 8, 2021

wemeetagain Apr 9, 2021

Choose a reason for hiding this comment

wemeetagain Apr 9, 2021

Choose a reason for hiding this comment

wemeetagain Apr 9, 2021

Choose a reason for hiding this comment

wemeetagain Apr 9, 2021

Choose a reason for hiding this comment

twoeths Apr 10, 2021

Choose a reason for hiding this comment

3xtr4t3rr3str14l Apr 12, 2021

Choose a reason for hiding this comment

twoeths Apr 12, 2021

Choose a reason for hiding this comment

codeclimate bot commented Apr 6, 2021 •

edited