[Merged by Bors] - Maintain attestations that reference unknown blocks #2319

divagant-martian · 2021-04-21T18:23:26Z

Issue Addressed

Proposed Changes

Keep attestations that reference a block we have not seen for 30secs before being re processed
If we do import the block before that time elapses, it is reprocessed in that moment
The first time it fails, do nothing wrt to gossipsub propagation or peer downscoring. If after being re processed it fails, downscore with a LowToleranceError and ignore the message.

terencechain · 2021-04-27T15:35:18Z

Hi lighthouse team, we are looking to merge a prysm bug fix:

Background, currently prysm validators always wait 1/3 of a slot before sending out attestation. We are a bit hesitant to merge the "timely" fix because @potuz pointed out that lighthouse doesn't hold attestations for unknown block. By merging the fix, it may impact large stakers where they often propose and attest in the same slot. I'm wondering if you guys have any thoughts on whether we can merge our timely fix or a timeline for #2319? Thanks so much!

paulhauner

Heyo! This looks really good, I can't fault it structurally. I had a couple of suggestions, which I've either added to my branch or I've made as single-line Github suggestions.

Feel free to merge or cherry-pick from my branch, as you see fit. There's some cases where I've made a comment on this PR and then implemented my suggestion on my branch; in such cases I've also provided a link to the specific commit.

For convenience, here's a full diff of my suggested changes: https://github.com/paulhauner/lighthouse/compare/e762f175b753fd61469a532ff139844265a76998..dca09d012155f6224291b0a56f009aa6a61ef0a2

I hope that providing actual commits instead of just PR comments is helpful. I've found that it's easier to communicate these things in Rust rather than English, plus it helps me filter out some dumb suggestions before they waste your time 😅

beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs

beacon_node/network/src/beacon_processor/worker/gossip_methods.rs

beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs

michaelsproul · 2021-06-30T05:07:05Z

beacon_node/network/src/beacon_processor/work_reprocessing_queue.rs

+const MAXIMUM_QUEUED_BLOCKS: usize = 16;
+
+/// How many attestations we keep before new ones get dropped.
+const MAXIMUM_QUEUED_ATTESTATIONS: usize = 1_024;


@dapplion astutely pointed out that this value may be too low if a large percentage of validators attest early. Assuming 250k validators, that's 7.8k per slot. We could set the max capacity to 8192 to give us a bit of headroom. What do you think @paulhauner, @divagant-martian?

I agree. I also expected this to be a bit small but was planning to just run it and see what happens whilst it's in unstable.

That being said, I think 8,192 is a better starting point.

Why not compute these numbers on start-up depending on the preset constants?

Why not compute these numbers on start-up depending on the preset constants?

I'm not sure I understand. Are you saying to compute it based on the current validator count?

I'm hesitant to over-think this, I'm keen to get it onto our staging infra, monitor it on Grafana and then start tweaking. The worst case is that the queue fills up and we fall back to the same behaviour as before this PR.

Co-authored-by: Paul Hauner <[email protected]>

paulhauner · 2021-07-09T01:37:29Z

Hey @divagant-martian, just checking if this is still "waiting on author" or if it's "ready for review" 🙏

paulhauner · 2021-07-12T05:21:47Z

I've marked this as ready-for-review as per offline discussions with @divagant-martian. Feel free to remove the label if I misread Diva 🙂

divagant-martian · 2021-07-12T15:09:54Z

Hey! I'm back. It's ready for review. I included all your proposed changes via merge. It was missing a decision about queue length but since that's solved you can give it a check again :)

michaelsproul · 2021-07-13T05:01:55Z

I think this needs to merge unstable again to bring in the Altair changes. Even though there are no textual conflicts I suspect there will be semantic ones

… into att-queueing

divagant-martian · 2021-07-13T21:56:03Z

Merge went out fine. What kind of conflicts to you think we are not noticing? Also it seems to be there is now a cargo audit issue that I'm checking

michaelsproul · 2021-07-13T22:46:33Z

Ah cool. I thought there might be some state.slots that needed to change to state.slot() or something, nothing major

paulhauner

This is great, happy to merge once cargo-audit is appeased. I think @michaelsproul is working on that ☺️

michaelsproul · 2021-07-14T05:23:56Z

bors r+

## Issue Addressed #635 ## Proposed Changes - Keep attestations that reference a block we have not seen for 30secs before being re processed - If we do import the block before that time elapses, it is reprocessed in that moment - The first time it fails, do nothing wrt to gossipsub propagation or peer downscoring. If after being re processed it fails, downscore with a `LowToleranceError` and ignore the message.

bors · 2021-07-14T06:40:57Z

Pull request successfully merged into unstable.

Build succeeded:

divagant-martian added 17 commits April 5, 2021 14:02

wip

5166a3a

wip compiles

c48fafa

move file names

157e5db

make ethspec unpin

ced9b1a

add queued types

bbd8a87

wip

316a0e2

solving unpin problems

c5b9861

wip

9c0c982

do not lose the attestation info on error

c6ed553

add glue

32bd149

add more glue

587c634

almost there

e859725

Merge branch 'unstable' into att-queueing

1432a5c

wip

d366868

update docs and constants

1bad064

add metrics

1b88a1b

fix delay

4213847

terencechain mentioned this pull request Apr 27, 2021

Invert attest-timely flag prysmaticlabs/prysm#8827

Merged

divagant-martian added 11 commits April 27, 2021 11:40

add initial test

3b5b253

better first test

a62bbdf

second test

117af5d

clippy lints

23d3a6e

cleanup

cbd74a9

ignore attestation message for unknown block only after retry

0edffed

Comment style

aea9c3b

Merge branch 'unstable' into att-queueing

587a214

fmt and spelling

07b8b16

update delay

5e9c6b0

update delay

c3e1e28

Add metrics to reprocessing queue

dca09d0

paulhauner requested changes Jun 21, 2021

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jun 21, 2021

michaelsproul reviewed Jun 30, 2021

View reviewed changes

This was referenced Jul 2, 2021

Reprocess attestations in batch ChainSafe/lodestar#2754

Closed

Add unknown block attestation cache ChainSafe/lodestar#2788

Closed

improve logs

c33d3cf

Co-authored-by: Paul Hauner <[email protected]>

divagant-martian requested a review from paulhauner July 5, 2021 16:03

adjust attestations delay before re-processing

4ba921e

Co-authored-by: Paul Hauner <[email protected]>

paulhauner added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jul 12, 2021

divagant-martian added 2 commits July 13, 2021 09:13

Merge branch 'unstable' into att-queueing

cb7c56c

Merge branch 'att-queueing' of github.com:divagant-martian/lighthouse…

f117888

… into att-queueing

paulhauner approved these changes Jul 14, 2021

View reviewed changes

michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 14, 2021

bors bot changed the title ~~Maintain attestations that reference unknown blocks~~ [Merged by Bors] - Maintain attestations that reference unknown blocks Jul 14, 2021

bors bot closed this Jul 14, 2021

paulhauner mentioned this pull request Aug 2, 2021

Maintain Attestations who reference unknown blocks #635

Closed

divagant-martian deleted the att-queueing branch December 27, 2021 18:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Maintain attestations that reference unknown blocks #2319

[Merged by Bors] - Maintain attestations that reference unknown blocks #2319

divagant-martian commented Apr 21, 2021 •

edited

Loading

terencechain commented Apr 27, 2021

paulhauner left a comment •

edited

Loading

michaelsproul Jun 30, 2021

paulhauner Jul 6, 2021

dapplion Jul 6, 2021

paulhauner Jul 9, 2021

paulhauner commented Jul 9, 2021

paulhauner commented Jul 12, 2021

divagant-martian commented Jul 12, 2021

michaelsproul commented Jul 13, 2021

divagant-martian commented Jul 13, 2021

michaelsproul commented Jul 13, 2021

paulhauner left a comment

michaelsproul commented Jul 14, 2021

bors bot commented Jul 14, 2021

[Merged by Bors] - Maintain attestations that reference unknown blocks #2319

[Merged by Bors] - Maintain attestations that reference unknown blocks #2319

Conversation

divagant-martian commented Apr 21, 2021 • edited Loading

Issue Addressed

Proposed Changes

terencechain commented Apr 27, 2021

paulhauner left a comment • edited Loading

Choose a reason for hiding this comment

michaelsproul Jun 30, 2021

Choose a reason for hiding this comment

paulhauner Jul 6, 2021

Choose a reason for hiding this comment

dapplion Jul 6, 2021

Choose a reason for hiding this comment

paulhauner Jul 9, 2021

Choose a reason for hiding this comment

paulhauner commented Jul 9, 2021

paulhauner commented Jul 12, 2021

divagant-martian commented Jul 12, 2021

michaelsproul commented Jul 13, 2021

divagant-martian commented Jul 13, 2021

michaelsproul commented Jul 13, 2021

paulhauner left a comment

Choose a reason for hiding this comment

michaelsproul commented Jul 14, 2021

bors bot commented Jul 14, 2021

divagant-martian commented Apr 21, 2021 •

edited

Loading

paulhauner left a comment •

edited

Loading