-
Notifications
You must be signed in to change notification settings - Fork 25
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
GSM: split OnlyBootstrap into PreSyncing and Syncing #975
Conversation
4e9512c
to
d0fc114
Compare
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.
LGTM (Great!). A few minor requests.
ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Node/GSM.hs
Outdated
Show resolved
Hide resolved
ouroboros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/Node/GSM.hs
Show resolved
Hide resolved
...oros-consensus-diffusion/src/ouroboros-consensus-diffusion/Ouroboros/Consensus/NodeKernel.hs
Show resolved
Hide resolved
d0fc114
to
f411dff
Compare
@@ -78,11 +81,23 @@ data CandidateVersusSelection = | |||
-- ^ Whether the candidate is better than the selection | |||
deriving (Eq, Show) | |||
|
|||
data GsmState = |
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.
Is it worth clarifying what GSM is in the comments?
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.
Added a comment expanding the acronym (it is also in the module header, but that is a bit distanced)
-> QSM.Commands Command Response | ||
-> QC.Property | ||
prop_sequential1 j0 cmds = runSimQC $ do | ||
prop_sequential1 ctx cmds = runSimQC $ do | ||
let s0 = case j0 of |
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.
Maybe s0
and j0
could have more descriptive names.
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.
Renamed 👍 83cd2fe
We still use `LedgerStateJudgement` for initialization, with TooOld corresponding to PreSyncing and YoungEnough corresponding to CaughtUp.
Compared to the previous static predicate, this has the following advantages: - Confirms necessity of `avoidTransientState` (or a similar mechanism) in the ModelPreSyncing logic - Makes necessity of using `isHaaSatisfied` check in `initModel` explicit. - The tests with less than 3 peers perform many more useful state transitions.
f411dff
to
83cd2fe
Compare
This PR splits the previous OnlyBootstrap state of the GSM into separate PreSyncing and Syncing states, making it ready to be used for Genesis, while allowing to still use the GSM in conjunction with bootstrap peers.
Compared to the Bootstrap Peers IER, there is one more state and two more transition rules, both based on the Honest Availability Assumption (HHA), which the Network layer will try to establish and inform us about, likely by ensuring a minimum amount of (big) ledger peers, see
isHaaSatisfied
inGsmView
.The new state transition diagram looks like this: