Reprocess unknown block root attestations #3564
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #3564 +/- ##
==========================================
- Coverage 37.40% 37.17% -0.23%
==========================================
Files 311 312 +1
Lines 8374 8452 +78
Branches 1299 1317 +18
==========================================
+ Hits 3132 3142 +10
- Misses 5093 5161 +68
Partials 149 149 |
|
Code Climate has analyzed commit fbad895 and detected 3 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
Performance Report✔️ no performance regression detected Full benchmark results
|
twoeths
requested review from
dapplion,
g11tech,
mpetrunic and
wemeetagain
as code owners
|
thanks to the more gossip attestations we accepted, average peer score is an improvement too
|
|
@tuyennhv I've changed the design a bit as in my opinion make the change more manageable:
Please review my changes and keep them only if you agree. Please make sure I haven't broken any assumptions from our previous design |
wemeetagain
reviewed
Jan 13, 2022
| * reprocess attestations, it should control when the attestations are ready to reprocess instead. | ||
| */ | ||
| export class ReprocessController { | ||
| private readonly awaitingPromisesByRootBySlot: MapDef<Slot, MapDef<RootHex, AwaitingAttestationPromise[]>>; |
There was a problem hiding this comment.
I wonder if we can make do with MapDef<Slot, MapDef<RootHex, AwaitingAttestationPromise>>
Reusing the same promise for attestations relying on the same block.
We would just be tracking the maximum wait time before resolve/reject per block.
|
@tuyennhv I've moved back the code to handlers to implemented #3613 in the future without nasty stuff. I think it looks decently clean now and allows for extension here: |
|
thanks @dapplion , it looks great now. The latest update works well on contabo-20 👍 |
wemeetagain
previously approved these changes
Jan 14, 2022
dapplion
previously approved these changes
Jan 14, 2022
|
@tuyennhv File conflicts from pass attestations to the forkchoice |
twoeths
force-pushed
the
tuyen/reprocess-attestations
branch
from
c264a3c
to
fbad895
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Right now when attestations come before the block, we ignore attestations. We should follow other clients to queue and reprocess those attestations when the block becomes known. This:
Description
Since we used to have performance issue with this
reprocesswork, the approach is quite conservativeReprocessControllerto wait for the block, add awaiting promises toawaitingPromisesByRootBySlotawaitingPromisesByRootBySlotper clock slot since we don't want to reprocess old slot attestationsREPROCESS_MIN_TIME_TO_NEXT_SLOT_SECconstantCloses #3560
Some considerations
js-libp2p-gossipsub, I do this reprocess in the gossip handlerTest
I don't see any big differences with the metrics, except for the peer count with=> peer count is the same after running 2 instances for 9 days. Although total attestations reprocessed per slot is only around 20 - 60/slot, the total attestations accepted is a big difference after I leave 2 instances (contabo 19 vs cotabo 20) for 9 dayssubscribeAllSubnetsflag