Repository navigation
Implement gossip verification of inclusion lists - #10182
mergify[bot] merged 16 commits into
Conversation
|
tests claude thinks we might want to add: inclusion_list_verification/tests.rs
network_beacon_processor/tests.rs |
|
@eserilev added all of these, thanks! |
|
Adding one more case we might want to add a test for - from claude too: To be added in Possible dependent block at the boundary. The |
| "Successfully verified gossip inclusion list" | ||
| ); | ||
|
|
||
| // TODO(heze): emit the inclusion_list SSE event |
There was a problem hiding this comment.
PR #10228 which enabled inclusion_list SSE event to be emitted was merged in the focil devnet branches so can those changes be incorporated in this PR itself or is the plan to introduce it in a subsequent PR when this PR is merged?
There was a problem hiding this comment.
good call, thanks! pulled it in here
| let current_slot = ctx | ||
| .slot_clock | ||
| .now() | ||
| .ok_or(InclusionListVerificationError::UnableToReadSlot)?; | ||
| let is_timely = slot == current_slot | ||
| && ctx | ||
| .slot_clock | ||
| .millis_from_current_slot_start() | ||
| .is_some_and(|time_into_slot| time_into_slot < ctx.spec.get_inclusion_list_due()); |
There was a problem hiding this comment.
we are reading the slot clock twice, this could cause a race condition. it's on on the order of a microsecond, so not realistic tbh. but still i think its cleaner to read the slot clock once
something like:
let now = ctx
.slot_clock
.now_duration()
.ok_or(InclusionListVerificationError::UnableToReadSlot)?;
let current_slot = ctx
.slot_clock
.slot_of(now)
.ok_or(InclusionListVerificationError::UnableToReadSlot)?;There was a problem hiding this comment.
that makes sense, thanks! switched to reading it once
| validator_counts: HashMap<u64, usize>, | ||
| /// Count of valid inclusion lists seen per dependent root and validator, for the | ||
| /// first-or-second rule. | ||
| validator_counts: HashMap<(DependentRoot, u64), usize>, |
There was a problem hiding this comment.
i'm wondering if validator_counts is even necessary? could we just check equivocators directly?
There was a problem hiding this comment.
good point, it was always the same as being in equivocators, so removed it
| InvalidTransactions(InclusionListTransactionsError), | ||
| /// The block with root `dependent_root` has not been seen. | ||
| DependentRootUnknown { dependent_root: Hash256 }, | ||
| /// The block with root `dependent_root` is not before the start of the lookahead epoch. |
There was a problem hiding this comment.
nit:
// The block with root dependent_root is after dependent_slot.
| } | ||
| } | ||
|
|
||
| let now = ctx |
There was a problem hiding this comment.
I think we need to get the actual seen_timestamp (like when we actually first saw the IL). if we calculate it here we are ignoring the time that the IL sat in the beacon processor/got requeued etc
you can get that info in router.rs and pass it down to here
There was a problem hiding this comment.
done, it's now passed down from the router and used for the timeliness check
| } | ||
| } | ||
|
|
||
| pub fn verify_inclusion_list_for_gossip( |
There was a problem hiding this comment.
can we add gossip metrics for IL's? we do a similar thing for payload atteestations.
we can add a counter for
- IL processing requests
- IL processing sucecess
- IL verification errors
also maybe a metric for equivocations
There was a problem hiding this comment.
added the request, success and per-type error counters, plus one for equivocations
|
Queued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 30 minutes 10 seconds in the queue, including 28 minutes 19 seconds running CI. Required conditions to merge
|
Issue Addressed
Part of FOCIL (EIP-7805). Follow-up to #9846, which wired up the
inclusion_listtopic but ignored every message.Proposed Changes
inclusion_list_verificationmodule that runs thevalidate_inclusion_list_gossipchecks in the order defined by the spec, with unit tests intests.rs.(dependent_root, validator), following consensus-specs#5605.InclusionListStore::process_inclusion_listtake aGossipVerifiedInclusionListinstead of a raw list and timeliness flag.inclusion_list_signature_setto take thegenesis_validators_rootinstead of a state.ChainSpec::get_inclusion_list_duefor the timeliness check.Additional Info
The transaction bounds check is copied from #10008 and
get_inclusion_list_duefrom conache#8, so whichever PR merges second will need a small rebase.