feat(core): implement core/bcast#480
Conversation
|
@claude review this PR |
|
Claude finished @iamquang95's task in 4m 6s —— View job Review:
|
| Component | Go | Rust | Match | Notes |
|---|---|---|---|---|
Duty routing (Broadcast) |
present | present | yes | Deprecated/Randao/Prepare cases match |
| Electra validator-index backfill | present | present | yes | warn + verify + match-by-sig |
PriorAttestationKnown swallow |
strings.Contains |
to_string().contains |
yes | |
Attester delay slotDuration/3 |
*1/3 |
div_duration(_,3) |
yes | |
Agg/SyncContrib delay *2/3 |
*2/3 |
mul-then-div | yes | rounding preserved (commit 018fd26) |
firstSlotInCurrentEpoch |
wraps on negative age | errors on negative age | partial | safer; future-genesis only |
| Recaster store / older-slot skip | >= |
>= |
yes | |
Recaster FirstInEpoch gate |
present | present | yes | |
Metric names core_bcast_* |
namespace+subsystem | prefix="core_bcast" |
yes | matches codebase _total convention |
recast source pregen/downstream |
Slot > 0 |
inner() > 0 |
yes | |
| Exit error propagation | last-iteration wins | any-failure wins | no | see Medium finding |
Tests
- Not run locally. CI is green: Build and test code, Linter check, Semver checks, and create cluster Charon parity all pass (run
27532635119). - Strong unit coverage added: duty routing, prior-known swallow, Electra backfill, set-conversion error strings, active-validator filtering, recaster epoch/active-set behavior, and
to_consensus_version/resolve_fork_versiontie-break tests.
Open questions
- Is the
broadcast_exitserror-propagation change (any-failure vs Go's last-iteration) intentional? If so, a one-line comment would help future parity reviews. - Confirm the conditional delay observation is the desired metrics behavior (it diverges slightly from Go's always-observe).
· branchiamquang95/core-bcast
emlautarom1
left a comment
emlautarom1
left a comment
There was a problem hiding this comment.
LGTM, minot things only
| pub const fn to_consensus_version(self) -> Result<crate::ConsensusVersion, VersionError> { | ||
| use crate::ConsensusVersion; | ||
| match self { | ||
| DataVersion::Phase0 => Ok(ConsensusVersion::Phase0), | ||
| DataVersion::Altair => Ok(ConsensusVersion::Altair), | ||
| DataVersion::Bellatrix => Ok(ConsensusVersion::Bellatrix), | ||
| DataVersion::Capella => Ok(ConsensusVersion::Capella), | ||
| DataVersion::Deneb => Ok(ConsensusVersion::Deneb), | ||
| DataVersion::Electra => Ok(ConsensusVersion::Electra), | ||
| DataVersion::Fulu => Ok(ConsensusVersion::Fulu), | ||
| DataVersion::Unknown => Err(VersionError::UnknownDataVersion), | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: Prefer From trait; this is also implemented in #172.
| fn duration_to_std(duration: Duration, context: &'static str) -> Result<StdDuration> { | ||
| duration.to_std().map_err(|_| Error::InvalidTime { | ||
| context, | ||
| value: duration.num_milliseconds(), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Single usage, inline it
| fn duration_from_std(duration: StdDuration, context: &'static str) -> Result<Duration> { | ||
| Duration::from_std(duration).map_err(|_| Error::ArithmeticOverflow { context }) | ||
| } |
There was a problem hiding this comment.
Single usage, inline it
| fn boxed(source: impl StdError + Send + Sync + 'static) -> BoxError { | ||
| Box::new(source) | ||
| } |
There was a problem hiding this comment.
Prefer to inline this
| self.broadcast_builder_registration(&duty, &set).await?; | ||
| } | ||
| DutyType::Exit => self.broadcast_exits(&duty, &set).await?, | ||
| DutyType::Randao | DutyType::PrepareAggregator | DutyType::PrepareSyncContribution => {} |
There was a problem hiding this comment.
nit: we could port the comments on why these are no-ops
| let value = match payload { | ||
| versioned::AttestationPayload::Phase0(attestation) | ||
| | versioned::AttestationPayload::Altair(attestation) | ||
| | versioned::AttestationPayload::Bellatrix(attestation) | ||
| | versioned::AttestationPayload::Capella(attestation) | ||
| | versioned::AttestationPayload::Deneb(attestation) => { | ||
| serde_json::to_value(attestation).map_err(error_message)? | ||
| } | ||
| versioned::AttestationPayload::Electra(_) | versioned::AttestationPayload::Fulu(_) => { | ||
| return Err(unexpected_response( | ||
| "attestation request body", | ||
| "electra payload in pre-electra request", | ||
| )); | ||
| } | ||
| }; | ||
| items.push(serde_json::from_value(value).map_err(error_message)?); |
There was a problem hiding this comment.
This is a bit of a hack since we're serializing and then deserializing instead of doing direct struct mappings. Not ideal, but I don't know how expensive would be to add the mappings manually.
2 participants
No description provided.