Add AttestationPool to aggregate attestations on the go #2679
Conversation
dapplion
requested review from
mpetrunic,
twoeths and
wemeetagain
as code owners
github-actions
bot
added
Api
scope-networking
|
Code Climate has analyzed commit 8b20e7c and detected 0 issues on this pull request. View more on Code Climate. |
dapplion
force-pushed
the
dapplion/attestation_pool
branch
from
04cb993
to
df3af57
Compare
dapplion
force-pushed
the
dapplion/attestation_pool
branch
from
df3af57
to
889a928
Compare
| @@ -336,8 +338,9 @@ export async function onErrorAttestation(this: BeaconChain, err: AttestationErro | |||
| this.pendingAttestations.putByBlock(err.type.beaconBlockRoot, err.job); | |||
| break; | |||
|
|
|||
| default: | |||
| await this.db.attestation.remove(err.job.attestation); | |||
| // TODO: Why is necessary to remove the attestation from the DB on error? | |||
There was a problem hiding this comment.
I think this was failsafe since we didn't have api validation before so on some occasion, validator would put some invalid attestation which would prevent all blocks from being processed
There was a problem hiding this comment.
How could an invalid attestation prevent all blocks from being processed?
There was a problem hiding this comment.
process block failed when processing attestations and attestation was put in every block we were producing
There was a problem hiding this comment.
But how could an invalid attestation got included in the attestation pool?
There was a problem hiding this comment.
this error handler for attestation was for forkchoice, not when we process block.
Since it was for forkchoice, it's not really necessary to remove that attestation from db. The next time we see that same error from that same attestation, we could just throw error and handle the same way but this is rare.
| @@ -25,6 +25,18 @@ export async function assembleBody( | |||
| blockSlot: Slot, | |||
| syncAggregateData: {parentSlot: Slot; parentBlockRoot: Root} | |||
| ): Promise<allForks.BeaconBlockBody> { | |||
| // TODO: | |||
There was a problem hiding this comment.
I wonder if we should, if number of agg attestations is < MAX_ATTESTATIONS, take some attestations from attestations memory pool. Assuming we delete attestation that are aggregated?
There was a problem hiding this comment.
Right, the logic to do that successfully is tricky. Since we were not doing that previously it should be done in another PR if we decide to.
| throw new AttestationPoolError({code: AttestationPoolErrorCode.SLOT_TOO_LOW, slot, lowestPermissibleSlot}); | ||
| } | ||
|
|
||
| const dataRoot = ssz.phase0.AttestationData.hashTreeRoot(attestation.data); |
There was a problem hiding this comment.
wouldn't more future safe way be getForkTypes(slot).AttestationData?
There was a problem hiding this comment.
Yes but we should change it everywhere at once in another PR. This logic in unchanged in this PR just moved around.
Motivation
See #2415
Description
Substitutes the current disk-persisted attestation repo with a memory-only attestation pool. It pre-aggregates attestations saving memory and reducing disk reads and writes.
Closes #2415