Conversation
Greptile OverviewGreptile SummaryThis PR implements the Boole domain fork infrastructure, enabling dynamic domain type selection based on slot/epoch to support smooth transition between Alan and Boole domains. Key Changes:
Transition Window Handling: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Node as SSV Node
participant NetCfg as NetworkConfig
participant Disc as Discovery (dv5)
participant P2P as P2P Layer
participant Valid as Message Validator
participant QBFT as QBFT Controller
Note over Node,QBFT: Pre-Boole Fork (current domain)
Node->>NetCfg: EstimatedCurrentSlot()
NetCfg->>NetCfg: Check if slot >= Boole fork epoch
NetCfg-->>Node: Returns current slot
Node->>NetCfg: DomainTypeAtSlot(slot)
alt Slot < Boole Fork
NetCfg-->>Node: Return DomainType (Alan)
else Slot >= Boole Fork
NetCfg-->>Node: Return NextDomainType (Boole)
end
Note over Node,QBFT: Peer Discovery with Domain Compatibility
Disc->>NetCfg: currentDomainType()
NetCfg-->>Disc: Current domain
Disc->>NetCfg: nextDomainType()
NetCfg-->>Disc: Next domain
Disc->>Disc: Check peer's domain & nextDomain
alt In Transition Window
Disc->>Disc: Accept if peer matches current OR next domain
else Outside Transition
Disc->>Disc: Accept only if peer matches current domain
end
Note over Node,QBFT: P2P Handshake Filter Setup
P2P->>NetCfg: InBooleTransitionWindow(slot)
alt In Transition Window
P2P->>P2P: Create NetworkIDFilterAny([current, next])
else Outside Transition
P2P->>P2P: Create NetworkIDFilter(current)
end
Note over Node,QBFT: Message Creation & Validation
QBFT->>NetCfg: DomainTypeAtSlot(height as slot)
NetCfg-->>QBFT: Domain type for slot
QBFT->>QBFT: Create identifier with domain+pubkey+role
QBFT->>P2P: Broadcast message
P2P->>Valid: Validate incoming message
Valid->>Valid: Extract slot from message
Valid->>NetCfg: DomainTypeAtSlot(slot)
NetCfg-->>Valid: Expected domain for slot
Valid->>Valid: Compare message domain with expected
alt Domain matches
Valid-->>P2P: Accept message
else Domain mismatch
Valid-->>P2P: Reject (ErrWrongDomain)
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
nkryuchkov
changed the base branch from
deploy/nettop+aggcomm-2
to
aggregator-committee
nkryuchkov
force-pushed
the
boole-domain
branch
from
0987c5a to
a6c2e48
Compare
| domain := r.BaseRunner.NetworkConfig.DomainTypeAtSlot(decidedValue.Duty.Slot) | ||
| msgID := spectypes.NewMsgID(domain, r.GetShare().ValidatorPubKey[:], r.BaseRunner.RunnerRoleType) |
There was a problem hiding this comment.
should we send old aggregator runner messages using the new domain type after fork? I'm not sure about the change here..
There was a problem hiding this comment.
It shouldn't be possible because we won't have any aggregator runner duties during Boole epochs. We could have used r.BaseRunner.NetworkConfig.DomainType but I like the current code better, because it keeps the runner choice logic abstract and independent of the runner code, otherwise it would look like a hardcoded value
nkryuchkov
force-pushed
the
boole-domain
branch
from
41dd1af to
80d9de2
Compare
# Conflicts: # go.mod # go.sum # ssvsigner/go.mod # ssvsigner/go.sum
iurii-ssv
left a comment
iurii-ssv
left a comment
There was a problem hiding this comment.
LGTM, with maybe a minor adjustment
There was a problem hiding this comment.
Looks good, but maybe we could simplify the current/next domain handling a bit
| // During transition, accept either configured fork domain for compatibility. | ||
| return nodeDomainType == dvs.netCfg.DomainType || | ||
| nodeNextDomainType == dvs.netCfg.DomainType || | ||
| nodeDomainType == dvs.netCfg.NextDomainType || | ||
| nodeNextDomainType == dvs.netCfg.NextDomainType |
There was a problem hiding this comment.
I wonder if we could simplify the current/next domain handling to something like this ?
- publisher will only publish the current domain
- the receiver would check that ^ against its current/next domain by doing something like:
func (dvs *DiscV5Service) isDomainCompatible(nodeDomainType spectypes.DomainType) bool {
if nodeDomainType == dvs.netCfg.CurrentDomainType() {
return true
}
if dvs.netCfg.InBooleTransitionWindow(dvs.netCfg.EstimatedCurrentSlot()) {
return nodeDomainType == dvs.netCfg.DomainType || nodeDomainType == dvs.netCfg.NextDomainType
}
return false
}There was a problem hiding this comment.
@iurii-ssv probably, it might be good to check nodeNextDomainType == dvs.netCfg.DomainType || nodeDomainType == dvs.netCfg.NextDomainType too to avoid any clock skew. I think it's still possible to be in the fork and get a handshake from a node that was sent before the fork
There was a problem hiding this comment.
I'm thinking: the InBooleTransitionWindow is designed to take care of the clock-skews,
I think it's still possible to be in the fork and get a handshake from a node that was sent before the fork
so once our node is past Boole-fork we'll have 1 slot of time accepting peers who broadcast the older domainType ... after that 1 slot passes those peers must switch to broadcasting the nextDomainType. It is possible that we'll ignore some peers during the transition because of p2p delays, but it doesn't seem like it should cause any real issues (while the simpler code is always preferable).
But maybe it's not worth changing it if you already have tested the current solution in this PR to work well.
There was a problem hiding this comment.
Thinking more about it, maybe InBooleTransitionWindow isn't even the "right" window to use here,
what we really need is to wait a sufficient amount of time (however long it takes for all SSV nodes to start publishing/advertising themselves with the nextDomainType value) after Boole-fork epoch starts ... I don't think 1 slot is enough
^ we'd need that ^ if we were to simplify as I suggested above, your current solution in this PR probably works already regardless
3 participants
The PR must be rebased onto theboole-forkbranch after both #2302 and #2503 are mergedThe PR must be rebased onto theboole-forkbranch after #2503 is merged