-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Use explicit slot component timing configs #15999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1879fec to
a05f0c7
Compare
| // SlotComponentDuration returns the duration representing the given portion (in basis points) of a slot. | ||
| func (b *BeaconChainConfig) SlotComponentDuration(bp primitives.BP) time.Duration { | ||
| ms := uint64(bp) * b.SlotDurationMillis() / uint64(BasisPoints) | ||
| return time.Duration(ms) * time.Millisecond |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a duplicate of ComponentDuration in time/slots/slottime.go.
maybe we can remove that one.
| "AGGREGATE_DUE_BPS", | ||
| "AGGREGATE_DUE_BPS_GLOAS", | ||
| "ATTESTATION_DEADLINE", | ||
| "ATTESTATION_DUE_BPS", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be added to assertEqualConfigs()? (SyncMessageDueBPS is there)
same for CONTRIBUTION_DUE_BPS and PROPOSER_REORG_CUTOFF_BPS
config/params/mainnet_config.go
Outdated
| ProposerReorgCutoffBPS: primitives.BP(1667), | ||
| AttestationDueBPS: primitives.BP(3333), | ||
| AggregrateDueBPS: primitives.BP(6667), | ||
|
|
||
| SyncMessageDueBPS: primitives.BP(3333), | ||
| ContributionDueBPS: primitives.BP(6667), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ProposerReorgCutoffBPS: primitives.BP(1667), | |
| AttestationDueBPS: primitives.BP(3333), | |
| AggregrateDueBPS: primitives.BP(6667), | |
| SyncMessageDueBPS: primitives.BP(3333), | |
| ContributionDueBPS: primitives.BP(6667), | |
| ProposerReorgCutoffBPS: primitives.BP(1667), | |
| AttestationDueBPS: primitives.BP(3333), | |
| AggregrateDueBPS: primitives.BP(6667), | |
| SyncMessageDueBPS: primitives.BP(3333), | |
| ContributionDueBPS: primitives.BP(6667), |
if the last two don't belong here in Fork choice algorithm constants, we should move them to another group.
maybe it's also a good idea to move all of these to a new Time parameters group since there is more stuff coming in gloas.
beacon-chain/sync/subscriber_test.go
Outdated
| func TestFilterSubnetPeers(t *testing.T) { | ||
| params.SetupTestConfigCleanup(t) | ||
| cfg := params.MainnetConfig() | ||
| cfg.SecondsPerSlot = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| cfg.SecondsPerSlot = 1 |
| func TestWaitUntilSlotComponent_ContextCancelReturnsImmediately(t *testing.T) { | ||
| params.SetupTestConfigCleanup(t) | ||
| cfg := params.BeaconConfig().Copy() | ||
| cfg.SlotDurationMilliseconds = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant 10000 ms here? because otherwise the waiting time for function return (2s) is much longer than the attestation deadline of the slot (3ms).
so the waitUntilSlotComponent() will always return under 2s, regardless of context cancellation.
a05f0c7 to
b2a7377
Compare
b2a7377 to
2413cdf
Compare
2413cdf to
f5f4627
Compare
* Use new timing configs (due BPS) * Bastin's feedback
This PR introduces explicit slot component timing configs by adding
SlotDurationMillisecondsplus new basis-point deadlines (*_BPS) to config/params and added validator-side helpers (wait_helpers.go) that translate configured basis points into concrete deadlines; attestation, aggregation, sync messages, and contribution submissions now go through these helpers and expose distinct tracing spansReference: ethereum/consensus-specs#4476