-
Notifications
You must be signed in to change notification settings - Fork 23
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
simplify the LSQ server test #937
base: main
Are you sure you want to change the base?
Conversation
For example, I removed the replication of the Immutable/VolatileTip acquire messages, since the test is so simple that the number of those is currently irrelevant---it's misleading to replicate them, as if that was already expected to matter.
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.
Looks good!
(Before merging, the "WIP" from the commit message should ofc be removed.)
@@ -144,7 +148,7 @@ checkOutcome k chain = conjoin . map (uncurry checkResult) | |||
| otherwise | |||
-> tabulate "Acquired" ["AcquireFailurePointNotOnChain"] $ property True | |||
Left AcquireFailurePointTooOld | |||
| pointSlot pt >= immutableSlot | |||
| pointSlot pt >= immutableSlot -- TODO what if the immtip is a multi-leader slot? |
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.
When we ask for a point with the same slot as the immutable tip, but with a different hash, we get a AcquireFailurePointNotOnChain
, right?
Lines 47 to 51 in ef8186d
Nothing | |
| pointSlot pt < pointSlot immutablePoint | |
-> SendMsgFailure AcquireFailurePointTooOld idle | |
| otherwise | |
-> SendMsgFailure AcquireFailurePointNotOnChain idle |
-- A random sequence of targets: one for each block in the tree and also a | ||
-- random number of immtip/voltip queries |
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.
A random sequence of targets: one for each block in the tree and also a random number of immtip/voltip queries
Isn't the change that is explained in the PR description exactly about removing the fact that there is a random number of voltip queries, instead just using one?
For example, I removed the replication of the Immutable/VolatileTip acquire messages, since the test is so simple that the number of those is currently irrelevant---it's misleading to replicate them, as if that was already expected to matter.